Re: [Qemu-devel] [PATCH v2 1/3] glib: bump min required glib library version to 2.40
On 06.06.2018 19:32, Daniel P. Berrangé wrote: > Per supported platforms doc, the various min glib on relevant distros is: > > RHEL-7: 2.50.3 > Debian (Stretch): 2.50.3 > Debian (Jessie): 2.42.1 > OpenBSD (Ports): 2.54.3 > FreeBSD (Ports): 2.50.3 > OpenSUSE Leap 15: 2.54.3 > Ubuntu (Xenial): 2.48.0 > macOS (Homebrew): 2.56.0 > > This suggests that a minimum glib of 2.42 is a reasonable target. > > The GLibC compile farm, however, uses Ubuntu 14.04 (Trusty) which only > has glib 2.40.0, and this is needed for testing during merge. Thus an > exception is made to the documented platform support policy to allow for > all three current LTS releases to be supported. > > Signed-off-by: Daniel P. Berrangé > --- > configure | 6 +- > crypto/hash-glib.c | 4 - > crypto/hmac-glib.c | 36 - > include/glib-compat.h | 319 > qga/commands.c | 11 +- > tests/ivshmem-test.c| 6 - > tests/test-qmp-event.c | 8 +- > tests/tpm-emu.h | 4 +- > tests/vhost-user-test.c | 26 +--- > trace/simple.c | 6 +- > util/osdep.c| 14 -- > 11 files changed, 11 insertions(+), 429 deletions(-) Great diffstat! And the changes look fine to me: Reviewed-by: Thomas Huth
[Qemu-devel] create a domain failed
hi everyone: my order: virt-install --virt-type kvm -n centos -r 1024 --disk centos.img,format=qcow2,size=10 --cdrom /home/CentOS-7-aarch64-Everything.iso the failed log: qemu-kvm: -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1:MSI-X is not supported by interrupt controller. my environment: arm64 platform;centos7.5 for arm; libvirt-3.9.0;qemu-2.10.0;virt-install-1.4.3 I think the qemu is the point,but it's hard for me to into the code. Do you have met the issue ever?Any suggestion? -- Have a good day
Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
On 06/07/2018 11:17 AM, Peter Xu wrote:
On Wed, Jun 06, 2018 at 06:11:50PM +0800, Wei Wang wrote:
I got similar comments from Michael, and it will be
while (1) {
lock;
func();
unlock();
}
All the unlock inside the body will be gone.
Ok I think I have more question on this part...
Actually AFAICT this new feature uses iothread in a way very similar
to the block layer, so I digged a bit on how block layer used the
iothreads. I see that the block code is using something like
virtio_queue_aio_set_host_notifier_handler() to hook up the
iothread/aiocontext and the ioeventfd, however here you are manually
creating one QEMUBH and bound that to the new context. Should you
also use something like the block layer? Then IMHO you can avoid
using a busy loop there (assuming the performance does not really
matter that much here for page hintings), and all the packet handling
can again be based on interrupts from the guest (ioeventfd).
[1]
Also mentioned in another discussion thread that it's better to not let
guest send notifications. Otherwise, we would have used the virtqueue
door bell to notify host.
So we need to use polling here, and Michael suggested to implemented in
BH, which sounds good to me.
[...]
+static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+.name = "virtio-balloon-device/free-page-report",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = virtio_balloon_free_page_support,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
+VMSTATE_UINT32(poison_val, VirtIOBalloon),
(could we move all the poison-related lines into another patch or
postpone? after all we don't support it yet, do we?)
We don't support migrating poison value, but guest maybe use it, so we are
actually disabling this feature in that case. Probably good to leave the
code together to handle that case.
Could we just avoid declaring that feature bit in emulation code
completely? I mean, we support VIRTIO_BALLOON_F_FREE_PAGE_HINT first
as the first step (as you mentioned in commit message, the POISON is a
TODO). Then when you really want to completely support the POISON
bit, you can put all that into a separate patch. Would that work?
Not really. The F_PAGE_POISON isn't a feature configured via QEMU cmd
line like F_FREE_PAGE_HINT. We always set F_PAGE_POISON if
F_FREE_PAGE_HINT is enabled. It is used to detect if the guest is using
page poison.
+if (virtio_has_feature(s->host_features,
+ VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, NULL);
+s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+s->free_page_report_cmd_id =
+ VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN - 1;
Why explicitly -1? I thought ID_MIN would be fine too?
Yes, that will also be fine. Since we states that the cmd id will be from
[MIN, MAX], and we make s->free_page_report_cmd_id++ in start(), using
VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN here will make it [MIN + 1, MAX].
Then I would prefer we just use the MIN value, otherwise IMO we'd
better have a comment mentioning about why that -1 is there.
Sure, we can do that.
Best,
Wei
Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
On 06/06/2018 07:02 PM, Peter Xu wrote:
On Wed, Jun 06, 2018 at 06:04:23PM +0800, Wei Wang wrote:
On 06/06/2018 01:42 PM, Peter Xu wrote:
IMHO migration states do not suite here. IMHO bitmap syncing is too
frequently an operation, especially at the end of a precopy migration.
If you really want to introduce some notifiers, I would prefer
something new rather than fiddling around with migration state. E.g.,
maybe a new migration event notifiers, then introduce two new events
for both start/end of bitmap syncing.
Please see if below aligns to what you meant:
MigrationState {
...
+ int ram_save_state;
}
typedef enum RamSaveState {
RAM_SAVE_BEGIN = 0,
RAM_SAVE_END = 1,
RAM_SAVE_MAX = 2
}
then at the step 1) and 3) you concluded somewhere below, we change the
state and invoke the callback.
I mean something like this:
1693c64c27 ("postcopy: Add notifier chain", 2018-03-20)
That was a postcopy-only notifier. Maybe we can generalize it into a
more common notifier for the migration framework so that we can even
register with non-postcopy events like bitmap syncing?
Precopy already has its own notifiers: git 99a0db9b
If we want to reuse, that one would be more suitable. I think mixing
non-related events into one notifier list isn't nice.
Btw, the migration_state_notifiers is already there, but seems not really
used (I only tracked spice-core.c called
add_migration_state_change_notifier). I thought adding new migration states
can reuse all that we have.
What's your real concern about that? (not sure how defining new events would
make a difference)
Migration state is exposed via control path (QMP). Adding new states
mean that the QMP clients will see more. IMO that's not really
anything that a QMP client will need to know, instead we can keep it
internally. That's a reason from compatibility pov.
Meanwhile, it's not really a state-thing at all for me. It looks
really more like hook or event (start/stop of sync).
Thanks for sharing your concerns in detail, which are quite helpful for
the discussion. To reuse 99a0db9b, we can also add sub-states (or say
events), instead of new migration states.
For example, we can still define "enum RamSaveState" as above, which can
be an indication for the notifier queued on the 99a0db9b notider_list to
decide whether to call start or stop.
Does this solve your concern?
I would suggest to focus on the supplied interface and its usage in live
migration. That is, now we have two APIs, start() and stop(), to start and
stop the optimization.
1) where in the migration code should we use them (do you agree with the
step (1), (2), (3) you concluded below?)
2) how should we use them, directly do global call or via notifiers?
I don't know how Dave and Juan might think; here I tend to agree with
Michael that some notifier framework should be nicer.
What would be the advantages of using notifiers here?
Isolation of modules? Then migration/ram.c at least won't need to
include something like "balloon.h".
And I think it's also possible too if some other modules would like to
hook at these places someday.
OK, I can implement it with notifiers, but this (framework) is usually
added when someday there is a second user who needs a callback at the
same place.
This is not that obvious to me. For now I think it's true, since when
we call stop() we'll take the mutex, meanwhile the mutex is actually
always held by the iothread (in the big loop in
virtio_balloon_poll_free_page_hints) until either:
- it sleeps in qemu_cond_wait() [1], or
- it leaves the big loop [2]
Since I don't see anyone who will set dev->block_iothread to true for
the balloon device, then [1] cannot happen;
there is a case in virtio_balloon_set_status which sets dev->block_iothread
to true.
Did you mean the free_page_lock mutex? it is released at the bottom of the
while() loop in virtio_balloon_poll_free_page_hint. It's actually released
for every hint. That is,
while(1){
take the lock;
process 1 hint from the vq;
release the lock;
}
Ah, so now I understand why you need the lock to be inside the loop,
since the loop is busy polling actually. Is it possible to do this in
an async way?
We need to use polling here because of some back story in the guest side
(due to some locks being held) that makes it a barrier to sending
notifications for each hints.
I'm a bit curious on how much time will it use to do
one round of the free page hints (e.g., an idle guest with 8G mem, or
any configuration you tested)? I suppose during that time the
iothread will be held steady with 100% cpu usage, am I right?
Compared to the time spent by the legacy migration to send free pages,
that small amount of CPU usage spent on filtering free pages could be
neglected.
Grinding a chopper will not hold up the work of cutting firewood :)
Best,
Wei
Re: [Qemu-devel] Stair step trace output since 12fb0ac05
On Thu, Jun 7, 2018 at 6:56 AM, Thomas Huth wrote: > On 06.06.2018 20:56, Markus Armbruster wrote: >> BALATON Zoltan writes: >> >>> Hello, >>> >>> Since 12fb0ac05 (char: Remove unwanted crlf conversion) trace output >>> is printed in stair steps when using -trace and -serial stdio >>> together. E.g. >>> $ qemu-system-i386 -trace 'pci*' -serial stdio >>> >>> Regards, >>> BALATON Zoltan >> >> I cc'ed the people involved with this patch for you. > > Is it OK if you revert the change to chardev/char-stdio.c, but not > chardev/char-serial.c ? That's what I did, and it was enough to fix the issues I had. Thanks, Laurent
Re: [Qemu-devel] Stair step trace output since 12fb0ac05
On 06.06.2018 20:56, Markus Armbruster wrote: > BALATON Zoltan writes: > >> Hello, >> >> Since 12fb0ac05 (char: Remove unwanted crlf conversion) trace output >> is printed in stair steps when using -trace and -serial stdio >> together. E.g. >> $ qemu-system-i386 -trace 'pci*' -serial stdio >> >> Regards, >> BALATON Zoltan > > I cc'ed the people involved with this patch for you. Is it OK if you revert the change to chardev/char-stdio.c, but not chardev/char-serial.c ? Thomas
Re: [Qemu-devel] [PATCH v2 3/3] util: remove redundant include of glib.h
On Wed, Jun 06, 2018 at 07:31:53PM +0100, Peter Maydell wrote: > On 6 June 2018 at 18:32, Daniel P. Berrangé wrote: > > Code must only ever include glib.h indirectly via the glib-compat.h > > header file, because we will need some macros set before glib.h is > > pulled in. Adding extra includes of glib.h will (soon) cause compile > > failures such as: > > > > In file included from /home/berrange/src/virt/qemu/include/qemu/osdep.h:107, > > from > > /home/berrange/src/virt/qemu/include/qemu/iova-tree.h:26, > > from util/iova-tree.c:13: > > /home/berrange/src/virt/qemu/include/glib-compat.h:22: error: > > "GLIB_VERSION_MIN_REQUIRED" redefined [-Werror] > > #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_40 > > > > In file included from /usr/include/glib-2.0/glib/gtypes.h:34, > > from /usr/include/glib-2.0/glib/galloca.h:32, > > from /usr/include/glib-2.0/glib.h:30, > > from util/iova-tree.c:12: > > /usr/include/glib-2.0/glib/gversionmacros.h:237: note: this is the location > > of the previous definition > > # define GLIB_VERSION_MIN_REQUIRED (GLIB_VERSION_CUR_STABLE) > > > > Signed-off-by: Daniel P. Berrangé > > --- > > util/iova-tree.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/util/iova-tree.c b/util/iova-tree.c > > index 2d9cebfc89..d39cd8bb29 100644 > > --- a/util/iova-tree.c > > +++ b/util/iova-tree.c > > @@ -9,7 +9,6 @@ > > * This work is licensed under the terms of the GNU GPL, version 2 or > > later. > > */ > > > > -#include > > #include "qemu/iova-tree.h" > > While we're fixing up the headers in this file: > it should start with an include of qemu/osdep.h, > and qemu/iova-tree.h should not include osdep.h... Sorry to messed this up. It was used for hwaddr definition. Maybe we can just replace hwaddr usage in iova-tree.[ch] with something like uint64_t? Then I think we can drop the osdep.h. Regards, -- Peter Xu
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/5] qapi: add x-block-dirty-bitmap-enable/disable
On Wed, Jun 06, 2018 at 02:24:46PM -0400, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Expose the ability to turn bitmaps "on" or "off". This is experimental
> and principally for the sake of the Libvirt Checkpoints API, and it may
> or may not be committed for 3.0.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 42 ++
> qapi/block-core.json | 42 ++
> 2 files changed, 84 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8de95be8f4..fb402edc94 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2923,6 +2923,48 @@ void qmp_block_dirty_bitmap_clear(const char *node,
> const char *name,
> bdrv_clear_dirty_bitmap(bitmap, NULL);
> }
>
> +void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
> + Error **errp)
> +{
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *bitmap;
> +
> +bitmap = block_dirty_bitmap_lookup(node, name, , errp);
> +if (!bitmap) {
> +return;
> +}
> +
> +if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +error_setg(errp,
> + "Bitmap '%s' is currently frozen and cannot be enabled",
> + name);
> +return;
> +}
> +
> +bdrv_enable_dirty_bitmap(bitmap);
> +}
> +
> +void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
> +Error **errp)
> +{
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *bitmap;
> +
> +bitmap = block_dirty_bitmap_lookup(node, name, , errp);
> +if (!bitmap) {
> +return;
> +}
> +
> +if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +error_setg(errp,
> + "Bitmap '%s' is currently frozen and cannot be disabled",
> + name);
> +return;
> +}
> +
> +bdrv_disable_dirty_bitmap(bitmap);
> +}
> +
> BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char
> *node,
>const char
> *name,
>Error **errp)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4b1de474a9..02de674f5f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1808,6 +1808,48 @@
> { 'command': 'block-dirty-bitmap-clear',
>'data': 'BlockDirtyBitmap' }
>
> +##
> +# @x-block-dirty-bitmap-enable:
> +#
> +# Enables a dirty bitmap so that it will begin tracking disk changes.
> +#
> +# Returns: nothing on success
> +# If @node is not a valid block device, DeviceNotFound
> +# If @name is not found, GenericError with an explanation
> +#
> +# Since: 3.0
> +#
> +# Example:
> +#
> +# -> { "execute": "x-block-dirty-bitmap-enable",
> +# "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> + { 'command': 'x-block-dirty-bitmap-enable',
> +'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @x-block-dirty-bitmap-disable:
> +#
> +# Disables a dirty bitmap so that it will stop tracking disk changes.
> +#
> +# Returns: nothing on success
> +# If @node is not a valid block device, DeviceNotFound
> +# If @name is not found, GenericError with an explanation
> +#
> +# Since: 3.0
> +#
> +# Example:
> +#
> +# -> { "execute": "x-block-dirty-bitmap-disable",
> +# "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'x-block-dirty-bitmap-disable',
> + 'data': 'BlockDirtyBitmap' }
> +
> ##
> # @BlockDirtyBitmapSha256:
> #
> --
> 2.14.3
>
>
Re: [Qemu-devel] [Qemu-block] [PATCH v2 4/5] qapi: add x-block-dirty-bitmap-merge
On Wed, Jun 06, 2018 at 02:24:48PM -0400, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> block/dirty-bitmap.c | 18 ++
> blockdev.c | 30 ++
> include/block/dirty-bitmap.h | 3 ++-
> qapi/block-core.json | 38 ++
> 4 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 56234257f4..4159d3929e 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -757,3 +757,21 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap
> *bitmap, uint64_t offset)
> {
> return hbitmap_next_zero(bitmap->bitmap, offset);
> }
> +
> +void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap
> *src,
> + Error **errp)
> +{
> +/* only bitmaps from one bds are supported */
> +assert(dest->mutex == src->mutex);
> +
> +qemu_mutex_lock(dest->mutex);
> +
> +assert(bdrv_dirty_bitmap_enabled(dest));
> +assert(!bdrv_dirty_bitmap_readonly(dest));
> +
> +if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
> +error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +}
> +
> +qemu_mutex_unlock(dest->mutex);
> +}
> diff --git a/blockdev.c b/blockdev.c
> index af705738cd..dc40a358b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3044,6 +3044,36 @@ void qmp_x_block_dirty_bitmap_disable(const char
> *node, const char *name,
> bdrv_disable_dirty_bitmap(bitmap);
> }
>
> +void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
> +const char *src_name, Error **errp)
> +{
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *dst, *src;
> +
> +dst = block_dirty_bitmap_lookup(node, dst_name, , errp);
> +if (!dst) {
> +return;
> +}
> +
> +if (bdrv_dirty_bitmap_frozen(dst)) {
> +error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
> + dst_name);
> +return;
> +} else if (bdrv_dirty_bitmap_readonly(dst)) {
> +error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> + dst_name);
> +return;
> +}
> +
> +src = bdrv_find_dirty_bitmap(bs, src_name);
> +if (!src) {
> +error_setg(errp, "Dirty bitmap '%s' not found", src_name);
> +return;
> +}
> +
> +bdrv_merge_dirty_bitmap(dst, src, errp);
> +}
> +
> BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char
> *node,
>const char
> *name,
>Error **errp)
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 1ff8949b1b..1e14743032 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -70,7 +70,8 @@ void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap
> *bitmap, bool value);
> void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
> bool persistent);
> void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
> qmp_locked);
> -
> +void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap
> *src,
> + Error **errp);
>
> /* Functions that require manual locking. */
> void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 02de674f5f..9d4ab93190 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1740,6 +1740,20 @@
>'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> '*persistent': 'bool', '*autoload': 'bool' } }
>
> +##
> +# @BlockDirtyBitmapMerge:
> +#
> +# @node: name of device/node which the bitmap is tracking
> +#
> +# @dst_name: name of the destination dirty bitmap
> +#
> +# @src_name: name of the source dirty bitmap
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'BlockDirtyBitmapMerge',
> + 'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
> +
> ##
> # @block-dirty-bitmap-add:
> #
> @@ -1850,6 +1864,30 @@
> { 'command': 'x-block-dirty-bitmap-disable',
>'data': 'BlockDirtyBitmap' }
>
> +##
> +# @x-block-dirty-bitmap-merge:
> +#
> +# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
> +# bitmap is unchanged. On error, @dst_name is unchanged.
> +#
> +# Returns: nothing on success
> +# If @node is not a valid block device, DeviceNotFound
> +# If @dst_name or @src_name is not found, GenericError
> +# If bitmaps has different sizes or granularities, GenericError
> +#
> +# Since: 3.0
> +#
> +# Example:
> +#
> +# -> { "execute": "x-block-dirty-bitmap-merge",
> +# "arguments": { "node":
Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/5] qmp: transaction support for x-block-dirty-bitmap-enable/disable
On Wed, Jun 06, 2018 at 02:24:47PM -0400, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> [Added x- prefix. --js]
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> blockdev.c| 81
> ++-
> qapi/transaction.json | 4 +++
> 2 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index fb402edc94..af705738cd 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2052,6 +2052,7 @@ typedef struct BlockDirtyBitmapState {
> BlockDriverState *bs;
> HBitmap *backup;
> bool prepared;
> +bool was_enabled;
> } BlockDirtyBitmapState;
>
> static void block_dirty_bitmap_add_prepare(BlkActionState *common,
> @@ -2151,6 +2152,74 @@ static void
> block_dirty_bitmap_clear_commit(BlkActionState *common)
> hbitmap_free(state->backup);
> }
>
> +static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
> + Error **errp)
> +{
> +BlockDirtyBitmap *action;
> +BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> +if (action_check_completion_mode(common, errp) < 0) {
> +return;
> +}
> +
> +action = common->action->u.x_block_dirty_bitmap_enable.data;
> +state->bitmap = block_dirty_bitmap_lookup(action->node,
> + action->name,
> + NULL,
> + errp);
> +if (!state->bitmap) {
> +return;
> +}
> +
> +state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
> +{
> +BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> +if (!state->was_enabled) {
> +bdrv_disable_dirty_bitmap(state->bitmap);
> +}
> +}
> +
> +static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
> + Error **errp)
> +{
> +BlockDirtyBitmap *action;
> +BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> +if (action_check_completion_mode(common, errp) < 0) {
> +return;
> +}
> +
> +action = common->action->u.x_block_dirty_bitmap_disable.data;
> +state->bitmap = block_dirty_bitmap_lookup(action->node,
> + action->name,
> + NULL,
> + errp);
> +if (!state->bitmap) {
> +return;
> +}
> +
> +state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +bdrv_disable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
> +{
> +BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> +if (state->was_enabled) {
> +bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +}
> +
> static void abort_prepare(BlkActionState *common, Error **errp)
> {
> error_setg(errp, "Transaction aborted using Abort action");
> @@ -2211,7 +2280,17 @@ static const BlkActionOps actions[] = {
> .prepare = block_dirty_bitmap_clear_prepare,
> .commit = block_dirty_bitmap_clear_commit,
> .abort = block_dirty_bitmap_clear_abort,
> -}
> +},
> +[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +.instance_size = sizeof(BlockDirtyBitmapState),
> +.prepare = block_dirty_bitmap_enable_prepare,
> +.abort = block_dirty_bitmap_enable_abort,
> +},
> +[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +.instance_size = sizeof(BlockDirtyBitmapState),
> +.prepare = block_dirty_bitmap_disable_prepare,
> +.abort = block_dirty_bitmap_disable_abort,
> + }
> };
>
> /**
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index bd312792da..d7e4274550 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -46,6 +46,8 @@
> # - @abort: since 1.6
> # - @block-dirty-bitmap-add: since 2.5
> # - @block-dirty-bitmap-clear: since 2.5
> +# - @x-block-dirty-bitmap-enable: since 3.0
> +# - @x-block-dirty-bitmap-disable: since 3.0
> # - @blockdev-backup: since 2.3
> # - @blockdev-snapshot: since 2.5
> # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -59,6 +61,8 @@
> 'abort': 'Abort',
> 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
> 'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
> + 'x-block-dirty-bitmap-enable':
Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
On Wed, Jun 06, 2018 at 06:11:50PM +0800, Wei Wang wrote:
> On 06/06/2018 02:43 PM, Peter Xu wrote:
> > On Tue, Apr 24, 2018 at 02:13:47PM +0800, Wei Wang wrote:
> >
> > [...]
> >
> > +if (elem->out_num) {
> > +size = iov_to_buf(elem->out_sg, elem->out_num, 0, ,
> > sizeof(id));
> > +virtqueue_push(vq, elem, size);
> > Silly question: is this sending the same id back to guest? Why?
>
> No. It's just giving back the used buffer.
Oops, sorry!
>
> >
> > > +g_free(elem);
> > > +
> > > +virtio_tswap32s(vdev, );
> > > +if (unlikely(size != sizeof(id))) {
> > > +virtio_error(vdev, "received an incorrect cmd id");
> > Forgot to unlock?
> >
> > Maybe we can just move the lock operations outside:
> >
> >mutex_lock();
> >while (1) {
> > ...
> > if (block) {
> >qemu_cond_wait();
> > }
> > ...
> > if (skip) {
> >continue;
> > }
> > ...
> > if (error) {
> >break;
> > }
> > ...
> >}
> >mutex_unlock();
>
>
> I got similar comments from Michael, and it will be
> while (1) {
> lock;
> func();
> unlock();
> }
>
> All the unlock inside the body will be gone.
Ok I think I have more question on this part...
Actually AFAICT this new feature uses iothread in a way very similar
to the block layer, so I digged a bit on how block layer used the
iothreads. I see that the block code is using something like
virtio_queue_aio_set_host_notifier_handler() to hook up the
iothread/aiocontext and the ioeventfd, however here you are manually
creating one QEMUBH and bound that to the new context. Should you
also use something like the block layer? Then IMHO you can avoid
using a busy loop there (assuming the performance does not really
matter that much here for page hintings), and all the packet handling
can again be based on interrupts from the guest (ioeventfd).
[1]
>
> > [...]
> > > +static const VMStateDescription vmstate_virtio_balloon_free_page_report
> > > = {
> > > +.name = "virtio-balloon-device/free-page-report",
> > > +.version_id = 1,
> > > +.minimum_version_id = 1,
> > > +.needed = virtio_balloon_free_page_support,
> > > +.fields = (VMStateField[]) {
> > > +VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> > > +VMSTATE_UINT32(poison_val, VirtIOBalloon),
> > (could we move all the poison-related lines into another patch or
> > postpone? after all we don't support it yet, do we?)
> >
>
> We don't support migrating poison value, but guest maybe use it, so we are
> actually disabling this feature in that case. Probably good to leave the
> code together to handle that case.
Could we just avoid declaring that feature bit in emulation code
completely? I mean, we support VIRTIO_BALLOON_F_FREE_PAGE_HINT first
as the first step (as you mentioned in commit message, the POISON is a
TODO). Then when you really want to completely support the POISON
bit, you can put all that into a separate patch. Would that work?
>
>
> > > +if (virtio_has_feature(s->host_features,
> > > + VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > > +s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> > > NULL);
> > > +s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > > +s->free_page_report_cmd_id =
> > > + VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN -
> > > 1;
> > Why explicitly -1? I thought ID_MIN would be fine too?
>
> Yes, that will also be fine. Since we states that the cmd id will be from
> [MIN, MAX], and we make s->free_page_report_cmd_id++ in start(), using
> VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN here will make it [MIN + 1, MAX].
Then I would prefer we just use the MIN value, otherwise IMO we'd
better have a comment mentioning about why that -1 is there.
>
> >
> > > +if (s->iothread) {
> > > +object_ref(OBJECT(s->iothread));
> > > +s->free_page_bh =
> > > aio_bh_new(iothread_get_aio_context(s->iothread),
> > > +
> > > virtio_balloon_poll_free_page_hints, s);
> > Just to mention that now we can create internal iothreads. Please
> > have a look at iothread_create().
>
> Thanks. I noticed that, but I think configuring via the cmd line can let
> people share the iothread with other devices that need it.
Ok. Please have a look at my previous comment at [1]. I'm not sure
whether my understanding is correct. But in case if so, not sure
whether we can avoid this QEMUBH here.
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 1/3] block: Make bdrv_is_writable() public
On Wed, Jun 06, 2018 at 09:37:00PM +0200, Max Reitz wrote:
> This is a useful function for the whole block layer, so make it public.
> At the same time, users outside of block.c probably do not need to make
> use of the reopen functionality, so rename the current function to
> bdrv_is_writable_after_reopen() create a new bdrv_is_writable() function
> that just passes NULL to it for the reopen queue.
>
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
Thanks!
Reviewed-by: Jeff Cody
> ---
> include/block/block.h | 1 +
> block.c | 17 ++---
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 4dd4f1eab2..e677080c4e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -408,6 +408,7 @@ bool bdrv_is_read_only(BlockDriverState *bs);
> int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> bool ignore_allow_rdw, Error **errp);
> int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> +bool bdrv_is_writable(BlockDriverState *bs);
> bool bdrv_is_sg(BlockDriverState *bs);
> bool bdrv_is_inserted(BlockDriverState *bs);
> void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> diff --git a/block.c b/block.c
> index 9d577f65bb..50887087f3 100644
> --- a/block.c
> +++ b/block.c
> @@ -1620,13 +1620,24 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q,
> BlockDriverState *bs)
>
> /* Returns whether the image file can be written to after the reopen queue @q
> * has been successfully applied, or right now if @q is NULL. */
> -static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)
> +static bool bdrv_is_writable_after_reopen(BlockDriverState *bs,
> + BlockReopenQueue *q)
> {
> int flags = bdrv_reopen_get_flags(q, bs);
>
> return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR;
> }
>
> +/*
> + * Return whether the BDS can be written to. This is not necessarily
> + * the same as !bdrv_is_read_only(bs), as inactivated images may not
> + * be written to but do not count as read-only images.
> + */
> +bool bdrv_is_writable(BlockDriverState *bs)
> +{
> +return bdrv_is_writable_after_reopen(bs, NULL);
> +}
> +
> static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
> BdrvChild *c, const BdrvChildRole *role,
> BlockReopenQueue *reopen_queue,
> @@ -1664,7 +1675,7 @@ static int bdrv_check_perm(BlockDriverState *bs,
> BlockReopenQueue *q,
>
> /* Write permissions never work with read-only images */
> if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
> -!bdrv_is_writable(bs, q))
> +!bdrv_is_writable_after_reopen(bs, q))
> {
> error_setg(errp, "Block node is read-only");
> return -EPERM;
> @@ -1956,7 +1967,7 @@ void bdrv_format_default_perms(BlockDriverState *bs,
> BdrvChild *c,
>, );
>
> /* Format drivers may touch metadata even if the guest doesn't write
> */
> -if (bdrv_is_writable(bs, reopen_queue)) {
> +if (bdrv_is_writable_after_reopen(bs, reopen_queue)) {
> perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> }
>
> --
> 2.17.0
>
Re: [Qemu-devel] [PATCH v2 2/3] qcow2: Do not mark inactive images corrupt
On Wed, Jun 06, 2018 at 09:37:01PM +0200, Max Reitz wrote: > When signaling a corruption on a read-only image, qcow2 already makes > fatal events non-fatal (i.e., they will not result in the image being > closed, and the image header's corrupt flag will not be set). This is > necessary because we cannot set the corrupt flag on read-only images, > and it is possible because further corruption of read-only images is > impossible. > > Inactive images are effectively read-only, too, so we should do the same > for them. bdrv_is_writable() can tell us whether an image can actually > be written to, so use its result instead of !bs->read_only. > > (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in > bdrv_co_pwritev() will fail, crashing qemu.) > > Cc: qemu-sta...@nongnu.org > Signed-off-by: Max Reitz Reviewed-by: Jeff Cody > --- > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 6b2d88759d..6fa5e1d71a 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -4569,7 +4569,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool > fatal, int64_t offset, > char *message; > va_list ap; > > -fatal = fatal && !bs->read_only; > +fatal = fatal && bdrv_is_writable(bs); > > if (s->signaled_corruption && > (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT))) > -- > 2.17.0 >
Re: [Qemu-devel] [PATCH 04/11] ppc/pnv: Add trailing '\n' to qemu_log() calls
On 06/06/2018 11:16 PM, David Gibson wrote:
> On Wed, Jun 06, 2018 at 12:21:21PM -0300, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé
>
> I'm not really sure what series this was part of, but I've applied it
> to ppc-for-3.0 anyway.
I did an effort to split the series, then add all the recipients to the
cover but I missed your mail indeed, sorry!
here is the cover:
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01266.html
Thanks!
>
> Acked-by: David Gibson
>
>> ---
>> hw/ppc/pnv_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index cbb64ad9e7..13ad7d9e04 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -97,7 +97,7 @@ static uint64_t pnv_core_xscom_read(void *opaque, hwaddr
>> addr,
>> val = 0x24full;
>> break;
>> default:
>> -qemu_log_mask(LOG_UNIMP, "Warning: reading reg=0x%" HWADDR_PRIx,
>> +qemu_log_mask(LOG_UNIMP, "Warning: reading reg=0x%" HWADDR_PRIx
>> "\n",
>>addr);
>> }
>>
>> @@ -107,7 +107,7 @@ static uint64_t pnv_core_xscom_read(void *opaque, hwaddr
>> addr,
>> static void pnv_core_xscom_write(void *opaque, hwaddr addr, uint64_t val,
>> unsigned int width)
>> {
>> -qemu_log_mask(LOG_UNIMP, "Warning: writing to reg=0x%" HWADDR_PRIx,
>> +qemu_log_mask(LOG_UNIMP, "Warning: writing to reg=0x%" HWADDR_PRIx "\n",
>>addr);
>> }
>>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 04/11] ppc/pnv: Add trailing '\n' to qemu_log() calls
On Wed, Jun 06, 2018 at 12:21:21PM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
I'm not really sure what series this was part of, but I've applied it
to ppc-for-3.0 anyway.
Acked-by: David Gibson
> ---
> hw/ppc/pnv_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index cbb64ad9e7..13ad7d9e04 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -97,7 +97,7 @@ static uint64_t pnv_core_xscom_read(void *opaque, hwaddr
> addr,
> val = 0x24full;
> break;
> default:
> -qemu_log_mask(LOG_UNIMP, "Warning: reading reg=0x%" HWADDR_PRIx,
> +qemu_log_mask(LOG_UNIMP, "Warning: reading reg=0x%" HWADDR_PRIx "\n",
>addr);
> }
>
> @@ -107,7 +107,7 @@ static uint64_t pnv_core_xscom_read(void *opaque, hwaddr
> addr,
> static void pnv_core_xscom_write(void *opaque, hwaddr addr, uint64_t val,
> unsigned int width)
> {
> -qemu_log_mask(LOG_UNIMP, "Warning: writing to reg=0x%" HWADDR_PRIx,
> +qemu_log_mask(LOG_UNIMP, "Warning: writing to reg=0x%" HWADDR_PRIx "\n",
>addr);
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 0/3] mac_newworld: add MAINTAINERS entries, fix qemu_log trailing '\n'
On Wed, Jun 06, 2018 at 11:59:18AM -0300, Philippe Mathieu-Daudé wrote: > Nothing very exciting here. > I sometimes miss to notice some trace events, running with -d unimp,trace... > then using 'grep ^...'. This is only due to a missing '\n' :) Applied to ppc-for-3.0, thanks. > > Philippe Mathieu-Daudé (3): > MAINTAINERS: Add an entry for the MacIO device headers > MAINTAINERS: Add entries for the MOS6522 VIA device > hw/misc/mos6522: Add trailing '\n' to qemu_log() calls > > hw/misc/mos6522.c | 4 ++-- > MAINTAINERS | 5 - > 2 files changed, 6 insertions(+), 3 deletions(-) > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[Qemu-devel] Enable the Multisampling with virglrenderer using SDL
Hi, This is Wonsang Ryu. I'm developing the open embedded OS using QEMU with virglrenderer. Our OS is very light, so it doesn't support multi resolution. Our OS supports only 1920x1080. So, we want to resize to 1280x720 the QEMU window for developers. If QEMU window size is 1920x1080, it is clear. But, in small window size, it does not clear. So, we want to use multisampling(anti-aliasing) in QEMU. I checked the virglrenderer has multisampling feature in code, but QEMU does not use it. Does multisampling support in QEMU with virglrenderer? How can I enable the multisampling feature of GLES in QEMU? Please help or advise me. Thanks.
Re: [Qemu-devel] [PATCH] Improve file-backed RAM
Hi, This series failed build test on s390x host. Please find the details below. Type: series Message-id: 20180606181352.61144-1-...@google.com Subject: [Qemu-devel] [PATCH] Improve file-backed RAM === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch set -e echo "=== ENV ===" env echo "=== PACKAGES ===" rpm -qa echo "=== TEST BEGIN ===" CC=$HOME/bin/cc INSTALL=$PWD/install BUILD=$PWD/build echo -n "Using CC: " realpath $CC mkdir -p $BUILD $INSTALL SRC=$PWD cd $BUILD $SRC/configure --cc=$CC --prefix=$INSTALL make -j4 # XXX: we need reliable clean up # make check -j4 V=1 make install === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 11c09c2651 Improve file-backed RAM === OUTPUT BEGIN === === ENV === LANG=en_US.UTF-8 XDG_SESSION_ID=216587 USER=fam PWD=/var/tmp/patchew-tester-tmp-opnjqdtz/src HOME=/home/fam SHELL=/bin/sh SHLVL=2 PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug LOGNAME=fam DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus XDG_RUNTIME_DIR=/run/user/1012 PATH=/usr/bin:/bin _=/usr/bin/env === PACKAGES === gpg-pubkey-873529b8-54e386ff glibc-debuginfo-common-2.24-10.fc25.s390x fedora-release-26-1.noarch dejavu-sans-mono-fonts-2.35-4.fc26.noarch xemacs-filesystem-21.5.34-22.20170124hgf412e9f093d4.fc26.noarch bash-4.4.12-7.fc26.s390x libSM-1.2.2-5.fc26.s390x libmpc-1.0.2-6.fc26.s390x libaio-0.3.110-7.fc26.s390x libverto-0.2.6-7.fc26.s390x perl-Scalar-List-Utils-1.48-1.fc26.s390x iptables-libs-1.6.1-2.fc26.s390x tcl-8.6.6-2.fc26.s390x libxshmfence-1.2-4.fc26.s390x expect-5.45-23.fc26.s390x perl-Thread-Queue-3.12-1.fc26.noarch perl-encoding-2.19-6.fc26.s390x keyutils-1.5.10-1.fc26.s390x gmp-devel-6.1.2-4.fc26.s390x enchant-1.6.0-16.fc26.s390x python-gobject-base-3.24.1-1.fc26.s390x python3-enchant-1.6.10-1.fc26.noarch python-lockfile-0.11.0-6.fc26.noarch python2-pyparsing-2.1.10-3.fc26.noarch python2-lxml-4.1.1-1.fc26.s390x librados2-10.2.7-2.fc26.s390x trousers-lib-0.3.13-7.fc26.s390x libdatrie-0.2.9-4.fc26.s390x libsoup-2.58.2-1.fc26.s390x passwd-0.79-9.fc26.s390x bind99-libs-9.9.10-3.P3.fc26.s390x python3-rpm-4.13.0.2-1.fc26.s390x systemd-233-7.fc26.s390x virglrenderer-0.6.0-1.20170210git76b3da97b.fc26.s390x s390utils-ziomon-1.36.1-3.fc26.s390x s390utils-osasnmpd-1.36.1-3.fc26.s390x libXrandr-1.5.1-2.fc26.s390x libglvnd-glx-1.0.0-1.fc26.s390x texlive-ifxetex-svn19685.0.5-33.fc26.2.noarch texlive-psnfss-svn33946.9.2a-33.fc26.2.noarch texlive-dvipdfmx-def-svn40328-33.fc26.2.noarch texlive-natbib-svn20668.8.31b-33.fc26.2.noarch texlive-xdvi-bin-svn40750-33.20160520.fc26.2.s390x texlive-cm-svn32865.0-33.fc26.2.noarch texlive-beton-svn15878.0-33.fc26.2.noarch texlive-fpl-svn15878.1.002-33.fc26.2.noarch texlive-mflogo-svn38628-33.fc26.2.noarch texlive-texlive-docindex-svn41430-33.fc26.2.noarch texlive-luaotfload-bin-svn34647.0-33.20160520.fc26.2.noarch texlive-koma-script-svn41508-33.fc26.2.noarch texlive-pst-tree-svn24142.1.12-33.fc26.2.noarch texlive-breqn-svn38099.0.98d-33.fc26.2.noarch texlive-xetex-svn41438-33.fc26.2.noarch gstreamer1-plugins-bad-free-1.12.3-1.fc26.s390x xorg-x11-font-utils-7.5-33.fc26.s390x ghostscript-fonts-5.50-36.fc26.noarch libXext-devel-1.3.3-5.fc26.s390x libusbx-devel-1.0.21-2.fc26.s390x libglvnd-devel-1.0.0-1.fc26.s390x emacs-25.3-3.fc26.s390x alsa-lib-devel-1.1.4.1-1.fc26.s390x kbd-2.0.4-2.fc26.s390x dconf-0.26.0-2.fc26.s390x mc-4.8.19-5.fc26.s390x doxygen-1.8.13-9.fc26.s390x dpkg-1.18.24-1.fc26.s390x libtdb-1.3.13-1.fc26.s390x python2-pynacl-1.1.1-1.fc26.s390x perl-Filter-1.58-1.fc26.s390x python2-pip-9.0.1-11.fc26.noarch dnf-2.7.5-2.fc26.noarch bind-license-9.11.2-1.P1.fc26.noarch libtasn1-4.13-1.fc26.s390x cpp-7.3.1-2.fc26.s390x pkgconf-1.3.12-2.fc26.s390x python2-fedora-0.10.0-1.fc26.noarch cmake-filesystem-3.10.1-11.fc26.s390x python3-requests-kerberos-0.12.0-1.fc26.noarch libmicrohttpd-0.9.59-1.fc26.s390x GeoIP-GeoLite-data-2018.01-1.fc26.noarch python2-libs-2.7.14-7.fc26.s390x libidn2-2.0.4-3.fc26.s390x p11-kit-devel-0.23.10-1.fc26.s390x perl-Errno-1.25-396.fc26.s390x libdrm-2.4.90-2.fc26.s390x sssd-common-1.16.1-1.fc26.s390x boost-random-1.63.0-11.fc26.s390x urw-fonts-2.4-24.fc26.noarch ccache-3.3.6-1.fc26.s390x glibc-debuginfo-2.24-10.fc25.s390x dejavu-fonts-common-2.35-4.fc26.noarch bind99-license-9.9.10-3.P3.fc26.noarch ncurses-libs-6.0-8.20170212.fc26.s390x libpng-1.6.28-2.fc26.s390x libICE-1.0.9-9.fc26.s390x perl-Text-ParseWords-3.30-366.fc26.noarch libtool-ltdl-2.4.6-17.fc26.s390x libselinux-utils-2.6-7.fc26.s390x userspace-rcu-0.9.3-2.fc26.s390x perl-Class-Inspector-1.31-3.fc26.noarch keyutils-libs-devel-1.5.10-1.fc26.s390x isl-0.16.1-1.fc26.s390x libsecret-0.18.5-3.fc26.s390x compat-openssl10-1.0.2m-1.fc26.s390x python3-iniparse-0.4-24.fc26.noarch python3-dateutil-2.6.0-3.fc26.noarch python3-firewall-0.4.4.5-1.fc26.noarch
Re: [Qemu-devel] [PATCH] Improve file-backed RAM
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180606181352.61144-1-...@google.com Subject: [Qemu-devel] [PATCH] Improve file-backed RAM === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 11c09c2651 Improve file-backed RAM === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-y32_9olx/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-y32_9olx/src' GEN /var/tmp/patchew-tester-tmp-y32_9olx/src/docker-src.2018-06-06-17.05.57.12083/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-y32_9olx/src/docker-src.2018-06-06-17.05.57.12083/qemu.tar.vroot'... done. Checking out files: 48% (3002/6210) Checking out files: 49% (3043/6210) Checking out files: 50% (3105/6210) Checking out files: 51% (3168/6210) Checking out files: 52% (3230/6210) Checking out files: 53% (3292/6210) Checking out files: 54% (3354/6210) Checking out files: 55% (3416/6210) Checking out files: 56% (3478/6210) Checking out files: 57% (3540/6210) Checking out files: 58% (3602/6210) Checking out files: 59% (3664/6210) Checking out files: 60% (3726/6210) Checking out files: 61% (3789/6210) Checking out files: 62% (3851/6210) Checking out files: 63% (3913/6210) Checking out files: 64% (3975/6210) Checking out files: 65% (4037/6210) Checking out files: 66% (4099/6210) Checking out files: 67% (4161/6210) Checking out files: 68% (4223/6210) Checking out files: 69% (4285/6210) Checking out files: 70% (4347/6210) Checking out files: 71% (4410/6210) Checking out files: 72% (4472/6210) Checking out files: 73% (4534/6210) Checking out files: 74% (4596/6210) Checking out files: 75% (4658/6210) Checking out files: 76% (4720/6210) Checking out files: 77% (4782/6210) Checking out files: 78% (4844/6210) Checking out files: 79% (4906/6210) Checking out files: 80% (4968/6210) Checking out files: 81% (5031/6210) Checking out files: 82% (5093/6210) Checking out files: 83% (5155/6210) Checking out files: 84% (5217/6210) Checking out files: 85% (5279/6210) Checking out files: 86% (5341/6210) Checking out files: 87% (5403/6210) Checking out files: 88% (5465/6210) Checking out files: 89% (5527/6210) Checking out files: 90% (5589/6210) Checking out files: 91% (5652/6210) Checking out files: 92% (5714/6210) Checking out files: 93% (5776/6210) Checking out files: 94% (5838/6210) Checking out files: 95% (5900/6210) Checking out files: 96% (5962/6210) Checking out files: 97% (6024/6210) Checking out files: 98% (6086/6210) Checking out files: 99% (6148/6210) Checking out files: 100% (6210/6210) Checking out files: 100% (6210/6210), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-y32_9olx/src/docker-src.2018-06-06-17.05.57.12083/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-y32_9olx/src/docker-src.2018-06-06-17.05.57.12083/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-mingw in qemu:fedora Packages installed: SDL2-devel-2.0.8-5.fc28.x86_64 bc-1.07.1-5.fc28.x86_64 bison-3.0.4-9.fc28.x86_64 bluez-libs-devel-5.49-3.fc28.x86_64 brlapi-devel-0.6.7-12.fc28.x86_64 bzip2-1.0.6-26.fc28.x86_64 bzip2-devel-1.0.6-26.fc28.x86_64 ccache-3.4.2-2.fc28.x86_64 clang-6.0.0-5.fc28.x86_64 device-mapper-multipath-devel-0.7.4-2.git07e7bd5.fc28.x86_64 findutils-4.6.0-19.fc28.x86_64 flex-2.6.1-7.fc28.x86_64 gcc-8.1.1-1.fc28.x86_64 gcc-c++-8.1.1-1.fc28.x86_64 gettext-0.19.8.1-14.fc28.x86_64 git-2.17.1-2.fc28.x86_64 glib2-devel-2.56.1-3.fc28.x86_64 glusterfs-api-devel-4.0.2-1.fc28.x86_64 gnutls-devel-3.6.2-1.fc28.x86_64 gtk3-devel-3.22.30-1.fc28.x86_64 hostname-3.20-3.fc28.x86_64 libaio-devel-0.3.110-11.fc28.x86_64 libasan-8.1.1-1.fc28.x86_64 libattr-devel-2.4.47-23.fc28.x86_64 libcap-devel-2.25-9.fc28.x86_64 libcap-ng-devel-0.7.9-1.fc28.x86_64 libcurl-devel-7.59.0-3.fc28.x86_64 libfdt-devel-1.4.6-4.fc28.x86_64 libpng-devel-1.6.34-3.fc28.x86_64
Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory) wrote: > >> > Okay, we can move to the symbolic names. Do you want them to be that >> long, or >> > would: >> > >> > nvdimm-cap-cpu >> > nvdimm-cap-mem-ctrl >> > nvdimm-cap-mirroring >> >> Wait, why is mirroring part of this? > > This data structure is intended to report any kind of platform capability, not > just platform persistence capabilities. Yes, but here's nothing actionable that a qemu guest OS can do with that mirroring information, so there's no need at this time to add cli cruft and code to support it.
Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
> > Okay, we can move to the symbolic names. Do you want them to be that > long, or > > would: > > > > nvdimm-cap-cpu > > nvdimm-cap-mem-ctrl > > nvdimm-cap-mirroring > > Wait, why is mirroring part of this? This data structure is intended to report any kind of platform capability, not just platform persistence capabilities. We could add a short symbolic name to the definitions in ACPI that matches the ones selected for this command line option, if that'll help people find the right names to use. I recommend mc rather than mem-ctrl to keep dashes as special.
Re: [Qemu-devel] [RFC PATCH] configure: Enable out-of-tree acceptance tests
On 06/06/2018 05:23 PM, Cleber Rosa wrote: > On 06/06/2018 04:11 PM, Eduardo Habkost wrote: >> On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote: >>> On 06/06/2018 04:24 PM, Eduardo Habkost wrote: On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote: [... something about config files ...] >> And after that, the following would run all "console" tests: >> >> avocado run -t console >> >> How does this sound? > > For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about > use_test_dir_when_no_references_given. Well, I can't give an opinion because I couldn't understand what's the final goal here. >>> >>> You cut too much, the relevant part is: >>> >>> On 05/30/2018 10:06 PM, Cleber Rosa wrote: >>> > But at the same, there are security implications: `list` won't >>> > load/execute any test code (different from, say, standard Python >>> > unittests), while "run" obviously will. So "avocado run" may end up >>> > running what users don't want if a malicious user controls >>> > "$avocado_datadir_paths_test_dir". >> >> The final goal still isn't clear to me. Why would somebody want >> to use $avocado_datadir_paths_test_dir instead of just specifying >> "." in the command-line? Oh I guess I misunderstood your first review, I'll just improve the commit message and repost. >> >> >>> What exactly is missing in the current solution? Why "avocado run -t console ." wouldn't work? >>> >>> It doesn't work in out-of-tree builds, I have to use the full path: >>> >>> >> build_dir$ avocado run >>> /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py >> >> Isn't the symlink you suggested a better solution than requiring >> the user to edit a config file? >> > > It's definitely the simplest, and would avoid a questionable knob and > behavior in Avocado. :)
Re: [Qemu-devel] [RFC PATCH v2] qmp.py: Fix exception parsing partial JSON
On 06/06/2018 05:05 PM, Eduardo Habkost wrote:
> On Wed, Jun 06, 2018 at 04:27:31PM -0300, Philippe Mathieu-Daudé wrote:
>> The readline() call returns partial data.
>
> How can this be reproduced? Despite not being forbidden by the
> QMP specification, QEMU normally doesn't break QMP replies in
> multiple lines, and readline() is not supposed to return a
> partial line unless it encounters EOF.
$ git rev-parse HEAD
c1c2a435905ae76b159c573b0c0d6f095b45ebc6
config copy/pasted from:
https://wiki.qemu.org/index.php/Documentation/QMP#Trying_it
(now looking at it, it seems I'm mixing configs...)
$ cat qmp.conf
[chardev "qmp"]
backend = "socket"
path = "/tmp/qmp.sock"
server = "on"
wait = "off"
[mon "qmp"]
mode = "control"
chardev = "qmp"
pretty = "on"
$ arm-softmmu/qemu-system-arm -M lm3s6965evb -kernel /dev/zero \
-readconfig qmp.conf -S
>
>
>> Keep appending until the JSON buffer is complete.
>>
>> This fixes:
>>
>> $ scripts/qmp/qmp-shell -v -p /tmp/qmp.sock
>> Traceback (most recent call last):
>> File "scripts/qmp/qmp-shell", line 456, in
>> main()
>> File "scripts/qmp/qmp-shell", line 441, in main
>> qemu.connect(negotiate)
>> File "scripts/qmp/qmp-shell", line 284, in connect
>> self._greeting = super(QMPShell, self).connect(negotiate)
>> File "scripts/qmp/qmp.py", line 143, in connect
>> return self.__negotiate_capabilities()
>> File "scripts/qmp/qmp.py", line 71, in __negotiate_capabilities
>> greeting = self.__json_read()
>> File "scripts/qmp/qmp.py", line 85, in __json_read
>> resp = json.loads(data)
>> File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
>> return _default_decoder.decode(s)
>> File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
>> obj, end = self.raw_decode(s, idx=_w(s, 0).end())
>> File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
>> obj, end = self.scan_once(s, idx)
>> ValueError: Expecting object: line 1 column 3 (char 2)
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> Since v1:
>> - addressed Daniel review: clean data after json.loads() succeeds
>> - add a XXX comment
>>
>> Daniel suggested this is due to blocking i/o.
>>
>> I'm sure there is a nicer/more pythonic way to do this, but this works for
>> me,
>> sorry :)
>
> It looks like there's no elegant solution for this:
> https://stackoverflow.com/a/21709058
>
>>
>> scripts/qmp/qmp.py | 13 ++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index 5c8cf6a056..14f0b48936 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -78,11 +78,18 @@ class QEMUMonitorProtocol(object):
>> raise QMPCapabilitiesError
>>
>> def __json_read(self, only_event=False):
>> +data = ""
>> while True:
>> -data = self.__sockfile.readline()
>> -if not data:
>> +tmp = self.__sockfile.readline()
>> +if not tmp:
>> return
>> -resp = json.loads(data)
>> +data += tmp
>> +try:
>> +resp = json.loads(data)
>> +except ValueError:
>> +# XXX: blindly loop, even if QEMU ever sends malformed data
>> +continue
>
> I was going to suggest using json.JSONDecoder.raw_decode() and
> saving the remaining data in case we already read partial data
> for a second JSON message.
>
> But the QMP specification says all messages are terminated with
> CRLF, so we we should never see the data for two different
> messages in a single readline() call. Noting this in a comment
> wouldn't hurt, though.
>
> The patch seems reasonable, but first I would like to understand
> how this bug can be triggered.
>
>> +data = ""
>> if 'event' in resp:
>> self.logger.debug("<<< %s", resp)
>> self.__events.append(resp)
>> --
>> 2.17.1
>>
>
[Qemu-devel] [PATCH 0/2] jobs: minor doc fixups
One is from my manual completion series, the other is from the recent refactor. John Snow (2): jobs: fix stale wording jobs: fix verb references in docs qapi/job.json | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH 1/2] jobs: fix stale wording
During the design for manual completion, we decided not to use the "manual" property as a shorthand for both auto-dismiss and auto-finalize. Fix the wording. Signed-off-by: John Snow --- qapi/job.json | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/qapi/job.json b/qapi/job.json index 17d10037c4..226443594b 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -50,16 +50,17 @@ # the last job in a transaction. # # @pending: The job has finished its work, but has finalization steps that it -# needs to make prior to completing. These changes may require -# manual intervention by the management process if manual was set -# to true. These changes may still fail. +# needs to make prior to completing. These changes will require +# manual intervention via @job-finalize if auto-finalize was set to +# false. These pending changes may still fail. # # @aborting: The job is in the process of being aborted, and will finish with #an error. The job will afterwards report that it is @concluded. #This status may not be visible to the management process. # -# @concluded: The job has finished all work. If manual was set to true, the job -# will remain in the query list until it is dismissed. +# @concluded: The job has finished all work. If auto-dismiss was set to false, +# the job will remain in the query list until it is dismissed via +# @job-dismiss. # # @null: The job is in the process of being dismantled. This state should not #ever be visible externally. -- 2.14.3
[Qemu-devel] [PATCH 2/2] jobs: fix verb references in docs
These point to the job versions now, not the blockjob versions which don't really exist anymore. Except set-speed, which does. It sticks out like a sore thumb. This patch doesn't fix that, but it doesn't make it any worse, either. Signed-off-by: John Snow --- qapi/job.json | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qapi/job.json b/qapi/job.json index 226443594b..9d074eb8d2 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -76,19 +76,19 @@ # # Represents command verbs that can be applied to a job. # -# @cancel: see @block-job-cancel +# @cancel: see @job-cancel # -# @pause: see @block-job-pause +# @pause: see @job-pause # -# @resume: see @block-job-resume +# @resume: see @job-resume # # @set-speed: see @block-job-set-speed # -# @complete: see @block-job-complete +# @complete: see @job-complete # -# @dismiss: see @block-job-dismiss +# @dismiss: see @job-dismiss # -# @finalize: see @block-job-finalize +# @finalize: see @job-finalize # # Since: 2.12 ## -- 2.14.3
Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC CPU
On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x801E.
>
> Disable TOPOEXT feature for legacy machines.
>
> Signed-off-by: Babu Moger
Now, I just noticed we have a problem here:
"-machine pc -cpu EPYC -smp 64" works today
This patch makes it stop working, but it shouldn't.
On the other hand, I believe you expect:
* "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
* "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
* What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
We also have other requirements, I will try to enumerate all of
them below:
0) "-topoext" explicitly configured (any machine-type):
* Must never enable topoext.
1) "+topoext" explicitly configured (any machine-type):
* Must validate topology and refuse to start if unsupported.
2) Older machine-types:
* Must never enable topoext automatically, even if using "EPYC"
or "threads=2"
3) "EPYC" CPU model (on new machine-types):
* Should enable topoext automatically, but only if topology is
supported.
* Must not error out if topology is not supported.
* Should this enable topoext automatically even if threads=1?
4) Other AMD CPU models with "threads=2" (on new machine-types):
* We might want to make this enable topoext automatically, too.
What do you think?
Is the above description accurate? Do you agree with these
requirements?
We're trying to use the "topoext" property to cover all cases
above, but it looks like we need at least 2 bits to represent all
possible cases.
Maybe we can represent the cases above with two properties:
"topoext" and "auto-topoext". Then each case would be
represented by:
0) "-topoext" explicitly configured (any machine-type):
* Will clear TOPOEXT on env->features and set TOPOEXT on
env->user_features
(already done today)
1) "+topoext" explicitly configured (any machine-type):
* Will set TOPOEXT on both env->user_features and env->features
(already done today)
2) Older machine-types:
* Will set auto-topoext=off (can be done on compat_props)
* Will set topoext=off on EPYC CPU model (so TOPOEXT won't be set
by default on env->features) (can be done on compat_props)
3) "EPYC" CPU model (on new machine-types):
* Will set auto-topoext=on (can be the default for all CPU
models)
* Will set TOPOEXT on env->features) (can be done on CPU model table)
4) Other AMD CPU models with "threads=2" (on new machine-types):
* Will set auto-topoext=on (can be the default on all CPU models)
* Will keep TOPOEXT disabled on env->features (done on the CPU
model table)
Then the rules would be:
if {auto_topoext && TOPOEXT not in env->user_features) {
if (supported_topology) {
if (threads > 1)
set TOPOEXT in env->features
} else
unset TOPOEXT in env->features
}
if (TOPOEXT in env->features && !supported_topology)
error;
}
I think this would fulfill all the requirements above. Please
help me confirm that.
--
Eduardo
[Qemu-devel] [Bug 1775366] Re: [Feature request] qemu-ga - Allow unexpected parameter
I see you point. Just close this issue.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1775366
Title:
[Feature request] qemu-ga - Allow unexpected parameter
Status in QEMU:
New
Bug description:
It whould be nice if the qemu-ga allowed received messages to contain
fields which is not part of the spec. In my example I have a host
which sends the following request:
{"execute":"guest-exec","arguments":{"path":"prl_nettool","capture-
output":true,"execute-in-shell":false,"arg":[...]}}
Right now this request is rejected with the following error:
{"error": {"class": "GenericError", "desc": "Parameter 'execute-in-
shell' is unexpected"}}
My situation is the hosting provider I use does have some customized
solution which sends some extra arguments. I have manually patched my
qemu-ga so it accepts the "execute-in-shell" parameter but I don't
think this should be necessary.
Instead of "Error" it should just be a "warning" returned to the user
of qemu-ga but the call should still be executed.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1775366/+subscriptions
Re: [Qemu-devel] [PATCH v12 2/4] i386: Verify if topoext feature can be supported
On Wed, Jun 06, 2018 at 10:36:44AM -0400, Babu Moger wrote:
> topoext feature cannot be supported in certain cases
> with large number of cores or threads. Add the check.
>
> Signed-off-by: Babu Moger
> ---
> target/i386/cpu.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..fc5c66d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -509,6 +509,20 @@ static void encode_topo_cpuid801e(CPUState *cs,
> X86CPU *cpu,
> }
>
> /*
> + * Check if we can support this topology
> + * Fail if number of cores are beyond the supported config
> + * or nr_threads is more than 2
> + */
> +static int topology_supports_topoext(int nr_cores, int nr_threads)
> +{
> +if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
> +(nr_threads > 2)) {
> +return 0;
> +}
> +return 1;
> +}
> +
> +/*
> * Definitions of the hardcoded cache entries we expose:
> * These are legacy cache values. If there is a need to change any
> * of these values please use builtin_x86_defs
> @@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
>
> qemu_init_vcpu(cs);
>
> +/* On AMD systems, check if we can support topoext feature */
> +if (IS_AMD_CPU(env) &&
> +(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> +if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> +/* Cannot support topoext */
> +error_setg(errp, "CPU model does not support topoext feature
> "
> + "with number of cores(%d) and threads(%d). "
> + "Please configure -smp options properly.",
> + cs->nr_cores, cs->nr_threads);
See error.h documentation:
* Error reporting system loosely patterned after Glib's GError.
*
* Create an error:
* error_setg(, "situation normal, all fouled up");
*
* Create an error and add additional explanation:
* error_setg(, "invalid quark");
* error_append_hint(, "Valid quarks are up, down, strange, "
* "charm, top, bottom.\n");
*
* Do *not* contract this to
* error_setg(, "invalid quark\n"
*"Valid quarks are up, down, strange, charm, top, bottom.");
*
I suggest something like this:
static bool topology_supports_topoext(int nr_cores, int nr_threads, Error
**errp)
{
if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))) {
error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
nr_cores);
error_append_hint(errp, "TOPOEXT supports only up to %d cores per
socket",
(MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
return false;
}
if (nr_threads > 2) {
error_setg(errp, "TOPOEXT unsupported with %d threads per core",
nr_threads);
error_append_hint(errp, "TOPOEXT supports only up to 2 threads per
core");
(MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
return false;
}
return true;
}
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
{
/* ... */
if (IS_AMD_CPU(env) &&
(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
!topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
return;
}
/* ... */
}
--
Eduardo
Re: [Qemu-devel] [PULL v2 7/8] hw: xilinx-pcie: Add support for Xilinx AXI PCIe Controller
On 6 June 2018 at 21:23, Paul Burton wrote: > Hi Peter, > > On Mon, Jun 04, 2018 at 11:29:47AM +0100, Peter Maydell wrote: >> Hi; I was looking at the code for this PCI controller as >> part of identifying RAM memory regions that don't migrate >> their contents, and this looks kind of odd. Can you explain >> why this is using a RAM region for the PCI bus's IO memory >> space? None of our other PCI controller models do this, so >> I suspect this is a bug. (The effect would be that PCI IO >> accesses to addresses which don't have a PCI BAR mapped there >> will behave as reads-as-written, which seems pretty unlikely >> to be what the hardware does.) >> >> What is the intended semantics for PCI IO accesses with this >> controller? (It seems to only be used with the MIPS boston >> board model.) > The controller here simply doesn't support I/O accesses, and the dummy > region is there to prevent the rest of QEMU's PCI code & device > emulation from needing to care about that fact. The region is never > mapped into the system memory and so software running on the emulated > system has no way to access it. Right, I wondered if that was the case. The region should not be RAM, then, it should be a dummy does-nothing region. I'll put a patch together at some point. thanks -- PMM
Re: [Qemu-devel] [PATCH v12 1/4] i386: Add support for CPUID_8000_001E for AMD
On Wed, Jun 06, 2018 at 10:36:43AM -0400, Babu Moger wrote: [...] > +/* > + * CPUID_Fn801E_EBX > + * 31:16 Reserved > + * 15:8 Threads per core (The number of threads per core is > + * Threads per core + 1) > + * 7:0 Core id (see bit decoding below) > + * SMT: > + * 4:3 node id > + * 2 Core complex id > + * 1:0 Core id > + * Non SMT: > + * 5:4 node id > + * 3 Core complex id > + * 1:0 Core id > + */ Where are those bit offsets documented? AMD Family 17h PPR just says "7:0 Core ID". -- Eduardo
Re: [Qemu-devel] [PATCH] Improve file-backed RAM
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180606205629.66987-1-...@google.com Subject: [Qemu-devel] [PATCH] Improve file-backed RAM === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 999270755b Improve file-backed RAM === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-w2qg86b7/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-w2qg86b7/src' GEN /var/tmp/patchew-tester-tmp-w2qg86b7/src/docker-src.2018-06-06-17.01.32.4789/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-w2qg86b7/src/docker-src.2018-06-06-17.01.32.4789/qemu.tar.vroot'... done. Checking out files: 45% (2855/6210) Checking out files: 46% (2857/6210) Checking out files: 47% (2919/6210) Checking out files: 48% (2981/6210) Checking out files: 49% (3043/6210) Checking out files: 50% (3105/6210) Checking out files: 51% (3168/6210) Checking out files: 52% (3230/6210) Checking out files: 53% (3292/6210) Checking out files: 54% (3354/6210) Checking out files: 55% (3416/6210) Checking out files: 56% (3478/6210) Checking out files: 57% (3540/6210) Checking out files: 58% (3602/6210) Checking out files: 59% (3664/6210) Checking out files: 60% (3726/6210) Checking out files: 61% (3789/6210) Checking out files: 62% (3851/6210) Checking out files: 63% (3913/6210) Checking out files: 64% (3975/6210) Checking out files: 65% (4037/6210) Checking out files: 66% (4099/6210) Checking out files: 67% (4161/6210) Checking out files: 68% (4223/6210) Checking out files: 69% (4285/6210) Checking out files: 70% (4347/6210) Checking out files: 71% (4410/6210) Checking out files: 72% (4472/6210) Checking out files: 73% (4534/6210) Checking out files: 74% (4596/6210) Checking out files: 75% (4658/6210) Checking out files: 76% (4720/6210) Checking out files: 77% (4782/6210) Checking out files: 78% (4844/6210) Checking out files: 79% (4906/6210) Checking out files: 80% (4968/6210) Checking out files: 81% (5031/6210) Checking out files: 82% (5093/6210) Checking out files: 83% (5155/6210) Checking out files: 84% (5217/6210) Checking out files: 85% (5279/6210) Checking out files: 86% (5341/6210) Checking out files: 87% (5403/6210) Checking out files: 88% (5465/6210) Checking out files: 89% (5527/6210) Checking out files: 90% (5589/6210) Checking out files: 91% (5652/6210) Checking out files: 92% (5714/6210) Checking out files: 93% (5776/6210) Checking out files: 94% (5838/6210) Checking out files: 95% (5900/6210) Checking out files: 96% (5962/6210) Checking out files: 97% (6024/6210) Checking out files: 98% (6086/6210) Checking out files: 99% (6148/6210) Checking out files: 100% (6210/6210) Checking out files: 100% (6210/6210), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-w2qg86b7/src/docker-src.2018-06-06-17.01.32.4789/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-w2qg86b7/src/docker-src.2018-06-06-17.01.32.4789/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-mingw in qemu:fedora Packages installed: SDL2-devel-2.0.8-5.fc28.x86_64 bc-1.07.1-5.fc28.x86_64 bison-3.0.4-9.fc28.x86_64 bluez-libs-devel-5.49-3.fc28.x86_64 brlapi-devel-0.6.7-12.fc28.x86_64 bzip2-1.0.6-26.fc28.x86_64 bzip2-devel-1.0.6-26.fc28.x86_64 ccache-3.4.2-2.fc28.x86_64 clang-6.0.0-5.fc28.x86_64 device-mapper-multipath-devel-0.7.4-2.git07e7bd5.fc28.x86_64 findutils-4.6.0-19.fc28.x86_64 flex-2.6.1-7.fc28.x86_64 gcc-8.1.1-1.fc28.x86_64 gcc-c++-8.1.1-1.fc28.x86_64 gettext-0.19.8.1-14.fc28.x86_64 git-2.17.1-2.fc28.x86_64 glib2-devel-2.56.1-3.fc28.x86_64 glusterfs-api-devel-4.0.2-1.fc28.x86_64 gnutls-devel-3.6.2-1.fc28.x86_64 gtk3-devel-3.22.30-1.fc28.x86_64 hostname-3.20-3.fc28.x86_64 libaio-devel-0.3.110-11.fc28.x86_64 libasan-8.1.1-1.fc28.x86_64 libattr-devel-2.4.47-23.fc28.x86_64 libcap-devel-2.25-9.fc28.x86_64 libcap-ng-devel-0.7.9-1.fc28.x86_64
Re: [Qemu-devel] storing machine data in qcow images?
Hi, > our actual qcow2v3 improvements, so no one ended up switching to that. So, > to some extent, various high-level consumers still have the notion that > 'raw' files are better/safer/faster than 'qcow2' files because of an > anecdote from years ago, even if we have since fixed the speed parity and > added locking to eliminate careless data loss. When I use raw images the reasons are different ones. Most of the time it is that I want use the image with something which isn't qemu and thus doesn't understand qcow2. Booting a image as container for example (systemd-nspawn -i $image.raw). cheers, Gerd
Re: [Qemu-devel] [PATCH] Improve file-backed RAM
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180606181352.61144-1-...@google.com Subject: [Qemu-devel] [PATCH] Improve file-backed RAM === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 11c09c2651 Improve file-backed RAM === OUTPUT BEGIN === Checking PATCH 1/1: Improve file-backed RAM... ERROR: do not initialise globals to 0 or NULL #300: FILE: vl.c:144: +int mem_file_shared = 0; /* map file-backed RAM in shared mode */ total: 1 errors, 0 warnings, 230 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH] Improve file-backed RAM
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180606205629.66987-1-...@google.com Subject: [Qemu-devel] [PATCH] Improve file-backed RAM === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20180606181352.61144-1-...@google.com -> patchew/20180606181352.61144-1-...@google.com * [new tag] patchew/20180606205629.66987-1-...@google.com -> patchew/20180606205629.66987-1-...@google.com Switched to a new branch 'test' 999270755b Improve file-backed RAM === OUTPUT BEGIN === Checking PATCH 1/1: Improve file-backed RAM... ERROR: do not initialise globals to 0 or NULL #300: FILE: vl.c:144: +int mem_file_shared = 0; /* map file-backed RAM in shared mode */ total: 1 errors, 0 warnings, 230 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[Qemu-devel] [PATCH] Improve file-backed RAM
1. Add support for all platforms
2. Add option to map in shared mode, allowing the guest to write
through to the backing file
Taken together, this allows one to write RAM snapshots as the guest is
running. Saving RAM snapshots is then equivalent to exiting the qemu
process or unmapping the file. This can be faster than waiting for a
lengthy explicit migration process.
Eventually, we want to go in the direction of allowing the launch of
multiple guest instances from the same RAM snapshot, which aids
virtualization-based integration testing; boot and other initializations
for multiple guest instances can be skipped, and the host OS will have
optimized shared RAM usage using its existing copy-on-write mechanisms.
Cc: Paolo Bonzini (maintainer:Overall)
Cc: Peter Crosthwaite (maintainer:Overall)
Cc: Richard Henderson (maintainer:Overall)
Cc: Eduardo Habkost (maintainer:NUMA)
Cc: qemu-devel@nongnu.org (open list:Overall)
Signed-off-by: Lingfeng Yang
---
exec.c | 18 +-
include/exec/memory.h | 2 --
include/sysemu/sysemu.h | 1 +
memory.c| 2 --
numa.c | 5 -
qemu-options.hx | 9 -
util/mmap-alloc.c | 40
vl.c| 4
8 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/exec.c b/exec.c
index f6645ede0c..8ee61d5fd6 100644
--- a/exec.c
+++ b/exec.c
@@ -65,9 +65,7 @@
#include "migration/vmstate.h"
#include "qemu/range.h"
-#ifndef _WIN32
#include "qemu/mmap-alloc.h"
-#endif
#include "monitor/monitor.h"
@@ -99,6 +97,9 @@ static MemoryRegion io_mem_unassigned;
*/
#define RAM_RESIZEABLE (1 << 2)
+/* RAM is a mapped file */
+#define RAM_MAPPED (1 << 3)
+
/* UFFDIO_ZEROPAGE is available on this RAMBlock to atomically
* zero the page and wake waiting processes.
* (Set during postcopy)
@@ -1667,6 +1668,10 @@ static int file_ram_open(const char *path,
return fd;
}
+#ifdef _WIN32
+#define MAP_FAILED 0
+#endif
+
static void *file_ram_alloc(RAMBlock *block,
ram_addr_t memory,
int fd,
@@ -1831,6 +1836,11 @@ bool qemu_ram_is_shared(RAMBlock *rb)
return rb->flags & RAM_SHARED;
}
+bool qemu_ram_is_mapped(RAMBlock *rb)
+{
+return rb->flags & RAM_MAPPED;
+}
+
/* Note: Only set at the start of postcopy */
bool qemu_ram_is_uf_zeroable(RAMBlock *rb)
{
@@ -2088,7 +2098,6 @@ static void ram_block_add(RAMBlock *new_block, Error
**errp, bool shared)
}
}
-#ifdef __linux__
RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
bool share, int fd,
Error **errp)
@@ -2132,7 +2141,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size,
MemoryRegion *mr,
new_block->mr = mr;
new_block->used_length = size;
new_block->max_length = size;
-new_block->flags = share ? RAM_SHARED : 0;
+new_block->flags = RAM_MAPPED | (share ? RAM_SHARED : 0);
new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
if (!new_block->host) {
g_free(new_block);
@@ -2174,7 +2183,6 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
MemoryRegion *mr,
return block;
}
-#endif
static
RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index eb2ba06519..02e7bbcf0f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -578,7 +578,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
uint64_t length,
void *host),
Error **errp);
-#ifdef __linux__
/**
* memory_region_init_ram_from_file: Initialize RAM memory region with a
*mmap-ed backend.
@@ -628,7 +627,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
bool share,
int fd,
Error **errp);
-#endif
/**
* memory_region_init_ram_ptr: Initialize RAM memory region from a
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e893f72f3b..279315b05a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -132,6 +132,7 @@ extern uint8_t qemu_extra_params_fw[2];
extern QEMUClockType rtc_clock;
extern const char *mem_path;
extern int mem_prealloc;
+extern int mem_file_shared;
#define MAX_NODES 128
#define NUMA_NODE_UNASSIGNED MAX_NODES
diff --git a/memory.c b/memory.c
index 3212acc7f4..6244f31e60 100644
--- a/memory.c
+++ b/memory.c
@@ -1545,7 +1545,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
}
-#ifdef __linux__
void memory_region_init_ram_from_file(MemoryRegion *mr,
[Qemu-devel] [PATCH] Improve file-backed RAM
1. Add support for all platforms
2. Add option to map in shared mode, allowing the guest to write
through to the backing file
Taken together, this allows one to write RAM snapshots as the guest is
running. Saving RAM snapshots is then equivalent to exiting the qemu
process or unmapping the file. This can be faster than waiting for a
lengthy explicit migration process.
Eventually, we want to go in the direction of allowing the launch of
multiple guest instances from the same RAM snapshot, which aids
virtualization-based integration testing; boot and other initializations
for multiple guest instances can be skipped, and the host OS will have
optimized shared RAM usage using its existing copy-on-write mechanisms.
Cc: Paolo Bonzini (maintainer:Overall)
Cc: Peter Crosthwaite (maintainer:Overall)
Cc: Richard Henderson (maintainer:Overall)
Cc: Eduardo Habkost (maintainer:NUMA)
Cc: qemu-devel@nongnu.org (open list:Overall)
Signed-off-by: Lingfeng Yang
---
exec.c | 18 +-
include/exec/memory.h | 2 --
include/sysemu/sysemu.h | 1 +
memory.c| 2 --
numa.c | 5 -
qemu-options.hx | 9 -
util/mmap-alloc.c | 40
vl.c| 4
8 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/exec.c b/exec.c
index f6645ede0c..8ee61d5fd6 100644
--- a/exec.c
+++ b/exec.c
@@ -65,9 +65,7 @@
#include "migration/vmstate.h"
#include "qemu/range.h"
-#ifndef _WIN32
#include "qemu/mmap-alloc.h"
-#endif
#include "monitor/monitor.h"
@@ -99,6 +97,9 @@ static MemoryRegion io_mem_unassigned;
*/
#define RAM_RESIZEABLE (1 << 2)
+/* RAM is a mapped file */
+#define RAM_MAPPED (1 << 3)
+
/* UFFDIO_ZEROPAGE is available on this RAMBlock to atomically
* zero the page and wake waiting processes.
* (Set during postcopy)
@@ -1667,6 +1668,10 @@ static int file_ram_open(const char *path,
return fd;
}
+#ifdef _WIN32
+#define MAP_FAILED 0
+#endif
+
static void *file_ram_alloc(RAMBlock *block,
ram_addr_t memory,
int fd,
@@ -1831,6 +1836,11 @@ bool qemu_ram_is_shared(RAMBlock *rb)
return rb->flags & RAM_SHARED;
}
+bool qemu_ram_is_mapped(RAMBlock *rb)
+{
+return rb->flags & RAM_MAPPED;
+}
+
/* Note: Only set at the start of postcopy */
bool qemu_ram_is_uf_zeroable(RAMBlock *rb)
{
@@ -2088,7 +2098,6 @@ static void ram_block_add(RAMBlock *new_block, Error
**errp, bool shared)
}
}
-#ifdef __linux__
RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
bool share, int fd,
Error **errp)
@@ -2132,7 +2141,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size,
MemoryRegion *mr,
new_block->mr = mr;
new_block->used_length = size;
new_block->max_length = size;
-new_block->flags = share ? RAM_SHARED : 0;
+new_block->flags = RAM_MAPPED | (share ? RAM_SHARED : 0);
new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
if (!new_block->host) {
g_free(new_block);
@@ -2174,7 +2183,6 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
MemoryRegion *mr,
return block;
}
-#endif
static
RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index eb2ba06519..02e7bbcf0f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -578,7 +578,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
uint64_t length,
void *host),
Error **errp);
-#ifdef __linux__
/**
* memory_region_init_ram_from_file: Initialize RAM memory region with a
*mmap-ed backend.
@@ -628,7 +627,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
bool share,
int fd,
Error **errp);
-#endif
/**
* memory_region_init_ram_ptr: Initialize RAM memory region from a
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e893f72f3b..279315b05a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -132,6 +132,7 @@ extern uint8_t qemu_extra_params_fw[2];
extern QEMUClockType rtc_clock;
extern const char *mem_path;
extern int mem_prealloc;
+extern int mem_file_shared;
#define MAX_NODES 128
#define NUMA_NODE_UNASSIGNED MAX_NODES
diff --git a/memory.c b/memory.c
index 3212acc7f4..6244f31e60 100644
--- a/memory.c
+++ b/memory.c
@@ -1545,7 +1545,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
}
-#ifdef __linux__
void memory_region_init_ram_from_file(MemoryRegion *mr,
Re: [Qemu-devel] storing machine data in qcow images?
On 06/06/2018 09:57 AM, Eric Blake wrote:
On 06/06/2018 09:43 AM, Michael S. Tsirkin wrote:
On Wed, Jun 06, 2018 at 01:02:53PM +0200, Max Reitz wrote:
Yeah, but why make qcow2 that format? That's what I completely fail to
understand.
Because why not? It's cheap to add it there and is much easier
than teaching people about a new container format.
tar is not a new container format, but it is a new format to various
toolchains - that said, if we popularize tar as the format for including
a config file alongside a qcow2 image, it's not that hard to fix the
stack to start passing that file around as the new preferred file type.
On a completely different front, 'qcow2' as a file format comes with
some psychological baggage. If someone was using it 8 years ago, before
we did coroutine optimizations, it was noticeably slower than raw, and
relatively easier to get into a corrupted image condition that resulted
in data loss. Just one VM lost, and it leaves a sour taste in your
mouth, where you are unwilling to trust that file format (even though
the file format was not necessarily the cause of the corruption).
Marketing-wise, we failed with our improvements ('qcow2v3' is so much
more of a mouthful than 'qcow3'), and it took years to flip the defaults
from v2 as the default to v3 as the default (moreso in downstream
distros than upstream), in part because we couldn't convince people of
the improvements they would be gaining by moving to v3. Historically,
there's also 'qed' which was promised as a way to fix some of the poor
performance of qcow2, but which ended up not being any better than our
actual qcow2v3 improvements, so no one ended up switching to that. So,
to some extent, various high-level consumers still have the notion that
'raw' files are better/safer/faster than 'qcow2' files because of an
anecdote from years ago, even if we have since fixed the speed parity
and added locking to eliminate careless data loss.
If we DO add a new tar-file block driver to qemu, that could serve as a
marketing opportunity to convince people that the new format has all of
the features that you can't get from just a raw file, and does not
suffer from the slowness or data corruption they were worried about in
qcow2. Thus, even if our new format is just a thin wrapper around a
config file plus the existing qcow2v3 we already know and love, the mere
fact that it is a new format may get people to move away from raw images
in situations where just the name 'qcow2' is unable to do so, at which
point we can help them take advantage of the features made possible by
qcow2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH] configure: Enable out-of-tree acceptance tests
On 06/06/2018 04:11 PM, Eduardo Habkost wrote: > On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote: >> On 06/06/2018 04:24 PM, Eduardo Habkost wrote: >>> On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote: >>> [... something about config files ...] > And after that, the following would run all "console" tests: > > avocado run -t console > > How does this sound? For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about use_test_dir_when_no_references_given. >>> >>> Well, I can't give an opinion because I couldn't understand >>> what's the final goal here. >> >> You cut too much, the relevant part is: >> >> On 05/30/2018 10:06 PM, Cleber Rosa wrote: >> > But at the same, there are security implications: `list` won't >> > load/execute any test code (different from, say, standard Python >> > unittests), while "run" obviously will. So "avocado run" may end up >> > running what users don't want if a malicious user controls >> > "$avocado_datadir_paths_test_dir". > > The final goal still isn't clear to me. Why would somebody want > to use $avocado_datadir_paths_test_dir instead of just specifying > "." in the command-line? > > >> >>> >>> What exactly is missing in the current solution? Why >>> "avocado run -t console ." wouldn't work? >> >> It doesn't work in out-of-tree builds, I have to use the full path: >> >> >> build_dir$ avocado run >> /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py > > Isn't the symlink you suggested a better solution than requiring > the user to edit a config file? > It's definitely the simplest, and would avoid a questionable knob and behavior in Avocado. - Cleber.
Re: [Qemu-devel] [PULL v2 7/8] hw: xilinx-pcie: Add support for Xilinx AXI PCIe Controller
Hi Peter,
On Mon, Jun 04, 2018 at 11:29:47AM +0100, Peter Maydell wrote:
> On 22 February 2017 at 00:21, Yongbok Kim wrote:
> > From: Paul Burton
> >
> > Add support for emulating the Xilinx AXI Root Port Bridge for PCI
> > Express as described by Xilinx' PG055 document. This is a PCIe
> > controller that can be used with certain series of Xilinx FPGAs, and is
> > used on the MIPS Boston board which will make use of this code.
> >
> > Signed-off-by: Paul Burton
> > [yongbok@imgtec.com:
> > removed returning on !level,
> > updated IRQ connection with GPIO logic,
> > moved xilinx_pcie_init() to boston.c
> > replaced stw_le_p() with pci_set_word()
> > and other cosmetic changes]
> > Signed-off-by: Yongbok Kim
> > ---
>
> > +static void xilinx_pcie_host_realize(DeviceState *dev, Error **errp)
> > +{
> > +PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> > +XilinxPCIEHost *s = XILINX_PCIE_HOST(dev);
> > +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
> > +
> > +snprintf(s->name, sizeof(s->name), "pcie%u", s->bus_nr);
> > +
> > +/* PCI configuration space */
> > +pcie_host_mmcfg_init(pex, s->cfg_size);
> > +
> > +/* MMIO region */
> > +memory_region_init(>mmio, OBJECT(s), "mmio", UINT64_MAX);
> > +memory_region_set_enabled(>mmio, false);
> > +
> > +/* dummy I/O region */
> > +memory_region_init_ram(>io, OBJECT(s), "io", 16, NULL);
> > +memory_region_set_enabled(>io, false);
>
> Hi; I was looking at the code for this PCI controller as
> part of identifying RAM memory regions that don't migrate
> their contents, and this looks kind of odd. Can you explain
> why this is using a RAM region for the PCI bus's IO memory
> space? None of our other PCI controller models do this, so
> I suspect this is a bug. (The effect would be that PCI IO
> accesses to addresses which don't have a PCI BAR mapped there
> will behave as reads-as-written, which seems pretty unlikely
> to be what the hardware does.)
>
> What is the intended semantics for PCI IO accesses with this
> controller? (It seems to only be used with the MIPS boston
> board model.)
>
> thanks
> -- PMM
The controller here simply doesn't support I/O accesses, and the dummy
region is there to prevent the rest of QEMU's PCI code & device
emulation from needing to care about that fact. The region is never
mapped into the system memory and so software running on the emulated
system has no way to access it.
Thanks,
Paul
Re: [Qemu-devel] [PATCH v2 0/3] qcow2: Do not mark inactive images corrupt
On 06/06/2018 03:36 PM, Max Reitz wrote: > The non-public logs in > https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal > this problem: > > $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry) > $ echo 'qemu-io none0 "read 0 512"' \ > | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \ > -monitor stdio \ > -incoming exec:'cat /dev/null' > QEMU 2.12.50 monitor - type 'help' for more information > (qemu) qemu-io none0 "read 0 512" > qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: > 0); further corruption events will be suppressed > qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion > `!(bs->open_flags & BDRV_O_INACTIVE)' failed. > [1]18444 done echo 'qemu-io none0 "read 0 512"' | >18445 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -drive > if=none,file=foo.qcow2 -monitor stdi > > Oops. > > > The first patch in this series makes a function public that the second > patch uses to fix the issue by treating all non-writable images like > read-only images (yes, there is a difference...) in this regard (which > most importantly means not trying to set the corrupt flag on them). > Inactive images count as non-writable images, but not as read-only > images, so that fixes it. > > The third patch adds an iotest case. > > > v2: > - Use bdrv_is_writable() instead of copying its functionality [Jeff] > > > git-backport-diff against v1: > > Key: > [] : patches are identical > [] : number of functional differences between upstream/downstream patch > [down] : patch is downstream-only > The flags [FC] indicate (F)unctional and (C)ontextual differences, > respectively > > 001/3:[down] 'block: Make bdrv_is_writable() public' > 002/3:[0004] [FC] 'qcow2: Do not mark inactive images corrupt' > 003/3:[] [--] 'iotests: Add case for a corrupted inactive image' > > > Max Reitz (3): > block: Make bdrv_is_writable() public > qcow2: Do not mark inactive images corrupt > iotests: Add case for a corrupted inactive image > > include/block/block.h | 1 + > block.c| 17 ++--- > block/qcow2.c | 2 +- > tests/qemu-iotests/060 | 30 ++ > tests/qemu-iotests/060.out | 14 ++ > 5 files changed, 60 insertions(+), 4 deletions(-) > Reviewed-by: John Snow
Re: [Qemu-devel] [RFC PATCH] configure: Enable out-of-tree acceptance tests
On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote: > On 06/06/2018 04:24 PM, Eduardo Habkost wrote: > > On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote: > > [... something about config files ...] > >>> And after that, the following would run all "console" tests: > >>> > >>> avocado run -t console > >>> > >>> How does this sound? > >> > >> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about > >> use_test_dir_when_no_references_given. > > > > Well, I can't give an opinion because I couldn't understand > > what's the final goal here. > > You cut too much, the relevant part is: > > On 05/30/2018 10:06 PM, Cleber Rosa wrote: > > But at the same, there are security implications: `list` won't > > load/execute any test code (different from, say, standard Python > > unittests), while "run" obviously will. So "avocado run" may end up > > running what users don't want if a malicious user controls > > "$avocado_datadir_paths_test_dir". The final goal still isn't clear to me. Why would somebody want to use $avocado_datadir_paths_test_dir instead of just specifying "." in the command-line? > > > > > What exactly is missing in the current solution? Why > > "avocado run -t console ." wouldn't work? > > It doesn't work in out-of-tree builds, I have to use the full path: > > >> build_dir$ avocado run > /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py Isn't the symlink you suggested a better solution than requiring the user to edit a config file? -- Eduardo
Re: [Qemu-devel] [PATCH] qemu-img: Remove deprecated -s snapshot_id_or_name option
On 2018-06-06 14:35, Thomas Huth wrote: > It has been marked as deprecated since QEMU v2.0 already, so it > is time now to finally remove it. > > Signed-off-by: Thomas Huth > --- > qemu-doc.texi | 7 --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 7 +-- > qemu-img.texi | 7 ++- > tests/qemu-iotests/029 | 2 +- > tests/qemu-iotests/080 | 4 ++-- > 6 files changed, 8 insertions(+), 23 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH v2] qmp.py: Fix exception parsing partial JSON
On Wed, Jun 06, 2018 at 04:27:31PM -0300, Philippe Mathieu-Daudé wrote:
> The readline() call returns partial data.
How can this be reproduced? Despite not being forbidden by the
QMP specification, QEMU normally doesn't break QMP replies in
multiple lines, and readline() is not supposed to return a
partial line unless it encounters EOF.
> Keep appending until the JSON buffer is complete.
>
> This fixes:
>
> $ scripts/qmp/qmp-shell -v -p /tmp/qmp.sock
> Traceback (most recent call last):
> File "scripts/qmp/qmp-shell", line 456, in
> main()
> File "scripts/qmp/qmp-shell", line 441, in main
> qemu.connect(negotiate)
> File "scripts/qmp/qmp-shell", line 284, in connect
> self._greeting = super(QMPShell, self).connect(negotiate)
> File "scripts/qmp/qmp.py", line 143, in connect
> return self.__negotiate_capabilities()
> File "scripts/qmp/qmp.py", line 71, in __negotiate_capabilities
> greeting = self.__json_read()
> File "scripts/qmp/qmp.py", line 85, in __json_read
> resp = json.loads(data)
> File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
> return _default_decoder.decode(s)
> File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
> obj, end = self.raw_decode(s, idx=_w(s, 0).end())
> File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
> obj, end = self.scan_once(s, idx)
> ValueError: Expecting object: line 1 column 3 (char 2)
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Since v1:
> - addressed Daniel review: clean data after json.loads() succeeds
> - add a XXX comment
>
> Daniel suggested this is due to blocking i/o.
>
> I'm sure there is a nicer/more pythonic way to do this, but this works for me,
> sorry :)
It looks like there's no elegant solution for this:
https://stackoverflow.com/a/21709058
>
> scripts/qmp/qmp.py | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 5c8cf6a056..14f0b48936 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -78,11 +78,18 @@ class QEMUMonitorProtocol(object):
> raise QMPCapabilitiesError
>
> def __json_read(self, only_event=False):
> +data = ""
> while True:
> -data = self.__sockfile.readline()
> -if not data:
> +tmp = self.__sockfile.readline()
> +if not tmp:
> return
> -resp = json.loads(data)
> +data += tmp
> +try:
> +resp = json.loads(data)
> +except ValueError:
> +# XXX: blindly loop, even if QEMU ever sends malformed data
> +continue
I was going to suggest using json.JSONDecoder.raw_decode() and
saving the remaining data in case we already read partial data
for a second JSON message.
But the QMP specification says all messages are terminated with
CRLF, so we we should never see the data for two different
messages in a single readline() call. Noting this in a comment
wouldn't hurt, though.
The patch seems reasonable, but first I would like to understand
how this bug can be triggered.
> +data = ""
> if 'event' in resp:
> self.logger.debug("<<< %s", resp)
> self.__events.append(resp)
> --
> 2.17.1
>
--
Eduardo
[Qemu-devel] [PULL v1 3/4] test: Pass TPM interface model to functions creating command line
Pass the TPM interface model, such as 'tpm-crb', through to the functions
that create the command line for QEMU.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
---
tests/tpm-crb-swtpm-test.c | 4 ++--
tests/tpm-tests.c | 13 -
tests/tpm-tests.h | 6 --
tests/tpm-util.c | 11 ++-
tests/tpm-util.h | 3 ++-
5 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/tests/tpm-crb-swtpm-test.c b/tests/tpm-crb-swtpm-test.c
index 4ac568..8c0a55f3ca 100644
--- a/tests/tpm-crb-swtpm-test.c
+++ b/tests/tpm-crb-swtpm-test.c
@@ -28,7 +28,7 @@ static void tpm_crb_swtpm_test(const void *data)
{
const TestState *ts = data;
-tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_crb_transfer);
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_crb_transfer, "tpm-crb");
}
static void tpm_crb_swtpm_migration_test(const void *data)
@@ -36,7 +36,7 @@ static void tpm_crb_swtpm_migration_test(const void *data)
const TestState *ts = data;
tpm_test_swtpm_migration_test(ts->src_tpm_path, ts->dst_tpm_path, ts->uri,
- tpm_util_crb_transfer);
+ tpm_util_crb_transfer, "tpm-crb");
}
int main(int argc, char **argv)
diff --git a/tests/tpm-tests.c b/tests/tpm-tests.c
index adf2c618c8..10c6592aac 100644
--- a/tests/tpm-tests.c
+++ b/tests/tpm-tests.c
@@ -18,7 +18,8 @@
#include "libqtest.h"
#include "tpm-tests.h"
-void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx)
+void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
+ const char *ifmodel)
{
char *args = NULL;
QTestState *s;
@@ -36,8 +37,8 @@ void tpm_test_swtpm_test(const char *src_tpm_path, tx_func
*tx)
args = g_strdup_printf(
"-chardev socket,id=chr,path=%s "
"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev",
-addr->u.q_unix.path);
+"-device %s,tpmdev=dev",
+addr->u.q_unix.path, ifmodel);
s = qtest_start(args);
g_free(args);
@@ -64,7 +65,8 @@ void tpm_test_swtpm_test(const char *src_tpm_path, tx_func
*tx)
void tpm_test_swtpm_migration_test(const char *src_tpm_path,
const char *dst_tpm_path,
- const char *uri, tx_func *tx)
+ const char *uri, tx_func *tx,
+ const char *ifmodel)
{
gboolean succ;
GPid src_tpm_pid, dst_tpm_pid;
@@ -87,7 +89,8 @@ void tpm_test_swtpm_migration_test(const char *src_tpm_path,
}
tpm_util_migration_start_qemu(_qemu, _qemu,
- src_tpm_addr, dst_tpm_addr, uri);
+ src_tpm_addr, dst_tpm_addr, uri,
+ ifmodel);
tpm_util_startup(src_qemu, tx);
tpm_util_pcrextend(src_qemu, tx);
diff --git a/tests/tpm-tests.h b/tests/tpm-tests.h
index 377f184c77..b97688fe75 100644
--- a/tests/tpm-tests.h
+++ b/tests/tpm-tests.h
@@ -15,10 +15,12 @@
#include "tpm-util.h"
-void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx);
+void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
+ const char *ifmodel);
void tpm_test_swtpm_migration_test(const char *src_tpm_path,
const char *dst_tpm_path,
- const char *uri, tx_func *tx);
+ const char *uri, tx_func *tx,
+ const char *ifmodel);
#endif /* TESTS_TPM_TESTS_H */
diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index e6e3b922fa..e1ac4d1bd5 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -248,25 +248,26 @@ void tpm_util_migration_start_qemu(QTestState **src_qemu,
QTestState **dst_qemu,
SocketAddress *src_tpm_addr,
SocketAddress *dst_tpm_addr,
- const char *miguri)
+ const char *miguri,
+ const char *ifmodel)
{
char *src_qemu_args, *dst_qemu_args;
src_qemu_args = g_strdup_printf(
"-chardev socket,id=chr,path=%s "
"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev ",
-src_tpm_addr->u.q_unix.path);
+"-device %s,tpmdev=dev ",
+src_tpm_addr->u.q_unix.path, ifmodel);
*src_qemu = qtest_init(src_qemu_args);
dst_qemu_args = g_strdup_printf(
"-chardev socket,id=chr,path=%s "
"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev "
+"-device %s,tpmdev=dev "
"-incoming %s",
dst_tpm_addr->u.q_unix.path,
-miguri);
+ifmodel, miguri);
*dst_qemu = qtest_init(dst_qemu_args);
diff --git
Re: [Qemu-devel] [PATCH 0/7] atapi: change unlimited recursion to while loop
On 06/06/2018 03:09 PM, John Snow wrote: > Real hardware doesn't have an unlimited stack, so the unlimited > recursion in the ATAPI code smells a bit. In fact, the call to > ide_transfer_start easily becomes a tail call with a small change > to the code (patch 5); however, we also need to turn the call back to > ide_atapi_cmd_reply_end into another tail call before turning the > (double) tail recursion into a while loop. > > In particular, patch 1 ensures that the call to the end_transfer_func is > the last thing in ide_transfer_start. To do so, it moves the write of > the PIO Setup FIS before the PIO transfer, which actually makes sense: > the FIS is sent by the device to inform the AHCI about the transfer, > so it cannot come after! This is the main change from the RFC, and > it simplifies the rest of the series (the RFC had to introduce an > "end_transfer" callback just for writing the PIO Setup FIS). > > I tested this manually with READ CD commands sent through sg_raw, > and the existing AHCI tests still pass. > > v2: reworked PIO Setup FIS based on spec reading, adjusted tests. > > John Snow (2): > libqos/ahci: track sector size > ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands > > Paolo Bonzini (5): > ide: push end_transfer_func out of start_transfer callback, rename > callback > ide: call ide_cmd_done from ide_transfer_stop > ide: make ide_transfer_stop idempotent > atapi: call ide_set_irq before ide_transfer_start > ide: introduce ide_transfer_start_norecurse > > hw/ide/ahci.c | 31 --- > hw/ide/atapi.c| 44 > hw/ide/core.c | 39 --- > hw/ide/trace-events | 2 +- > include/hw/ide/internal.h | 4 +++- > tests/libqos/ahci.c | 45 +++-- > tests/libqos/ahci.h | 3 +-- > 7 files changed, 88 insertions(+), 80 deletions(-) > I know this is weird, but it will make it easier for me later: Patches 3-7 (authored by Paolo): Reviewed-by: John Snow
[Qemu-devel] [PULL v1 4/4] test: Add swtpm migration test for the TPM TIS interface
Add a test case for testing swtpm migration with the TPM TIS
interface.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
---
tests/Makefile.include | 3 +++
tests/tpm-tis-swtpm-test.c | 66 ++
tests/tpm-util.c | 48 +
tests/tpm-util.h | 3 +++
4 files changed, 120 insertions(+)
create mode 100644 tests/tpm-tis-swtpm-test.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 392273317f..0eaa835b8a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -299,6 +299,7 @@ check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) +=
tests/vhost-user-test
endif
check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-swtpm-test$(EXESUF)
check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-test$(EXESUF)
+check-qtest-i386-$(CONFIG_TPM) += tests/tpm-tis-swtpm-test$(EXESUF)
check-qtest-i386-$(CONFIG_TPM) += tests/tpm-tis-test$(EXESUF)
check-qtest-i386-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
@@ -726,6 +727,8 @@ tests/test-io-channel-socket$(EXESUF):
tests/test-io-channel-socket.o \
tests/tpm-crb-swtpm-test$(EXESUF): tests/tpm-crb-swtpm-test.o tests/tpm-emu.o \
tests/tpm-util.o tests/tpm-tests.o $(test-io-obj-y)
tests/tpm-crb-test$(EXESUF): tests/tpm-crb-test.o tests/tpm-emu.o
$(test-io-obj-y)
+tests/tpm-tis-swtpm-test$(EXESUF): tests/tpm-tis-swtpm-test.o tests/tpm-emu.o \
+ tests/tpm-util.o tests/tpm-tests.o $(test-io-obj-y)
tests/tpm-tis-test$(EXESUF): tests/tpm-tis-test.o tests/tpm-emu.o
$(test-io-obj-y)
tests/test-io-channel-file$(EXESUF): tests/test-io-channel-file.o \
tests/io-channel-helpers.o $(test-io-obj-y)
diff --git a/tests/tpm-tis-swtpm-test.c b/tests/tpm-tis-swtpm-test.c
new file mode 100644
index 00..7dcd1d3912
--- /dev/null
+++ b/tests/tpm-tis-swtpm-test.c
@@ -0,0 +1,66 @@
+/*
+ * QTest testcase for TPM TIS talking to external swtpm and swtpm migration
+ *
+ * Copyright (c) 2018 IBM Corporation
+ * with parts borrowed from migration-test.c that is:
+ * Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ * Stefan Berger
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include
+
+#include "libqtest.h"
+#include "tpm-tests.h"
+
+typedef struct TestState {
+char *src_tpm_path;
+char *dst_tpm_path;
+char *uri;
+} TestState;
+
+static void tpm_tis_swtpm_test(const void *data)
+{
+const TestState *ts = data;
+
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_tis_transfer, "tpm-tis");
+}
+
+static void tpm_tis_swtpm_migration_test(const void *data)
+{
+const TestState *ts = data;
+
+tpm_test_swtpm_migration_test(ts->src_tpm_path, ts->dst_tpm_path, ts->uri,
+ tpm_util_tis_transfer, "tpm-tis");
+}
+
+int main(int argc, char **argv)
+{
+int ret;
+TestState ts = { 0 };
+
+ts.src_tpm_path = g_dir_make_tmp("qemu-tpm-tis-swtpm-test.XX", NULL);
+ts.dst_tpm_path = g_dir_make_tmp("qemu-tpm-tis-swtpm-test.XX", NULL);
+ts.uri = g_strdup_printf("unix:%s/migsocket", ts.src_tpm_path);
+
+module_call_init(MODULE_INIT_QOM);
+g_test_init(, , NULL);
+
+qtest_add_data_func("/tpm/tis-swtpm/test", , tpm_tis_swtpm_test);
+qtest_add_data_func("/tpm/tis-swtpm-migration/test", ,
+tpm_tis_swtpm_migration_test);
+ret = g_test_run();
+
+g_rmdir(ts.dst_tpm_path);
+g_free(ts.dst_tpm_path);
+g_rmdir(ts.src_tpm_path);
+g_free(ts.src_tpm_path);
+g_free(ts.uri);
+
+return ret;
+}
diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index e1ac4d1bd5..672cedf905 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -19,6 +19,9 @@
#include "tpm-util.h"
#include "qapi/qmp/qdict.h"
+#define TIS_REG(LOCTY, REG) \
+(TPM_TIS_ADDR_BASE + ((LOCTY) << 12) + REG)
+
static bool got_stop;
void tpm_util_crb_transfer(QTestState *s,
@@ -52,6 +55,51 @@ void tpm_util_crb_transfer(QTestState *s,
qtest_memread(s, raddr, rsp, rsp_size);
}
+void tpm_util_tis_transfer(QTestState *s,
+ const unsigned char *req, size_t req_size,
+ unsigned char *rsp, size_t rsp_size)
+{
+uint32_t sts;
+uint16_t bcount;
+size_t i;
+
+/* request use of locality 0 */
+qtest_writeb(s, TIS_REG(0, TPM_TIS_REG_ACCESS),
TPM_TIS_ACCESS_REQUEST_USE);
+qtest_writel(s, TIS_REG(0, TPM_TIS_REG_STS), TPM_TIS_STS_COMMAND_READY);
+
+sts = qtest_readl(s, TIS_REG(0, TPM_TIS_REG_STS));
+bcount = (sts >> 8) & 0x;
+g_assert_cmpint(bcount, >=, req_size);
+
+/* transmit command */
+for (i = 0; i < req_size; i++) {
+qtest_writeb(s, TIS_REG(0, TPM_TIS_REG_DATA_FIFO), req[i]);
+}
+
+/* start processing */
+
[Qemu-devel] [PULL v1 0/4] Merge tpm 2018/06/06
This series of patches refactors existing TPM test code and adds another TPM TIS test case testing TPM migration. Stefan The following changes since commit c1c2a435905ae76b159c573b0c0d6f095b45ebc6: Merge remote-tracking branch 'remotes/stsquad/tags/pull-docker-updates-050618-1' into staging (2018-06-05 17:06:23 +0100) are available in the Git repository at: git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-06-06-1 for you to fetch changes up to 70663851ed4242435676c0bc288efbc1bc4ccf87: test: Add swtpm migration test for the TPM TIS interface (2018-06-06 15:44:12 -0400) Merge tpm 2018/06/06 v1 Stefan Berger (4): test: Move reusable code from tpm-crb-swtpm-test.c to tpm-util.c test: Move common TPM test functions to tpm-tests.c test: Pass TPM interface model to functions creating command line test: Add swtpm migration test for the TPM TIS interface tests/Makefile.include | 5 +- tests/tpm-crb-swtpm-test.c | 189 + tests/tpm-tests.c | 127 ++ tests/tpm-tests.h | 26 +++ tests/tpm-tis-swtpm-test.c | 66 tests/tpm-util.c | 138 + tests/tpm-util.h | 14 7 files changed, 379 insertions(+), 186 deletions(-) create mode 100644 tests/tpm-tests.c create mode 100644 tests/tpm-tests.h create mode 100644 tests/tpm-tis-swtpm-test.c -- 2.14.4
[Qemu-devel] [PULL v1 1/4] test: Move reusable code from tpm-crb-swtpm-test.c to tpm-util.c
Move code we can reuse from tpm-crb-swtpm-test.c into tpm-util.c
and prefix functions with 'tpm_util_'.
Remove some unnecessary #include's.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
---
tests/tpm-crb-swtpm-test.c | 100 +++--
tests/tpm-util.c | 89
tests/tpm-util.h | 10 +
3 files changed, 104 insertions(+), 95 deletions(-)
diff --git a/tests/tpm-crb-swtpm-test.c b/tests/tpm-crb-swtpm-test.c
index c2bde0cbaa..2a22b032ca 100644
--- a/tests/tpm-crb-swtpm-test.c
+++ b/tests/tpm-crb-swtpm-test.c
@@ -15,12 +15,8 @@
#include "qemu/osdep.h"
#include
-#include "hw/acpi/tpm.h"
-#include "io/channel-socket.h"
#include "libqtest.h"
#include "tpm-util.h"
-#include "sysemu/tpm.h"
-#include "qapi/qmp/qdict.h"
typedef struct TestState {
char *src_tpm_path;
@@ -28,93 +24,6 @@ typedef struct TestState {
char *uri;
} TestState;
-bool got_stop;
-
-static void migrate(QTestState *who, const char *uri)
-{
-QDict *rsp;
-gchar *cmd;
-
-cmd = g_strdup_printf("{ 'execute': 'migrate',"
- "'arguments': { 'uri': '%s' } }",
- uri);
-rsp = qtest_qmp(who, cmd);
-g_free(cmd);
-g_assert(qdict_haskey(rsp, "return"));
-qobject_unref(rsp);
-}
-
-/*
- * Events can get in the way of responses we are actually waiting for.
- */
-static QDict *wait_command(QTestState *who, const char *command)
-{
-const char *event_string;
-QDict *response;
-
-response = qtest_qmp(who, command);
-
-while (qdict_haskey(response, "event")) {
-/* OK, it was an event */
-event_string = qdict_get_str(response, "event");
-if (!strcmp(event_string, "STOP")) {
-got_stop = true;
-}
-qobject_unref(response);
-response = qtest_qmp_receive(who);
-}
-return response;
-}
-
-static void wait_for_migration_complete(QTestState *who)
-{
-while (true) {
-QDict *rsp, *rsp_return;
-bool completed;
-const char *status;
-
-rsp = wait_command(who, "{ 'execute': 'query-migrate' }");
-rsp_return = qdict_get_qdict(rsp, "return");
-status = qdict_get_str(rsp_return, "status");
-completed = strcmp(status, "completed") == 0;
-g_assert_cmpstr(status, !=, "failed");
-qobject_unref(rsp);
-if (completed) {
-return;
-}
-usleep(1000);
-}
-}
-
-static void migration_start_qemu(QTestState **src_qemu, QTestState **dst_qemu,
- SocketAddress *src_tpm_addr,
- SocketAddress *dst_tpm_addr,
- const char *miguri)
-{
-char *src_qemu_args, *dst_qemu_args;
-
-src_qemu_args = g_strdup_printf(
-"-chardev socket,id=chr,path=%s "
-"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev ",
-src_tpm_addr->u.q_unix.path);
-
-*src_qemu = qtest_init(src_qemu_args);
-
-dst_qemu_args = g_strdup_printf(
-"-chardev socket,id=chr,path=%s "
-"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev "
-"-incoming %s",
-dst_tpm_addr->u.q_unix.path,
-miguri);
-
-*dst_qemu = qtest_init(dst_qemu_args);
-
-free(src_qemu_args);
-free(dst_qemu_args);
-}
-
static void tpm_crb_swtpm_test(const void *data)
{
char *args = NULL;
@@ -183,8 +92,9 @@ static void tpm_crb_swtpm_migration_test(const void *data)
goto err_src_tpm_kill;
}
-migration_start_qemu(_qemu, _qemu, src_tpm_addr, dst_tpm_addr,
- ts->uri);
+tpm_util_migration_start_qemu(_qemu, _qemu,
+ src_tpm_addr, dst_tpm_addr,
+ ts->uri);
tpm_util_startup(src_qemu, tpm_util_crb_transfer);
tpm_util_pcrextend(src_qemu, tpm_util_crb_transfer);
@@ -197,8 +107,8 @@ static void tpm_crb_swtpm_migration_test(const void *data)
tpm_util_pcrread(src_qemu, tpm_util_crb_transfer, tpm_pcrread_resp,
sizeof(tpm_pcrread_resp));
-migrate(src_qemu, ts->uri);
-wait_for_migration_complete(src_qemu);
+tpm_util_migrate(src_qemu, ts->uri);
+tpm_util_wait_for_migration_complete(src_qemu);
tpm_util_pcrread(dst_qemu, tpm_util_crb_transfer, tpm_pcrread_resp,
sizeof(tpm_pcrread_resp));
diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index c9b3947c1c..e6e3b922fa 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -17,6 +17,9 @@
#include "hw/acpi/tpm.h"
#include "libqtest.h"
#include "tpm-util.h"
+#include "qapi/qmp/qdict.h"
+
+static bool got_stop;
void tpm_util_crb_transfer(QTestState *s,
const unsigned char *req, size_t req_size,
@@ -184,3 +187,89 @@ void tpm_util_swtpm_kill(GPid pid)
kill(pid, SIGKILL);
}
+
+void
[Qemu-devel] [PULL v1 2/4] test: Move common TPM test functions to tpm-tests.c
Move common TPM test functions from tpm-crb-swtpm-test.c to tpm-tests.c
so that for example test cases with the TPM TIS interface can use the
same code. Prefix all funcions with 'tpm_test_'.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
---
tests/Makefile.include | 2 +-
tests/tpm-crb-swtpm-test.c | 99 ++--
tests/tpm-tests.c | 124 +
tests/tpm-tests.h | 24 +
4 files changed, 153 insertions(+), 96 deletions(-)
create mode 100644 tests/tpm-tests.c
create mode 100644 tests/tpm-tests.h
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 9854e7794b..392273317f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -724,7 +724,7 @@ tests/test-io-task$(EXESUF): tests/test-io-task.o
$(test-io-obj-y)
tests/test-io-channel-socket$(EXESUF): tests/test-io-channel-socket.o \
tests/io-channel-helpers.o tests/socket-helpers.o $(test-io-obj-y)
tests/tpm-crb-swtpm-test$(EXESUF): tests/tpm-crb-swtpm-test.o tests/tpm-emu.o \
- tests/tpm-util.o $(test-io-obj-y)
+ tests/tpm-util.o tests/tpm-tests.o $(test-io-obj-y)
tests/tpm-crb-test$(EXESUF): tests/tpm-crb-test.o tests/tpm-emu.o
$(test-io-obj-y)
tests/tpm-tis-test$(EXESUF): tests/tpm-tis-test.o tests/tpm-emu.o
$(test-io-obj-y)
tests/test-io-channel-file$(EXESUF): tests/test-io-channel-file.o \
diff --git a/tests/tpm-crb-swtpm-test.c b/tests/tpm-crb-swtpm-test.c
index 2a22b032ca..4ac568 100644
--- a/tests/tpm-crb-swtpm-test.c
+++ b/tests/tpm-crb-swtpm-test.c
@@ -16,7 +16,7 @@
#include
#include "libqtest.h"
-#include "tpm-util.h"
+#include "tpm-tests.h"
typedef struct TestState {
char *src_tpm_path;
@@ -26,108 +26,17 @@ typedef struct TestState {
static void tpm_crb_swtpm_test(const void *data)
{
-char *args = NULL;
-QTestState *s;
-SocketAddress *addr = NULL;
-gboolean succ;
-GPid swtpm_pid;
-GError *error = NULL;
const TestState *ts = data;
-succ = tpm_util_swtpm_start(ts->src_tpm_path, _pid, , );
-/* succ may be false if swtpm is not available */
-if (!succ) {
-return;
-}
-
-args = g_strdup_printf(
-"-chardev socket,id=chr,path=%s "
-"-tpmdev emulator,id=dev,chardev=chr "
-"-device tpm-crb,tpmdev=dev",
-addr->u.q_unix.path);
-
-s = qtest_start(args);
-g_free(args);
-
-tpm_util_startup(s, tpm_util_crb_transfer);
-tpm_util_pcrextend(s, tpm_util_crb_transfer);
-
-unsigned char tpm_pcrread_resp[] =
-"\x80\x01\x00\x00\x00\x3e\x00\x00\x00\x00\x00\x00\x00\x16\x00\x00"
-"\x00\x01\x00\x0b\x03\x00\x04\x00\x00\x00\x00\x01\x00\x20\xf6\x85"
-"\x98\xe5\x86\x8d\xe6\x8b\x97\x29\x99\x60\xf2\x71\x7d\x17\x67\x89"
-"\xa4\x2f\x9a\xae\xa8\xc7\xb7\xaa\x79\xa8\x62\x56\xc1\xde";
-tpm_util_pcrread(s, tpm_util_crb_transfer, tpm_pcrread_resp,
- sizeof(tpm_pcrread_resp));
-
-qtest_end();
-tpm_util_swtpm_kill(swtpm_pid);
-
-if (addr) {
-g_unlink(addr->u.q_unix.path);
-qapi_free_SocketAddress(addr);
-}
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_crb_transfer);
}
static void tpm_crb_swtpm_migration_test(const void *data)
{
const TestState *ts = data;
-gboolean succ;
-GPid src_tpm_pid, dst_tpm_pid;
-SocketAddress *src_tpm_addr = NULL, *dst_tpm_addr = NULL;
-GError *error = NULL;
-QTestState *src_qemu, *dst_qemu;
-
-succ = tpm_util_swtpm_start(ts->src_tpm_path, _tpm_pid,
-_tpm_addr, );
-/* succ may be false if swtpm is not available */
-if (!succ) {
-return;
-}
-
-succ = tpm_util_swtpm_start(ts->dst_tpm_path, _tpm_pid,
-_tpm_addr, );
-/* succ may be false if swtpm is not available */
-if (!succ) {
-goto err_src_tpm_kill;
-}
-
-tpm_util_migration_start_qemu(_qemu, _qemu,
- src_tpm_addr, dst_tpm_addr,
- ts->uri);
-
-tpm_util_startup(src_qemu, tpm_util_crb_transfer);
-tpm_util_pcrextend(src_qemu, tpm_util_crb_transfer);
-
-unsigned char tpm_pcrread_resp[] =
-"\x80\x01\x00\x00\x00\x3e\x00\x00\x00\x00\x00\x00\x00\x16\x00\x00"
-"\x00\x01\x00\x0b\x03\x00\x04\x00\x00\x00\x00\x01\x00\x20\xf6\x85"
-"\x98\xe5\x86\x8d\xe6\x8b\x97\x29\x99\x60\xf2\x71\x7d\x17\x67\x89"
-"\xa4\x2f\x9a\xae\xa8\xc7\xb7\xaa\x79\xa8\x62\x56\xc1\xde";
-tpm_util_pcrread(src_qemu, tpm_util_crb_transfer, tpm_pcrread_resp,
- sizeof(tpm_pcrread_resp));
-
-tpm_util_migrate(src_qemu, ts->uri);
-tpm_util_wait_for_migration_complete(src_qemu);
-
-tpm_util_pcrread(dst_qemu, tpm_util_crb_transfer, tpm_pcrread_resp,
- sizeof(tpm_pcrread_resp));
-
-qtest_quit(dst_qemu);
-qtest_quit(src_qemu);
-
-tpm_util_swtpm_kill(dst_tpm_pid);
[Qemu-devel] [Bug 1775366] Re: [Feature request] qemu-ga - Allow unexpected parameter
This sounds an awful lot like your hosting provider expects you to be
using a specialized version of qemu-ga which you are not using.
It is my opinion that it's dangerous for a client to accept partial
commands and try to execute them anyway, as those ignored parameters
drastically change the semantics of various commands.
We don't know what we don't know, so this doesn't sound safe.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1775366
Title:
[Feature request] qemu-ga - Allow unexpected parameter
Status in QEMU:
New
Bug description:
It whould be nice if the qemu-ga allowed received messages to contain
fields which is not part of the spec. In my example I have a host
which sends the following request:
{"execute":"guest-exec","arguments":{"path":"prl_nettool","capture-
output":true,"execute-in-shell":false,"arg":[...]}}
Right now this request is rejected with the following error:
{"error": {"class": "GenericError", "desc": "Parameter 'execute-in-
shell' is unexpected"}}
My situation is the hosting provider I use does have some customized
solution which sends some extra arguments. I have manually patched my
qemu-ga so it accepts the "execute-in-shell" parameter but I don't
think this should be necessary.
Instead of "Error" it should just be a "warning" returned to the user
of qemu-ga but the call should still be executed.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1775366/+subscriptions
Re: [Qemu-devel] [PATCH 4/7] ide: call ide_cmd_done from ide_transfer_stop
On 06/06/2018 04:09 PM, John Snow wrote:
> From: Paolo Bonzini
>
> The code can simply be moved to the sole caller that has notify == true.
>
> Signed-off-by: Paolo Bonzini
> Signed-off-by: John Snow
Reviewed-by: Philippe Mathieu-Daudé
> ---
> hw/ide/core.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 1a6cb337bf..54799ea6fb 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -548,26 +548,23 @@ static void ide_cmd_done(IDEState *s)
> }
>
> static void ide_transfer_halt(IDEState *s,
> - void(*end_transfer_func)(IDEState *),
> - bool notify)
> + void(*end_transfer_func)(IDEState *))
> {
> s->end_transfer_func = end_transfer_func;
> s->data_ptr = s->io_buffer;
> s->data_end = s->io_buffer;
> s->status &= ~DRQ_STAT;
> -if (notify) {
> -ide_cmd_done(s);
> -}
> }
>
> void ide_transfer_stop(IDEState *s)
> {
> -ide_transfer_halt(s, ide_transfer_stop, true);
> +ide_transfer_halt(s, ide_transfer_stop);
> +ide_cmd_done(s);
> }
>
> static void ide_transfer_cancel(IDEState *s)
> {
> -ide_transfer_halt(s, ide_transfer_cancel, false);
> +ide_transfer_halt(s, ide_transfer_cancel);
> }
>
> int64_t ide_get_sector(IDEState *s)
>
Re: [Qemu-devel] [PATCH 1/7] libqos/ahci: track sector size
On 06/06/2018 04:09 PM, John Snow wrote:
> It's not always 512, and it does wind up mattering for PIO tranfers,
> because this means DRQ blocks are four times as big for ATAPI.
> Replace an instance of 2048 with the correct define, too.
>
> This patch by itself winds changing no behavior. fis->count is ignored
> for CMD_PACKET, and sect_count only gets used in non-ATAPI cases.
>
> Signed-off-by: John Snow
Reviewed-by: Philippe Mathieu-Daudé
> ---
> tests/libqos/ahci.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index bc201d762b..63e1f9b92d 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -90,6 +90,7 @@ struct AHCICommand {
> uint32_t interrupts;
> uint64_t xbytes;
> uint32_t prd_size;
> +uint32_t sector_size;
> uint64_t buffer;
> AHCICommandProp *props;
> /* Data to be transferred to the guest */
> @@ -796,7 +797,7 @@ static void command_header_init(AHCICommand *cmd)
> static void command_table_init(AHCICommand *cmd)
> {
> RegH2DFIS *fis = &(cmd->fis);
> -uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +uint16_t sect_count = (cmd->xbytes / cmd->sector_size);
>
> fis->fis_type = REG_H2D_FIS;
> fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */
> @@ -819,7 +820,7 @@ static void command_table_init(AHCICommand *cmd)
> if (cmd->props->lba28 || cmd->props->lba48) {
> fis->device = ATA_DEVICE_LBA;
> }
> -fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +fis->count = (cmd->xbytes / cmd->sector_size);
> }
> fis->icc = 0x00;
> fis->control = 0x00;
> @@ -857,6 +858,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> cmd->xbytes = props->size;
> cmd->prd_size = 4096;
> cmd->buffer = 0xabad1dea;
> +cmd->sector_size = props->atapi ? ATAPI_SECTOR_SIZE : AHCI_SECTOR_SIZE;
>
> if (!cmd->props->ncq) {
> cmd->interrupts = AHCI_PX_IS_DHRS;
> @@ -1033,7 +1035,7 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t
> buffer)
> static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
> {
> unsigned char *cbd = cmd->atapi_cmd;
> -uint64_t nsectors = xbytes / 2048;
> +uint64_t nsectors = xbytes / ATAPI_SECTOR_SIZE;
> uint32_t tmp;
> g_assert(cbd);
>
> @@ -1080,7 +1082,7 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t
> xbytes,
> cmd->prd_size = prd_size;
> }
> cmd->xbytes = xbytes;
> -sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +sect_count = (cmd->xbytes / cmd->sector_size);
>
> if (cmd->props->ncq) {
> NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
>
Re: [Qemu-devel] [PATCH 00/11] misc: Add trailing '\n' to qemu_log() calls
On 06/06/2018 11:21 AM, Philippe Mathieu-Daudé wrote: > Nothing very exciting here. > I sometimes miss to notice some trace events, running with -d unimp,trace... > then using 'grep ^...'. This is only due to a missing '\n' :) > so error_setg must be used WITHOUT \n and logging must happen with \n? If we're sure that's the way we want to have things laid out, we really ought to augment checkpatch to catch this -- because there's 0% chance that we'll keep it straight on our own otherwise. --js > Philippe Mathieu-Daudé (11): > hw/sd/milkymist-memcard: Add trailing '\n' to qemu_log() call > hw/digic: Add trailing '\n' to qemu_log() calls > xilinx-dp: Add trailing '\n' to qemu_log() call > ppc/pnv: Add trailing '\n' to qemu_log() calls > hw/core/register: Add trailing '\n' to qemu_log() call > hw/mips/boston: Add trailing '\n' to qemu_log() calls > stellaris: Add trailing '\n' to qemu_log() calls > target/arm: Add trailing '\n' to qemu_log() calls > target/m68k: Add trailing '\n' to qemu_log() call > RISC-V: Add trailing '\n' to qemu_log() calls > RFC target/xtensa: Add trailing '\n' to qemu_log() calls > > hw/arm/stellaris.c| 11 ++- > hw/char/digic-uart.c | 4 ++-- > hw/core/register.c| 2 +- > hw/display/xlnx_dp.c | 4 +++- > hw/mips/boston.c | 8 > hw/ppc/pnv_core.c | 4 ++-- > hw/sd/milkymist-memcard.c | 2 +- > hw/timer/digic-timer.c| 4 ++-- > target/arm/helper.c | 4 ++-- > target/m68k/translate.c | 2 +- > target/riscv/op_helper.c | 6 -- > target/xtensa/translate.c | 6 +++--- > 12 files changed, 31 insertions(+), 26 deletions(-) >
Re: [Qemu-devel] [PATCH 5/7] ide: make ide_transfer_stop idempotent
On 06/06/2018 04:09 PM, John Snow wrote:
> From: Paolo Bonzini
>
> There is code checking s->end_transfer_func and it was not taught about
> ide_transfer_cancel. We can just use ide_transfer_stop because
> s->end_transfer_func is only ever called in the DRQ phase.
>
> ide_transfer_cancel can then be removed, since it would just be
> calling ide_transfer_halt.
>
> Signed-off-by: Paolo Bonzini
> Signed-off-by: John Snow
Reviewed-by: Philippe Mathieu-Daudé
> ---
> hw/ide/core.c | 14 --
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 54799ea6fb..9c4864ae54 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -547,10 +547,9 @@ static void ide_cmd_done(IDEState *s)
> }
> }
>
> -static void ide_transfer_halt(IDEState *s,
> - void(*end_transfer_func)(IDEState *))
> +static void ide_transfer_halt(IDEState *s)
> {
> -s->end_transfer_func = end_transfer_func;
> +s->end_transfer_func = ide_transfer_stop;
> s->data_ptr = s->io_buffer;
> s->data_end = s->io_buffer;
> s->status &= ~DRQ_STAT;
> @@ -558,15 +557,10 @@ static void ide_transfer_halt(IDEState *s,
>
> void ide_transfer_stop(IDEState *s)
> {
> -ide_transfer_halt(s, ide_transfer_stop);
> +ide_transfer_halt(s);
> ide_cmd_done(s);
> }
>
> -static void ide_transfer_cancel(IDEState *s)
> -{
> -ide_transfer_halt(s, ide_transfer_cancel);
> -}
> -
> int64_t ide_get_sector(IDEState *s)
> {
> int64_t sector_num;
> @@ -1361,7 +1355,7 @@ static bool cmd_nop(IDEState *s, uint8_t cmd)
> static bool cmd_device_reset(IDEState *s, uint8_t cmd)
> {
> /* Halt PIO (in the DRQ phase), then DMA */
> -ide_transfer_cancel(s);
> +ide_transfer_halt(s);
> ide_cancel_dma_sync(s);
>
> /* Reset any PIO commands, reset signature, etc */
>
Re: [Qemu-devel] [libvirt] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
On 6/6/2018 11:52 AM, Ján Tomko wrote: On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote: On 6/4/2018 7:06 PM, Jason Wang wrote: On 2018年06月05日 09:41, Samudrala, Sridhar wrote: Ping on this patch now that the kernel patches are accepted into davem's net-next tree. https://patchwork.ozlabs.org/cover/920005/ On 5/7/2018 4:09 PM, Sridhar Samudrala wrote: This feature bit can be used by hypervisor to indicate virtio_net device to act as a standby for another device with the same MAC address. I tested this with a small change to the patch to mark the STANDBY feature 'true' by default as i am using libvirt to start the VMs. Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt XML file? Maybe you can try qemu command line passthrough: https://libvirt.org/drvqemu.html#qemucommand It looks like this can be used to pass command line arguments to qemu. Is it possible to specify a virtio specific attribute via this method? Yes, for testing purposes you should be able to do this via using QEMU's -set command line argument: http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html i.e.: xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'> ... Thanks. Will try this. For ex: to say mrg_rxbuf is off we can add the following line to virtio section of the domain xml file. I think libvirt needs to be extended to to support the new 'standby' attribute via this mechanism. Adding Liane Stump and libvirt to the CC list. *Laine Michael, Can we start with getting this patch into Qemu and an update to libvirt to support the 'standby' feature so that this feature can be enabled via some scripts/orchestration layer for now. We could improve this solution by enhancing Qemu to do automatic management of the addition/deletion of the primary device based on feature negotiation as a later patch. If that means the libvirt attribute would no longer be needed, I don't see the reason to add it to libvirt in the first place. I think we still need this attribute supported via libvirt so that a user/admin can enable this feature via domain XML specification.
[Qemu-devel] [PATCH v2 3/3] iotests: Add case for a corrupted inactive image
Reviewed-by: John Snow
Tested-by: Jeff Cody
Reviewed-by: Jeff Cody
Signed-off-by: Max Reitz
---
tests/qemu-iotests/060 | 30 ++
tests/qemu-iotests/060.out | 14 ++
2 files changed, 44 insertions(+)
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 6c7407f499..7bdf609f3f 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -440,6 +440,36 @@ echo "{'execute': 'qmp_capabilities'}
-drive if=none,node-name=drive,file="$TEST_IMG",driver=qcow2 \
| _filter_qmp | _filter_qemu_io
+echo
+echo "=== Testing incoming inactive corrupted image ==="
+echo
+
+_make_test_img 64M
+# Create an unaligned L1 entry, so qemu will signal a corruption when
+# reading from the covered area
+poke_file "$TEST_IMG" "$l1_offset" "\x00\x00\x00\x00\x2a\x2a\x2a\x2a"
+
+# Inactive images are effectively read-only images, so this should be a
+# non-fatal corruption (which does not modify the image)
+echo "{'execute': 'qmp_capabilities'}
+ {'execute': 'human-monitor-command',
+ 'arguments': {'command-line': 'qemu-io drive \"read 0 512\"'}}
+ {'execute': 'quit'}" \
+| $QEMU -qmp stdio -nographic -nodefaults \
+-blockdev "{'node-name': 'drive',
+'driver': 'qcow2',
+'file': {
+'driver': 'file',
+'filename': '$TEST_IMG'
+}}" \
+-incoming exec:'cat /dev/null' \
+2>&1 \
+| _filter_qmp | _filter_qemu_io
+
+echo
+# Image should not have been marked corrupt
+_img_info --format-specific | grep 'corrupt:'
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5f4264cff6..bff023d889 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -420,4 +420,18 @@ write failed: Input/output error
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
+
+=== Testing incoming inactive corrupted image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+QMP_VERSION
+{"return": {}}
+qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0);
further non-fatal corruption events will be suppressed
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset
0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
+read failed: Input/output error
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
+
+corrupt: false
*** done
--
2.17.0
Re: [Qemu-devel] [PATCH v4] iotests: Fix 219's timing
On 2018-06-06 21:06, Max Reitz wrote: > 219 has two issues that may lead to sporadic failure, both of which are > the result of issuing query-jobs too early after a job has been > modified. This can then lead to different results based on whether the > modification has taken effect already or not. > > First, query-jobs is issued right after the job has been created. > Besides its current progress possibly being in any random state (which > has already been taken care of), its total progress too is basically > arbitrary, because the job may not yet have been able to determine it. > This patch addresses this by just filtering the total progress, like > what has been done for the current progress already. However, for more > clarity, the filtering is changed to replace the values by a string > 'FILTERED' instead of deleting them. > > Secondly, query-jobs is issued right after a job has been resumed. The > job may or may not yet have had the time to actually perform any I/O, > and thus its current progress may or may not have advanced. To make > sure it has indeed advanced (which is what the reference output already > assumes), keep querying it until it has. > > Signed-off-by: Max Reitz > --- > v4: Drop the "import time" that has become unnecessary > > v3: Keep querying until the job has advanced instead of waiting for a > fixed amount of time [Peter in v2, Eric in v1] > --- > tests/qemu-iotests/219 | 26 -- > tests/qemu-iotests/219.out | 10 +- > 2 files changed, 25 insertions(+), 11 deletions(-) Applied to my block branch. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 0/3] qcow2: Do not mark inactive images corrupt
On 2018-06-06 21:36, Max Reitz wrote: > The non-public logs in > https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal > this problem: > > $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry) > $ echo 'qemu-io none0 "read 0 512"' \ > | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \ > -monitor stdio \ > -incoming exec:'cat /dev/null' > QEMU 2.12.50 monitor - type 'help' for more information > (qemu) qemu-io none0 "read 0 512" > qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: > 0); further corruption events will be suppressed > qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion > `!(bs->open_flags & BDRV_O_INACTIVE)' failed. > [1]18444 done echo 'qemu-io none0 "read 0 512"' | >18445 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -drive > if=none,file=foo.qcow2 -monitor stdi > > Oops. And I forgot to CC qemu-stable this time. Oops ×2 signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 0/3] qcow2: Do not mark inactive images corrupt
The non-public logs in https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal this problem: $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry) $ echo 'qemu-io none0 "read 0 512"' \ | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \ -monitor stdio \ -incoming exec:'cat /dev/null' QEMU 2.12.50 monitor - type 'help' for more information (qemu) qemu-io none0 "read 0 512" qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: 0); further corruption events will be suppressed qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed. [1]18444 done echo 'qemu-io none0 "read 0 512"' | 18445 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 -monitor stdi Oops. The first patch in this series makes a function public that the second patch uses to fix the issue by treating all non-writable images like read-only images (yes, there is a difference...) in this regard (which most importantly means not trying to set the corrupt flag on them). Inactive images count as non-writable images, but not as read-only images, so that fixes it. The third patch adds an iotest case. v2: - Use bdrv_is_writable() instead of copying its functionality [Jeff] git-backport-diff against v1: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/3:[down] 'block: Make bdrv_is_writable() public' 002/3:[0004] [FC] 'qcow2: Do not mark inactive images corrupt' 003/3:[] [--] 'iotests: Add case for a corrupted inactive image' Max Reitz (3): block: Make bdrv_is_writable() public qcow2: Do not mark inactive images corrupt iotests: Add case for a corrupted inactive image include/block/block.h | 1 + block.c| 17 ++--- block/qcow2.c | 2 +- tests/qemu-iotests/060 | 30 ++ tests/qemu-iotests/060.out | 14 ++ 5 files changed, 60 insertions(+), 4 deletions(-) -- 2.17.0
[Qemu-devel] [PATCH v2 2/3] qcow2: Do not mark inactive images corrupt
When signaling a corruption on a read-only image, qcow2 already makes fatal events non-fatal (i.e., they will not result in the image being closed, and the image header's corrupt flag will not be set). This is necessary because we cannot set the corrupt flag on read-only images, and it is possible because further corruption of read-only images is impossible. Inactive images are effectively read-only, too, so we should do the same for them. bdrv_is_writable() can tell us whether an image can actually be written to, so use its result instead of !bs->read_only. (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in bdrv_co_pwritev() will fail, crashing qemu.) Cc: qemu-sta...@nongnu.org Signed-off-by: Max Reitz --- block/qcow2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2.c b/block/qcow2.c index 6b2d88759d..6fa5e1d71a 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4569,7 +4569,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, char *message; va_list ap; -fatal = fatal && !bs->read_only; +fatal = fatal && bdrv_is_writable(bs); if (s->signaled_corruption && (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT))) -- 2.17.0
[Qemu-devel] [PATCH v2 1/3] block: Make bdrv_is_writable() public
This is a useful function for the whole block layer, so make it public.
At the same time, users outside of block.c probably do not need to make
use of the reopen functionality, so rename the current function to
bdrv_is_writable_after_reopen() create a new bdrv_is_writable() function
that just passes NULL to it for the reopen queue.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz
---
include/block/block.h | 1 +
block.c | 17 ++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 4dd4f1eab2..e677080c4e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -408,6 +408,7 @@ bool bdrv_is_read_only(BlockDriverState *bs);
int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
bool ignore_allow_rdw, Error **errp);
int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+bool bdrv_is_writable(BlockDriverState *bs);
bool bdrv_is_sg(BlockDriverState *bs);
bool bdrv_is_inserted(BlockDriverState *bs);
void bdrv_lock_medium(BlockDriverState *bs, bool locked);
diff --git a/block.c b/block.c
index 9d577f65bb..50887087f3 100644
--- a/block.c
+++ b/block.c
@@ -1620,13 +1620,24 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q,
BlockDriverState *bs)
/* Returns whether the image file can be written to after the reopen queue @q
* has been successfully applied, or right now if @q is NULL. */
-static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)
+static bool bdrv_is_writable_after_reopen(BlockDriverState *bs,
+ BlockReopenQueue *q)
{
int flags = bdrv_reopen_get_flags(q, bs);
return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR;
}
+/*
+ * Return whether the BDS can be written to. This is not necessarily
+ * the same as !bdrv_is_read_only(bs), as inactivated images may not
+ * be written to but do not count as read-only images.
+ */
+bool bdrv_is_writable(BlockDriverState *bs)
+{
+return bdrv_is_writable_after_reopen(bs, NULL);
+}
+
static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
BdrvChild *c, const BdrvChildRole *role,
BlockReopenQueue *reopen_queue,
@@ -1664,7 +1675,7 @@ static int bdrv_check_perm(BlockDriverState *bs,
BlockReopenQueue *q,
/* Write permissions never work with read-only images */
if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
-!bdrv_is_writable(bs, q))
+!bdrv_is_writable_after_reopen(bs, q))
{
error_setg(errp, "Block node is read-only");
return -EPERM;
@@ -1956,7 +1967,7 @@ void bdrv_format_default_perms(BlockDriverState *bs,
BdrvChild *c,
, );
/* Format drivers may touch metadata even if the guest doesn't write */
-if (bdrv_is_writable(bs, reopen_queue)) {
+if (bdrv_is_writable_after_reopen(bs, reopen_queue)) {
perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
}
--
2.17.0
Re: [Qemu-devel] [RFC PATCH] configure: Enable out-of-tree acceptance tests
On 06/06/2018 04:24 PM, Eduardo Habkost wrote: > On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote: > [... something about config files ...] >>> And after that, the following would run all "console" tests: >>> >>> avocado run -t console >>> >>> How does this sound? >> >> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about >> use_test_dir_when_no_references_given. > > Well, I can't give an opinion because I couldn't understand > what's the final goal here. You cut too much, the relevant part is: On 05/30/2018 10:06 PM, Cleber Rosa wrote: > But at the same, there are security implications: `list` won't > load/execute any test code (different from, say, standard Python > unittests), while "run" obviously will. So "avocado run" may end up > running what users don't want if a malicious user controls > "$avocado_datadir_paths_test_dir". > > What exactly is missing in the current solution? Why > "avocado run -t console ." wouldn't work? It doesn't work in out-of-tree builds, I have to use the full path: >> build_dir$ avocado run /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py Don't worry about this RFC patch, I don't have problem to use the full path for now, I suppose the Avocado team will resolve this differently via a configurable option. Regards, Phil.
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qcow2: Do not mark inactive images corrupt
On 2018-06-04 20:58, John Snow wrote: > > > On 06/04/2018 10:14 AM, Max Reitz wrote: >> The non-public logs in >> https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal >> this problem: >> >> $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry) >> $ echo 'qemu-io none0 "read 0 512"' \ >> | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \ >> -monitor stdio \ >> -incoming exec:'cat /dev/null' >> QEMU 2.12.50 monitor - type 'help' for more information >> (qemu) qemu-io none0 "read 0 512" >> qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 >> index: 0); further corruption events will be suppressed >> qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion >> `!(bs->open_flags & BDRV_O_INACTIVE)' failed. >> [1]18444 done echo 'qemu-io none0 "read 0 512"' | >>18445 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -drive >> if=none,file=foo.qcow2 -monitor stdi >> >> Oops. >> >> >> The first patch in this series fixes this by treating inactive images >> like read-only images in this regard (which most importantly means not >> trying to set the corrupt flag on them), the second one adds an iotest >> case. >> >> >> Max Reitz (2): >> qcow2: Do not mark inactive images corrupt >> iotests: Add case for a corrupted inactive image >> >> block/qcow2.c | 4 +++- >> tests/qemu-iotests/060 | 30 ++ >> tests/qemu-iotests/060.out | 14 ++ >> 3 files changed, 47 insertions(+), 1 deletion(-) >> > > Makes sense to me, provided it's safe to check via BDRV_O_RDWR instead > of bs->read_only. (I assume it is.) It's all a mess is all I can say. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4] iotests: Fix 219's timing
On 06/06/2018 02:06 PM, Max Reitz wrote: 219 has two issues that may lead to sporadic failure, both of which are the result of issuing query-jobs too early after a job has been modified. This can then lead to different results based on whether the modification has taken effect already or not. First, query-jobs is issued right after the job has been created. Besides its current progress possibly being in any random state (which has already been taken care of), its total progress too is basically arbitrary, because the job may not yet have been able to determine it. This patch addresses this by just filtering the total progress, like what has been done for the current progress already. However, for more clarity, the filtering is changed to replace the values by a string 'FILTERED' instead of deleting them. Secondly, query-jobs is issued right after a job has been resumed. The job may or may not yet have had the time to actually perform any I/O, and thus its current progress may or may not have advanced. To make sure it has indeed advanced (which is what the reference output already assumes), keep querying it until it has. Signed-off-by: Max Reitz --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Qemu-devel] [RFC PATCH v2] qmp.py: Fix exception parsing partial JSON
The readline() call returns partial data.
Keep appending until the JSON buffer is complete.
This fixes:
$ scripts/qmp/qmp-shell -v -p /tmp/qmp.sock
Traceback (most recent call last):
File "scripts/qmp/qmp-shell", line 456, in
main()
File "scripts/qmp/qmp-shell", line 441, in main
qemu.connect(negotiate)
File "scripts/qmp/qmp-shell", line 284, in connect
self._greeting = super(QMPShell, self).connect(negotiate)
File "scripts/qmp/qmp.py", line 143, in connect
return self.__negotiate_capabilities()
File "scripts/qmp/qmp.py", line 71, in __negotiate_capabilities
greeting = self.__json_read()
File "scripts/qmp/qmp.py", line 85, in __json_read
resp = json.loads(data)
File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
return _default_decoder.decode(s)
File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
obj, end = self.scan_once(s, idx)
ValueError: Expecting object: line 1 column 3 (char 2)
Signed-off-by: Philippe Mathieu-Daudé
---
Since v1:
- addressed Daniel review: clean data after json.loads() succeeds
- add a XXX comment
Daniel suggested this is due to blocking i/o.
I'm sure there is a nicer/more pythonic way to do this, but this works for me,
sorry :)
scripts/qmp/qmp.py | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 5c8cf6a056..14f0b48936 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -78,11 +78,18 @@ class QEMUMonitorProtocol(object):
raise QMPCapabilitiesError
def __json_read(self, only_event=False):
+data = ""
while True:
-data = self.__sockfile.readline()
-if not data:
+tmp = self.__sockfile.readline()
+if not tmp:
return
-resp = json.loads(data)
+data += tmp
+try:
+resp = json.loads(data)
+except ValueError:
+# XXX: blindly loop, even if QEMU ever sends malformed data
+continue
+data = ""
if 'event' in resp:
self.logger.debug("<<< %s", resp)
self.__events.append(resp)
--
2.17.1
Re: [Qemu-devel] [PATCH v4 0/5] Acceptance/functional tests
On Tue, Jun 05, 2018 at 11:49:07AM -0300, Philippe Mathieu-Daudé wrote: > Hi Eduardo, > > On 05/30/2018 03:41 PM, Cleber Rosa wrote: > > TL;DR > > = > > > > Another version, with a minimalist approach, of the acceptance tests > > infrastructure for QEMU, based on the Avocado Testing Framework. > > What do you think of this series? > > Are you OK to take it via your tree, or should it goes via Fam's "Build > and test automation"? I can take it via my tree, I just couldn't reserve the time to check the existing review comments and finish reviewing it. But if Fam or other maintainer already thinks the series look good and can be merged, please don't delay this because of me. Acked-by: Eduardo Habkost -- Eduardo
Re: [Qemu-devel] [RFC PATCH] configure: Enable out-of-tree acceptance tests
On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote: [... something about config files ...] > > And after that, the following would run all "console" tests: > > > > avocado run -t console > > > > How does this sound? > > For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about > use_test_dir_when_no_references_given. Well, I can't give an opinion because I couldn't understand what's the final goal here. What exactly is missing in the current solution? Why "avocado run -t console ." wouldn't work? -- Eduardo
[Qemu-devel] [PATCH] hw/i2c: Add trace events
Signed-off-by: Philippe Mathieu-Daudé
---
$ qemu-system ... -d trace:i2c_recv,trace:i2c_send
4486@1528311614.709959:i2c_recv recv(addr:0x50) data:0x00
4486@1528311614.709994:i2c_send send(addr:0x50) data:0x05
4486@1528311614.710060:i2c_recv recv(addr:0x50) data:0x02
4486@1528311614.710161:i2c_recv recv(addr:0x50) data:0x40
4486@1528311614.710185:i2c_send send(addr:0x50) data:0x02
4486@1528311614.710233:i2c_recv recv(addr:0x50) data:0x08
4486@1528311614.710380:i2c_recv recv(addr:0x50) data:0x0d
4486@1528311614.710396:i2c_send send(addr:0x50) data:0x1f
4486@1528311614.710437:i2c_recv recv(addr:0x50) data:0x40
or
$ qemu-system ... -d trace:i2c\*
4486@1528311614.698315:i2c_event start(addr:0x50)
4486@1528311614.698338:i2c_recv recv(addr:0x50) data:0x82
4486@1528311614.698349:i2c_event finish(addr:0x50)
4486@1528311614.698354:i2c_event start(addr:0x50)
4486@1528311614.698357:i2c_send send(addr:0x50) data:0x11
4486@1528311614.698377:i2c_event finish(addr:0x50)
4486@1528311614.698380:i2c_event start(addr:0x50)
4486@1528311614.698402:i2c_recv recv(addr:0x50) data:0x04
4486@1528311614.698485:i2c_event finish(addr:0x50)
Makefile.objs | 1 +
hw/i2c/core.c | 25 ++---
hw/i2c/trace-events | 7 +++
3 files changed, 26 insertions(+), 7 deletions(-)
create mode 100644 hw/i2c/trace-events
diff --git a/Makefile.objs b/Makefile.objs
index 2c8cb72407..7a9828da28 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -213,6 +213,7 @@ trace-events-subdirs += hw/char
trace-events-subdirs += hw/display
trace-events-subdirs += hw/dma
trace-events-subdirs += hw/hppa
+trace-events-subdirs += hw/i2c
trace-events-subdirs += hw/i386
trace-events-subdirs += hw/i386/xen
trace-events-subdirs += hw/ide
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index ab72d5bf2b..b54725985a 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -9,6 +9,7 @@
#include "qemu/osdep.h"
#include "hw/i2c/i2c.h"
+#include "trace.h"
#define I2C_BROADCAST 0x00
@@ -130,14 +131,16 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int
recv)
}
QLIST_FOREACH(node, >current_devs, next) {
+I2CSlave *s = node->elt;
int rv;
-sc = I2C_SLAVE_GET_CLASS(node->elt);
+sc = I2C_SLAVE_GET_CLASS(s);
/* If the bus is already busy, assume this is a repeated
start condition. */
if (sc->event) {
-rv = sc->event(node->elt, recv ? I2C_START_RECV : I2C_START_SEND);
+trace_i2c_event("start", s->address);
+rv = sc->event(s, recv ? I2C_START_RECV : I2C_START_SEND);
if (rv && !bus->broadcast) {
if (bus_scanned) {
/* First call, terminate the transfer. */
@@ -156,9 +159,11 @@ void i2c_end_transfer(I2CBus *bus)
I2CNode *node, *next;
QLIST_FOREACH_SAFE(node, >current_devs, next, next) {
-sc = I2C_SLAVE_GET_CLASS(node->elt);
+I2CSlave *s = node->elt;
+sc = I2C_SLAVE_GET_CLASS(s);
if (sc->event) {
-sc->event(node->elt, I2C_FINISH);
+trace_i2c_event("finish", s->address);
+sc->event(s, I2C_FINISH);
}
QLIST_REMOVE(node, next);
g_free(node);
@@ -169,14 +174,17 @@ void i2c_end_transfer(I2CBus *bus)
int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
{
I2CSlaveClass *sc;
+I2CSlave *s;
I2CNode *node;
int ret = 0;
if (send) {
QLIST_FOREACH(node, >current_devs, next) {
-sc = I2C_SLAVE_GET_CLASS(node->elt);
+s = node->elt;
+sc = I2C_SLAVE_GET_CLASS(s);
if (sc->send) {
-ret = ret || sc->send(node->elt, *data);
+trace_i2c_send(s->address, *data);
+ret = ret || sc->send(s, *data);
} else {
ret = -1;
}
@@ -189,7 +197,9 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(>current_devs)->elt);
if (sc->recv) {
-ret = sc->recv(QLIST_FIRST(>current_devs)->elt);
+s = QLIST_FIRST(>current_devs)->elt;
+ret = sc->recv(s);
+trace_i2c_recv(s->address, ret);
if (ret < 0) {
return ret;
} else {
@@ -226,6 +236,7 @@ void i2c_nack(I2CBus *bus)
QLIST_FOREACH(node, >current_devs, next) {
sc = I2C_SLAVE_GET_CLASS(node->elt);
if (sc->event) {
+trace_i2c_event("nack", node->elt->address);
sc->event(node->elt, I2C_NACK);
}
}
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
new file mode 100644
index 00..d339b61202
--- /dev/null
+++ b/hw/i2c/trace-events
@@ -0,0 +1,7 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# hw/i2c/core.c
+
+i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
+i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
+i2c_recv(uint8_t
[Qemu-devel] [PATCH 7/7] ide: introduce ide_transfer_start_norecurse
From: Paolo Bonzini
For the case where the end_transfer_func is also the caller of
ide_transfer_start, the mutual recursion can lead to unlimited
stack usage. Introduce a new version that can be used to change
tail recursion into a loop, and use it in trace_ide_atapi_cmd_reply_end.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/atapi.c| 42 +++---
hw/ide/core.c | 16
include/hw/ide/internal.h | 2 ++
3 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 7168ff55a7..39e473f9c2 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -245,15 +245,11 @@ static uint16_t atapi_byte_count_limit(IDEState *s)
void ide_atapi_cmd_reply_end(IDEState *s)
{
int byte_count_limit, size, ret;
-trace_ide_atapi_cmd_reply_end(s, s->packet_transfer_size,
- s->elementary_transfer_size,
- s->io_buffer_index);
-if (s->packet_transfer_size <= 0) {
-/* end of transfer */
-ide_atapi_cmd_ok(s);
-ide_set_irq(s->bus);
-trace_ide_atapi_cmd_reply_end_eot(s, s->status);
-} else {
+while (s->packet_transfer_size > 0) {
+trace_ide_atapi_cmd_reply_end(s, s->packet_transfer_size,
+ s->elementary_transfer_size,
+ s->io_buffer_index);
+
/* see if a new sector must be read */
if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
if (!s->elementary_transfer_size) {
@@ -279,11 +275,6 @@ void ide_atapi_cmd_reply_end(IDEState *s)
size = s->cd_sector_size - s->io_buffer_index;
if (size > s->elementary_transfer_size)
size = s->elementary_transfer_size;
-s->packet_transfer_size -= size;
-s->elementary_transfer_size -= size;
-s->io_buffer_index += size;
-ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
- size, ide_atapi_cmd_reply_end);
} else {
/* a new transfer is needed */
s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
@@ -306,13 +297,26 @@ void ide_atapi_cmd_reply_end(IDEState *s)
size = (s->cd_sector_size - s->io_buffer_index);
}
trace_ide_atapi_cmd_reply_end_new(s, s->status);
-s->packet_transfer_size -= size;
-s->elementary_transfer_size -= size;
-s->io_buffer_index += size;
-ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
- size, ide_atapi_cmd_reply_end);
+}
+s->packet_transfer_size -= size;
+s->elementary_transfer_size -= size;
+s->io_buffer_index += size;
+
+/* Some adapters process PIO data right away. In that case, we need
+ * to avoid mutual recursion between ide_transfer_start
+ * and ide_atapi_cmd_reply_end.
+ */
+if (!ide_transfer_start_norecurse(s,
+ s->io_buffer + s->io_buffer_index -
size,
+ size, ide_atapi_cmd_reply_end)) {
+return;
}
}
+
+/* end of transfer */
+trace_ide_atapi_cmd_reply_end_eot(s, s->status);
+ide_atapi_cmd_ok(s);
+ide_set_irq(s->bus);
}
/* send a reply of 'size' bytes in s->io_buffer to an ATAPI command */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9c4864ae54..2c62efc536 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -523,8 +523,8 @@ static void ide_clear_retry(IDEState *s)
}
/* prepare data transfer and tell what to do after */
-void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
-EndTransferFunc *end_transfer_func)
+bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
+ EndTransferFunc *end_transfer_func)
{
s->data_ptr = buf;
s->data_end = buf + size;
@@ -534,10 +534,18 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int
size,
}
if (!s->bus->dma->ops->pio_transfer) {
s->end_transfer_func = end_transfer_func;
-return;
+return false;
}
s->bus->dma->ops->pio_transfer(s->bus->dma);
-end_transfer_func(s);
+return true;
+}
+
+void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
+EndTransferFunc *end_transfer_func)
+{
+if (ide_transfer_start_norecurse(s, buf, size, end_transfer_func)) {
+end_transfer_func(s);
+}
}
static void ide_cmd_done(IDEState *s)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index f3de6f9b73..594081e57f 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -623,6 +623,8 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val);
void
Re: [Qemu-devel] [PATCH 0/7] atapi: change unlimited recursion to while loop
MEH I forgot to v2 this. On 06/06/2018 03:09 PM, John Snow wrote: > Real hardware doesn't have an unlimited stack, so the unlimited > recursion in the ATAPI code smells a bit. In fact, the call to > ide_transfer_start easily becomes a tail call with a small change > to the code (patch 5); however, we also need to turn the call back to > ide_atapi_cmd_reply_end into another tail call before turning the > (double) tail recursion into a while loop. > > In particular, patch 1 ensures that the call to the end_transfer_func is > the last thing in ide_transfer_start. To do so, it moves the write of > the PIO Setup FIS before the PIO transfer, which actually makes sense: > the FIS is sent by the device to inform the AHCI about the transfer, > so it cannot come after! This is the main change from the RFC, and > it simplifies the rest of the series (the RFC had to introduce an > "end_transfer" callback just for writing the PIO Setup FIS). > > I tested this manually with READ CD commands sent through sg_raw, > and the existing AHCI tests still pass. > > v2: reworked PIO Setup FIS based on spec reading, adjusted tests. > > John Snow (2): > libqos/ahci: track sector size > ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands > > Paolo Bonzini (5): > ide: push end_transfer_func out of start_transfer callback, rename > callback > ide: call ide_cmd_done from ide_transfer_stop > ide: make ide_transfer_stop idempotent > atapi: call ide_set_irq before ide_transfer_start > ide: introduce ide_transfer_start_norecurse > > hw/ide/ahci.c | 31 --- > hw/ide/atapi.c| 44 > hw/ide/core.c | 39 --- > hw/ide/trace-events | 2 +- > include/hw/ide/internal.h | 4 +++- > tests/libqos/ahci.c | 45 +++-- > tests/libqos/ahci.h | 3 +-- > 7 files changed, 88 insertions(+), 80 deletions(-) >
[Qemu-devel] [PATCH 3/7] ide: push end_transfer_func out of start_transfer callback, rename callback
From: Paolo Bonzini
Now that end_transfer_func is a tail call in ahci_start_transfer,
formalize the fact that the callback (of which ahci_start_transfer is
the sole implementation) takes care of the transfer too: rename it to
pio_transfer and, if it is present, call the end_transfer_func as soon
as it returns.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/ahci.c | 13 ++---
hw/ide/core.c | 8 +---
hw/ide/trace-events | 2 +-
include/hw/ide/internal.h | 2 +-
4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 5871333686..89127f5fb6 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1279,8 +1279,8 @@ out:
return 0;
}
-/* DMA dev <-> ram */
-static void ahci_start_transfer(IDEDMA *dma)
+/* Transfer PIO data between RAM and device */
+static void ahci_pio_transfer(IDEDMA *dma)
{
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
IDEState *s = >port.ifs[0];
@@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma)
has_sglist = 1;
}
-trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
- size, is_atapi ? "atapi" : "ata",
- has_sglist ? "" : "o");
+trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
+size, is_atapi ? "atapi" : "ata",
+has_sglist ? "" : "o");
if (has_sglist && size) {
if (is_write) {
@@ -1321,7 +1321,6 @@ static void ahci_start_transfer(IDEDMA *dma)
out:
/* declare that we processed everything */
s->data_ptr = s->data_end;
-s->end_transfer_func(s);
}
static void ahci_start_dma(IDEDMA *dma, IDEState *s,
@@ -1437,7 +1436,7 @@ static const IDEDMAOps ahci_dma_ops = {
.start_dma = ahci_start_dma,
.restart = ahci_restart,
.restart_dma = ahci_restart_dma,
-.start_transfer = ahci_start_transfer,
+.pio_transfer = ahci_pio_transfer,
.prepare_buf = ahci_dma_prepare_buf,
.commit_buf = ahci_commit_buf,
.rw_buf = ahci_dma_rw_buf,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index cc9ca28c33..1a6cb337bf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -526,16 +526,18 @@ static void ide_clear_retry(IDEState *s)
void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
EndTransferFunc *end_transfer_func)
{
-s->end_transfer_func = end_transfer_func;
s->data_ptr = buf;
s->data_end = buf + size;
ide_set_retry(s);
if (!(s->status & ERR_STAT)) {
s->status |= DRQ_STAT;
}
-if (s->bus->dma->ops->start_transfer) {
-s->bus->dma->ops->start_transfer(s->bus->dma);
+if (!s->bus->dma->ops->pio_transfer) {
+s->end_transfer_func = end_transfer_func;
+return;
}
+s->bus->dma->ops->pio_transfer(s->bus->dma);
+end_transfer_func(s);
}
static void ide_cmd_done(IDEState *s)
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 5c0e59ec42..153a7a62c9 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -101,7 +101,7 @@ handle_cmd_badport(void *s, int port) "ahci(%p)[%d]: guest
accessed unused port"
handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid
cmd FIS"
handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]:
dma_memory_map failed, 0x%02"PRIx64" != 0x80"
handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t
b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"
-ahci_start_transfer(void *s, int port, const char *rw, uint32_t size, const
char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist"
+ahci_pio_transfer(void *s, int port, const char *rw, uint32_t size, const char
*tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist"
ahci_start_dma(void *s, int port) "ahci(%p)[%d]: start dma"
ahci_dma_prepare_buf(void *s, int port, int32_t io_buffer_size, int32_t limit)
"ahci(%p)[%d]: prepare buf limit=%"PRId32" prepared=%"PRId32
ahci_dma_prepare_buf_fail(void *s, int port) "ahci(%p)[%d]: sglist population
failed"
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 88212f59df..f3de6f9b73 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -444,7 +444,7 @@ struct IDEState {
struct IDEDMAOps {
DMAStartFunc *start_dma;
-DMAVoidFunc *start_transfer;
+DMAVoidFunc *pio_transfer;
DMAInt32Func *prepare_buf;
DMAu32Func *commit_buf;
DMAIntFunc *rw_buf;
--
2.14.3
[Qemu-devel] [PATCH 6/7] atapi: call ide_set_irq before ide_transfer_start
From: Paolo Bonzini
The ATAPI_INT_REASON_IO interrupt is raised when I/O starts, but in the
AHCI case ide_set_irq was actually called at the end of a mutual recursion.
Move it early, with the side effect that ide_transfer_start becomes a tail
call in ide_atapi_cmd_reply_end.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/atapi.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index c0509c8bf5..7168ff55a7 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -287,6 +287,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
} else {
/* a new transfer is needed */
s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
+ide_set_irq(s->bus);
byte_count_limit = atapi_byte_count_limit(s);
trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
size = s->packet_transfer_size;
@@ -304,13 +305,12 @@ void ide_atapi_cmd_reply_end(IDEState *s)
if (size > (s->cd_sector_size - s->io_buffer_index))
size = (s->cd_sector_size - s->io_buffer_index);
}
-s->packet_transfer_size -= size;
-s->elementary_transfer_size -= size;
-s->io_buffer_index += size;
-ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
- size, ide_atapi_cmd_reply_end);
-ide_set_irq(s->bus);
trace_ide_atapi_cmd_reply_end_new(s, s->status);
+s->packet_transfer_size -= size;
+s->elementary_transfer_size -= size;
+s->io_buffer_index += size;
+ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
+ size, ide_atapi_cmd_reply_end);
}
}
}
--
2.14.3
[Qemu-devel] [PATCH 2/7] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
The PIO Setup FIS is written in the PIO:Entry state, which comes before
the ATA and ATAPI data transfer states. As a result, the PIO Setup FIS
interrupt is now raised before DMA ends for ATAPI commands, and tests have
to be adjusted.
This is also hinted by the description of the command header in the AHCI
specification, where the "A" bit is described as
When ‘1’, indicates that a PIO setup FIS shall be sent by the device
indicating a transfer for the ATAPI command.
and also by the description of the ACMD (ATAPI command region):
The ATAPI command must be either 12 or 16 bytes in length. The length
transmitted by the HBA is determined by the PIO setup FIS that is sent
by the device requesting the ATAPI command.
QEMU, which conflates the "generator" and the "receiver" of the FIS into
one device, always uses ATAPI_PACKET_SIZE, aka 12, for the length.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/ahci.c | 18 ++
tests/libqos/ahci.c | 35 +--
tests/libqos/ahci.h | 3 +--
3 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 24dbad5125..5871333686 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1198,7 +1198,6 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
g_free(pretty_fis);
}
s->dev[port].done_atapi_packet = false;
-/* XXX send PIO setup FIS */
}
ide_state->error = 0;
@@ -1292,10 +1291,12 @@ static void ahci_start_transfer(IDEDMA *dma)
int is_atapi = opts & AHCI_CMD_ATAPI;
int has_sglist = 0;
+/* PIO FIS gets written prior to transfer */
+ahci_write_fis_pio(ad, size);
+
if (is_atapi && !ad->done_atapi_packet) {
/* already prepopulated iobuffer */
ad->done_atapi_packet = true;
-size = 0;
goto out;
}
@@ -1315,19 +1316,12 @@ static void ahci_start_transfer(IDEDMA *dma)
}
}
-out:
-/* declare that we processed everything */
-s->data_ptr = s->data_end;
-
/* Update number of transferred bytes, destroy sglist */
dma_buf_commit(s, size);
-
+out:
+/* declare that we processed everything */
+s->data_ptr = s->data_end;
s->end_transfer_func(s);
-
-if (!(s->status & DRQ_STAT)) {
-/* done with PIO send/receive */
-ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
-}
}
static void ahci_start_dma(IDEDMA *dma, IDEState *s,
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 63e1f9b92d..7264e085d0 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -478,10 +478,10 @@ void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t
port, uint8_t slot)
g_free(d2h);
}
-void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
-uint8_t slot, size_t buffsize)
+void ahci_port_check_pio_sanity(AHCIQState *ahci, AHCICommand *cmd)
{
PIOSetupFIS *pio = g_malloc0(0x20);
+uint8_t port = cmd->port;
/* We cannot check the Status or E_Status registers, because
* the status may have again changed between the PIO Setup FIS
@@ -489,15 +489,22 @@ void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t
port,
qtest_memread(ahci->parent->qts, ahci->port[port].fb + 0x20, pio, 0x20);
g_assert_cmphex(pio->fis_type, ==, 0x5f);
-/* BUG: PIO Setup FIS as utilized by QEMU tries to fit the entire
- * transfer size in a uint16_t field. The maximum transfer size can
- * eclipse this; the field is meant to convey the size of data per
- * each Data FIS, not the entire operation as a whole. For now,
- * we will sanity check the broken case where applicable. */
-if (buffsize <= UINT16_MAX) {
-g_assert_cmphex(le16_to_cpu(pio->tx_count), ==, buffsize);
+/* Data transferred by PIO will either be:
+ * (1) 12 or 16 bytes for an ATAPI command packet (QEMU always uses 12), or
+ * (2) Actual data from the drive.
+ * If we do both, (2) winds up erasing any evidence of (1).
+ */
+if (cmd->props->atapi && (cmd->xbytes == 0 || cmd->props->dma)) {
+g_assert(le16_to_cpu(pio->tx_count) == 12 ||
+ le16_to_cpu(pio->tx_count) == 16);
+} else {
+/* The AHCI test suite here does not test any PIO command that
specifies
+ * a DRQ block larger than one sector (like 0xC4), so this should
always
+ * be one sector or less. */
+size_t pio_len = ((cmd->xbytes % cmd->sector_size) ?
+ (cmd->xbytes % cmd->sector_size) : cmd->sector_size);
+g_assert_cmphex(le16_to_cpu(pio->tx_count), ==, pio_len);
}
-
g_free(pio);
}
@@ -832,9 +839,9 @@ void ahci_command_enable_atapi_dma(AHCICommand *cmd)
RegH2DFIS *fis = &(cmd->fis);
g_assert(cmd->props->atapi);
fis->feature_low |= 0x01;
-cmd->interrupts &= ~AHCI_PX_IS_PSS;
+/* PIO is still used to transfer the ATAPI command
[Qemu-devel] [PATCH 0/7] atapi: change unlimited recursion to while loop
Real hardware doesn't have an unlimited stack, so the unlimited recursion in the ATAPI code smells a bit. In fact, the call to ide_transfer_start easily becomes a tail call with a small change to the code (patch 5); however, we also need to turn the call back to ide_atapi_cmd_reply_end into another tail call before turning the (double) tail recursion into a while loop. In particular, patch 1 ensures that the call to the end_transfer_func is the last thing in ide_transfer_start. To do so, it moves the write of the PIO Setup FIS before the PIO transfer, which actually makes sense: the FIS is sent by the device to inform the AHCI about the transfer, so it cannot come after! This is the main change from the RFC, and it simplifies the rest of the series (the RFC had to introduce an "end_transfer" callback just for writing the PIO Setup FIS). I tested this manually with READ CD commands sent through sg_raw, and the existing AHCI tests still pass. v2: reworked PIO Setup FIS based on spec reading, adjusted tests. John Snow (2): libqos/ahci: track sector size ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands Paolo Bonzini (5): ide: push end_transfer_func out of start_transfer callback, rename callback ide: call ide_cmd_done from ide_transfer_stop ide: make ide_transfer_stop idempotent atapi: call ide_set_irq before ide_transfer_start ide: introduce ide_transfer_start_norecurse hw/ide/ahci.c | 31 --- hw/ide/atapi.c| 44 hw/ide/core.c | 39 --- hw/ide/trace-events | 2 +- include/hw/ide/internal.h | 4 +++- tests/libqos/ahci.c | 45 +++-- tests/libqos/ahci.h | 3 +-- 7 files changed, 88 insertions(+), 80 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH 5/7] ide: make ide_transfer_stop idempotent
From: Paolo Bonzini
There is code checking s->end_transfer_func and it was not taught about
ide_transfer_cancel. We can just use ide_transfer_stop because
s->end_transfer_func is only ever called in the DRQ phase.
ide_transfer_cancel can then be removed, since it would just be
calling ide_transfer_halt.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/core.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 54799ea6fb..9c4864ae54 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -547,10 +547,9 @@ static void ide_cmd_done(IDEState *s)
}
}
-static void ide_transfer_halt(IDEState *s,
- void(*end_transfer_func)(IDEState *))
+static void ide_transfer_halt(IDEState *s)
{
-s->end_transfer_func = end_transfer_func;
+s->end_transfer_func = ide_transfer_stop;
s->data_ptr = s->io_buffer;
s->data_end = s->io_buffer;
s->status &= ~DRQ_STAT;
@@ -558,15 +557,10 @@ static void ide_transfer_halt(IDEState *s,
void ide_transfer_stop(IDEState *s)
{
-ide_transfer_halt(s, ide_transfer_stop);
+ide_transfer_halt(s);
ide_cmd_done(s);
}
-static void ide_transfer_cancel(IDEState *s)
-{
-ide_transfer_halt(s, ide_transfer_cancel);
-}
-
int64_t ide_get_sector(IDEState *s)
{
int64_t sector_num;
@@ -1361,7 +1355,7 @@ static bool cmd_nop(IDEState *s, uint8_t cmd)
static bool cmd_device_reset(IDEState *s, uint8_t cmd)
{
/* Halt PIO (in the DRQ phase), then DMA */
-ide_transfer_cancel(s);
+ide_transfer_halt(s);
ide_cancel_dma_sync(s);
/* Reset any PIO commands, reset signature, etc */
--
2.14.3
[Qemu-devel] [PATCH 4/7] ide: call ide_cmd_done from ide_transfer_stop
From: Paolo Bonzini
The code can simply be moved to the sole caller that has notify == true.
Signed-off-by: Paolo Bonzini
Signed-off-by: John Snow
---
hw/ide/core.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1a6cb337bf..54799ea6fb 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -548,26 +548,23 @@ static void ide_cmd_done(IDEState *s)
}
static void ide_transfer_halt(IDEState *s,
- void(*end_transfer_func)(IDEState *),
- bool notify)
+ void(*end_transfer_func)(IDEState *))
{
s->end_transfer_func = end_transfer_func;
s->data_ptr = s->io_buffer;
s->data_end = s->io_buffer;
s->status &= ~DRQ_STAT;
-if (notify) {
-ide_cmd_done(s);
-}
}
void ide_transfer_stop(IDEState *s)
{
-ide_transfer_halt(s, ide_transfer_stop, true);
+ide_transfer_halt(s, ide_transfer_stop);
+ide_cmd_done(s);
}
static void ide_transfer_cancel(IDEState *s)
{
-ide_transfer_halt(s, ide_transfer_cancel, false);
+ide_transfer_halt(s, ide_transfer_cancel);
}
int64_t ide_get_sector(IDEState *s)
--
2.14.3
[Qemu-devel] [PATCH 1/7] libqos/ahci: track sector size
It's not always 512, and it does wind up mattering for PIO tranfers,
because this means DRQ blocks are four times as big for ATAPI.
Replace an instance of 2048 with the correct define, too.
This patch by itself winds changing no behavior. fis->count is ignored
for CMD_PACKET, and sect_count only gets used in non-ATAPI cases.
Signed-off-by: John Snow
---
tests/libqos/ahci.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index bc201d762b..63e1f9b92d 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -90,6 +90,7 @@ struct AHCICommand {
uint32_t interrupts;
uint64_t xbytes;
uint32_t prd_size;
+uint32_t sector_size;
uint64_t buffer;
AHCICommandProp *props;
/* Data to be transferred to the guest */
@@ -796,7 +797,7 @@ static void command_header_init(AHCICommand *cmd)
static void command_table_init(AHCICommand *cmd)
{
RegH2DFIS *fis = &(cmd->fis);
-uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+uint16_t sect_count = (cmd->xbytes / cmd->sector_size);
fis->fis_type = REG_H2D_FIS;
fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */
@@ -819,7 +820,7 @@ static void command_table_init(AHCICommand *cmd)
if (cmd->props->lba28 || cmd->props->lba48) {
fis->device = ATA_DEVICE_LBA;
}
-fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+fis->count = (cmd->xbytes / cmd->sector_size);
}
fis->icc = 0x00;
fis->control = 0x00;
@@ -857,6 +858,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
cmd->xbytes = props->size;
cmd->prd_size = 4096;
cmd->buffer = 0xabad1dea;
+cmd->sector_size = props->atapi ? ATAPI_SECTOR_SIZE : AHCI_SECTOR_SIZE;
if (!cmd->props->ncq) {
cmd->interrupts = AHCI_PX_IS_DHRS;
@@ -1033,7 +1035,7 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t
buffer)
static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
{
unsigned char *cbd = cmd->atapi_cmd;
-uint64_t nsectors = xbytes / 2048;
+uint64_t nsectors = xbytes / ATAPI_SECTOR_SIZE;
uint32_t tmp;
g_assert(cbd);
@@ -1080,7 +1082,7 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t
xbytes,
cmd->prd_size = prd_size;
}
cmd->xbytes = xbytes;
-sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+sect_count = (cmd->xbytes / cmd->sector_size);
if (cmd->props->ncq) {
NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
--
2.14.3
Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > wrote:
> > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > >> > wrote:
> > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > >> > Add a machine command line option to allow the user to control
> > >> > >> > the Platform
> > >> > >> > Capabilities Structure in the virtualized NFIT. This Platform
> > >> > >> > Capabilities
> > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> > >> >
> > >> > >> > Signed-off-by: Ross Zwisler
> > >> > >>
> > >> > >> I tried playing with it and encoding the capabilities is
> > >> > >> quite awkward.
> > >> > >>
> > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >> > >>
> > >> > >> How about:
> > >> > >>
> > >> > >> "cpu-flush-on-power-loss-cap"
> > >> > >> "memory-flush-on-power-loss-cap"
> > >> > >> "byte-addressable-mirroring-cap"
> > >> > >
> > >> > > Hmmm...I don't like that as much because:
> > >> > >
> > >> > > a) It's very verbose. Looking at my current qemu command line few
> > >> > > other
> > >> > >options require that many characters, and you'd commonly be
> > >> > > defining more
> > >> > >than one of these for a given VM.
> > >> > >
> > >> > > b) It means that the QEMU will need to be updated if/when new flags
> > >> > > are added,
> > >> > >because we'll have to have new options for each flag. The current
> > >> > >implementation is more future-proof because you can specify any
> > >> > > flags
> > >> > >value you want.
> > >> > >
> > >> > > However, if you feel strongly about this, I'll make the change.
> > >> >
> > >> > Straw-man: Could we do something similar with what we are doing in
> > >> > ndctl?
> > >> >
> > >> > enum ndctl_persistence_domain {
> > >> > PERSISTENCE_NONE = 0,
> > >> > PERSISTENCE_MEM_CTRL = 10,
> > >> > PERSISTENCE_CPU_CACHE = 20,
> > >> > PERSISTENCE_UNKNOWN = INT_MAX,
> > >> > };
> > >> >
> > >> > ...and have the command line take a number where "10" and "20" are
> > >> > supported today, but allows us to adapt to new persistence domains in
> > >> > the future.
> > >>
> > >> I'm fine with that except can we have symbolic names instead of numbers
> > >> on command line?
> > >>
> > >> --
> > >> MST
> > >
> > > Okay, we can move to the symbolic names. Do you want them to be that
> > > long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> >
> > Wait, why is mirroring part of this?
> >
> > I was thinking this option would be:
> >
> > --persistence-domain={cpu,mem-ctrl}
> >
> > ...and try not to let ACPI specifics leak into the qemu command line
> > interface. For example PowerPC qemu could have a persistence domain
> > communicated via Open Firmware or some other mechanism.
>
> Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> somewhere so it's clear what the option affects.
>
> nvdimm-persistence={cpu,mem-ctrl} maybe?
>
> Michael, does this work for you?
Sounds good to me.
Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > wrote:
> > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > >> > wrote:
> > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin
> > > > >> > > wrote:
> > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > > >> > >> > Add a machine command line option to allow the user to
> > > > >> > >> > control the Platform
> > > > >> > >> > Capabilities Structure in the virtualized NFIT. This
> > > > >> > >> > Platform Capabilities
> > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > >> > >> >
> > > > >> > >> > Signed-off-by: Ross Zwisler
> > > > >> > >>
> > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > >> > >> quite awkward.
> > > > >> > >>
> > > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > > >> > >>
> > > > >> > >> How about:
> > > > >> > >>
> > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > >> > >> "byte-addressable-mirroring-cap"
> > > > >> > >
> > > > >> > > Hmmm...I don't like that as much because:
> > > > >> > >
> > > > >> > > a) It's very verbose. Looking at my current qemu command line
> > > > >> > > few other
> > > > >> > >options require that many characters, and you'd commonly be
> > > > >> > > defining more
> > > > >> > >than one of these for a given VM.
> > > > >> > >
> > > > >> > > b) It means that the QEMU will need to be updated if/when new
> > > > >> > > flags are added,
> > > > >> > >because we'll have to have new options for each flag. The
> > > > >> > > current
> > > > >> > >implementation is more future-proof because you can specify
> > > > >> > > any flags
> > > > >> > >value you want.
> > > > >> > >
> > > > >> > > However, if you feel strongly about this, I'll make the change.
> > > > >> >
> > > > >> > Straw-man: Could we do something similar with what we are doing in
> > > > >> > ndctl?
> > > > >> >
> > > > >> > enum ndctl_persistence_domain {
> > > > >> > PERSISTENCE_NONE = 0,
> > > > >> > PERSISTENCE_MEM_CTRL = 10,
> > > > >> > PERSISTENCE_CPU_CACHE = 20,
> > > > >> > PERSISTENCE_UNKNOWN = INT_MAX,
> > > > >> > };
> > > > >> >
> > > > >> > ...and have the command line take a number where "10" and "20" are
> > > > >> > supported today, but allows us to adapt to new persistence domains
> > > > >> > in
> > > > >> > the future.
> > > > >>
> > > > >> I'm fine with that except can we have symbolic names instead of
> > > > >> numbers
> > > > >> on command line?
> > > > >>
> > > > >> --
> > > > >> MST
> > > > >
> > > > > Okay, we can move to the symbolic names. Do you want them to be that
> > > > > long, or
> > > > > would:
> > > > >
> > > > > nvdimm-cap-cpu
> > > > > nvdimm-cap-mem-ctrl
> > > > > nvdimm-cap-mirroring
> > > >
> > > > Wait, why is mirroring part of this?
> > > >
> > > > I was thinking this option would be:
> > > >
> > > > --persistence-domain={cpu,mem-ctrl}
> > > >
> > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > interface. For example PowerPC qemu could have a persistence domain
> > > > communicated via Open Firmware or some other mechanism.
> > >
> > > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > > somewhere so it's clear what the option affects.
> > >
> > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > >
> > > Michael, does this work for you?
> >
> > Hmm...also, the version of these patches that used numeric values did go
> > upstream in QEMU. So, do we need to leave that interface intact, and just
> > add
> > a new one that uses symbolic names? Or did you still just want to replace
> > the
> > numeric one since it hasn't appeared in a QEMU release yet?
>
> I guess another alternative would be to just add symbolic name options to the
> existing command line option, i.e. allow all of these:
>
> nvdimm-cap=3 # CPU cache
> nvdimm-cap=cpu# CPU cache
> nvdimm-cap=2 # memory controller
> nvdimm-cap=mem-ctrl # memory controller
>
> And just have them as aliases. This retains backwards compatibility with
> what is already there, allows for other numeric values without QEMU updates if
> other bits are defined (though we are still tightly tied to ACPI in this
> case), and provides a symbolic name which is easier to use.
>
> Thoughts?
I'm inclined to say let's just keep the symbolic names.
Less of a chance users configure something
[Qemu-devel] [PATCH v4] iotests: Fix 219's timing
219 has two issues that may lead to sporadic failure, both of which are
the result of issuing query-jobs too early after a job has been
modified. This can then lead to different results based on whether the
modification has taken effect already or not.
First, query-jobs is issued right after the job has been created.
Besides its current progress possibly being in any random state (which
has already been taken care of), its total progress too is basically
arbitrary, because the job may not yet have been able to determine it.
This patch addresses this by just filtering the total progress, like
what has been done for the current progress already. However, for more
clarity, the filtering is changed to replace the values by a string
'FILTERED' instead of deleting them.
Secondly, query-jobs is issued right after a job has been resumed. The
job may or may not yet have had the time to actually perform any I/O,
and thus its current progress may or may not have advanced. To make
sure it has indeed advanced (which is what the reference output already
assumes), keep querying it until it has.
Signed-off-by: Max Reitz
---
v4: Drop the "import time" that has become unnecessary
v3: Keep querying until the job has advanced instead of waiting for a
fixed amount of time [Peter in v2, Eric in v1]
---
tests/qemu-iotests/219 | 26 --
tests/qemu-iotests/219.out | 10 +-
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index 898a26eef0..c03bbdb294 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -42,11 +42,24 @@ def test_pause_resume(vm):
iotests.log(vm.qmp(pause_cmd, **{pause_arg: 'job0'}))
pause_wait(vm, 'job0')
iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
-iotests.log(vm.qmp('query-jobs'))
+result = vm.qmp('query-jobs')
+iotests.log(result)
+
+old_progress = result['return'][0]['current-progress']
+total_progress = result['return'][0]['total-progress']
iotests.log(vm.qmp(resume_cmd, **{resume_arg: 'job0'}))
iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
-iotests.log(vm.qmp('query-jobs'))
+if old_progress < total_progress:
+# Wait for the job to advance
+while result['return'][0]['current-progress'] == old_progress:
+result = vm.qmp('query-jobs')
+iotests.log(result)
+else:
+# Already reached the end, so the job cannot advance
+# any further; therefore, the query-jobs result can be
+# logged immediately
+iotests.log(vm.qmp('query-jobs'))
def test_job_lifecycle(vm, job, job_args, has_ready=False):
iotests.log('')
@@ -58,12 +71,13 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):
iotests.log(vm.qmp(job, job_id='job0', **job_args))
# Depending on the storage, the first request may or may not have completed
-# yet, so filter out the progress. Later query-job calls don't need the
-# filtering because the progress is made deterministic by the block job
-# speed
+# yet (and the total progress may not have been fully determined yet), so
+# filter out the progress. Later query-job calls don't need the filtering
+# because the progress is made deterministic by the block job speed
result = vm.qmp('query-jobs')
for j in result['return']:
-del j['current-progress']
+j['current-progress'] = 'FILTERED'
+j['total-progress'] = 'FILTERED'
iotests.log(result)
# undefined -> created -> running
diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out
index 346801b655..6dc07bc41e 100644
--- a/tests/qemu-iotests/219.out
+++ b/tests/qemu-iotests/219.out
@@ -3,7 +3,7 @@ Launching VM...
Starting block job: drive-mirror (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'mirror'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id': u'job0', u'type': u'mirror'}]}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'created', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'running', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
@@ -93,7 +93,7 @@ Waiting for PENDING state...
Starting block job: drive-backup (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'backup'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id':
Re: [Qemu-devel] [PATCH v3] iotests: Fix 219's timing
On 2018-06-06 21:04, Max Reitz wrote: > 219 has two issues that may lead to sporadic failure, both of which are > the result of issuing query-jobs too early after a job has been > modified. This can then lead to different results based on whether the > modification has taken effect already or not. > > First, query-jobs is issued right after the job has been created. > Besides its current progress possibly being in any random state (which > has already been taken care of), its total progress too is basically > arbitrary, because the job may not yet have been able to determine it. > This patch addresses this by just filtering the total progress, like > what has been done for the current progress already. However, for more > clarity, the filtering is changed to replace the values by a string > 'FILTERED' instead of deleting them. > > Secondly, query-jobs is issued right after a job has been resumed. The > job may or may not yet have had the time to actually perform any I/O, > and thus its current progress may or may not have advanced. To make > sure it has indeed advanced (which is what the reference output already > assumes), keep querying it until it has. > > Signed-off-by: Max Reitz > --- > v3: Keep querying until the job has advanced instead of waiting for a > fixed amount of time [Peter in v2, Eric in v1] > --- > tests/qemu-iotests/219 | 27 +-- > tests/qemu-iotests/219.out | 10 +- > 2 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219 > index 898a26eef0..1a0329c6a0 100755 > --- a/tests/qemu-iotests/219 > +++ b/tests/qemu-iotests/219 > @@ -20,6 +20,7 @@ > # Check using the job-* QMP commands with block jobs > > import iotests > +import time Urgh, now of course we don't need this any more... Will send a v4. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > wrote:
> > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > >> > wrote:
> > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > >> > >> > Add a machine command line option to allow the user to control
> > > >> > >> > the Platform
> > > >> > >> > Capabilities Structure in the virtualized NFIT. This Platform
> > > >> > >> > Capabilities
> > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > >> > >> >
> > > >> > >> > Signed-off-by: Ross Zwisler
> > > >> > >>
> > > >> > >> I tried playing with it and encoding the capabilities is
> > > >> > >> quite awkward.
> > > >> > >>
> > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > >> > >>
> > > >> > >> How about:
> > > >> > >>
> > > >> > >> "cpu-flush-on-power-loss-cap"
> > > >> > >> "memory-flush-on-power-loss-cap"
> > > >> > >> "byte-addressable-mirroring-cap"
> > > >> > >
> > > >> > > Hmmm...I don't like that as much because:
> > > >> > >
> > > >> > > a) It's very verbose. Looking at my current qemu command line few
> > > >> > > other
> > > >> > >options require that many characters, and you'd commonly be
> > > >> > > defining more
> > > >> > >than one of these for a given VM.
> > > >> > >
> > > >> > > b) It means that the QEMU will need to be updated if/when new
> > > >> > > flags are added,
> > > >> > >because we'll have to have new options for each flag. The
> > > >> > > current
> > > >> > >implementation is more future-proof because you can specify any
> > > >> > > flags
> > > >> > >value you want.
> > > >> > >
> > > >> > > However, if you feel strongly about this, I'll make the change.
> > > >> >
> > > >> > Straw-man: Could we do something similar with what we are doing in
> > > >> > ndctl?
> > > >> >
> > > >> > enum ndctl_persistence_domain {
> > > >> > PERSISTENCE_NONE = 0,
> > > >> > PERSISTENCE_MEM_CTRL = 10,
> > > >> > PERSISTENCE_CPU_CACHE = 20,
> > > >> > PERSISTENCE_UNKNOWN = INT_MAX,
> > > >> > };
> > > >> >
> > > >> > ...and have the command line take a number where "10" and "20" are
> > > >> > supported today, but allows us to adapt to new persistence domains in
> > > >> > the future.
> > > >>
> > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > >> on command line?
> > > >>
> > > >> --
> > > >> MST
> > > >
> > > > Okay, we can move to the symbolic names. Do you want them to be that
> > > > long, or
> > > > would:
> > > >
> > > > nvdimm-cap-cpu
> > > > nvdimm-cap-mem-ctrl
> > > > nvdimm-cap-mirroring
> > >
> > > Wait, why is mirroring part of this?
> > >
> > > I was thinking this option would be:
> > >
> > > --persistence-domain={cpu,mem-ctrl}
> > >
> > > ...and try not to let ACPI specifics leak into the qemu command line
> > > interface. For example PowerPC qemu could have a persistence domain
> > > communicated via Open Firmware or some other mechanism.
> >
> > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > somewhere so it's clear what the option affects.
> >
> > nvdimm-persistence={cpu,mem-ctrl} maybe?
> >
> > Michael, does this work for you?
>
> Hmm...also, the version of these patches that used numeric values did go
> upstream in QEMU. So, do we need to leave that interface intact, and just add
> a new one that uses symbolic names? Or did you still just want to replace the
> numeric one since it hasn't appeared in a QEMU release yet?
The later. No release => no stable APIs.
--
MST
[Qemu-devel] [PATCH v3] iotests: Fix 219's timing
219 has two issues that may lead to sporadic failure, both of which are
the result of issuing query-jobs too early after a job has been
modified. This can then lead to different results based on whether the
modification has taken effect already or not.
First, query-jobs is issued right after the job has been created.
Besides its current progress possibly being in any random state (which
has already been taken care of), its total progress too is basically
arbitrary, because the job may not yet have been able to determine it.
This patch addresses this by just filtering the total progress, like
what has been done for the current progress already. However, for more
clarity, the filtering is changed to replace the values by a string
'FILTERED' instead of deleting them.
Secondly, query-jobs is issued right after a job has been resumed. The
job may or may not yet have had the time to actually perform any I/O,
and thus its current progress may or may not have advanced. To make
sure it has indeed advanced (which is what the reference output already
assumes), keep querying it until it has.
Signed-off-by: Max Reitz
---
v3: Keep querying until the job has advanced instead of waiting for a
fixed amount of time [Peter in v2, Eric in v1]
---
tests/qemu-iotests/219 | 27 +--
tests/qemu-iotests/219.out | 10 +-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index 898a26eef0..1a0329c6a0 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -20,6 +20,7 @@
# Check using the job-* QMP commands with block jobs
import iotests
+import time
iotests.verify_image_format(supported_fmts=['qcow2'])
@@ -42,11 +43,24 @@ def test_pause_resume(vm):
iotests.log(vm.qmp(pause_cmd, **{pause_arg: 'job0'}))
pause_wait(vm, 'job0')
iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
-iotests.log(vm.qmp('query-jobs'))
+result = vm.qmp('query-jobs')
+iotests.log(result)
+
+old_progress = result['return'][0]['current-progress']
+total_progress = result['return'][0]['total-progress']
iotests.log(vm.qmp(resume_cmd, **{resume_arg: 'job0'}))
iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
-iotests.log(vm.qmp('query-jobs'))
+if old_progress < total_progress:
+# Wait for the job to advance
+while result['return'][0]['current-progress'] == old_progress:
+result = vm.qmp('query-jobs')
+iotests.log(result)
+else:
+# Already reached the end, so the job cannot advance
+# any further; therefore, the query-jobs result can be
+# logged immediately
+iotests.log(vm.qmp('query-jobs'))
def test_job_lifecycle(vm, job, job_args, has_ready=False):
iotests.log('')
@@ -58,12 +72,13 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):
iotests.log(vm.qmp(job, job_id='job0', **job_args))
# Depending on the storage, the first request may or may not have completed
-# yet, so filter out the progress. Later query-job calls don't need the
-# filtering because the progress is made deterministic by the block job
-# speed
+# yet (and the total progress may not have been fully determined yet), so
+# filter out the progress. Later query-job calls don't need the filtering
+# because the progress is made deterministic by the block job speed
result = vm.qmp('query-jobs')
for j in result['return']:
-del j['current-progress']
+j['current-progress'] = 'FILTERED'
+j['total-progress'] = 'FILTERED'
iotests.log(result)
# undefined -> created -> running
diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out
index 346801b655..6dc07bc41e 100644
--- a/tests/qemu-iotests/219.out
+++ b/tests/qemu-iotests/219.out
@@ -3,7 +3,7 @@ Launching VM...
Starting block job: drive-mirror (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'mirror'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id': u'job0', u'type': u'mirror'}]}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'created', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'running', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
@@ -93,7 +93,7 @@ Waiting for PENDING state...
Starting block job: drive-backup (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'backup'}]}
Re: [Qemu-devel] Stair step trace output since 12fb0ac05
BALATON Zoltan writes: > Hello, > > Since 12fb0ac05 (char: Remove unwanted crlf conversion) trace output > is printed in stair steps when using -trace and -serial stdio > together. E.g. > $ qemu-system-i386 -trace 'pci*' -serial stdio > > Regards, > BALATON Zoltan I cc'ed the people involved with this patch for you.
Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:
> On 6/4/2018 7:06 PM, Jason Wang wrote:
> >
> >
> > On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
> > > Ping on this patch now that the kernel patches are accepted into
> > > davem's net-next tree.
> > > https://patchwork.ozlabs.org/cover/920005/
> > >
> > >
> > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > This feature bit can be used by hypervisor to indicate
> > > > virtio_net device to
> > > > act as a standby for another device with the same MAC address.
> > > >
> > > > I tested this with a small change to the patch to mark the
> > > > STANDBY feature 'true'
> > > > by default as i am using libvirt to start the VMs.
> > > > Is there a way to pass the newly added feature bit 'standby' to
> > > > qemu via libvirt
> > > > XML file?
> > > >
> >
> > Maybe you can try qemu command line passthrough:
> >
> > https://libvirt.org/drvqemu.html#qemucommand
>
> It looks like this can be used to pass command line arguments to qemu.
> Is it possible to specify a virtio specific attribute via this method?
>
> For ex: to say mrg_rxbuf is off we can add the following line to virtio
> section of the domain xml file.
>
>
> I think libvirt needs to be extended to to support the new 'standby' attribute
> via this mechanism.
> Adding Liane Stump and libvirt to the CC list.
>
> Michael,
> Can we start with getting this patch into Qemu and an update to libvirt to
> support the 'standby' feature so that this feature can be enabled via
> some scripts/orchestration layer for now.
Unfortunately we don't give the hypothetical orchestration layer
enough info about driver being bound, so it does not know
when is it safe to add a primary. And a similar issue around reset.
We could add events for that which would be a small patch,
but the issue then is that guest can trigger a storm of
these events.
> We could improve this solution by enhancing Qemu to do automatic management
> of the
> addition/deletion of the primary device based on feature negotiation as a
> later patch.
I'm not sure what the point of the back and forth would be though.
Typically after people invest in a specific config and get it working,
it's very hard to move them to another solution.
>
> >
> > The patch looks good to me. Let's see if Michael have any comment.
> >
> > Thanks
> >
> > > > Signed-off-by: Sridhar Samudrala
> > > > ---
> > > > hw/net/virtio-net.c | 2 ++
> > > > include/standard-headers/linux/virtio_net.h | 3 +++
> > > > 2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 90502fca7c..38b3140670 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> > > > true),
> > > > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
> > > > SPEED_UNKNOWN),
> > > > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> > > > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
> > > > VIRTIO_NET_F_STANDBY,
> > > > + false),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > > diff --git a/include/standard-headers/linux/virtio_net.h
> > > > b/include/standard-headers/linux/virtio_net.h
> > > > index e9f255ea3f..01ec09684c 100644
> > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > @@ -57,6 +57,9 @@
> > > > * Steering */
> > > > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> > > > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for
> > > > another device
> > > > + * with the same MAC.
> > > > + */
> > > > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set
> > > > linkspeed and duplex */
> > > > #ifndef VIRTIO_NET_NO_LEGACY
> > >
> >
Re: [Qemu-devel] [libvirt] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote: On 6/4/2018 7:06 PM, Jason Wang wrote: On 2018年06月05日 09:41, Samudrala, Sridhar wrote: Ping on this patch now that the kernel patches are accepted into davem's net-next tree. https://patchwork.ozlabs.org/cover/920005/ On 5/7/2018 4:09 PM, Sridhar Samudrala wrote: This feature bit can be used by hypervisor to indicate virtio_net device to act as a standby for another device with the same MAC address. I tested this with a small change to the patch to mark the STANDBY feature 'true' by default as i am using libvirt to start the VMs. Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt XML file? Maybe you can try qemu command line passthrough: https://libvirt.org/drvqemu.html#qemucommand It looks like this can be used to pass command line arguments to qemu. Is it possible to specify a virtio specific attribute via this method? Yes, for testing purposes you should be able to do this via using QEMU's -set command line argument: http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html i.e.: ... For ex: to say mrg_rxbuf is off we can add the following line to virtio section of the domain xml file. I think libvirt needs to be extended to to support the new 'standby' attribute via this mechanism. Adding Liane Stump and libvirt to the CC list. *Laine Michael, Can we start with getting this patch into Qemu and an update to libvirt to support the 'standby' feature so that this feature can be enabled via some scripts/orchestration layer for now. We could improve this solution by enhancing Qemu to do automatic management of the addition/deletion of the primary device based on feature negotiation as a later patch. If that means the libvirt attribute would no longer be needed, I don't see the reason to add it to libvirt in the first place. Jano signature.asc Description: Digital signature
Re: [Qemu-devel] [PATCH] iotests: Fix 219's timing
On 2018-06-06 20:41, Peter Maydell wrote: > On 6 June 2018 at 19:37, Max Reitz wrote: >> 219 has two issues that may lead to sporadic failure, both of which are >> the result of issuing query-jobs too early after a job has been >> modified. This can then lead to different results based on whether the >> modification has taken effect already or not. >> >> First, query-jobs is issued right after the job has been created. >> Besides its current progress possibly being in any random state (which >> has already been taken care of), its total progress too is basically >> arbitrary, because the job may not yet have been able to determine it. >> This patch addresses this by just filtering the total progress, like >> what has been done for the current progress already. However, for more >> clarity, the filtering is changed to replace the values by a string >> 'FILTERED' instead of deleting them. >> >> Secondly, query-jobs is issued right after a job has been resumed. The >> job may or may not yet have had the time to actually perform any I/O, >> and thus its current progress may or may not have advanced. To make >> sure it has indeed advanced (which is what the reference output already >> assumes), insert a sleep of 100 ms before query-jobs is invoked. With a >> slice time of 100 ms, a buffer size of 64 kB and a speed of 256 kB/s, >> this should be the right amount of time to let the job advance by >> exactly 64 kB. >> >> Signed-off-by: Max Reitz >> --- >> v2: Changed the query-jobs progress filtering [Eric] >> --- > > I know nothing about the iotests, so this might be off-base, > but this looks rather like "try to fix a race condition by > adding a sleep", which generally doesn't work very well ? The job tested here already has its own timing (copying 64 kB four times a second, in 100 ms steps), so a sleep is not too bad. What is happening is that the job is put to sleep, then reawakened and it should do one copy step immediately afterwards. Then it won't do anything for 250 ms. Now waiting 100 ms should really be enough to make that "immediate" step actually happening, and it doesn't really hurt because we have 250 ms anyway. But I think it should be possible without the sleep (by repeatedly querying the progress), I'll give it a try. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/2] slirp: correct size computation while concatenating mbuf
Hello Samuel, +-- On Wed, 6 Jun 2018, Samuel Thibault wrote --+ | > From: Prasad J Pandit | > | > While reassembling incoming fragmented datagrams, 'm_cat' routine | > extends the 'mbuf' buffer, if it has insufficient room. It computes | > a wrong buffer size, which leads to overwriting adjacent heap buffer | > area. Correct this size computation in m_cat. | > | > Reported-by: ZDI Disclosures | > Signed-off-by: Prasad J Pandit | | Applied to my tree with a couple documentation improvements, thanks! -> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01144.html This is patch v1 with indentation fix flagged by checkpatch.pl. In case you prefer this one. Thank you. -- - P J P 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] iotests: Fix 219's timing
On 6 June 2018 at 19:37, Max Reitz wrote: > 219 has two issues that may lead to sporadic failure, both of which are > the result of issuing query-jobs too early after a job has been > modified. This can then lead to different results based on whether the > modification has taken effect already or not. > > First, query-jobs is issued right after the job has been created. > Besides its current progress possibly being in any random state (which > has already been taken care of), its total progress too is basically > arbitrary, because the job may not yet have been able to determine it. > This patch addresses this by just filtering the total progress, like > what has been done for the current progress already. However, for more > clarity, the filtering is changed to replace the values by a string > 'FILTERED' instead of deleting them. > > Secondly, query-jobs is issued right after a job has been resumed. The > job may or may not yet have had the time to actually perform any I/O, > and thus its current progress may or may not have advanced. To make > sure it has indeed advanced (which is what the reference output already > assumes), insert a sleep of 100 ms before query-jobs is invoked. With a > slice time of 100 ms, a buffer size of 64 kB and a speed of 256 kB/s, > this should be the right amount of time to let the job advance by > exactly 64 kB. > > Signed-off-by: Max Reitz > --- > v2: Changed the query-jobs progress filtering [Eric] > --- I know nothing about the iotests, so this might be off-base, but this looks rather like "try to fix a race condition by adding a sleep", which generally doesn't work very well ? thanks -- PMM
Re: [Qemu-devel] [PATCH] iotests: Fix 219's timing
On 2018-06-06 20:37, Max Reitz wrote: > 219 has two issues that may lead to sporadic failure, both of which are > the result of issuing query-jobs too early after a job has been > modified. This can then lead to different results based on whether the > modification has taken effect already or not. > > First, query-jobs is issued right after the job has been created. > Besides its current progress possibly being in any random state (which > has already been taken care of), its total progress too is basically > arbitrary, because the job may not yet have been able to determine it. > This patch addresses this by just filtering the total progress, like > what has been done for the current progress already. However, for more > clarity, the filtering is changed to replace the values by a string > 'FILTERED' instead of deleting them. > > Secondly, query-jobs is issued right after a job has been resumed. The > job may or may not yet have had the time to actually perform any I/O, > and thus its current progress may or may not have advanced. To make > sure it has indeed advanced (which is what the reference output already > assumes), insert a sleep of 100 ms before query-jobs is invoked. With a > slice time of 100 ms, a buffer size of 64 kB and a speed of 256 kB/s, > this should be the right amount of time to let the job advance by > exactly 64 kB. > > Signed-off-by: Max Reitz > --- > v2: Changed the query-jobs progress filtering [Eric] > --- > tests/qemu-iotests/219 | 12 > tests/qemu-iotests/219.out | 10 +- > 2 files changed, 13 insertions(+), 9 deletions(-) Oops, forgot the "v2" tag. Forgive me. Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] iotests: Fix 219's timing
219 has two issues that may lead to sporadic failure, both of which are
the result of issuing query-jobs too early after a job has been
modified. This can then lead to different results based on whether the
modification has taken effect already or not.
First, query-jobs is issued right after the job has been created.
Besides its current progress possibly being in any random state (which
has already been taken care of), its total progress too is basically
arbitrary, because the job may not yet have been able to determine it.
This patch addresses this by just filtering the total progress, like
what has been done for the current progress already. However, for more
clarity, the filtering is changed to replace the values by a string
'FILTERED' instead of deleting them.
Secondly, query-jobs is issued right after a job has been resumed. The
job may or may not yet have had the time to actually perform any I/O,
and thus its current progress may or may not have advanced. To make
sure it has indeed advanced (which is what the reference output already
assumes), insert a sleep of 100 ms before query-jobs is invoked. With a
slice time of 100 ms, a buffer size of 64 kB and a speed of 256 kB/s,
this should be the right amount of time to let the job advance by
exactly 64 kB.
Signed-off-by: Max Reitz
---
v2: Changed the query-jobs progress filtering [Eric]
---
tests/qemu-iotests/219 | 12
tests/qemu-iotests/219.out | 10 +-
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index 898a26eef0..6931c5e449 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -20,6 +20,7 @@
# Check using the job-* QMP commands with block jobs
import iotests
+import time
iotests.verify_image_format(supported_fmts=['qcow2'])
@@ -46,6 +47,8 @@ def test_pause_resume(vm):
iotests.log(vm.qmp(resume_cmd, **{resume_arg: 'job0'}))
iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
+# Let the job proceed before querying its progress
+time.sleep(0.1)
iotests.log(vm.qmp('query-jobs'))
def test_job_lifecycle(vm, job, job_args, has_ready=False):
@@ -58,12 +61,13 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):
iotests.log(vm.qmp(job, job_id='job0', **job_args))
# Depending on the storage, the first request may or may not have completed
-# yet, so filter out the progress. Later query-job calls don't need the
-# filtering because the progress is made deterministic by the block job
-# speed
+# yet (and the total progress may not have been fully determined yet), so
+# filter out the progress. Later query-job calls don't need the filtering
+# because the progress is made deterministic by the block job speed
result = vm.qmp('query-jobs')
for j in result['return']:
-del j['current-progress']
+j['current-progress'] = 'FILTERED'
+j['total-progress'] = 'FILTERED'
iotests.log(result)
# undefined -> created -> running
diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out
index 346801b655..6dc07bc41e 100644
--- a/tests/qemu-iotests/219.out
+++ b/tests/qemu-iotests/219.out
@@ -3,7 +3,7 @@ Launching VM...
Starting block job: drive-mirror (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'mirror'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id': u'job0', u'type': u'mirror'}]}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'created', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'running', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
@@ -93,7 +93,7 @@ Waiting for PENDING state...
Starting block job: drive-backup (auto-finalize: True; auto-dismiss: True)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'backup'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id': u'job0', u'type': u'backup'}]}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'created', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'running', u'id': u'job0'}, u'event': u'JOB_STATUS_CHANGE'}
@@ -144,7 +144,7 @@ Waiting for PENDING state...
Starting block job: drive-backup (auto-finalize: True; auto-dismiss: False)
{u'return': {}}
-{u'return': [{u'status': u'running', u'total-progress': 4194304, u'id':
u'job0', u'type': u'backup'}]}
+{u'return': [{u'status': u'running', u'current-progress': 'FILTERED',
u'total-progress': 'FILTERED', u'id':
Re: [Qemu-devel] storing machine data in qcow images?
On Wed, Jun 06, 2018 at 08:33:39PM +0200, Michal Suchánek wrote: [...] > Lastly we are missing a developer of a management layer committed to > support such appliances. This is important. Without developers of management tools willing to help specify the requirements and implement the feature, all the work in the lower layers would be useless. -- Eduardo
Re: [Qemu-devel] storing machine data in qcow images?
On Wed, 6 Jun 2018 20:02:54 +0200 Max Reitz wrote: > On 2018-06-06 17:25, Michal Suchánek wrote: > > On Wed, 6 Jun 2018 16:55:08 +0200 > > Max Reitz wrote: > > > >> On 2018-06-06 16:41, Dr. David Alan Gilbert wrote: > >>> * Max Reitz (mre...@redhat.com) wrote: > >> Users using a whole VM stack plus management, but then handling two > >> files instead of one is too much to ask? > > > > What you don't seem to realize is there are cases when there is an > > 'administrator' who has set up the VM stack plus management and 'joe > > user' who wants to run some random VM on that stack. > > > > And if you download an appliance compatible with the stack it should > > just work. For a long time the 'appliance' for qemu based > > virtualization was a simple qcow2 file which was sized sufficiently > > for the VM to run but shrunk for transport. And although it is > > technically wrong it JustWorked(tm). > > Hm, yes. As I replied to Dave, I understand, but I would think this > then requires a real appliance solution. I think you do want such a > solution, but Dave doesn't. Yes, Dave wants a poor man's half-assed appliance and insists on it not being an appliance. Duh. In the pc world to maintain the status quo with minimum changes you only need to know if the image uses EFI or legacy BIOS and you can maintain the illusion that the TimeProvenSolution(tm) JustWorks(tm). Sneaking that single piece of information somewhere seems to be the goal here. > > My problem is that I cannot accept Dave's arguments on why to include > this blob in qcow2 if someone else already plans on making that blob > the basis for qcow2 appliances. > > And I still do not think that qcow2 is the right format for VM > appliances. To convince me, we'd first need a consensus on what the > appliances are for (Michael seems to want them for qemu directly, Let's put this straight: qemu as is cannot run appliances. It is not designed for that and it would be a big feature to create enough management inside qemu (or around qemu but part of qemu distribution) to change that. As it stands qemu always takes the configuration from the outside - either from the user directly or from a separate management layer. > apparently you want them for something higher up the stack) and thus > what they are supposed to be capable of exactly. Using qcow2 would be kind of cool but it has its limitations and drawbacks as well. You could use qcow2 as transport format and convert the VM to use raw disks or whatever if you need the performance. And you could run the VM directly from qcow2 without additional processing which is its advantage. It would fail miserably with tools not aware of the extra metadata, however. Lastly we are missing a developer of a management layer committed to support such appliances. > > Like, one thing that is important to discuss is this (but please not > in this thread...): If we agree on making an appliance format (qcow2 > or not), is it for running VMs off or do we just want it for VM > export/import? The former might mean we need qcow2, because there is > no good way to offer good performance with multiple disks otherwise > (but this would constrain us e.g. in the disk image format -- no raw > images for you, then). But the latter can work just fine with a > normal archival format as long as building/decomposing it is possible > without copying. Indeed, using an archive should be good enough for the 1-click download solution. It will take time to extract but it will typically take even more time to download or publish. So optimizing the format for speed of export/import might be misplaced. Thanks Michal pgpto6o8p1SOH.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] Stair step trace output since 12fb0ac05
On 6 June 2018 at 18:15, BALATON Zoltan wrote: > Hello, > > Since 12fb0ac05 (char: Remove unwanted crlf conversion) trace output is > printed in stair steps when using -trace and -serial stdio together. E.g. > $ qemu-system-i386 -trace 'pci*' -serial stdio I had a feeling that we were going to find that there were situations where we really wanted to retain the crlf processing... thanks -- PMM
Re: [Qemu-devel] [PATCH v2 3/3] util: remove redundant include of glib.h
On 6 June 2018 at 18:32, Daniel P. Berrangé wrote: > Code must only ever include glib.h indirectly via the glib-compat.h > header file, because we will need some macros set before glib.h is > pulled in. Adding extra includes of glib.h will (soon) cause compile > failures such as: > > In file included from /home/berrange/src/virt/qemu/include/qemu/osdep.h:107, > from > /home/berrange/src/virt/qemu/include/qemu/iova-tree.h:26, > from util/iova-tree.c:13: > /home/berrange/src/virt/qemu/include/glib-compat.h:22: error: > "GLIB_VERSION_MIN_REQUIRED" redefined [-Werror] > #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_40 > > In file included from /usr/include/glib-2.0/glib/gtypes.h:34, > from /usr/include/glib-2.0/glib/galloca.h:32, > from /usr/include/glib-2.0/glib.h:30, > from util/iova-tree.c:12: > /usr/include/glib-2.0/glib/gversionmacros.h:237: note: this is the location > of the previous definition > # define GLIB_VERSION_MIN_REQUIRED (GLIB_VERSION_CUR_STABLE) > > Signed-off-by: Daniel P. Berrangé > --- > util/iova-tree.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/util/iova-tree.c b/util/iova-tree.c > index 2d9cebfc89..d39cd8bb29 100644 > --- a/util/iova-tree.c > +++ b/util/iova-tree.c > @@ -9,7 +9,6 @@ > * This work is licensed under the terms of the GNU GPL, version 2 or later. > */ > > -#include > #include "qemu/iova-tree.h" While we're fixing up the headers in this file: it should start with an include of qemu/osdep.h, and qemu/iova-tree.h should not include osdep.h... thanks -- PMM
[Qemu-devel] [PATCH v2 3/5] qmp: transaction support for x-block-dirty-bitmap-enable/disable
From: Vladimir Sementsov-Ogievskiy
Signed-off-by: Vladimir Sementsov-Ogievskiy
[Added x- prefix. --js]
Signed-off-by: John Snow
---
blockdev.c| 81 ++-
qapi/transaction.json | 4 +++
2 files changed, 84 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index fb402edc94..af705738cd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2052,6 +2052,7 @@ typedef struct BlockDirtyBitmapState {
BlockDriverState *bs;
HBitmap *backup;
bool prepared;
+bool was_enabled;
} BlockDirtyBitmapState;
static void block_dirty_bitmap_add_prepare(BlkActionState *common,
@@ -2151,6 +2152,74 @@ static void
block_dirty_bitmap_clear_commit(BlkActionState *common)
hbitmap_free(state->backup);
}
+static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
+ Error **errp)
+{
+BlockDirtyBitmap *action;
+BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+if (action_check_completion_mode(common, errp) < 0) {
+return;
+}
+
+action = common->action->u.x_block_dirty_bitmap_enable.data;
+state->bitmap = block_dirty_bitmap_lookup(action->node,
+ action->name,
+ NULL,
+ errp);
+if (!state->bitmap) {
+return;
+}
+
+state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
+bdrv_enable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_enable_abort(BlkActionState *common)
+{
+BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+if (!state->was_enabled) {
+bdrv_disable_dirty_bitmap(state->bitmap);
+}
+}
+
+static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
+ Error **errp)
+{
+BlockDirtyBitmap *action;
+BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+if (action_check_completion_mode(common, errp) < 0) {
+return;
+}
+
+action = common->action->u.x_block_dirty_bitmap_disable.data;
+state->bitmap = block_dirty_bitmap_lookup(action->node,
+ action->name,
+ NULL,
+ errp);
+if (!state->bitmap) {
+return;
+}
+
+state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
+bdrv_disable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_abort(BlkActionState *common)
+{
+BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+if (state->was_enabled) {
+bdrv_enable_dirty_bitmap(state->bitmap);
+}
+}
+
static void abort_prepare(BlkActionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
@@ -2211,7 +2280,17 @@ static const BlkActionOps actions[] = {
.prepare = block_dirty_bitmap_clear_prepare,
.commit = block_dirty_bitmap_clear_commit,
.abort = block_dirty_bitmap_clear_abort,
-}
+},
+[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
+.instance_size = sizeof(BlockDirtyBitmapState),
+.prepare = block_dirty_bitmap_enable_prepare,
+.abort = block_dirty_bitmap_enable_abort,
+},
+[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_DISABLE] = {
+.instance_size = sizeof(BlockDirtyBitmapState),
+.prepare = block_dirty_bitmap_disable_prepare,
+.abort = block_dirty_bitmap_disable_abort,
+ }
};
/**
diff --git a/qapi/transaction.json b/qapi/transaction.json
index bd312792da..d7e4274550 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -46,6 +46,8 @@
# - @abort: since 1.6
# - @block-dirty-bitmap-add: since 2.5
# - @block-dirty-bitmap-clear: since 2.5
+# - @x-block-dirty-bitmap-enable: since 3.0
+# - @x-block-dirty-bitmap-disable: since 3.0
# - @blockdev-backup: since 2.3
# - @blockdev-snapshot: since 2.5
# - @blockdev-snapshot-internal-sync: since 1.7
@@ -59,6 +61,8 @@
'abort': 'Abort',
'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
+ 'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+ 'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
'blockdev-backup': 'BlockdevBackup',
'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
--
2.14.3
[Qemu-devel] [PATCH v2 5/5] qapi: add disabled parameter to block-dirty-bitmap-add
From: Vladimir Sementsov-Ogievskiy
This is needed, for example, to create a new bitmap and merge several
disabled bitmaps into a new one. Without this flag we will have to
put block-dirty-bitmap-add and block-dirty-bitmap-disable into one
transaction.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: John Snow
Reviewed-by: Jeff Cody
---
blockdev.c | 10 ++
qapi/block-core.json | 6 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index dc40a358b0..041f5d594f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2073,6 +2073,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState
*common,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
action->has_autoload, action->autoload,
+ action->has_x_disabled, action->x_disabled,
_err);
if (!local_err) {
@@ -2880,6 +2881,7 @@ void qmp_block_dirty_bitmap_add(const char *node, const
char *name,
bool has_granularity, uint32_t granularity,
bool has_persistent, bool persistent,
bool has_autoload, bool autoload,
+bool has_disabled, bool disabled,
Error **errp)
{
BlockDriverState *bs;
@@ -2914,6 +2916,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const
char *name,
warn_report("Autoload option is deprecated and its value is ignored");
}
+if (!has_disabled) {
+disabled = false;
+}
+
if (persistent &&
!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
{
@@ -2925,6 +2931,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const
char *name,
return;
}
+if (disabled) {
+bdrv_disable_dirty_bitmap(bitmap);
+}
+
bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d4ab93190..fff23fc82b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1734,11 +1734,15 @@
#Currently, all dirty tracking bitmaps are loaded from Qcow2 on
#open.
#
+# @x-disabled: the bitmap is created in the disabled state, which means that
+# it will not track drive changes. The bitmap may be enabled with
+# x-block-dirty-bitmap-enable. Default is false. (Since: 3.0)
+#
# Since: 2.4
##
{ 'struct': 'BlockDirtyBitmapAdd',
'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool', '*autoload': 'bool' } }
+'*persistent': 'bool', '*autoload': 'bool', '*x-disabled': 'bool'
} }
##
# @BlockDirtyBitmapMerge:
--
2.14.3
[Qemu-devel] [PATCH v2 4/5] qapi: add x-block-dirty-bitmap-merge
From: Vladimir Sementsov-Ogievskiy
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 18 ++
blockdev.c | 30 ++
include/block/dirty-bitmap.h | 3 ++-
qapi/block-core.json | 38 ++
4 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 56234257f4..4159d3929e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -757,3 +757,21 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap
*bitmap, uint64_t offset)
{
return hbitmap_next_zero(bitmap->bitmap, offset);
}
+
+void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+ Error **errp)
+{
+/* only bitmaps from one bds are supported */
+assert(dest->mutex == src->mutex);
+
+qemu_mutex_lock(dest->mutex);
+
+assert(bdrv_dirty_bitmap_enabled(dest));
+assert(!bdrv_dirty_bitmap_readonly(dest));
+
+if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
+error_setg(errp, "Bitmaps are incompatible and can't be merged");
+}
+
+qemu_mutex_unlock(dest->mutex);
+}
diff --git a/blockdev.c b/blockdev.c
index af705738cd..dc40a358b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3044,6 +3044,36 @@ void qmp_x_block_dirty_bitmap_disable(const char *node,
const char *name,
bdrv_disable_dirty_bitmap(bitmap);
}
+void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
+const char *src_name, Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *dst, *src;
+
+dst = block_dirty_bitmap_lookup(node, dst_name, , errp);
+if (!dst) {
+return;
+}
+
+if (bdrv_dirty_bitmap_frozen(dst)) {
+error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
+ dst_name);
+return;
+} else if (bdrv_dirty_bitmap_readonly(dst)) {
+error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+ dst_name);
+return;
+}
+
+src = bdrv_find_dirty_bitmap(bs, src_name);
+if (!src) {
+error_setg(errp, "Dirty bitmap '%s' not found", src_name);
+return;
+}
+
+bdrv_merge_dirty_bitmap(dst, src, errp);
+}
+
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
const char *name,
Error **errp)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1ff8949b1b..1e14743032 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,7 +70,8 @@ void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap,
bool value);
void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
bool persistent);
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
qmp_locked);
-
+void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+ Error **errp);
/* Functions that require manual locking. */
void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 02de674f5f..9d4ab93190 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1740,6 +1740,20 @@
'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
'*persistent': 'bool', '*autoload': 'bool' } }
+##
+# @BlockDirtyBitmapMerge:
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @dst_name: name of the destination dirty bitmap
+#
+# @src_name: name of the source dirty bitmap
+#
+# Since: 3.0
+##
+{ 'struct': 'BlockDirtyBitmapMerge',
+ 'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
+
##
# @block-dirty-bitmap-add:
#
@@ -1850,6 +1864,30 @@
{ 'command': 'x-block-dirty-bitmap-disable',
'data': 'BlockDirtyBitmap' }
+##
+# @x-block-dirty-bitmap-merge:
+#
+# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
+# bitmap is unchanged. On error, @dst_name is unchanged.
+#
+# Returns: nothing on success
+# If @node is not a valid block device, DeviceNotFound
+# If @dst_name or @src_name is not found, GenericError
+# If bitmaps has different sizes or granularities, GenericError
+#
+# Since: 3.0
+#
+# Example:
+#
+# -> { "execute": "x-block-dirty-bitmap-merge",
+# "arguments": { "node": "drive0", "dst_name": "bitmap0",
+# "src_name": "bitmap1" } }
+# <- { "return": {} }
+#
+##
+ { 'command': 'x-block-dirty-bitmap-merge',
+'data': 'BlockDirtyBitmapMerge' }
+
##
# @BlockDirtyBitmapSha256:
#
--
2.14.3
[Qemu-devel] [PATCH v2 1/5] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
From: Vladimir Sementsov-Ogievskiy
Add locks and remove comments about BQL accordingly to
dirty_bitmap_mutex definition in block_int.h.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: John Snow
Reviewed-by: Jeff Cody
---
block/dirty-bitmap.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 967159479d..56234257f4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -442,18 +442,20 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState
*bs,
}
}
-/* Called with BQL taken. */
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+bdrv_dirty_bitmap_lock(bitmap);
assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = true;
+bdrv_dirty_bitmap_unlock(bitmap);
}
-/* Called with BQL taken. */
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+bdrv_dirty_bitmap_lock(bitmap);
assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = false;
+bdrv_dirty_bitmap_unlock(bitmap);
}
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
--
2.14.3
