Re: [PATCH-for-5.0 00/12] hw: Add missing error-propagation code

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> Hi Markus, Peter.
>
> On 3/31/20 3:23 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>>
>>> This series is inspired of Peter fix:
>>> "hw/arm/xlnx-zynqmp.c: fix some error-handling code"
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg691636.html
>>>
>>> Add a cocci script to fix the other places.
>>>
>>> Based-on: <20200324134947.15384-1-peter.mayd...@linaro.org>
>>
>> I skimmed the code patches [PATCH 02-12/12], and they look like bug
>> fixes.  Other reviewers raised a few issues.
>>
>> I also skimmed the Coccinelle script [PATCH 01].  Peter pointed out a
>> few things it apparently missed (e.g. in review of PATCH 06+11).
>> Moreover, the bug pattern applies beyond object_property_set() &
>> friends.  Perhaps the script can be generalized.  No reason to hold
>> fixes.  We may want to add suitable notes to the scipt, though.
>>
>> Can you address the reviews in a v2, so we can get the fixes into -rc1,
>> due today?
>
> Status on this series (sorry I didn't update earlier).
>
> I addressed Peter's comments, improved/simplified/documented the cocci
> script (which I split in smaller ones).
>
> Peter suggested other functions can be checked too, not only the
> "^object_property_set_.*" matches. Indeed, more patches added. Some
> are big.
>
> Another suggestion is replace in init() 'NULL' Error* final argument
> by _abort. This can be another series on top.
> However I noticed we can reduce the error_propagate() generated calls
> in many places, when both init()/realize() exist and the property set
> is not dependent of parent operation, by moving these calls from
> realize() to init(). Another cocci script. But to make sense it has to
> be run previous the "add missing error_propagate" one.
>
> While writing the cocci patches, I had 3 different Coccinelle failures.
> Failures not due to a spatch bug, but timeout because C source hard to
> process. Indeed the C source code was dubious, could get some
> simplification rewrite. Then spatch could transform them. More patches
> in the middle.
>
> Now I'm at 47 patches, the reviewed patches at the end of the series.
> Too much for RC2. Since I don't think these are critical bugs, but
> improvements, are you OK to postpone this series to 5.1?

Punting bug fixes to a later release is always kind of sad, but getting
that many patches reviewed properly in time for -rc2 feels hopeless, and
the bugs old and unthreatending on first glance.

> If you think a patch deserves to be in 5.0, point me at it and I can
> send it ASAP with comments addressed. Else I'll post my series as
> -for-5.1 soon.

I'm okay with punting the whole series to 5.1.

We have quite some Error work pending for 5.1.  Beware of conflicts.

In particular, I now plan to make object_property_set() & friends return
a useful value.  This should make your patches a bit smaller.

[PATCH v5 0/2] Replaced locks with lock guard macros

[dnbrdsky]

From: Daniel Brodsky 

This patch set adds:
- a fix for lock guard macros so they can be used multiple times in
the same function
- replacement of locks with lock guards where appropriate

v4 -> v5:
- added G_GNUC_UNUSED to lock guard macro to supress unused var warning

v3 -> v4:
- removed unneeded unlocks from areas where lock guards are now used
- dropped change to lock guard in iscsi.c as it changed old functionality

v2 -> v3:
- added __COUNTER__ fix for additional lock guard macro
- added missing include header in platform.c

v1 -> v2:
- fixed whitespace churn
- added cover letter so patch set referenced correctly

Daniel Brodsky (2):
  lockable: fix __COUNTER__ macro to be referenced properly
  lockable: replaced locks with lock guard macros where appropriate

 block/iscsi.c   |  7 ++
 block/nfs.c | 51 +++--
 cpus-common.c   | 14 ---
 hw/display/qxl.c| 43 --
 hw/vfio/platform.c  |  5 ++--
 include/qemu/lockable.h |  7 +++---
 include/qemu/rcu.h  |  2 +-
 migration/migration.c   |  3 +--
 migration/multifd.c |  8 +++
 migration/ram.c |  3 +--
 monitor/misc.c  |  4 +---
 ui/spice-display.c  | 14 +--
 util/log.c  |  4 ++--
 util/qemu-timer.c   | 17 +++---
 util/rcu.c  |  8 +++
 util/thread-pool.c  |  3 +--
 util/vfio-helpers.c |  5 ++--
 17 files changed, 88 insertions(+), 110 deletions(-)

-- 
2.26.0

[PATCH v5 2/2] lockable: replaced locks with lock guard macros where appropriate

[dnbrdsky]

From: Daniel Brodsky 

- ran regexp "qemu_mutex_lock\(.*\).*\n.*if" to find targets
- replaced result with QEMU_LOCK_GUARD if all unlocks at function end
- replaced result with WITH_QEMU_LOCK_GUARD if unlock not at end

Signed-off-by: Daniel Brodsky 
---
 block/iscsi.c |  7 ++
 block/nfs.c   | 51 ---
 cpus-common.c | 14 +---
 hw/display/qxl.c  | 43 +---
 hw/vfio/platform.c|  5 ++---
 migration/migration.c |  3 +--
 migration/multifd.c   |  8 +++
 migration/ram.c   |  3 +--
 monitor/misc.c|  4 +---
 ui/spice-display.c| 14 ++--
 util/log.c|  4 ++--
 util/qemu-timer.c | 17 +++
 util/rcu.c|  8 +++
 util/thread-pool.c|  3 +--
 util/vfio-helpers.c   |  5 ++---
 15 files changed, 83 insertions(+), 106 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 682abd8e09..f86a53c096 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1395,20 +1395,17 @@ static void iscsi_nop_timed_event(void *opaque)
 {
 IscsiLun *iscsilun = opaque;
 
-qemu_mutex_lock(>mutex);
+QEMU_LOCK_GUARD(>mutex);
 if (iscsi_get_nops_in_flight(iscsilun->iscsi) >= MAX_NOP_FAILURES) {
 error_report("iSCSI: NOP timeout. Reconnecting...");
 iscsilun->request_timed_out = true;
 } else if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL) != 0) 
{
 error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
-goto out;
+return;
 }
 
 timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 
NOP_INTERVAL);
 iscsi_set_events(iscsilun);
-
-out:
-qemu_mutex_unlock(>mutex);
 }
 
 static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
diff --git a/block/nfs.c b/block/nfs.c
index 9a6311e270..09a78aede8 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -273,15 +273,14 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 nfs_co_init_task(bs, );
 task.iov = iov;
 
-qemu_mutex_lock(>mutex);
-if (nfs_pread_async(client->context, client->fh,
-offset, bytes, nfs_co_generic_cb, ) != 0) {
-qemu_mutex_unlock(>mutex);
-return -ENOMEM;
-}
+WITH_QEMU_LOCK_GUARD(>mutex) {
+if (nfs_pread_async(client->context, client->fh,
+offset, bytes, nfs_co_generic_cb, ) != 0) {
+return -ENOMEM;
+}
 
-nfs_set_events(client);
-qemu_mutex_unlock(>mutex);
+nfs_set_events(client);
+}
 while (!task.complete) {
 qemu_coroutine_yield();
 }
@@ -320,19 +319,18 @@ static int coroutine_fn nfs_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 buf = iov->iov[0].iov_base;
 }
 
-qemu_mutex_lock(>mutex);
-if (nfs_pwrite_async(client->context, client->fh,
- offset, bytes, buf,
- nfs_co_generic_cb, ) != 0) {
-qemu_mutex_unlock(>mutex);
-if (my_buffer) {
-g_free(buf);
+WITH_QEMU_LOCK_GUARD(>mutex) {
+if (nfs_pwrite_async(client->context, client->fh,
+ offset, bytes, buf,
+ nfs_co_generic_cb, ) != 0) {
+if (my_buffer) {
+g_free(buf);
+}
+return -ENOMEM;
 }
-return -ENOMEM;
-}
 
-nfs_set_events(client);
-qemu_mutex_unlock(>mutex);
+nfs_set_events(client);
+}
 while (!task.complete) {
 qemu_coroutine_yield();
 }
@@ -355,15 +353,14 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
 
 nfs_co_init_task(bs, );
 
-qemu_mutex_lock(>mutex);
-if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
-) != 0) {
-qemu_mutex_unlock(>mutex);
-return -ENOMEM;
-}
+WITH_QEMU_LOCK_GUARD(>mutex) {
+if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
+) != 0) {
+return -ENOMEM;
+}
 
-nfs_set_events(client);
-qemu_mutex_unlock(>mutex);
+nfs_set_events(client);
+}
 while (!task.complete) {
 qemu_coroutine_yield();
 }
diff --git a/cpus-common.c b/cpus-common.c
index eaf590cb38..55d5df8923 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -22,6 +22,7 @@
 #include "exec/cpu-common.h"
 #include "hw/core/cpu.h"
 #include "sysemu/cpus.h"
+#include "qemu/lockable.h"
 
 static QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
@@ -71,7 +72,7 @@ static int cpu_get_free_index(void)
 
 void cpu_list_add(CPUState *cpu)
 {
-qemu_mutex_lock(_cpu_list_lock);
+QEMU_LOCK_GUARD(_cpu_list_lock);
 if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
 cpu->cpu_index = cpu_get_free_index();
 assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
@@ -79,15 +80,13 @@ void 

[PATCH v5 1/2] lockable: fix __COUNTER__ macro to be referenced properly

[dnbrdsky]

From: Daniel Brodsky 

- __COUNTER__ doesn't work with ## concat
- replaced ## with glue() macro so __COUNTER__ is evaluated

Fixes: 3284c3ddc4

Signed-off-by: Daniel Brodsky 
---
 include/qemu/lockable.h | 7 ---
 include/qemu/rcu.h  | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 1aeb2cb1a6..b620023141 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -152,7 +152,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable, 
qemu_lockable_auto_unlock)
  *   }
  */
 #define WITH_QEMU_LOCK_GUARD(x) \
-WITH_QEMU_LOCK_GUARD_((x), qemu_lockable_auto##__COUNTER__)
+WITH_QEMU_LOCK_GUARD_((x), glue(qemu_lockable_auto, __COUNTER__))
 
 /**
  * QEMU_LOCK_GUARD - Lock an object until the end of the scope
@@ -169,8 +169,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable, 
qemu_lockable_auto_unlock)
  *   return; <-- mutex is automatically unlocked
  *   }
  */
-#define QEMU_LOCK_GUARD(x) \
-g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ = \
+#define QEMU_LOCK_GUARD(x)   \
+g_autoptr(QemuLockable)  \
+glue(qemu_lockable_auto, __COUNTER__) G_GNUC_UNUSED =\
 qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
 
 #endif
diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 9c82683e37..570aa603eb 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -170,7 +170,7 @@ static inline void rcu_read_auto_unlock(RCUReadAuto *r)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(RCUReadAuto, rcu_read_auto_unlock)
 
 #define WITH_RCU_READ_LOCK_GUARD() \
-WITH_RCU_READ_LOCK_GUARD_(_rcu_read_auto##__COUNTER__)
+WITH_RCU_READ_LOCK_GUARD_(glue(_rcu_read_auto, __COUNTER__))
 
 #define WITH_RCU_READ_LOCK_GUARD_(var) \
 for (g_autoptr(RCUReadAuto) var = rcu_read_auto_lock(); \
-- 
2.26.0

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Andrey Shinkevich]

From: Alberto Garcia 
Sent: Friday, April 3, 2020 7:57 PM
To: qemu-devel@nongnu.org
Cc: Alberto Garcia; qemu-bl...@nongnu.org; Andrey Shinkevich; Max Reitz; Kevin 
Wolf; Vladimir Sementsov-Ogievskiy; Pavel Butsykin
Subject: [PATCH for-5.0] qcow2: Check request size in 
qcow2_co_pwritev_compressed_part()

When issuing a compressed write request the number of bytes must be a
multiple of the cluster size.

With the current code such requests are allowed and we hit an
assertion:

   $ qemu-img create -f qcow2 img.qcow2 1M
   $ qemu-io -c 'write -c 0 32k' img.qcow2

   qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
   Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
  (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' 
failed.
   Aborted

This patch fixes a regression introduced in 0d483dce38

The condition that QEMU supports writing compressed data of the size equal to 
one cluster was introduced with earlier patches.

Andrey

Signed-off-by: Alberto Garcia 
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..923ad428f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
 return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
 }

-if (offset_into_cluster(s, offset)) {
+if (offset_into_cluster(s, offset | bytes)) {
 return -EINVAL;
 }

--
2.20.1

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200403165752.18009-1-be...@igalia.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Not run: 259
Failures: 053
Failed 1 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs
  TESTcheck-qtest-aarch64: tests/qtest/qos-test
Traceback (most recent call last):
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=bbedd703733d47e09453ee5d9ae135e1', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-u4oill6e/src/docker-src.2020-04-03-22.39.14.28831:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=bbedd703733d47e09453ee5d9ae135e1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-u4oill6e/src'
make: *** [docker-run-test-quick@centos7] Error 2

real14m1.546s
user0m8.497s


The full log is available at
http://patchew.org/logs/20200403165752.18009-1-be...@igalia.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v10 10/14] iotests: add hmp helper with logging

[John Snow]

On 4/3/20 4:57 AM, Max Reitz wrote:
> On 02.04.20 20:27, John Snow wrote:
> 
> [...]
> 
>> Are we squared up for this series? I am actually not sure.
> 
> As far as I’m concerned, yes.  I just had this question on how to use mypy.

Oh, whoops, Kevin's comment.

I do want to address that one.

Sorry, I am a bit distracted right now. The last few weeks have been
tough. Having a hard time keeping track of things right now.

--js

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Nicholas Piggin]

Cédric Le Goater's on April 4, 2020 1:47 am:
> On 4/3/20 3:12 PM, Nicholas Piggin wrote:
>> Nicholas Piggin's on April 3, 2020 5:57 pm:
>>> Cédric Le Goater's on March 26, 2020 2:38 am:
 [ Please use c...@kaod.org ! ]

 On 3/25/20 3:41 PM, Nicholas Piggin wrote:
> This implements the NMI interface for the PNV machine, similarly to
> commit 3431648272d ("spapr: Add support for new NMI interface") for
> SPAPR.
>
> Signed-off-by: Nicholas Piggin 

 one minor comment,

 Reviewed-by: Cédric Le Goater 

> ---
>  hw/ppc/pnv.c | 30 +-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b75ad06390..671535ebe6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -27,6 +27,7 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/hw_accel.h"
>  #include "target/ppc/cpu.h"
>  #include "qemu/log.h"
>  #include "hw/ppc/fdt.h"
> @@ -34,6 +35,7 @@
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/loader.h"
> +#include "hw/nmi.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
> @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool 
> value, Error **errp)
>  }
>  }
>
> +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> +{
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +CPUPPCState *env = >env;
> +
> +cpu_synchronize_state(cs);
> +ppc_cpu_do_system_reset(cs);
> +/*
> + * SRR1[42:45] is set to 0100 which the ISA defines as implementation

 I see 'System Reset' in ISA 3.0
> + * dependent. POWER processors use this for xscom triggered 
> interrupts,
> + * which come from the BMC or NMI IPIs.
> + */
> +env->spr[SPR_SRR1] |= PPC_BIT(43);

 So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 
>>>
>>> Ah, that's only for power-saving wakeup. But you got me to dig further
>>> and I think I've got a few things wrong here.
>>>
>>> The architectural power save wakeup due to sreset bit 43 needs to be
>>> set, probably in excp_helper.c if (msr_pow) test.
>>>
>>> case POWERPC_EXCP_RESET: /* System reset exception  
>>>  */
>>> /* A power-saving exception sets ME, otherwise it is unchanged */
>>> if (msr_pow) {
>>> /* indicate that we resumed from power save mode */
>>> msr |= 0x1;
>>> new_msr |= ((target_ulong)1 << MSR_ME);
>>> }
>> 
>> Sorry I'm wrong, that's the old MSR_POW thing I guess G5 had it.
>> 
>> 'stop' state wakeup is powerpc_reset_wakeup of course, which seems to
>> do the right thing with SRR1.
>> 
>> Something like this patch should fix this issue in the ppc-5.1 branch.
>> This appears to do the right thing with SRR1 in testing with Linux.
>> 
>> ---
>>  hw/ppc/pnv.c | 21 +++--
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index ac83b8698b..596ccfd99e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1964,12 +1964,21 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, 
>> run_on_cpu_data arg)
>>  
>>  cpu_synchronize_state(cs);
>>  ppc_cpu_do_system_reset(cs);
>> -/*
>> - * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>> - * dependent. POWER processors use this for xscom triggered interrupts,
>> - * which come from the BMC or NMI IPIs.
>> - */
>> -env->spr[SPR_SRR1] |= PPC_BIT(43);
>> +if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
>> +/* system reset caused wake from power saving state */
>> +if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
>> +warn_report("ppc_cpu_do_system_reset does not set system reset 
>> wakeup reason");
>> +env->spr[SPR_SRR1] |= PPC_BIT(43);
>> +}
>> +} else {
>> +/*
>> + * For non-powersave wakeup system resets, SRR1[42:45] are defined to
>> + * be implementation dependent. Set to 0b0010, which POWER9 UM defines
>> + * to be interrupt caused by SCOM, which can come from the BMC or NMI
>> + * IPIs.
>> + */
>> +env->spr[SPR_SRR1] |= PPC_BIT(44);
>> +}
>>  }
> 
> That looks correct according to the UM.
> 
> Do we care about the other non-powersave wakeup system resets ? 
> 
>   0011 Interrupt caused by hypervisor door bell.
>   0101 Interrupt caused by privileged door bell.

I think that's a typo in the UM, and those are powersave ones.

> 
> The reason is that I sometime see CPU lockups under load, or with a KVM 
> guest, 
> and I haven't found why yet.

I can't tell what's going on there, does it keep taking e80 interrupts
and never clear the 

Re: QEMU participation to Google Season of Docs

[John Snow]

On 4/1/20 12:37 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Google recently announced their 'Season of Docs' project:
> https://developers.google.com/season-of-docs
> 
> QEMU project seems to fit all the requirements.
> 
> Who is interested in [co-]mentoring?
> 
> Relevant links:
> https://developers.google.com/season-of-docs/docs/admin-guide
> https://developers.google.com/season-of-docs/docs/timeline
> 
> [Following is extracted from the previous links:]
> 
> Example projects:
> 
> * Build a documentation site on a platform to be decided
>   by the technical writer and open source mentor, and publish
>   an initial set of basic documents on the site. Examples of
>   platforms include:
> 
>   - A static site generator such as Hugo, Jekyll, Sphinx, ...
> 
> * Refactor the open source project's existing documentation to
>   provide an improved user experience or a more accessible
>   information architecture.
> 
> * Write a conceptual overview of, or introduction to, a product
>   or feature. Often a team creates their technical documentation
>   from the bottom up, with the result that there's a lot of
>   detail but it's hard to understand the product as a whole. A
>   technical writer can fix this.
> 
> * Create a tutorial for a high-profile use case.
> 
> * Create a set of focused how-to guides for specific tasks.
> 
> * Create a contributor’s guide that includes basic information
>   about getting started as a contributor to the open source
>   project, as well as any rules around licence agreements,
>   processes for pull requests and reviews, building the project,
>   and so on.
> 
> Previous experience with similar programs, such as Google Summer
> of Code or others: If you or any of your mentors have taken part
> in Google Summer of Code or a similar program, mention this in
> your application. Describe your achievements in that program.
> Explain how this experience may influence the way you work in
> Season of Docs.
> 
> The 2020 season of Season of Docs is limited to a maximum of
> 50 technical writing projects in total.
> As a guideline, we expect to accept a maximum of 2 projects
> per organization, so that we don't end up with too many
> accepted projects. However, if the free selection process
> doesn't fill all the slots, the Google program administrators
> may allocate additional slots to some organizations.
> 

This looks like it could be very good for us.

My only concern is that the scope and breadth of QEMU is huge and it may
be a lot for a newcomer to tackle appropriately for top-level docs, so I
feel like it requires a mentor who has a good understanding of the broad
picture of QEMU.

Like the description says, we often write things bottom-up in areas of
very specific focus. The broad picture is sometimes harder to conjure
accurately.

I have a lot of opinions and thoughts on python and how docs should be
laid out, but I'm afraid I'm not so good at understanding all of the
options and "use cases" of QEMU to confidently lay out a top-level TOC.
Maybe if we collaborated on a TOC we could give a clear project
guideline to a GSoC/GSoD contributor.

(Or maybe I'm overthinking it.)

--js

Re: [PATCH v3 08/12] linux-user: factor out reading of /proc/self/maps

[Richard Henderson]

On 4/3/20 12:11 PM, Alex Bennée wrote:
> +e->is_read  = fields[1][0] == 'r' ? true : false;
> +e->is_write = fields[1][1] == 'w' ? true : false;
> +e->is_exec  = fields[1][2] == 'x' ? true : false;
> +e->is_priv  = fields[1][3] == 'p' ? true : false;

Drop the redundant ? true : false.  That is of course the result of the boolean
operation.

> +errors += qemu_strtoi(fields[4], NULL, 10, >inode);

The root of the typedef chain for ino_t is

/usr/include/asm-generic/posix_types.h:typedef __kernel_ulong_t __kernel_ino_t;

so I think you should just go ahead and use unsigned long here too.  Or maybe
even uint64_t, because 32-bit has ino64_t, and could in fact have a Large
Number here.


r~

Re: plugin interface function qemu_plugin_mem_size_shift

[Richard Henderson]

On 4/3/20 11:55 AM, Alex Bennée wrote:
> + * The size_shift is the scale of access, e.g. << 3 is a 64 bit wide
> + * access.

Maybe better as "e.g. 1 << 3 is an 8-byte access."?


r~

Re: [PATCH for-5.0?] docs: Improve our gdbstub documentation

[Richard Henderson]

On 4/3/20 2:40 AM, Peter Maydell wrote:
> The documentation of our -s and -gdb options is quite old; in
> particular it still claims that it will cause QEMU to stop and wait
> for the gdb connection, when this has not been true for some time:
> you also need to pass -S if you want to make QEMU not launch the
> guest on startup.
> 
> Improve the documentation to mention this requirement in the
> executable's --help output, the documentation of the -gdb option in
> the manual, and in the "GDB usage" chapter.
> 
> Includes some minor tweaks to these paragraphs of documentation
> since I was editing them anyway (such as dropping the description
> of our gdb support as "primitive").
> 
> Signed-off-by: Peter Maydell 
> ---
> Maybe 5.0 material since it's just a docs improvement?

Reviewed-by: Richard Henderson 


r~

Re: [PATCH-for-5.0 v2] target/rx/translate: Add missing fall through comment

[Richard Henderson]

On 4/3/20 11:44 AM, Philippe Mathieu-Daudé wrote:
> Coverity reported a missing fall through comment, add it.
> 
> Fixes: e5918d7d7f0 ("target/rx: TCG translation")
> Reported-by: Coverity (CID 142 MISSING_BREAK)
> Suggested-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: Fixed typo 'comments -> comment'
> ---
>  target/rx/translate.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH-for-5.0] target/rx/translate: Add missing fall through comment

[Richard Henderson]

On 4/3/20 11:42 AM, Philippe Mathieu-Daudé wrote:
> Coverity reported a missing fall through comments, add it.
> 
> Fixes: e5918d7d7f0 ("target/rx: TCG translation")
> Reported-by: Coverity (CID 142 MISSING_BREAK)
> Suggested-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/rx/translate.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson 


r~

[Bug 1805913] Re: readdir() returns NULL (errno=EOVERFLOW) for 32-bit user-static qemu on 64-bit host

[Eicke Herbertz]

After reading through the discussion on the mailing list, as it's all about 
ext4, I got curious...
I'm testing with qemu-user-static and regulary build arm images in a tmpfs. 
This show similar behaviour and readdir() fails. However, running in the same 
root copied onto a btrfs, it seems fine.
Maybe this is an even less bad workaround for some folks?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1805913

Title:
  readdir() returns NULL (errno=EOVERFLOW) for 32-bit user-static qemu
  on 64-bit host

Status in QEMU:
  Confirmed

Bug description:
  This can be simply reproduced by compiling and running the attached C
  code (readdir-bug.c) under 32-bit user-static qemu, such as qemu-arm-
  static:

  # Setup docker for user-static binfmt
  docker run --rm --privileged multiarch/qemu-user-static:register --reset
  # Compile the code and run (readdir for / is fine, so create a new directory 
/test).
  docker run -v /path/to/qemu-arm-static:/usr/bin/qemu-arm-static -v 
/path/to/readdir-bug.c:/tmp/readdir-bug.c -it --rm arm32v7/ubuntu:18.10 bash -c 
'{ apt update && apt install -y gcc; } >&/dev/null && mkdir -p /test && cd 
/test && gcc /tmp/readdir-bug.c && ./a.out'
  dir=0xff5b4150
  readdir(dir)=(nil)
  errno=75: Value too large for defined data type

  Do remember to replace the /path/to/qemu-arm-static and /path/to
  /readdir-bug.c to the actual paths of the files.

  The root cause is in glibc:
  
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getdents.c;h=6d09a5be7057e2792be9150d3a2c7b293cf6fc34;hb=a5275ba5378c9256d18e582572b4315e8edfcbfb#l87

  By C standard, the return type of readdir() is DIR*, in which the
  inode number and offset are 32-bit integers, therefore, glibc calls
  getdents64() and check if the inode number and offset fits the 32-bit
  range, and reports EOVERFLOW if not.

  The problem here is for 32-bit user-static qemu running on 64-bit
  host, getdents64 simply passing through the inode number and offset
  from underlying getdents64 syscall (from 64-bit kernel), which is very
  likely to not fit into 32-bit range. On real hardware, the 32-bit
  kernel creates 32-bit inode numbers, therefore works properly.

  The glibc code makes sense to do the check to be conformant with C
  standard, therefore ideally it should be a fix on qemu side. I admit
  this is difficult because qemu has to maintain a mapping between
  underlying 64-bit inode numbers and 32-bit inode numbers, which would
  severely hurt the performance. I don't expect this could be fix
  anytime soon (or even there would be a fix), but it would be
  worthwhile to surface this issue.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1805913/+subscriptions

Re: [PATCH v1 0/5] hyperv: VMBus implementation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200403142308.82990-1-ari...@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v1 0/5] hyperv: VMBus implementation
Message-id: 20200403142308.82990-1-ari...@gmail.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f79ca2e i386: Hyper-V VMBus ACPI DSDT entry
d2715b7 vmbus: vmbus implementation
00f328e vmbus: add vmbus protocol definitions
8a097ec hyperv: SControl is optional to enable SynIc
2dfe4b0 hyperv: expose API to determine if synic is enabled

=== OUTPUT BEGIN ===
1/5 Checking commit 2dfe4b0090be (hyperv: expose API to determine if synic is 
enabled)
2/5 Checking commit 8a097ec3667f (hyperv: SControl is optional to enable SynIc)
3/5 Checking commit 00f328eb4254 (vmbus: add vmbus protocol definitions)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#17: 
new file mode 100644

ERROR: do not use C99 // comments
#135: FILE: include/hw/vmbus/vmbus-proto.h:114:
+uint8_t  monitor_flags;  // VMBUS_OFFER_MONITOR_*

ERROR: do not use C99 // comments
#136: FILE: include/hw/vmbus/vmbus-proto.h:115:
+uint16_t interrupt_flags;// VMBUS_OFFER_INTERRUPT_*

ERROR: do not use C99 // comments
#213: FILE: include/hw/vmbus/vmbus-proto.h:192:
+uint32_t feature_bits; // VMBUS_RING_BUFFER_FEAT_*

total: 3 errors, 1 warnings, 222 lines checked

Patch 3/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/5 Checking commit d2715b789821 (vmbus: vmbus implementation)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#106: 
new file mode 100644

ERROR: memory barrier without comment
#645: FILE: hw/vmbus/vmbus.c:505:
+smp_mb();

ERROR: memory barrier without comment
#706: FILE: hw/vmbus/vmbus.c:566:
+smp_mb();

ERROR: "(foo*)" should be "(foo *)"
#1547: FILE: hw/vmbus/vmbus.c:1407:
+vmbus_msg = (struct vmbus_message_header*)msg->payload;

ERROR: "(foo*)" should be "(foo *)"
#2036: FILE: hw/vmbus/vmbus.c:1896:
+msg = (struct vmbus_message_header*)msgdata;

ERROR: space required before the open parenthesis '('
#2088: FILE: hw/vmbus/vmbus.c:1948:
+switch(vmbus->state) {

WARNING: line over 80 characters
#2325: FILE: hw/vmbus/vmbus.c:2185:
+VMSTATE_STRUCT_VARRAY_POINTER_UINT16(channels, VMBusDevice, 
num_channels,

ERROR: spaces required around that '*' (ctx:VxV)
#2532: FILE: hw/vmbus/vmbus.c:2392:
+.subsections = (const VMStateDescription*[]) {
 ^

total: 6 errors, 2 warnings, 2585 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/5 Checking commit f79ca2e47cca (i386: Hyper-V VMBus ACPI DSDT entry)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200403142308.82990-1-ari...@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v4 for-5.0] configure: warn if not using a separate build directory

[Aleksandar Markovic]

16:44 Pet, 03.04.2020. Eric Blake  је написао/ла:
>
> On 4/3/20 9:02 AM, Eric Blake wrote:
> > On 4/3/20 8:53 AM, Daniel P. Berrangé wrote:
> >> Running configure directly from the source directory is a build
> >> configuration that will go away in future. It is also not currently
> >> covered by any automated testing. Display a deprecation warning if
> >> the user attempts to use an in-srcdir build setup, so that they are
> >> aware that they're building QEMU in an undesirable manner.
> >>
> >> Reviewed-by: Eric Blake 
> >> Reviewed-by: Markus Armbruster 
> >> Reviewed-by: Philippe Mathieu-Daudé 
> >> Tested-by: Philippe Mathieu-Daudé 
> >> Signed-off-by: Daniel P. Berrangé 
> >> ---
> >>
> >> Changed in v4:
> >>- Adopted Eric's suggested wording
> >
> >> +if test "$in_srcdir" = "yes"; then
> >> +echo
> >> +echo "WARNING: SUPPORT FOR BUILDING IN THE SOURCE DIR IS
DEPRECATED"
> >> +echo
> >> +echo "Support for running the 'configure' script directly from
the"
> >> +echo "source directory is deprecated. In-tree builds are not
> >> covered"
> >> +echo "by automated testing and thus may not correctly build QEMU."
> >> +echo "Users are recommended to use a separate build directory:"
> >> +echo
> >> +echo "  $ mkdir build"
> >> +echo "  $ cd build"
> >> +echo "  $ ../configure"
> >> +echo "  $ make"
> >
> > Late question, but:
> >
> > Since this is just a warning, we still manage to complete the
> > ./configure run, including whatever generated files it leaves in-tree.
> > Is there any additional step we need to recommend prior to 'mkdir
build'
> > that will clean up the in-tree artifacts, so that the user then
> > attempting the VPATH build won't still have a broken build due to the
> > leftovers from the in-tree attempt?  'make distclean', perhaps?
> >
> > /me starts testing; I'll reply back once it finishes...
>
> tl;dr: 'make distclean' isn't perfect (it still leaves 2 directories
> behind), but does clean up a lot of directories and .mak files, and IS
> necessary before you can build in the subdirectory; but at least make
> warns you.  Still, I'd prefer adding that step in the warning, rather
> than getting an error several steps later.
>
> On a fresh git checkout:
>
> $ ./configure
> ...
> $ git clean -dfxn
> Would remove aarch64-linux-user/
> Would remove aarch64-softmmu/
> Would remove aarch64_be-linux-user/
> Would remove alpha-linux-user/
> Would remove alpha-softmmu/
> Would remove arm-linux-user/
> Would remove arm-softmmu/
> Would remove armeb-linux-user/
> Would remove config-all-disas.mak
> Would remove config-host.mak
> Would remove config.log
> Would remove config.status
> Would remove cris-linux-user/
> Would remove cris-softmmu/
> Would remove docs/sphinx/__pycache__/
> Would remove hppa-linux-user/
> Would remove hppa-softmmu/
> Would remove i386-linux-user/
> Would remove i386-softmmu/
> Would remove linux-headers/asm
> Would remove lm32-softmmu/
> Would remove m68k-linux-user/
> Would remove m68k-softmmu/
> Would remove microblaze-linux-user/
> Would remove microblaze-softmmu/
> Would remove microblazeel-linux-user/
> Would remove microblazeel-softmmu/
> Would remove mips-linux-user/
> Would remove mips-softmmu/
> Would remove mips64-linux-user/
> Would remove mips64-softmmu/
> Would remove mips64el-linux-user/
> Would remove mips64el-softmmu/
> Would remove mipsel-linux-user/
> Would remove mipsel-softmmu/
> Would remove mipsn32-linux-user/
> Would remove mipsn32el-linux-user/
> Would remove moxie-softmmu/
> Would remove nios2-linux-user/
> Would remove nios2-softmmu/
> Would remove or1k-linux-user/
> Would remove or1k-softmmu/
> Would remove ppc-linux-user/
> Would remove ppc-softmmu/
> Would remove ppc64-linux-user/
> Would remove ppc64-softmmu/
> Would remove ppc64abi32-linux-user/
> Would remove ppc64le-linux-user/
> Would remove riscv32-linux-user/
> Would remove riscv32-softmmu/
> Would remove riscv64-linux-user/
> Would remove riscv64-softmmu/
> Would remove rx-softmmu/
> Would remove s390x-linux-user/
> Would remove s390x-softmmu/
> Would remove sh4-linux-user/
> Would remove sh4-softmmu/
> Would remove sh4eb-linux-user/
> Would remove sh4eb-softmmu/
> Would remove sparc-linux-user/
> Would remove sparc-softmmu/
> Would remove sparc32plus-linux-user/
> Would remove sparc64-linux-user/
> Would remove sparc64-softmmu/
> Would remove tests/qemu-iotests/common.env
> Would remove tests/qgraph/
> Would remove tests/tcg/config-aarch64-linux-user.mak
> Would remove tests/tcg/config-aarch64-softmmu.mak
> Would remove tests/tcg/config-aarch64_be-linux-user.mak
> Would remove tests/tcg/config-alpha-linux-user.mak
> Would remove tests/tcg/config-alpha-softmmu.mak
> Would remove tests/tcg/config-arm-linux-user.mak
> Would remove tests/tcg/config-arm-softmmu.mak
> Would remove tests/tcg/config-armeb-linux-user.mak
> Would remove tests/tcg/config-cris-linux-user.mak
> Would remove tests/tcg/config-cris-softmmu.mak
> Would remove 

Re: [PATCH] nvdimm-utils: clean up headers and add license comment

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200403140018.13531-1-imamm...@redhat.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

--- /tmp/qemu-test/src/tests/qemu-iotests/040.out   2020-04-03 
13:54:30.0 +
+++ /tmp/qemu-test/build/tests/qemu-iotests/040.out.bad 2020-04-03 
21:58:02.494078702 +
@@ -1,3 +1,5 @@
+WARNING:qemu.machine:qemu received signal 9: 
/tmp/qemu-test/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 
-display none -vga none -chardev 
socket,id=mon,path=/tmp/tmp.ZVJAkdsYy9/qemu-20624-monitor.sock -mon 
chardev=mon,mode=control -qtest 
unix:path=/tmp/tmp.ZVJAkdsYy9/qemu-20624-qtest.sock -accel qtest -nodefaults 
-display none -accel qtest -drive 
if=none,id=drive0,file=/tmp/qemu-test/test.img,format=qcow2,cache=writeback,aio=threads,node-name=top,backing.node-name=mid,backing.backing.node-name=base
 -device virtio-scsi-pci -device scsi-hd,id=scsi0,drive=drive0
+WARNING:qemu.machine:qemu received signal 9: 
/tmp/qemu-test/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 
-display none -vga none -chardev 
socket,id=mon,path=/tmp/tmp.ZVJAkdsYy9/qemu-20624-monitor.sock -mon 
chardev=mon,mode=control -qtest 
unix:path=/tmp/tmp.ZVJAkdsYy9/qemu-20624-qtest.sock -accel qtest -nodefaults 
-display none -accel qtest -drive 
if=none,id=drive0,file=/tmp/qemu-test/test.img,format=qcow2,cache=writeback,aio=threads,node-name=top,backing.node-name=mid,backing.backing.node-name=base
 -device virtio-scsi-pci -device scsi-hd,id=scsi0,drive=drive0
 ...
 --
 Ran 59 tests
---
Not run: 259
Failures: 040
Failed 1 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=f83247c63c6242558aed52ecb1ff0bfd', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-8t1gmbs5/src/docker-src.2020-04-03-17.51.47.11538:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=f83247c63c6242558aed52ecb1ff0bfd
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-8t1gmbs5/src'
make: *** [docker-run-test-quick@centos7] Error 2

real14m7.306s
user0m9.304s


The full log is available at
http://patchew.org/logs/20200403140018.13531-1-imamm...@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Maciej S. Szmigiero]

It seems to me that Roman might not be getting our e-mails since his
new e-mail address seems to be "rvka...@yandex-team.ru".

@Roman, are you with us?

Thanks,
Maciej

On 03.04.2020 19:18, Maciej S. Szmigiero wrote:
> Hi Jon,
> 
> The patches are available here:
> https://github.com/maciejsszmigiero/qemu.git in "vmbus-patches" branch.
> 
> Please note that these patches don't have Roman's "Signed-off-by:" tags,
> so I haven't applied mine, either.
> 
> If you are able to establish a proper SoB chain then please also add:
> "Signed-off-by: Maciej S. Szmigiero ".
> 
> Thanks for the effort,
> Maciej
> 
> On 03.04.2020 17:30, Jon Doron wrote:
>>  Thank you Maciej it seems like your version is really ahead I'll do
>> the required work and merge it so i can submit a v2 with the latest
>> patchset from Roman
>>
>> On Fri, Apr 3, 2020 at 6:06 PM Jon Doron  wrote:
>>>
>>> Thank you Maciej, I based it on top of what Denis (d...@openvz.org) gave me
>>> which was this:
>>> https://ftp.openvz.org/virtuozzo/releases/openvz-7.0.12-288/source/SRPMS/q/qemu-kvm-vz-2.12.0-33.vz7.14.4.src.rpm
>>>
>>> Do you think you have a more recent version I dont mind diffing and
>>> resubmitting a new version of the patchset?
>>>
>>> Thanks,
>>> -- Jon.
>>>
>>> On Fri, Apr 3, 2020 at 5:56 PM Maciej S. Szmigiero
>>>  wrote:

 Hi Jon,

 On 03.04.2020 16:23, Jon Doron wrote:
> Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
> entry to DSDT in case vmbus has been enabled.
>
> Experimentally Windows guests were found to require this entry to
> include two IRQ resources, so this patch adds two semi-arbitrarily
> chosen ones (7 and 13).  This results, in particular, in parallel port
> conflicting with vmbus.
>
> TODO: discover and use spare IRQs to avoid conflicts.
>
> Signed-off-by: Evgeny Yakovlev 
> Signed-off-by: Roman Kagan 
> Signed-off-by: Jon Doron 

 Nice work, thanks!

 However, it seems to be based on the code version that was posted in
 February 2018, and not the latest version in OpenVZ qemu repository
 dated October 2019:
 https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus

 This newer version has slightly different API here and there.
 Any particular reason for selecting that older version for porting?

 I have actually rebased this latest version on the top of the current
 QEMU master, and it basically seems to work fine.
 However, I haven't done extensive tests whether there isn't a memory leak
 somewhere or so on.

 Maciej
> 

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Philippe Mathieu-Daudé]

On 3/24/20 6:36 PM, Peter Maydell wrote:

In write_elf_section() we set the 'shdr' pointer to point to local
structures shdr32 or shdr64, which we fill in to be written out to
the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
has a spurious '&' operator, so instead of writing out the section
header we write out the literal pointer value followed by whatever is
on the stack after the 'shdr' local variable.


How did you notice this? While reviewing around?

Reviewed-by: Philippe Mathieu-Daudé 



Pass the correct address into fd_write_vmcore().

Signed-off-by: Peter Maydell 
---
I have not tested this because I can't reproduce the conditions
under which we try to actually use write_elf_section() (they
must be rare, because currently we produce a bogus ELF file
for this code path). In dump_init() s->list.num must be
at least UINT16_MAX-1, which I think means it has to be a
paging-enabled dump and the guest's page table must be
extremely fragmented ?
---
  dump/dump.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index 6fb6e1245ad..22ed1d3b0d4 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -364,7 +364,7 @@ static void write_elf_section(DumpState *s, int type, Error 
**errp)
  shdr = 
  }
  
-ret = fd_write_vmcore(, shdr_size, s);

+ret = fd_write_vmcore(shdr, shdr_size, s);
  if (ret < 0) {
  error_setg_errno(errp, -ret,
   "dump: failed to write section header table");

Re: [PATCH v3 04/12] linux-user: more debug for init_guest_space

[Philippe Mathieu-Daudé]

On 4/3/20 9:11 PM, Alex Bennée wrote:

Searching for memory space can cause problems so lets extend the
CPU_LOG_PAGE output so you can watch init_guest_space fail to
allocate memory. A more involved fix is actually required to make this
function play nicely with the large guard pages the sanitiser likes to


TIL "in an effort to differentiate British English from American, many 
British publishers have begun giving -ise endings even to words that 
have always been spelled -ize."


https://grammarist.com/spelling/sanitise-sanitize/


use.

Signed-off-by: Alex Bennée 
Reviewed-by: Laurent Vivier 


Reviewed-by: Philippe Mathieu-Daudé 


---
  linux-user/elfload.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8198be04460..619c054cc48 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2172,6 +2172,8 @@ unsigned long init_guest_space(unsigned long host_start,
  
  /* Check to see if the address is valid.  */

  if (host_start && real_start != current_start) {
+qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
+  host_start, real_start, current_start);
  goto try_again;
  }
  
@@ -2240,7 +2242,11 @@ unsigned long init_guest_space(unsigned long host_start,

   * probably a bad strategy if not, which means we got here
   * because of trouble with ARM commpage setup.
   */
-munmap((void *)real_start, real_size);
+if (munmap((void *)real_start, real_size) != 0) {
+error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
+ real_start, real_size, strerror(errno));
+abort();
+}
  current_start += align;
  if (host_start == current_start) {
  /* Theoretically possible if host doesn't have any suitably

Re: [PATCH v16 QEMU 14/16] vfio: Add vfio_listener_log_sync to mark dirty pages

[Kirti Wankhede]

On 4/1/2020 11:20 AM, Yan Zhao wrote:

On Wed, Mar 25, 2020 at 05:09:12AM +0800, Kirti Wankhede wrote:

vfio_listener_log_sync gets list of dirty pages from container using
VFIO_IOMMU_GET_DIRTY_BITMAP ioctl and mark those pages dirty when all
devices are stopped and saving state.
Return early for the RAM block section of mapped MMIO region.

Signed-off-by: Kirti Wankhede 
Reviewed-by: Neo Jia 
---
  hw/vfio/common.c | 200 +--
  hw/vfio/trace-events |   1 +
  2 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4a2f0d6a2233..6d41e1ac5c2f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -29,6 +29,7 @@
  #include "hw/vfio/vfio.h"
  #include "exec/address-spaces.h"
  #include "exec/memory.h"
+#include "exec/ram_addr.h"
  #include "hw/hw.h"
  #include "qemu/error-report.h"
  #include "qemu/main-loop.h"
@@ -38,6 +39,7 @@
  #include "sysemu/reset.h"
  #include "trace.h"
  #include "qapi/error.h"
+#include "migration/migration.h"
  
  VFIOGroupList vfio_group_list =

  QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -288,6 +290,28 @@ const MemoryRegionOps vfio_region_ops = {
  };
  
  /*

+ * Device state interfaces
+ */
+
+static bool vfio_devices_are_stopped_and_saving(void)
+{
+VFIOGroup *group;
+VFIODevice *vbasedev;
+
+QLIST_FOREACH(group, _group_list, next) {
+QLIST_FOREACH(vbasedev, >device_list, next) {
+if ((vbasedev->device_state & VFIO_DEVICE_STATE_SAVING) &&
+!(vbasedev->device_state & VFIO_DEVICE_STATE_RUNNING)) {
+continue;
+} else {
+return false;
+}
+}
+}
+return true;
+}
+
+/*
   * DMA - Mapping and unmapping for the "type1" IOMMU interface used on x86
   */
  static int vfio_dma_unmap(VFIOContainer *container,
@@ -408,8 +432,8 @@ static bool 
vfio_listener_skipped_section(MemoryRegionSection *section)
  }
  
  /* Called with rcu_read_lock held.  */

-static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void **vaddr,
-   bool *read_only)
+static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
+   ram_addr_t *ram_addr, bool *read_only)
  {
  MemoryRegion *mr;
  hwaddr xlat;
@@ -440,9 +464,17 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void 
**vaddr,
  return false;
  }
  
-*vaddr = memory_region_get_ram_ptr(mr) + xlat;

-*read_only = !writable || mr->readonly;
+if (vaddr) {
+*vaddr = memory_region_get_ram_ptr(mr) + xlat;
+}
  
+if (ram_addr) {

+*ram_addr = memory_region_get_ram_addr(mr) + xlat;
+}
+
+if (read_only) {
+*read_only = !writable || mr->readonly;
+}
  return true;
  }
  
@@ -467,7 +499,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)

  rcu_read_lock();
  
  if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {

-if (!vfio_get_vaddr(iotlb, , _only)) {
+if (!vfio_get_xlat_addr(iotlb, , NULL, _only)) {
  goto out;
  }
  /*
@@ -813,9 +845,167 @@ static void vfio_listener_region_del(MemoryListener 
*listener,
  }
  }
  
+static int vfio_get_dirty_bitmap(MemoryListener *listener,

+ MemoryRegionSection *section)
+{
+VFIOContainer *container = container_of(listener, VFIOContainer, listener);
+VFIOGuestIOMMU *giommu;
+IOMMUTLBEntry iotlb;
+hwaddr granularity, address_limit, iova;
+int ret;
+
+if (memory_region_is_iommu(section->mr)) {
+QLIST_FOREACH(giommu, >giommu_list, giommu_next) {
+if (MEMORY_REGION(giommu->iommu) == section->mr &&
+giommu->n.start == section->offset_within_region) {
+break;
+}
+}
+
+if (!giommu) {
+return -EINVAL;
+}
+}
+
+if (memory_region_is_iommu(section->mr)) {
+granularity = memory_region_iommu_get_min_page_size(giommu->iommu);
+
+address_limit = MIN(int128_get64(section->size),
+
memory_region_iommu_get_address_limit(giommu->iommu,
+ int128_get64(section->size)));
+} else {
+granularity = memory_region_size(section->mr);
+address_limit = int128_get64(section->size);
+}
+
+iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
+
+RCU_READ_LOCK_GUARD();
+


the requirement of iova < address_limit is not right. reason as blow:
when vIOMMU is NOT on,
iova is section->offset_within_address_space,
address_limit is section->size,
but iova has not reason to be less than address_limit.

for example, when vm memory size is large than 3G (e.g.4G)
for memory region section of range (0x1-0x13fff),
its iova is 0x1, address_limit is 0x4000,
then as iova is not less than address_limit, dirty pages query for 

[PATCH v3 12/12] configure: Add -Werror to PIE probe

[Alex Bennée]

From: Richard Henderson 

Without -Werror, the probe may succeed, but then compilation fails
later when -Werror is added for other reasons.  Shows up on windows,
where the compiler complains about -fPIC.

Signed-off-by: Richard Henderson 
Signed-off-by: Alex Bennée 
Message-Id: <20200401214756.6559-1-richard.hender...@linaro.org>
---
 configure | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 22870f38672..233c671aaa9 100755
--- a/configure
+++ b/configure
@@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then
 fi
 
 if test "$static" = "yes"; then
-  if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then
+  if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; 
then
 QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
 QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS"
 pie="yes"
@@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then
 elif test "$pie" = "no"; then
   QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS"
   QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS"
-elif compile_prog "-fPIE -DPIE" "-pie"; then
+elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then
   QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
   QEMU_LDFLAGS="-pie $QEMU_LDFLAGS"
   pie="yes"
-- 
2.20.1

[PATCH v3 09/12] linux-user: clean-up padding on /proc/self/maps

[Alex Bennée]

Don't use magic spaces, calculate the justification for the file
field like the kernel does with seq_pad.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 

---
v2
  - change for unsigned long update
---
 linux-user/syscall.c | 32 +++-
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 666be20f3ff..8a610b91402 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7235,6 +7235,7 @@ static int open_self_maps(void *cpu_env, int fd)
 TaskState *ts = cpu->opaque;
 GSList *map_info = read_self_maps();
 GSList *s;
+int count;
 
 for (s = map_info; s; s = g_slist_next(s)) {
 MapInfo *e = (MapInfo *) s->data;
@@ -7253,20 +7254,24 @@ static int open_self_maps(void *cpu_env, int fd)
 }
 
 if (h2g(min) == ts->info->stack_limit) {
-path = "  [stack]";
+path = "[stack]";
 } else {
 path = e->path;
 }
 
-dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-" %c%c%c%c %08" PRIx64 " %s %d %s%s\n",
-h2g(min), h2g(max - 1) + 1,
-e->is_read ? 'r' : '-',
-e->is_write ? 'w' : '-',
-e->is_exec ? 'x' : '-',
-e->is_priv ? 'p' : '-',
-(uint64_t) e->offset, e->dev, e->inode,
-path ? " " : "", path ? path : "");
+count = dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
+" %c%c%c%c %08" PRIx64 " %s %d",
+h2g(min), h2g(max - 1) + 1,
+e->is_read ? 'r' : '-',
+e->is_write ? 'w' : '-',
+e->is_exec ? 'x' : '-',
+e->is_priv ? 'p' : '-',
+(uint64_t) e->offset, e->dev, e->inode);
+if (path) {
+dprintf(fd, "%*s%s\n", 73 - count, "", path);
+} else {
+dprintf(fd, "\n");
+}
 }
 }
 
@@ -7277,9 +7282,10 @@ static int open_self_maps(void *cpu_env, int fd)
  * We only support execution from the vsyscall page.
  * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
  */
-dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
-" --xp  00:00 0 [vsyscall]\n",
-TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
+count = dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
+" --xp  00:00 0",
+TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + 
TARGET_PAGE_SIZE);
+dprintf(fd, "%*s%s\n", 73 - count, "",  "[vsyscall]");
 #endif
 
 return 0;
-- 
2.20.1

[PATCH v3 10/12] target/arm: don't expose "ieee_half" via gdbstub

[Alex Bennée]

While support for parsing ieee_half in the XML description was added
to gdb in 2019 (a6d0f249) there is no easy way for the gdbstub to know
if the gdb end will understand it. Disable it for now and allow older
gdbs to successfully connect to the default -cpu max SVE enabled
QEMUs.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 
---
 target/arm/gdbstub.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index d9ef7d2187c..8efc535f2a0 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -192,7 +192,12 @@ static const struct TypeSize vec_lanes[] = {
 /* 16 bit */
 { "uint16", 16, 'h', 'u' },
 { "int16", 16, 'h', 's' },
-{ "ieee_half", 16, 'h', 'f' },
+/*
+ * TODO: currently there is no reliable way of telling
+ * if the remote gdb actually understands ieee_half so
+ * we don't expose it in the target description for now.
+ * { "ieee_half", 16, 'h', 'f' },
+ */
 /* bytes */
 { "uint8", 8, 'b', 'u' },
 { "int8", 8, 'b', 's' },
-- 
2.20.1

[PATCH v3 06/12] gdbstub: fix compiler complaining

[Alex Bennée]

From: Denis Plotnikov 

./gdbstub.c: In function ‘handle_query_thread_extra’:
/usr/include/glib-2.0/glib/glib-autocleanups.h:28:10:
error: ‘cpu_name’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
g_free (*pp);
   ^
./gdbstub.c:2063:26: note: ‘cpu_name’ was declared here
g_autofree char *cpu_name;
 ^
cc1: all warnings being treated as errors

Signed-off-by: Denis Plotnikov 
Message-Id: <20200326151407.25046-1-dplotni...@virtuozzo.com>
Reported-by: Euler Robot 
Reported-by: Chen Qun 
Reviewed-by: Richard Henderson 
Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
---
 gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 013fb1ac0f1..171e1509509 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2060,8 +2060,8 @@ static void handle_query_thread_extra(GdbCmdContext 
*gdb_ctx, void *user_ctx)
 /* Print the CPU model and name in multiprocess mode */
 ObjectClass *oc = object_get_class(OBJECT(cpu));
 const char *cpu_model = object_class_get_name(oc);
-g_autofree char *cpu_name;
-cpu_name  = object_get_canonical_path_component(OBJECT(cpu));
+g_autofree char *cpu_name =
+object_get_canonical_path_component(OBJECT(cpu));
 g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
 cpu->halted ? "halted " : "running");
 } else {
-- 
2.20.1

[PATCH v3 08/12] linux-user: factor out reading of /proc/self/maps

[Alex Bennée]

Unfortunately reading /proc/self/maps is still considered the gold
standard for a process finding out about it's own memory layout. As we
will want this data in other contexts soon factor out the code to read
and parse the data. Rather than just blindly copying the existing
sscanf based code we use a more modern glib version of the parsing
code to make a more general purpose map structure.

Signed-off-by: Alex Bennée 

---
v2
  - s/uint64_t/unsigned long/ for MapInfo structure
  - limit to 6 fields and strip leading spaces
---
 include/qemu/selfmap.h | 44 
 linux-user/syscall.c   | 58 +++
 util/selfmap.c | 77 ++
 util/Makefile.objs |  1 +
 4 files changed, 150 insertions(+), 30 deletions(-)
 create mode 100644 include/qemu/selfmap.h
 create mode 100644 util/selfmap.c

diff --git a/include/qemu/selfmap.h b/include/qemu/selfmap.h
new file mode 100644
index 000..302c459d695
--- /dev/null
+++ b/include/qemu/selfmap.h
@@ -0,0 +1,44 @@
+/*
+ * Utility functions to read our own memory map
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _SELFMAP_H_
+#define _SELFMAP_H_
+
+typedef struct {
+unsigned long start;
+unsigned long end;
+
+/* flags */
+bool is_read;
+bool is_write;
+bool is_exec;
+bool is_priv;
+
+unsigned long offset;
+gchar *dev;
+int   inode;
+gchar *path;
+} MapInfo;
+
+
+/**
+ * read_self_maps:
+ *
+ * Read /proc/self/maps and return a list of MapInfo structures.
+ */
+GSList *read_self_maps(void);
+
+/**
+ * free_self_maps:
+ * @info: a GSlist
+ *
+ * Free a list of MapInfo structures.
+ */
+void free_self_maps(GSList *info);
+
+#endif /* _SELFMAP_H_ */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b679bc6b136..666be20f3ff 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -117,6 +117,7 @@
 
 #include "qemu.h"
 #include "qemu/guest-random.h"
+#include "qemu/selfmap.h"
 #include "user/syscall-trace.h"
 #include "qapi/error.h"
 #include "fd-trans.h"
@@ -7232,45 +7233,45 @@ static int open_self_maps(void *cpu_env, int fd)
 {
 CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
 TaskState *ts = cpu->opaque;
-FILE *fp;
-char *line = NULL;
-size_t len = 0;
-ssize_t read;
+GSList *map_info = read_self_maps();
+GSList *s;
 
-fp = fopen("/proc/self/maps", "r");
-if (fp == NULL) {
-return -1;
-}
+for (s = map_info; s; s = g_slist_next(s)) {
+MapInfo *e = (MapInfo *) s->data;
 
-while ((read = getline(, , fp)) != -1) {
-int fields, dev_maj, dev_min, inode;
-uint64_t min, max, offset;
-char flag_r, flag_w, flag_x, flag_p;
-char path[512] = "";
-fields = sscanf(line, "%"PRIx64"-%"PRIx64" %c%c%c%c %"PRIx64" %x:%x %d"
-" %512s", , , _r, _w, _x,
-_p, , _maj, _min, , path);
-
-if ((fields < 10) || (fields > 11)) {
-continue;
-}
-if (h2g_valid(min)) {
+if (h2g_valid(e->start)) {
+unsigned long min = e->start;
+unsigned long max = e->end;
 int flags = page_get_flags(h2g(min));
-max = h2g_valid(max - 1) ? max : (uintptr_t)g2h(GUEST_ADDR_MAX) + 
1;
+const char *path;
+
+max = h2g_valid(max - 1) ?
+max : (uintptr_t) g2h(GUEST_ADDR_MAX) + 1;
+
 if (page_check_range(h2g(min), max - min, flags) == -1) {
 continue;
 }
+
 if (h2g(min) == ts->info->stack_limit) {
-pstrcpy(path, sizeof(path), "  [stack]");
+path = "  [stack]";
+} else {
+path = e->path;
 }
+
 dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-" %c%c%c%c %08" PRIx64 " %02x:%02x %d %s%s\n",
-h2g(min), h2g(max - 1) + 1, flag_r, flag_w,
-flag_x, flag_p, offset, dev_maj, dev_min, inode,
-path[0] ? " " : "", path);
+" %c%c%c%c %08" PRIx64 " %s %d %s%s\n",
+h2g(min), h2g(max - 1) + 1,
+e->is_read ? 'r' : '-',
+e->is_write ? 'w' : '-',
+e->is_exec ? 'x' : '-',
+e->is_priv ? 'p' : '-',
+(uint64_t) e->offset, e->dev, e->inode,
+path ? " " : "", path ? path : "");
 }
 }
 
+free_self_maps(map_info);
+
 #ifdef TARGET_VSYSCALL_PAGE
 /*
  * We only support execution from the vsyscall page.
@@ -7281,9 +7282,6 @@ static int open_self_maps(void *cpu_env, int fd)
 TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
 #endif
 
-free(line);
-fclose(fp);
-
 return 0;
 }
 
diff --git a/util/selfmap.c 

[PATCH v3 05/12] target/xtensa: add FIXME for translation memory leak

[Alex Bennée]

Dynamically allocating a new structure within the DisasContext can
potentially leak as we can longjmp out of the translation loop (see
test_phys_mem). The proper fix would be to use static allocation
within the DisasContext but as the Xtensa translator imports it's code
from elsewhere I leave that as an exercise for the maintainer.

Signed-off-by: Alex Bennée 
Acked-by: Max Filippov 
---
 target/xtensa/translate.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 8aa972cafdf..37f65b1f030 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1174,6 +1174,11 @@ static void 
xtensa_tr_init_disas_context(DisasContextBase *dcbase,
 dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
XTENSA_TBFLAG_CALLINC_SHIFT);
 
+/*
+ * FIXME: This will leak when a failed instruction load or similar
+ * event causes us to longjump out of the translation loop and
+ * hence not clean-up in xtensa_tr_tb_stop
+ */
 if (dc->config->isa) {
 dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
 dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
-- 
2.20.1

[PATCH v3 11/12] hw/core: properly terminate loading .hex on EOF record

[Alex Bennée]

The https://makecode.microbit.org/#editor generates slightly weird
.hex files which work fine on a real microbit but causes QEMU to
choke. The reason is extraneous data after the EOF record which causes
the loader to attempt to write a bigger file than it should to the
"rom". According to the HEX file spec an EOF really should be the last
thing we process so lets do that.

Reported-by: Ursula Bennée 
Signed-off-by: Alex Bennée 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/core/loader.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index eeef6da9a1b..8bbb1797a4c 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1447,6 +1447,7 @@ typedef struct {
 uint32_t current_rom_index;
 uint32_t rom_start_address;
 AddressSpace *as;
+bool complete;
 } HexParser;
 
 /* return size or -1 if error */
@@ -1484,6 +1485,7 @@ static int handle_record_type(HexParser *parser)
   parser->current_rom_index,
   parser->rom_start_address, parser->as);
 }
+parser->complete = true;
 return parser->total_size;
 case EXT_SEG_ADDR_RECORD:
 case EXT_LINEAR_ADDR_RECORD:
@@ -1548,11 +1550,12 @@ static int parse_hex_blob(const char *filename, hwaddr 
*addr, uint8_t *hex_blob,
 .bin_buf = g_malloc(hex_blob_size),
 .start_addr = addr,
 .as = as,
+.complete = false
 };
 
 rom_transaction_begin();
 
-for (; hex_blob < end; ++hex_blob) {
+for (; hex_blob < end && !parser.complete; ++hex_blob) {
 switch (*hex_blob) {
 case '\r':
 case '\n':
-- 
2.20.1

[PATCH v3 04/12] linux-user: more debug for init_guest_space

[Alex Bennée]

Searching for memory space can cause problems so lets extend the
CPU_LOG_PAGE output so you can watch init_guest_space fail to
allocate memory. A more involved fix is actually required to make this
function play nicely with the large guard pages the sanitiser likes to
use.

Signed-off-by: Alex Bennée 
Reviewed-by: Laurent Vivier 
---
 linux-user/elfload.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8198be04460..619c054cc48 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2172,6 +2172,8 @@ unsigned long init_guest_space(unsigned long host_start,
 
 /* Check to see if the address is valid.  */
 if (host_start && real_start != current_start) {
+qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
+  host_start, real_start, current_start);
 goto try_again;
 }
 
@@ -2240,7 +2242,11 @@ unsigned long init_guest_space(unsigned long host_start,
  * probably a bad strategy if not, which means we got here
  * because of trouble with ARM commpage setup.
  */
-munmap((void *)real_start, real_size);
+if (munmap((void *)real_start, real_size) != 0) {
+error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
+ real_start, real_size, strerror(errno));
+abort();
+}
 current_start += align;
 if (host_start == current_start) {
 /* Theoretically possible if host doesn't have any suitably
-- 
2.20.1

[PATCH v3 07/12] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Alex Bennée]

From: Richard Henderson 

All other calls to normalize*Subnormal detect zero input before
the call -- this is the only outlier.  This case can happen with
+0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
the correct sign.

Reported-by: Coverity (CID 1421991)
Signed-off-by: Richard Henderson 
Signed-off-by: Alex Bennée 
Message-Id: <20200327232042.10008-1-richard.hender...@linaro.org>
---
 fpu/softfloat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 301ce3b537b..ae6ba718540 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, 
flag zSign,
 zSig1 = 0;
 zSig0 = aSig + bSig;
 if ( aExp == 0 ) {
+if (zSig0 == 0) {
+return packFloatx80(zSign, 0, 0);
+}
 normalizeFloatx80Subnormal( zSig0, ,  );
 goto roundAndPack;
 }
-- 
2.20.1

[PATCH v3 01/12] elf-ops: bail out if we have no function symbols

[Alex Bennée]

It's perfectly possible to have no function symbols in your elf file
and if we do the undefined behaviour sanitizer rightly complains about
us passing NULL to qsort. Check nsyms before we go ahead.

While we are at it lets drop the unchecked return value and cleanup
the fail leg by use of g_autoptr.

Another fix was proposed 101 weeks ago in:
Message-Id: 20180421232120.22208-1-f4...@amsat.org

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
---
 include/hw/elf_ops.h | 48 +++-
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index a1411bfcab6..e0bb47bb678 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -104,19 +104,21 @@ static int glue(symcmp, SZ)(const void *s0, const void 
*s1)
 : ((sym0->st_value > sym1->st_value) ? 1 : 0);
 }
 
-static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
-  int clear_lsb, symbol_fn_t sym_cb)
+static void glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
+   int clear_lsb, symbol_fn_t sym_cb)
 {
-struct elf_shdr *symtab, *strtab, *shdr_table = NULL;
-struct elf_sym *syms = NULL;
+struct elf_shdr *symtab, *strtab;
+g_autofree struct elf_shdr *shdr_table = NULL;
+g_autofree struct elf_sym *syms = NULL;
+g_autofree char *str = NULL;
 struct syminfo *s;
 int nsyms, i;
-char *str = NULL;
 
 shdr_table = load_at(fd, ehdr->e_shoff,
  sizeof(struct elf_shdr) * ehdr->e_shnum);
-if (!shdr_table)
-return -1;
+if (!shdr_table) {
+return ;
+}
 
 if (must_swab) {
 for (i = 0; i < ehdr->e_shnum; i++) {
@@ -125,23 +127,25 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, 
int fd, int must_swab,
 }
 
 symtab = glue(find_section, SZ)(shdr_table, ehdr->e_shnum, SHT_SYMTAB);
-if (!symtab)
-goto fail;
+if (!symtab) {
+return;
+}
 syms = load_at(fd, symtab->sh_offset, symtab->sh_size);
-if (!syms)
-goto fail;
+if (!syms) {
+return;
+}
 
 nsyms = symtab->sh_size / sizeof(struct elf_sym);
 
 /* String table */
 if (symtab->sh_link >= ehdr->e_shnum) {
-goto fail;
+return;
 }
 strtab = _table[symtab->sh_link];
 
 str = load_at(fd, strtab->sh_offset, strtab->sh_size);
 if (!str) {
-goto fail;
+return;
 }
 
 i = 0;
@@ -170,8 +174,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int 
fd, int must_swab,
 }
 i++;
 }
-syms = g_realloc(syms, nsyms * sizeof(*syms));
 
+/* check we have symbols left */
+if (nsyms == 0) {
+return;
+}
+
+syms = g_realloc(syms, nsyms * sizeof(*syms));
 qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
 for (i = 0; i < nsyms - 1; i++) {
 if (syms[i].st_size == 0) {
@@ -182,18 +191,11 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, 
int fd, int must_swab,
 /* Commit */
 s = g_malloc0(sizeof(*s));
 s->lookup_symbol = glue(lookup_symbol, SZ);
-glue(s->disas_symtab.elf, SZ) = syms;
+glue(s->disas_symtab.elf, SZ) = g_steal_pointer();
 s->disas_num_syms = nsyms;
-s->disas_strtab = str;
+s->disas_strtab = g_steal_pointer();
 s->next = syminfos;
 syminfos = s;
-g_free(shdr_table);
-return 0;
- fail:
-g_free(syms);
-g_free(str);
-g_free(shdr_table);
-return -1;
 }
 
 static int glue(elf_reloc, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
-- 
2.20.1

[PATCH v3 03/12] tests/tcg: remove extraneous pasting macros

[Alex Bennée]

We are not using them and they just get in the way.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
---
 tests/tcg/x86_64/system/boot.S | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/tcg/x86_64/system/boot.S b/tests/tcg/x86_64/system/boot.S
index 205cfbd3982..73b19a2bda6 100644
--- a/tests/tcg/x86_64/system/boot.S
+++ b/tests/tcg/x86_64/system/boot.S
@@ -41,10 +41,7 @@
 #define XEN_ELFNOTE_PHYS32_ENTRY  18
 
 #define __ASM_FORM(x)  x
-#define __ASM_FORM_RAW(x) x
-#define __ASM_FORM_COMMA(x) x,
-#define __ASM_SEL(a,b)   __ASM_FORM(b)
-#define __ASM_SEL_RAW(a,b)  __ASM_FORM_RAW(b)
+#define __ASM_SEL(a,b)  __ASM_FORM(b)
 #define _ASM_PTR   __ASM_SEL(.long, .quad)
 
ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,  _ASM_PTR 0x10)
-- 
2.20.1

[PATCH v3 02/12] linux-user: protect fcntl64 with an #ifdef

[Alex Bennée]

Checking TARGET_ABI_BITS is sketchy - we should check for the presence
of the define to be sure. Also clean up the white space while we are
there.

Signed-off-by: Alex Bennée 
Reviewed-by: Laurent Vivier 
Reviewed-by: Richard Henderson 
---
 linux-user/syscall.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5af55fca781..b679bc6b136 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11331,11 +11331,11 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
This is a hint, so ignoring and returning success is ok.  */
 return 0;
 #endif
-#if TARGET_ABI_BITS == 32
+#ifdef TARGET_NR_fcntl64
 case TARGET_NR_fcntl64:
 {
-   int cmd;
-   struct flock64 fl;
+int cmd;
+struct flock64 fl;
 from_flock64_fn *copyfrom = copy_from_user_flock64;
 to_flock64_fn *copyto = copy_to_user_flock64;
 
@@ -11346,7 +11346,7 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
 }
 #endif
 
-   cmd = target_to_host_fcntl_cmd(arg2);
+cmd = target_to_host_fcntl_cmd(arg2);
 if (cmd == -TARGET_EINVAL) {
 return cmd;
 }
-- 
2.20.1

[PATCH v3 for 5.0-rc2 00/12] a selection of random fixes

[Alex Bennée]

Hi,

Here is version 3 of my random fixes series. 

I've dropped the more involved re-factoring of init_guest_space as
it's going to take more thought and is best left to 5.1. I've left in
the earlier clean-ups which fix the spacing and of the /proc/self/maps
but I can drop them if they seem too radical for rc2.

The elf-ops fix is a little cleaner, dropping the return ignored
value and using autoptr to avoid the goto magic.

I've includes the .hex and ARM gdbstub fixes which were posted
separately because I didn't have another series to put them in.
Richard's configure fix is there just so I can run my CI runs but may
well get picked up via another tree?

Anyway I intend to cut the PR on Monday with whatever hasn't been
already pulled in by other trees.

The only un-reviewed patch is:

 - linux-user: factor out reading of /proc/self/maps

Alex Bennée (9):
  elf-ops: bail out if we have no function symbols
  linux-user: protect fcntl64 with an #ifdef
  tests/tcg: remove extraneous pasting macros
  linux-user: more debug for init_guest_space
  target/xtensa: add FIXME for translation memory leak
  linux-user: factor out reading of /proc/self/maps
  linux-user: clean-up padding on /proc/self/maps
  target/arm: don't expose "ieee_half" via gdbstub
  hw/core: properly terminate loading .hex on EOF record

Denis Plotnikov (1):
  gdbstub: fix compiler complaining

Richard Henderson (2):
  softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal
  configure: Add -Werror to PIE probe

 configure  |  4 +-
 include/hw/elf_ops.h   | 48 ++--
 include/qemu/selfmap.h | 44 +++
 fpu/softfloat.c|  3 ++
 gdbstub.c  |  4 +-
 hw/core/loader.c   |  5 ++-
 linux-user/elfload.c   |  8 +++-
 linux-user/syscall.c   | 80 ++
 target/arm/gdbstub.c   |  7 ++-
 target/xtensa/translate.c  |  5 +++
 util/selfmap.c | 77 
 tests/tcg/x86_64/system/boot.S |  5 +--
 util/Makefile.objs |  1 +
 13 files changed, 219 insertions(+), 72 deletions(-)
 create mode 100644 include/qemu/selfmap.h
 create mode 100644 util/selfmap.c

-- 
2.20.1

Re: qemu plugin exposure of register addresses

[Alex Bennée]

Robert Henry  writes:

> There is now a qemu plugin interface function
> qemu_plugin_register_vcpu_mem_cb which registers a plugin-side
> callback. This callback is later invoked at the start of each emulated
> instruction, and it receives information about memory addresses and
> read/write indicators.
>
> I'm wondering how hard it is to add a similar callback to expose
> register addresses and read/write indicators.  For example, executing
> `add r3, r1, $1` would generate two callbacks, one {write r3} and the
> other {read r1}. I'd like this for all kinds of registers such as simd
> regs, and, gulp, flags registers.

The problem with tracking registers directly from the plugin is the
internal variability of how QEMU does things. The core TCG isn't really
aware of registers as such and the individual translators can take all
sorts of approaches to loading and storing the results. Then you have
the issue of helper functions which TCG is only vaguely aware of if it
needs to save "globals" or not.

> With this information ISA simulators could examine the data flow graph
> and register dependencies.

The plugin itself can still do this by disassembling the instruction at
translation time and decoding which registers are going to be accessed.
It can pass this data to the callback via userdata mechanism.

> I'm not asking for register contents; we don't get memory contents
> either!

There is no conceptual problem with adding access to register contents
even if it's only read-only. The main issue is coming up with a clean
API for it.

See thread @

  Subject: Re: Qemu TCG Plugins - how to access guest registers?
  In-reply-to: <20200329111311.272958fe@luklap>
  Date: Mon, 30 Mar 2020 16:15:47 +0100
  Message-ID: <878sjhho0s@linaro.org>


> I gather there is some concern about exposing too much functionality
> to the plugin API, as a plugin might then be used to subvert some
> aspects of the GPL.  I don't understand the details of this concern,
> nor know where the "line in the sand" is.

We need to better document this - but basically we don't want 3rd
parties implementing what would be core functionality (e.g. device
models) but using plugins as some sort of end run around the GPL. While
I would love there to be more analysis plugins contributed upstream it's
less of a concern.

>
> Robert Henry


-- 
Alex Bennée

Re: plugin interface function qemu_plugin_mem_size_shift

[Alex Bennée]

Robert Henry  writes:

> I don't understand what
>   unsigned int qemu_plugin_mem_size_shift(qemu_plugin_meminfo_t info);
> does.   The documentation in qemu-plugin.h is silent on this matter.
>   It appears to expose more of the guts of qemu that I don't yet know.

It's the size as a power of 2, so:

0 = byte
1 = 16 bit
2 = 32 bit
3 = 64 bit
4 = 128 bit

and so on. Currently I think our largest memory accesses are around 256
bytes but we can go larger is required.

How about:

--8<---cut here---start->8---
plugins: document the meaning of size_shift

We describe memory accesses in powers of 2.

Signed-off-by: Alex Bennée 

1 file changed, 3 insertions(+)
include/qemu/qemu-plugin.h | 3 +++

modified   include/qemu/qemu-plugin.h
@@ -301,6 +301,9 @@ void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn 
*insn);
  * The anonymous qemu_plugin_meminfo_t and qemu_plugin_hwaddr types
  * can be used in queries to QEMU to get more information about a
  * given memory access.
+ *
+ * The size_shift is the scale of access, e.g. << 3 is a 64 bit wide
+ * access.
  */
 typedef uint32_t qemu_plugin_meminfo_t;
 struct qemu_plugin_hwaddr;
--8<---cut here---end--->8---


-- 
Alex Bennée

Re: [PATCH v1 0/5] dma/xlnx-zdma: Bug fixes

[Peter Maydell]

On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias  wrote:
>
> From: "Edgar E. Iglesias" 
>
> Hi,
>
> This series fixes a couple of bugs we've ran into with some
> proprietary test code and drivers using the Xilinx zDMA.

Thanks; as these are bugfixes I've applied them to target-arm.next
for 5.0. I'd appreciate it if you could take a look at the
endianness bugs I mention in a reply to patch 5 (probably
that is 5.1 work).

-- PMM

[PATCH-for-5.0 v2] target/rx/translate: Add missing fall through comment

[Philippe Mathieu-Daudé]

Coverity reported a missing fall through comment, add it.

Fixes: e5918d7d7f0 ("target/rx: TCG translation")
Reported-by: Coverity (CID 142 MISSING_BREAK)
Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Fixed typo 'comments -> comment'
---
 target/rx/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index b3d7305f23..61e86653a4 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -2362,6 +2362,7 @@ static void rx_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cs)
 break;
 case DISAS_UPDATE:
 tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next);
+/* fall through */
 case DISAS_EXIT:
 tcg_gen_exit_tb(NULL, 0);
 break;
-- 
2.21.1

[PATCH-for-5.0] target/rx/translate: Add missing fall through comment

[Philippe Mathieu-Daudé]

Coverity reported a missing fall through comments, add it.

Fixes: e5918d7d7f0 ("target/rx: TCG translation")
Reported-by: Coverity (CID 142 MISSING_BREAK)
Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/rx/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index b3d7305f23..61e86653a4 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -2362,6 +2362,7 @@ static void rx_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cs)
 break;
 case DISAS_UPDATE:
 tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next);
+/* fall through */
 case DISAS_EXIT:
 tcg_gen_exit_tb(NULL, 0);
 break;
-- 
2.21.1

Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

[Peter Maydell]

On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias  wrote:
>
> From: "Edgar E. Iglesias" 
>
> Reorganize the descriptor handling so that CUR_DSCR always
> points to the next descriptor to be processed.
>
> Signed-off-by: Edgar E. Iglesias 
> ---

This is just moved-code in this patch so I think it's
a separate issue, but it looks like you have an endianness
bug here:

> +static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
> +   unsigned int basereg)
> +{
> +uint64_t addr, next;
> +
> +if (type == DTYPE_LINEAR) {
> +addr = zdma_get_regaddr64(s, basereg);
> +next = addr + sizeof(s->dsc_dst);
> +} else {
> +addr = zdma_get_regaddr64(s, basereg);
> +addr += sizeof(s->dsc_dst);
> +address_space_read(s->dma_as, addr, s->attr, (void *) , 8);

This reads 8 bytes into the uint64_t 'next', which means
that the value from C's point of view will be different
for big-endian and little-endian hosts. You probably wanted
address_space_ldq_le(), assuming the h/w always does
little-endian reads and that these get_regaddr64 and
put_regaddr64 functions work with host-endian integers.

There's a similar problem elsewhere in the device where
it does this:
address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
and assumes the guest structure is the same layout and
endianness as the host struct XlnxDMADescr.

I'm not a huge fan of defining host C structs to match
guest data structures, but if you want to go that way
you ought to (a) be byteswapping the contents appropriately
and (b) have a compile-time assert that the size of the
struct is what you think it is and the compiler hasn't
inserted any helpful extra padding.

thanks
-- PMM

Re: [PATCH for-5.0] dump: Fix writing of ELF section

[Peter Maydell]

On Tue, 24 Mar 2020 at 17:49, Marc-André Lureau
 wrote:
>
> Hi
>
> On Tue, Mar 24, 2020 at 6:36 PM Peter Maydell  
> wrote:
> >
> > In write_elf_section() we set the 'shdr' pointer to point to local
> > structures shdr32 or shdr64, which we fill in to be written out to
> > the ELF dump.  Unfortunately the address we pass to fd_write_vmcore()
> > has a spurious '&' operator, so instead of writing out the section
> > header we write out the literal pointer value followed by whatever is
> > on the stack after the 'shdr' local variable.
> >
> > Pass the correct address into fd_write_vmcore().
> >
> > Signed-off-by: Peter Maydell 
>
> Reviewed-by: Marc-André Lureau 

Thanks for the review; since nobody else has picked the patch
up I'll put it in via target-arm.next just for convenience.

-- PMM

Re: [PATCH] target/arm: don't expose "ieee_half" via gdbstub

[Peter Maydell]

On Thu, 2 Apr 2020 at 15:39, Alex Bennée  wrote:
>
> While support for parsing ieee_half in the XML description was added
> to gdb in 2019 (a6d0f249) there is no easy way for the gdbstub to know
> if the gdb end will understand it. Disable it for now and allow older
> gdbs to successfully connect to the default -cpu max SVE enabled
> QEMUs.
>
> Signed-off-by: Alex Bennée 
> ---



Applied to target-arm.next for 5.0, thanks.

-- PMM

[PATCH for-5.0? v2] qcow2: Explicit mention of padding bytes

[Eric Blake]

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
relied on the rest of the text for implicitly covering 7 padding
bytes.  For consistency with other parts of the header (such as the
header extension format listing padding from n - m, or the snapshot
table entry mentioning variable padding), we might as well call out
the remaining 7 bytes as padding until such time (as any) as they gain
another meaning.

Signed-off-by: Eric Blake 
---

v2: Call out explicit byte range rather than '105 - m' [Max]

Safe for 5.0 as it is just a doc fix, but only if we actually want it.

 docs/interop/qcow2.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 640e0eca4000..80728bc2008d 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -210,3 +210,4 @@ version 2.
 Available compression type values:
 0: zlib 

+105 - 111:  Padding, leave as zero.

 === Header padding ===

-- 
2.26.0.rc2

[PATCH v5 3/7] qcow: Tolerate backing_fmt=, but warn on backing_fmt=raw

[Eric Blake]

qcow has no space in the metadata to store a backing format, and there
are existing qcow images backed both by raw or by other formats
(usually qcow) images, reliant on probing to tell the difference.
While we don't recommend the creation of new qcow images (as qcow2 is
hands-down better), we can at least insist that if the user does
request a specific format without using -u, then it must be non-raw
(as a raw backing file that gets inadvertently edited into some other
format can form a security hole); if the user does not request a
specific format or lies when using -u, then the status quo of probing
for the backing format remains intact (although an upcoming patch will
warn when omitting a format request).  Thus, when this series is
complete, the only way to use a backing file for qcow without
triggering a warning is when using -F if the backing file is non-raw
to begin with.  Note that this is only for QemuOpts usage; there is no
change to the QAPI to allow a format through -blockdev.

Add a new iotest 290 just for qcow, to demonstrate the new warning.

Signed-off-by: Eric Blake 
---
 block/qcow.c   | 16 -
 tests/qemu-iotests/290 | 72 ++
 tests/qemu-iotests/290.out | 42 ++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/290
 create mode 100644 tests/qemu-iotests/290.out

diff --git a/block/qcow.c b/block/qcow.c
index 8973e4e565a1..bbe48b7db4d7 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -940,11 +940,12 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
*drv,
 {
 BlockdevCreateOptions *create_options = NULL;
 BlockDriverState *bs = NULL;
-QDict *qdict;
+QDict *qdict = NULL;
 Visitor *v;
 const char *val;
 Error *local_err = NULL;
 int ret;
+char *backing_fmt;

 static const QDictRenames opt_renames[] = {
 { BLOCK_OPT_BACKING_FILE,   "backing-file" },
@@ -953,6 +954,13 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
*drv,
 };

 /* Parse options and convert legacy syntax */
+backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
+if (backing_fmt && !strcmp(backing_fmt, "raw")) {
+error_setg(errp, "qcow cannot store backing format; an explicit "
+   "backing format of raw is unsafe");
+ret = -EINVAL;
+goto fail;
+}
 qdict = qemu_opts_to_qdict_filtered(opts, NULL, _create_opts, true);

 val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
@@ -1018,6 +1026,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
*drv,

 ret = 0;
 fail:
+g_free(backing_fmt);
 qobject_unref(qdict);
 bdrv_unref(bs);
 qapi_free_BlockdevCreateOptions(create_options);
@@ -1152,6 +1161,11 @@ static QemuOptsList qcow_create_opts = {
 .type = QEMU_OPT_STRING,
 .help = "File name of a base image"
 },
+{
+.name = BLOCK_OPT_BACKING_FMT,
+.type = QEMU_OPT_STRING,
+.help = "Format of the backing image (caution: raw backing is 
unsafe)",
+},
 {
 .name = BLOCK_OPT_ENCRYPT,
 .type = QEMU_OPT_BOOL,
diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
new file mode 100755
index ..8482de58cb4b
--- /dev/null
+++ b/tests/qemu-iotests/290
@@ -0,0 +1,72 @@
+#!/usr/bin/env bash
+#
+# Test qcow backing file warnings
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow
+_supported_proto file
+_supported_os Linux
+
+size=128M
+
+echo
+echo "== qcow backed by qcow =="
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+_make_test_img -b "$TEST_IMG.base"
+_img_info
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT
+_img_info
+
+echo
+echo "== mismatched command line detection =="
+
+_make_test_img -b "$TEST_IMG.base" -F vmdk
+# Use of -u bypasses the backing format sanity check
+_make_test_img -u -b "$TEST_IMG.base" -F vmdk $size
+_img_info
+
+echo
+echo "== qcow backed by raw =="
+

[PATCH v5 6/7] block: Add support to warn on backing file change without format

[Eric Blake]

For now, this is a mechanical addition; all callers pass false. But
the next patch will use it to improve 'qemu-img rebase -u' when
selecting a backing file with no format.

Signed-off-by: Eric Blake 
Reviewed-by: Peter Krempa 
Reviewed-by: Ján Tomko 
---
 include/block/block.h |  4 ++--
 block.c   | 13 ++---
 block/qcow2.c |  2 +-
 block/stream.c|  2 +-
 blockdev.c|  3 ++-
 qemu-img.c|  4 ++--
 6 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c5b..098fc635e865 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -351,8 +351,8 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts 
*opts,
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
-int bdrv_change_backing_file(BlockDriverState *bs,
-const char *backing_file, const char *backing_fmt);
+int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
+ const char *backing_fmt, bool warn);
 void bdrv_register(BlockDriver *bdrv);
 int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
const char *backing_file_str);
diff --git a/block.c b/block.c
index 2e3905c99e8c..70cc6123dd91 100644
--- a/block.c
+++ b/block.c
@@ -1322,7 +1322,8 @@ static int bdrv_backing_update_filename(BdrvChild *c, 
BlockDriverState *base,
 }

 ret = bdrv_change_backing_file(parent, filename,
-   base->drv ? base->drv->format_name : "");
+   base->drv ? base->drv->format_name : "",
+   false);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Could not update backing file link");
 }
@@ -4566,8 +4567,8 @@ int bdrv_check(BlockDriverState *bs,
  *image file header
  * -ENOTSUP - format driver doesn't support changing the backing file
  */
-int bdrv_change_backing_file(BlockDriverState *bs,
-const char *backing_file, const char *backing_fmt)
+int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
+ const char *backing_fmt, bool warn)
 {
 BlockDriver *drv = bs->drv;
 int ret;
@@ -4581,6 +4582,12 @@ int bdrv_change_backing_file(BlockDriverState *bs,
 return -EINVAL;
 }

+if (warn && backing_file && !backing_fmt) {
+warn_report("Deprecated use of backing file without explicit "
+"backing format, use of this image requires "
+"potentially unsafe format probing");
+}
+
 if (drv->bdrv_change_backing_file != NULL) {
 ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
 } else {
diff --git a/block/qcow2.c b/block/qcow2.c
index e4b772726010..0a7e70bdf879 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3526,7 +3526,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
 }

 ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
-   backing_format);
+   backing_format, false);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
  "with format '%s'", qcow2_opts->backing_file,
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e37..310ccbaa4cfd 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -78,7 +78,7 @@ static int stream_prepare(Job *job)
 }
 }
 bdrv_set_backing_hd(bs, base, _err);
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+ret = bdrv_change_backing_file(bs, base_id, base_fmt, false);
 if (local_err) {
 error_report_err(local_err);
 return -EPERM;
diff --git a/blockdev.c b/blockdev.c
index fa8630cb412d..5bc9d78563ab 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3692,7 +3692,8 @@ void qmp_change_backing_file(const char *device,
 }

 ret = bdrv_change_backing_file(image_bs, backing_file,
-   image_bs->drv ? image_bs->drv->format_name : 
"");
+   image_bs->drv ? image_bs->drv->format_name 
: "",
+   false);

 if (ret < 0) {
 error_setg_errno(errp, -ret, "Could not change backing file to '%s'",
diff --git a/qemu-img.c b/qemu-img.c
index b167376bd72e..cb8fa5a0ee8b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3644,9 +3644,9 @@ static int img_rebase(int argc, char **argv)
  * doesn't change when we switch the backing file.
  */
 if (out_baseimg && *out_baseimg) {
-ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
+ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false);
 } else {
-   

[PATCH v5 7/7] qemu-img: Deprecate use of -b without -F

[Eric Blake]

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects).  However, if our probing algorithm ever changes,
or if other tools like libvirt determine a different probe result than
we do, then subsequent use of that backing file under a different
format will present corrupted data to the guest.  Start a deprecation
clock so that future qemu-img can refuse to create unsafe backing
chains that would rely on probing.  Most warnings are intentionally
emitted from bdrv_img_create() in the block layer, but qemu-img
convert uses bdrv_create() which cannot emit its own warning without
causing spurious warnings on other code paths.  In the end, all
command-line image creation or backing file rewriting now performs a
check.

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.  While touching it, expand it to
cover all of the various warnings enabled by this patch.  iotest 290
also shows a change to qcow messages; note that the fact that we now
make a probed format of 'raw' explicit now results in a double
warning, but no one should be creating new qcow images so it is not
worth cleaning up.

Signed-off-by: Eric Blake 
---
 docs/system/deprecated.rst | 20 
 block.c| 21 -
 qemu-img.c |  9 -
 tests/qemu-iotests/114 | 12 
 tests/qemu-iotests/114.out |  9 +
 tests/qemu-iotests/290.out |  5 -
 6 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index aac6be58917a..8abfd436820a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -429,6 +429,26 @@ image).  Rather, any changes to the backing chain should 
be performed
 with ``qemu-img rebase -u`` either before or after the remaining
 changes being performed by amend, as appropriate.

+qemu-img backing file without format (since 5.0.0)
+''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
+convert`` to create or modify an image that depends on a backing file
+now recommends that an explicit backing format be provided.  This is
+for safety: if QEMU probes a different format than what you thought,
+the data presented to the guest will be corrupt; similarly, presenting
+a raw image to a guest allows a potential security exploit if a future
+probe sees a non-raw image based on guest writes.
+
+To avoid the warning message, or even future refusal to create an
+unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand
+``-F`` during create) to specify the intended backing format.  You may
+use ``qemu-img rebase -u`` to retroactively add a backing format to an
+existing image.  However, be aware that there are already potential
+security risks to blindly using ``qemu-img info`` to probe the format
+of an untrusted backing image, when deciding what format to add into
+an existing image.
+
 ``qemu-img convert -n -o`` (since 4.2.0)
 

diff --git a/block.c b/block.c
index 70cc6123dd91..61871965eee4 100644
--- a/block.c
+++ b/block.c
@@ -6072,6 +6072,20 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
   "Could not open backing image to determine 
size.\n");
 goto out;
 } else {
+if (!backing_fmt) {
+warn_report("Deprecated use of backing file without explicit "
+"backing format (detected format of %s)",
+bs->drv->format_name);
+if (bs->drv == _raw) {
+/*
+ * A probe of raw is always correct, so in this one
+ * case, we can write that into the image.
+ */
+backing_fmt = bs->drv->format_name;
+qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt,
+ NULL);
+}
+}
 if (size == -1) {
 /* Opened BS, have no size */
 size = bdrv_getlength(bs);
@@ -6085,7 +6099,12 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 }
 bdrv_unref(bs);
 }
-} /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+/* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+} else if 

[PATCH v5 4/7] qcow2: Deprecate use of qemu-img amend to change backing file

[Eric Blake]

The use of 'qemu-img amend' to change qcow2 backing files is not
tested very well.  In particular, our implementation has a bug where
if a new backing file is provided without a format, then the prior
format is blindly reused, even if this results in data corruption, but
this is not caught by iotests.

There are also situations where amending other options needs access to
the original backing file (for example, on a downgrade to a v2 image,
knowing whether a v3 zero cluster must be allocated or may be left
unallocated depends on knowing whether the backing file already reads
as zero), but the command line does not have a nice way to tell us
both the backing file to use for opening the image as well as the
backing file to install after the operation is complete.

Even if we do allow changing the backing file, it is redundant with
the existing ability to change backing files via 'qemu-img rebase -u'.
It is time to deprecate this support (leaving the existing behavior
intact, even if it is buggy), and at a point in the future, require
the use of only 'qemu-img rebase' for adjusting backing chain
relations, saving 'qemu-img amend' for changes unrelated to the
backing chain.

Signed-off-by: Eric Blake 
---
 docs/system/deprecated.rst | 12 
 docs/tools/qemu-img.rst|  4 
 block/qcow2.c  |  5 +
 tests/qemu-iotests/061.out |  1 +
 tests/qemu-iotests/082.out |  2 ++
 5 files changed, 24 insertions(+)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index c633fe2beffd..aac6be58917a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -417,6 +417,18 @@ The above, converted to the current supported format::
 Related binaries
 

+qemu-img amend to adjust backing file (since 5.0.0)
+'''
+
+The use of ``qemu-img amend`` to modify the name or format of a qcow2
+backing image is deprecated; this functionality was never fully
+documented or tested, and interferes with other amend operations that
+need access to the original backing image (such as deciding whether a
+v3 zero cluster may be left unallocated when converting to a v2
+image).  Rather, any changes to the backing chain should be performed
+with ``qemu-img rebase -u`` either before or after the remaining
+changes being performed by amend, as appropriate.
+
 ``qemu-img convert -n -o`` (since 4.2.0)
 

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76c9..83d57586be96 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -254,6 +254,10 @@ Command description:
   Amends the image format specific *OPTIONS* for the image file
   *FILENAME*. Not all file formats support this operation.

+  The set of options that can be amended are dependent on the image
+  format, but note that amending the backing chain relationship should
+  instead be performed with ``qemu-img rebase``.
+
 .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] 
[--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] 
[--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] 
FILENAME

   Run a simple sequential I/O benchmark on the specified image. If ``-w`` is
diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b01494..e4b772726010 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5303,6 +5303,11 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 }

 if (backing_file || backing_format) {
+if (g_strcmp0(backing_file, s->image_backing_file) ||
+g_strcmp0(backing_format, s->image_backing_format)) {
+warn_report("Deprecated use of amend to alter the backing file; "
+"use qemu-img rebase instead");
+}
 ret = qcow2_change_backing_file(bs,
 backing_file ?: s->image_backing_file,
 backing_format ?: s->image_backing_format);
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 413cc4e0f4ab..048248c121cb 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -342,6 +342,7 @@ wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: warning: Deprecated use of amend to alter the backing file; use 
qemu-img rebase instead
 read 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 9d4ed4dc9d61..94ea990a2754 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -806,10 +806,12 @@ Creation options for 'qcow2':
 Note that not all of these options may be amendable.

 Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
+qemu-img: warning: 

[PATCH v5 2/7] vmdk: Add trivial backing_fmt support

[Eric Blake]

vmdk already requires that if backing_file is present, that it be
another vmdk image (see vmdk_co_do_create).  Meanwhile, we want to
move towards always being explicit about the backing format for other
drivers where it matters.  So for convenience, make qemu-img create -F
vmdk work, while rejecting all other explicit formats (note that this
is only for QemuOpts usage; there is no change to the QAPI to allow a
format through -blockdev).

Signed-off-by: Eric Blake 
---
 block/vmdk.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index 218d9c980059..7ecaf0ee9be3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2613,6 +2613,14 @@ static int coroutine_fn vmdk_co_create_opts(BlockDriver 
*drv,
 bool zeroed_grain;
 bool compat6;
 VMDKCreateOptsData data;
+char *backing_fmt = NULL;
+
+backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
+if (backing_fmt && strcmp(backing_fmt, "vmdk") != 0) {
+error_setg(errp, "backing_file must be a vmdk image");
+ret = -EINVAL;
+goto exit;
+}

 if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
 ret = -EINVAL;
@@ -2671,6 +2679,7 @@ static int coroutine_fn vmdk_co_create_opts(BlockDriver 
*drv,
 vmdk_co_create_opts_cb, , errp);

 exit:
+g_free(backing_fmt);
 g_free(adapter_type);
 g_free(backing_file);
 g_free(hw_version);
@@ -3007,6 +3016,11 @@ static QemuOptsList vmdk_create_opts = {
 .type = QEMU_OPT_STRING,
 .help = "File name of a base image"
 },
+{
+.name = BLOCK_OPT_BACKING_FMT,
+.type = QEMU_OPT_STRING,
+.help = "Must be 'vmdk' if present",
+},
 {
 .name = BLOCK_OPT_COMPAT6,
 .type = QEMU_OPT_BOOL,
-- 
2.26.0.rc2

[PATCH v5 1/7] sheepdog: Add trivial backing_fmt support

[Eric Blake]

Sheepdog already requires that if backing_file is present, that it be
another sheepdog image (see sd_co_create).  Meanwhile, we want to move
towards always being explicit about the backing format for other
drivers where it matters.  So for convenience, make qemu-img create -F
sheepdog work, while rejecting all other explicit formats (note that
this is only for QemuOpts usage; there is no change to the QAPI to
allow a format through -blockdev).

Signed-off-by: Eric Blake 
---
 block/sheepdog.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 59f7ebb1710f..8f558367d2d5 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2163,13 +2163,21 @@ static int coroutine_fn sd_co_create_opts(BlockDriver 
*drv,
   Error **errp)
 {
 BlockdevCreateOptions *create_options = NULL;
-QDict *qdict, *location_qdict;
+QDict *qdict = NULL, *location_qdict;
 Visitor *v;
-char *redundancy;
+char *redundancy = NULL;
 Error *local_err = NULL;
 int ret;
+char *backing_fmt = NULL;

 redundancy = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY);
+backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
+
+if (backing_fmt && strcmp(backing_fmt, "sheepdog") != 0) {
+error_setg(errp, "backing_file must be a sheepdog image");
+ret = -EINVAL;
+goto fail;
+}

 qdict = qemu_opts_to_qdict(opts, NULL);
 qdict_put_str(qdict, "driver", "sheepdog");
@@ -2234,6 +2242,7 @@ fail:
 qapi_free_BlockdevCreateOptions(create_options);
 qobject_unref(qdict);
 g_free(redundancy);
+g_free(backing_fmt);
 return ret;
 }

@@ -3191,6 +3200,11 @@ static QemuOptsList sd_create_opts = {
 .type = QEMU_OPT_STRING,
 .help = "File name of a base image"
 },
+{
+.name = BLOCK_OPT_BACKING_FMT,
+.type = QEMU_OPT_STRING,
+.help = "Must be 'sheepdog' if present",
+},
 {
 .name = BLOCK_OPT_PREALLOC,
 .type = QEMU_OPT_STRING,
-- 
2.26.0.rc2

[PATCH v5 for-5.0? 0/7] Tighten qemu-img rules on missing backing format

[Eric Blake]

v4 was here:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03775.html
In v5:
- fix 'qemu-img convert -B' to actually warn [Kashyap]
- squash in followups
- a couple more iotest improvements

If we decide this is not 5.0 material, then patches 4 and 7 need a
tweak to s/5.0/5.1/ as the start of the deprecation clock.

Eric Blake (7):
  sheepdog: Add trivial backing_fmt support
  vmdk: Add trivial backing_fmt support
  qcow: Tolerate backing_fmt=, but warn on backing_fmt=raw
  qcow2: Deprecate use of qemu-img amend to change backing file
  iotests: Specify explicit backing format where sensible
  block: Add support to warn on backing file change without format
  qemu-img: Deprecate use of -b without -F

 docs/system/deprecated.rst| 32 
 docs/tools/qemu-img.rst   |  4 ++
 include/block/block.h |  4 +-
 block.c   | 34 +++--
 block/qcow.c  | 16 +++-
 block/qcow2.c |  7 +++-
 block/sheepdog.c  | 18 -
 block/stream.c|  2 +-
 block/vmdk.c  | 14 +++
 blockdev.c|  3 +-
 qemu-img.c| 11 +-
 tests/qemu-iotests/017|  2 +-
 tests/qemu-iotests/017.out|  2 +-
 tests/qemu-iotests/018|  2 +-
 tests/qemu-iotests/018.out|  2 +-
 tests/qemu-iotests/019|  5 ++-
 tests/qemu-iotests/019.out|  2 +-
 tests/qemu-iotests/020|  4 +-
 tests/qemu-iotests/020.out|  4 +-
 tests/qemu-iotests/024|  8 ++--
 tests/qemu-iotests/024.out|  5 ++-
 tests/qemu-iotests/028|  4 +-
 tests/qemu-iotests/028.out|  2 +-
 tests/qemu-iotests/030| 26 +
 tests/qemu-iotests/034|  2 +-
 tests/qemu-iotests/034.out|  2 +-
 tests/qemu-iotests/037|  2 +-
 tests/qemu-iotests/037.out|  2 +-
 tests/qemu-iotests/038|  2 +-
 tests/qemu-iotests/038.out|  2 +-
 tests/qemu-iotests/039|  3 +-
 tests/qemu-iotests/039.out|  2 +-
 tests/qemu-iotests/040| 47 ---
 tests/qemu-iotests/041| 37 --
 tests/qemu-iotests/042|  4 +-
 tests/qemu-iotests/043| 18 -
 tests/qemu-iotests/043.out| 16 +---
 tests/qemu-iotests/046|  2 +-
 tests/qemu-iotests/046.out|  2 +-
 tests/qemu-iotests/050|  4 +-
 tests/qemu-iotests/050.out|  2 +-
 tests/qemu-iotests/051|  2 +-
 tests/qemu-iotests/051.out|  2 +-
 tests/qemu-iotests/051.pc.out |  2 +-
 tests/qemu-iotests/056|  3 +-
 tests/qemu-iotests/060|  2 +-
 tests/qemu-iotests/060.out|  2 +-
 tests/qemu-iotests/061| 10 ++---
 tests/qemu-iotests/061.out| 11 +++---
 tests/qemu-iotests/069|  2 +-
 tests/qemu-iotests/069.out|  2 +-
 tests/qemu-iotests/073|  2 +-
 tests/qemu-iotests/073.out|  2 +-
 tests/qemu-iotests/082| 10 +++--
 tests/qemu-iotests/082.out| 14 ---
 tests/qemu-iotests/085|  4 +-
 tests/qemu-iotests/085.out|  6 +--
 tests/qemu-iotests/089|  2 +-
 tests/qemu-iotests/089.out|  2 +-
 tests/qemu-iotests/095|  4 +-
 tests/qemu-iotests/095.out|  4 +-
 tests/qemu-iotests/097|  4 +-
 tests/qemu-iotests/097.out| 16 
 tests/qemu-iotests/098|  2 +-
 tests/qemu-iotests/098.out|  8 ++--
 tests/qemu-iotests/110|  4 +-
 tests/qemu-iotests/110.out|  4 +-
 tests/qemu-iotests/114| 12 ++
 tests/qemu-iotests/114.out|  9 +
 tests/qemu-iotests/122| 27 +++--
 tests/qemu-iotests/122.out|  8 ++--
 tests/qemu-iotests/126|  4 +-
 tests/qemu-iotests/126.out|  4 +-
 tests/qemu-iotests/127|  4 +-
 tests/qemu-iotests/127.out|  4 +-
 tests/qemu-iotests/129|  3 +-
 tests/qemu-iotests/133|  2 +-
 tests/qemu-iotests/133.out|  2 +-
 tests/qemu-iotests/139|  2 +-
 tests/qemu-iotests/141|  4 +-
 tests/qemu-iotests/141.out|  4 +-
 tests/qemu-iotests/142|  2 +-
 tests/qemu-iotests/142.out|  2 +-
 tests/qemu-iotests/153| 14 +++
 tests/qemu-iotests/153.out| 35 +
 tests/qemu-iotests/154| 42 ++--
 tests/qemu-iotests/154.out| 42 ++--
 tests/qemu-iotests/155| 12 --
 tests/qemu-iotests/156|  9 +++--
 tests/qemu-iotests/156.out|  6 +--
 tests/qemu-iotests/158|  2 +-
 tests/qemu-iotests/158.out|  2 +-
 tests/qemu-iotests/161|  8 ++--
 tests/qemu-iotests/161.out|  8 ++--
 tests/qemu-iotests/176|  4 +-
 tests/qemu-iotests/176.out| 32 
 tests/qemu-iotests/177|  2 +-
 tests/qemu-iotests/177.out|  2 +-
 tests/qemu-iotests/179|  2 +-
 tests/qemu-iotests/179.out|  2 +-
 tests/qemu-iotests/189|  2 +-
 tests/qemu-iotests/189.out|  2 +-
 tests/qemu-iotests/191| 12 +++---
 

Re: [PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Philippe Mathieu-Daudé]

On 4/3/20 7:49 PM, Eric Blake wrote:

On 4/3/20 12:37 PM, Eric Blake wrote:

On 4/3/20 11:57 AM, Peter Maydell wrote:
On Fri, 3 Apr 2020 at 17:54, Philippe Mathieu-Daudé 
 wrote:


When ./configure checks the sphinx version is new enough, it leaves
the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
option (don't write .py[co] files on import) via the
PYTHONDONTWRITEBYTECODE environment variable.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 


This only happens for an in-tree build, right? I think in
that case you're kind of OK with having random stuff
left in the source tree... It seems easy enough to suppress
them though, so I guess we might as well.


It happens in VPATH too - and what's more, in VPATH, it is still 
creating it under srcdir rather than builddir, which feels like 
unnecessary pollution.  I was trying to prove whether 'make distclean' 
got us back to a pristine state; this was one of the files that 
escaped 'make distclean', so our choice is to either add it to the 
clean rules, or to avoid creating it in the first place.  I like the 
approach of not creating it in the first place :)


Reviewed-by: Eric Blake 


Hmm, I spoke early.  Your patch only addresses the pollution during 
'./configure'.  But running 'make' (even in a VPATH build) equally 
creates the same pollution.  Which means we really ought to be cleaning 
it up during 'make distclean' rather than just trying to make 
'./configure' clever.


Oh I only checked ./configure indeed, sorry. Thanks for testing!

Re: [PATCH-for-5.0 00/12] hw: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Hi Markus, Peter.

On 3/31/20 3:23 PM, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


This series is inspired of Peter fix:
"hw/arm/xlnx-zynqmp.c: fix some error-handling code"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691636.html

Add a cocci script to fix the other places.

Based-on: <20200324134947.15384-1-peter.mayd...@linaro.org>


I skimmed the code patches [PATCH 02-12/12], and they look like bug
fixes.  Other reviewers raised a few issues.

I also skimmed the Coccinelle script [PATCH 01].  Peter pointed out a
few things it apparently missed (e.g. in review of PATCH 06+11).
Moreover, the bug pattern applies beyond object_property_set() &
friends.  Perhaps the script can be generalized.  No reason to hold
fixes.  We may want to add suitable notes to the scipt, though.

Can you address the reviews in a v2, so we can get the fixes into -rc1,
due today?


Status on this series (sorry I didn't update earlier).

I addressed Peter's comments, improved/simplified/documented the cocci 
script (which I split in smaller ones).


Peter suggested other functions can be checked too, not only the 
"^object_property_set_.*" matches. Indeed, more patches added. Some are big.


Another suggestion is replace in init() 'NULL' Error* final argument by 
_abort. This can be another series on top.
However I noticed we can reduce the error_propagate() generated calls in 
many places, when both init()/realize() exist and the property set is 
not dependent of parent operation, by moving these calls from realize() 
to init(). Another cocci script. But to make sense it has to be run 
previous the "add missing error_propagate" one.


While writing the cocci patches, I had 3 different Coccinelle failures.
Failures not due to a spatch bug, but timeout because C source hard to 
process. Indeed the C source code was dubious, could get some 
simplification rewrite. Then spatch could transform them. More patches 
in the middle.


Now I'm at 47 patches, the reviewed patches at the end of the series.
Too much for RC2. Since I don't think these are critical bugs, but 
improvements, are you OK to postpone this series to 5.1?


If you think a patch deserves to be in 5.0, point me at it and I can 
send it ASAP with comments addressed. Else I'll post my series as 
-for-5.1 soon.


Regards,

Phil.

Re: [PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Eric Blake]

On 4/3/20 12:37 PM, Eric Blake wrote:

On 4/3/20 11:57 AM, Peter Maydell wrote:
On Fri, 3 Apr 2020 at 17:54, Philippe Mathieu-Daudé 
 wrote:


When ./configure checks the sphinx version is new enough, it leaves
the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
option (don't write .py[co] files on import) via the
PYTHONDONTWRITEBYTECODE environment variable.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 


This only happens for an in-tree build, right? I think in
that case you're kind of OK with having random stuff
left in the source tree... It seems easy enough to suppress
them though, so I guess we might as well.


It happens in VPATH too - and what's more, in VPATH, it is still 
creating it under srcdir rather than builddir, which feels like 
unnecessary pollution.  I was trying to prove whether 'make distclean' 
got us back to a pristine state; this was one of the files that escaped 
'make distclean', so our choice is to either add it to the clean rules, 
or to avoid creating it in the first place.  I like the approach of not 
creating it in the first place :)


Reviewed-by: Eric Blake 


Hmm, I spoke early.  Your patch only addresses the pollution during 
'./configure'.  But running 'make' (even in a VPATH build) equally 
creates the same pollution.  Which means we really ought to be cleaning 
it up during 'make distclean' rather than just trying to make 
'./configure' clever.


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

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Eric Blake]

On 4/3/20 11:57 AM, Alberto Garcia wrote:

When issuing a compressed write request the number of bytes must be a
multiple of the cluster size.

With the current code such requests are allowed and we hit an
assertion:

$ qemu-img create -f qcow2 img.qcow2 1M
$ qemu-io -c 'write -c 0 32k' img.qcow2

qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
   (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' 
failed.
Aborted

This patch fixes a regression introduced in 0d483dce38

Signed-off-by: Alberto Garcia 
---
  block/qcow2.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Eric Blake 



diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..923ad428f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
  return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, 
NULL);
  }
  
-if (offset_into_cluster(s, offset)) {

+if (offset_into_cluster(s, offset | bytes)) {
  return -EINVAL;
  }
  



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

Re: [PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Eric Blake]

On 4/3/20 11:57 AM, Peter Maydell wrote:

On Fri, 3 Apr 2020 at 17:54, Philippe Mathieu-Daudé  wrote:


When ./configure checks the sphinx version is new enough, it leaves
the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
option (don't write .py[co] files on import) via the
PYTHONDONTWRITEBYTECODE environment variable.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 


This only happens for an in-tree build, right? I think in
that case you're kind of OK with having random stuff
left in the source tree... It seems easy enough to suppress
them though, so I guess we might as well.


It happens in VPATH too - and what's more, in VPATH, it is still 
creating it under srcdir rather than builddir, which feels like 
unnecessary pollution.  I was trying to prove whether 'make distclean' 
got us back to a pristine state; this was one of the files that escaped 
'make distclean', so our choice is to either add it to the clean rules, 
or to avoid creating it in the first place.  I like the approach of not 
creating it in the first place :)


Reviewed-by: Eric Blake 

That said,


+PYTHONDONTWRITEBYTECODE=yes "$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" 
"$TMPDIR1/sphinx/out" >/dev/null 2>&1


is now even a longer line; is it worth adding \-newline to split it up?

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

Re: [PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Philippe Mathieu-Daudé]

On 4/3/20 6:57 PM, Peter Maydell wrote:

On Fri, 3 Apr 2020 at 17:54, Philippe Mathieu-Daudé  wrote:


When ./configure checks the sphinx version is new enough, it leaves
the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
option (don't write .py[co] files on import) via the
PYTHONDONTWRITEBYTECODE environment variable.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 


This only happens for an in-tree build, right?


Correct.


I think in
that case you're kind of OK with having random stuff
left in the source tree... It seems easy enough to suppress
them though, so I guess we might as well.


Here is the post where Eric commented it:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg00631.html



thanks
-- PMM

[NOTFORMERGE PATCH 8/8] Kludge for Avocado issue #3661

[Philippe Mathieu-Daudé]

Use a feature from unreleased Avocado v78.0.
See https://github.com/avocado-framework/avocado/issues/3661

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/requirements.txt b/tests/requirements.txt
index f9c84b4ba1..d625b32dbb 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,5 +1,5 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
-avocado-framework==76.0
+-e 
git+https://github.com/avocado-framework/avocado.git@f9b4dc7c58a6424eb8d0ed6781a1d76ae3a5ab06#egg=avocado-framework
 pycdlib==1.9.0
-- 
2.21.1

[PATCH-for-5.0 5/8] .travis.yml: Cache acceptance-test assets

[Philippe Mathieu-Daudé]

Keep all acceptance-test assets in the same cache bucket.

As of v5.0.0-rc1, the cache is 2610.11MB:
https://travis-ci.org/github/philmd/qemu/jobs/670558103

Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index e0c72210b7..2fd63eceaa 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -318,8 +318,10 @@ jobs:
   env:
 - CONFIG="--enable-tools 
--target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
 - TEST_CMD="make check-acceptance"
+- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
   after_script:
 - python3 -c 'import json; r = 
json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
+- du -chs $HOME/avocado/data/cache
   addons:
 apt:
   packages:
-- 
2.21.1

[PATCH-for-5.1 6/8] tests/Makefile: Add fetch-acceptance-assets rule

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/Makefile.include | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 51de676298..90f457593c 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -906,6 +906,13 @@ get-vm-image-fedora-31-%: check-venv
 # download all vm images, according to defined targets
 get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, 
$(FEDORA_31_DOWNLOAD))
 
+fetch-acceptance-assets: check-venv
+   $(call quiet-command, \
+$(TESTS_VENV_DIR)/bin/python -m avocado \
+--show=$(if $(DEBUG),avocado.test,$(AVOCADO_SHOW)) assets fetch \
+tests/acceptance/*py, \
+"AVOCADO", "tests/acceptance")
+
 check-acceptance: check-venv $(TESTS_RESULTS_DIR) get-vm-images
$(call quiet-command, \
 $(TESTS_VENV_DIR)/bin/python -m avocado \
-- 
2.21.1

[PATCH-for-5.1 7/8] .travis.yml: Run fetch-acceptance-assets before check-acceptance

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 2fd63eceaa..c6b32da447 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -317,7 +317,7 @@ jobs:
   dist: bionic
   env:
 - CONFIG="--enable-tools 
--target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
-- TEST_CMD="make check-acceptance"
+- TEST_CMD="travis_retry make -j1 fetch-acceptance-assets 
check-acceptance DEBUG=1"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
   after_script:
 - python3 -c 'import json; r = 
json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
-- 
2.21.1

[PATCH-for-5.0 3/8] tests/acceptance/ppc_prep_40p: Use mirror for ftp.software.ibm.com

[Philippe Mathieu-Daudé]

To avoid regular failures on Travis-CI with ftp.software.ibm.com,
use a mirror.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/ppc_prep_40p.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
index 138064285a..1515561249 100644
--- a/tests/acceptance/ppc_prep_40p.py
+++ b/tests/acceptance/ppc_prep_40p.py
@@ -30,7 +30,8 @@ def test_factory_firmware_and_netbsd(self):
 :avocado: tags=machine:40p
 :avocado: tags=slowness:high
 """
-bios_url = ('ftp://ftp.boulder.ibm.com/rs6000/firmware/'
+bios_url = ('http://ftpmirror.your.org/pub/misc/'
+'ftp.software.ibm.com/rs6000/firmware/'
 '7020-40p/P12H0456.IMG')
 bios_hash = '1775face4e6dc27f3a6ed955ef6eb331bf817f03'
 bios_path = self.fetch_asset(bios_url, asset_hash=bios_hash)
-- 
2.21.1

[PATCH-for-5.0 1/8] Acceptance test: Fix to EXEC migration

[Philippe Mathieu-Daudé]

From: Oksana Vohchana 

The exec migration test isn't run a whole test scenario.
This patch fixes it

Fixes: 2e768cb682bf
Signed-off-by: Oksana Vohchana 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Wainer dos Santos Moschetta 
Message-Id: <20200325113138.20337-1-ovosh...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/migration.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
index a8367ca023..0365289cda 100644
--- a/tests/acceptance/migration.py
+++ b/tests/acceptance/migration.py
@@ -70,8 +70,8 @@ def test_migration_with_unix(self):
 
 @skipUnless(find_command('nc', default=False), "'nc' command not found")
 def test_migration_with_exec(self):
-"""
-The test works for both netcat-traditional and netcat-openbsd packages
-"""
+"""The test works for both netcat-traditional and netcat-openbsd 
packages."""
 free_port = self._get_free_port()
 dest_uri = 'exec:nc -l localhost %u' % free_port
+src_uri = 'exec:nc localhost %u' % free_port
+self.do_migrate(dest_uri, src_uri)
-- 
2.21.1

[PATCH-for-5.0 2/8] tests/acceptance/ppc_prep_40p: Use cdn.netbsd.org hostname

[Philippe Mathieu-Daudé]

Use NetBSD content delivery network to get faster downloads.

Suggested-by: Kamil Rytarowski 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20200211134504.9156-1-phi...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/ppc_prep_40p.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
index b27572f212..138064285a 100644
--- a/tests/acceptance/ppc_prep_40p.py
+++ b/tests/acceptance/ppc_prep_40p.py
@@ -34,7 +34,7 @@ def test_factory_firmware_and_netbsd(self):
 '7020-40p/P12H0456.IMG')
 bios_hash = '1775face4e6dc27f3a6ed955ef6eb331bf817f03'
 bios_path = self.fetch_asset(bios_url, asset_hash=bios_hash)
-drive_url = ('https://ftp.netbsd.org/pub/NetBSD/NetBSD-archive/'
+drive_url = ('https://cdn.netbsd.org/pub/NetBSD/NetBSD-archive/'
  'NetBSD-4.0/prep/installation/floppy/generic_com0.fs')
 drive_hash = 'dbcfc09912e71bd5f0d82c7c1ee43082fb596ceb'
 drive_path = self.fetch_asset(drive_url, asset_hash=drive_hash)
@@ -66,7 +66,7 @@ def test_openbios_and_netbsd(self):
 :avocado: tags=arch:ppc
 :avocado: tags=machine:40p
 """
-drive_url = ('https://ftp.netbsd.org/pub/NetBSD/iso/7.1.2/'
+drive_url = ('https://cdn.netbsd.org/pub/NetBSD/iso/7.1.2/'
  'NetBSD-7.1.2-prep.iso')
 drive_hash = 'ac6fa2707d888b36d6fa64de6e7fe48e'
 drive_path = self.fetch_asset(drive_url, asset_hash=drive_hash,
-- 
2.21.1

[PATCH-for-5.0 4/8] tests/acceptance/machine_sparc_leon3: Disable HelenOS test

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé 

This test was written/tested around beginning of 2019, but was
extracted from a bigger series and posted end of June 2019 [*].
Unfortunately I did not notice commit 162abf1a8 was merged by
then, which implements the AHB and APB plug and play devices.

HelenOS 0.6 is expecting the PnP registers to be not implemented
by QEMU, then forces the discovered AMBA devices (see [2]).

Before 162abf1a8, the console was displaying:

  HelenOS bootloader, release 0.6.0 (Elastic Horse)
  Built on 2014-12-21 20:17:42 for sparc32
  Copyright (c) 2001-2014 HelenOS project
   0x4000bf20|0x4000bf20: kernel image (496640/128466 bytes)
   0x4002b4f2|0x4002b4f2: ns image (154195/66444 bytes)
   0x4003b87e|0x4003b87e: loader image (153182/66437 bytes)
   0x4004bc03|0x4004bc03: init image (155339/66834 bytes)
   0x4005c115|0x4005c115: locsrv image (162063/70267 bytes)
   0x4006d390|0x4006d390: rd image (152678/65889 bytes)
   0x4007d4f1|0x4007d4f1: vfs image (168480/73394 bytes)
   0x4008f3a3|0x4008f3a3: logger image (158034/68368 bytes)
   0x4009feb3|0x4009feb3: ext4fs image (234510/100301 bytes)
   0x400b8680|0x400b8680: initrd image (8388608/1668901 bytes)
  ABMA devices:
  <1:00c> at 0x8100 irq 3
  <1:00d> at 0x8200
  <1:011> at 0x8300 irq 8
  Memory size: 64 MB

As of this commit, it is now confused:

  ABMA devices:
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  <1:3000> at 0x irq 0
  ...

As this test is not working as expected, simply disable it (by
skipping it) for now.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg627094.html
[2] 
https://github.com/HelenOS/helenos/blob/0.6.0/boot/arch/sparc32/src/ambapp.c#L75

Reviewed-by: Richard Henderson 
Tested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200331105048.27989-2-f4...@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/machine_sparc_leon3.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/acceptance/machine_sparc_leon3.py 
b/tests/acceptance/machine_sparc_leon3.py
index f77e210ccb..2405cd7a0d 100644
--- a/tests/acceptance/machine_sparc_leon3.py
+++ b/tests/acceptance/machine_sparc_leon3.py
@@ -7,12 +7,16 @@
 
 from avocado_qemu import Test
 from avocado_qemu import wait_for_console_pattern
+from avocado import skip
 
 
 class Leon3Machine(Test):
 
 timeout = 60
 
+@skip("Test currently broken")
+# A Window Underflow exception occurs before booting the kernel,
+# and QEMU exit calling cpu_abort(), which makes this test to fail.
 def test_leon3_helenos_uimage(self):
 """
 :avocado: tags=arch:sparc
-- 
2.21.1

[PATCH-for-5.0 0/8] Acceptance tests queue

[Philippe Mathieu-Daudé]

I'm planning to send the first 5 patches as
This series contains acceptance test fixes, and Travis-CI
improvements. I plan to send a pull request with the first
6 patches for v5.0-rc2. The last 3 patches helped debugging
painfully the Travis failures, but can not be used because
Avocado 78.0 is not released yet.

Patches 3 and 5 are not reviewed.

Up-to-patch 5 job:
https://travis-ci.org/github/philmd/qemu/jobs/670645531

All patches job:
https://travis-ci.org/github/philmd/qemu/jobs/670645611

Oksana Vohchana (1):
  Acceptance test: Fix to EXEC migration

Philippe Mathieu-Daudé (7):
  tests/acceptance/ppc_prep_40p: Use cdn.netbsd.org hostname
  tests/acceptance/ppc_prep_40p: Use mirror for ftp.software.ibm.com
  tests/acceptance/machine_sparc_leon3: Disable HelenOS test
  .travis.yml: Cache acceptance-test assets
  tests/Makefile: Add fetch-acceptance-assets rule
  .travis.yml: Run fetch-acceptance-assets before check-acceptance
  NOTFORMERGE Kludge for Avocado issue #3661

 .travis.yml | 4 +++-
 tests/Makefile.include  | 7 +++
 tests/acceptance/machine_sparc_leon3.py | 4 
 tests/acceptance/migration.py   | 6 +++---
 tests/acceptance/ppc_prep_40p.py| 7 ---
 tests/requirements.txt  | 2 +-
 6 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.21.1

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Maciej S. Szmigiero]

Hi Jon,

The patches are available here:
https://github.com/maciejsszmigiero/qemu.git in "vmbus-patches" branch.

Please note that these patches don't have Roman's "Signed-off-by:" tags,
so I haven't applied mine, either.

If you are able to establish a proper SoB chain then please also add:
"Signed-off-by: Maciej S. Szmigiero ".

Thanks for the effort,
Maciej

On 03.04.2020 17:30, Jon Doron wrote:
>  Thank you Maciej it seems like your version is really ahead I'll do
> the required work and merge it so i can submit a v2 with the latest
> patchset from Roman
> 
> On Fri, Apr 3, 2020 at 6:06 PM Jon Doron  wrote:
>>
>> Thank you Maciej, I based it on top of what Denis (d...@openvz.org) gave me
>> which was this:
>> https://ftp.openvz.org/virtuozzo/releases/openvz-7.0.12-288/source/SRPMS/q/qemu-kvm-vz-2.12.0-33.vz7.14.4.src.rpm
>>
>> Do you think you have a more recent version I dont mind diffing and
>> resubmitting a new version of the patchset?
>>
>> Thanks,
>> -- Jon.
>>
>> On Fri, Apr 3, 2020 at 5:56 PM Maciej S. Szmigiero
>>  wrote:
>>>
>>> Hi Jon,
>>>
>>> On 03.04.2020 16:23, Jon Doron wrote:
 Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
 entry to DSDT in case vmbus has been enabled.

 Experimentally Windows guests were found to require this entry to
 include two IRQ resources, so this patch adds two semi-arbitrarily
 chosen ones (7 and 13).  This results, in particular, in parallel port
 conflicting with vmbus.

 TODO: discover and use spare IRQs to avoid conflicts.

 Signed-off-by: Evgeny Yakovlev 
 Signed-off-by: Roman Kagan 
 Signed-off-by: Jon Doron 
>>>
>>> Nice work, thanks!
>>>
>>> However, it seems to be based on the code version that was posted in
>>> February 2018, and not the latest version in OpenVZ qemu repository
>>> dated October 2019:
>>> https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus
>>>
>>> This newer version has slightly different API here and there.
>>> Any particular reason for selecting that older version for porting?
>>>
>>> I have actually rebased this latest version on the top of the current
>>> QEMU master, and it basically seems to work fine.
>>> However, I haven't done extensive tests whether there isn't a memory leak
>>> somewhere or so on.
>>>
>>> Maciej

[PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Alberto Garcia]

When issuing a compressed write request the number of bytes must be a
multiple of the cluster size.

With the current code such requests are allowed and we hit an
assertion:

   $ qemu-img create -f qcow2 img.qcow2 1M
   $ qemu-io -c 'write -c 0 32k' img.qcow2

   qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
   Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
  (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' 
failed.
   Aborted

This patch fixes a regression introduced in 0d483dce38

Signed-off-by: Alberto Garcia 
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..923ad428f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
 return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
 }
 
-if (offset_into_cluster(s, offset)) {
+if (offset_into_cluster(s, offset | bytes)) {
 return -EINVAL;
 }
 
-- 
2.20.1

Re: [PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Peter Maydell]

On Fri, 3 Apr 2020 at 17:54, Philippe Mathieu-Daudé  wrote:
>
> When ./configure checks the sphinx version is new enough, it leaves
> the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
> option (don't write .py[co] files on import) via the
> PYTHONDONTWRITEBYTECODE environment variable.
>
> Reported-by: Eric Blake 
> Signed-off-by: Philippe Mathieu-Daudé 

This only happens for an in-tree build, right? I think in
that case you're kind of OK with having random stuff
left in the source tree... It seems easy enough to suppress
them though, so I guess we might as well.

thanks
-- PMM

[PATCH-for-5.0?] configure: Do not leave sphinx in-tree artifacts

[Philippe Mathieu-Daudé]

When ./configure checks the sphinx version is new enough, it leaves
the docs/sphinx/__pycache__/ directory. Avoid this by using the '-B'
option (don't write .py[co] files on import) via the
PYTHONDONTWRITEBYTECODE environment variable.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 22870f3867..ed524399c7 100755
--- a/configure
+++ b/configure
@@ -4936,7 +4936,7 @@ has_sphinx_build() {
 # sphinx-build doesn't exist at all or if it is too old.
 mkdir -p "$TMPDIR1/sphinx"
 touch "$TMPDIR1/sphinx/index.rst"
-"$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" 
"$TMPDIR1/sphinx/out" >/dev/null 2>&1
+PYTHONDONTWRITEBYTECODE=yes "$sphinx_build" -c "$source_path/docs" -b html 
"$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >/dev/null 2>&1
 }
 
 # Check if tools are available to build documentation.
-- 
2.21.1

Re: [PATCH v2] nvdimm-utils: clean up headers and add license comment

[Philippe Mathieu-Daudé]

Hi Igor,

On 4/3/20 4:41 PM, Igor Mammedov wrote:

Fixes: 3f350f6bb36233be50fc2bc18dc78b6a948a5dbe
Reported-by: Peter Maydell 
Signed-off-by: Igor Mammedov 
---
v2:
   - add license blob to header as well
   - trim comment a litle bit to remove unrelated NFIT/PMEM sentences
---
  include/qemu/nvdimm-utils.h | 23 +--
  util/nvdimm-utils.c | 23 +++
  2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/qemu/nvdimm-utils.h b/include/qemu/nvdimm-utils.h
index 4b8b198ba7..2d8cde085c 100644
--- a/include/qemu/nvdimm-utils.h
+++ b/include/qemu/nvdimm-utils.h
@@ -1,7 +1,26 @@
  #ifndef NVDIMM_UTILS_H
  #define NVDIMM_UTILS_H


I think most of the files in the tree use the pattern "license comment 
before the ifdef guard", but I won't write a script to prove me 
right/wrong :)



-
-#include "qemu/osdep.h"
+/*
+ * NVDIMM utilities
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */


So here goes the ifdef:

#ifndef NVDIMM_UTILS_H
#define NVDIMM_UTILS_H

  
  GSList *nvdimm_get_device_list(void);

  #endif
diff --git a/util/nvdimm-utils.c b/util/nvdimm-utils.c
index 5cc768ca47..3d570c3c3e 100644
--- a/util/nvdimm-utils.c
+++ b/util/nvdimm-utils.c
@@ -1,3 +1,26 @@
+/*
+ * NVDIMM utilities
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */
+
+#include "qemu/osdep.h"
  #include "qemu/nvdimm-utils.h"
  #include "hw/mem/nvdimm.h"
  

Re: [PULL 03/13] target/rx: TCG translation

[Richard Henderson]

On 4/3/20 9:41 AM, Peter Maydell wrote:
> On Tue, 17 Mar 2020 at 16:52, Philippe Mathieu-Daudé  wrote:
>>
>> From: Yoshinori Sato 
>>
>> This part only supported RXv1 instructions.
>>
>> Instruction manual:
>>   
>> https://www.renesas.com/us/en/doc/products/mpumcu/doc/rx_family/r01us0032ej0120_rxsm.pdf
> 
> Hi; Coverity points out a possible missing 'break' here
> (CID 142):
> 
>> +static void rx_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> +{
>> +DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +
>> +switch (ctx->base.is_jmp) {
>> +case DISAS_NEXT:
>> +case DISAS_TOO_MANY:
>> +gen_goto_tb(ctx, 0, dcbase->pc_next);
>> +break;
>> +case DISAS_JUMP:
>> +if (ctx->base.singlestep_enabled) {
>> +gen_helper_debug(cpu_env);
>> +} else {
>> +tcg_gen_lookup_and_goto_ptr();
>> +}
>> +break;
>> +case DISAS_UPDATE:
>> +tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next);
> 
> Should this have a "break" or a "/* fall through */" comment ?
> 
>> +case DISAS_EXIT:
>> +tcg_gen_exit_tb(NULL, 0);
>> +break;

This one should be a fall through.


r~

Re: [PULL 03/13] target/rx: TCG translation

[Peter Maydell]

On Tue, 17 Mar 2020 at 16:52, Philippe Mathieu-Daudé  wrote:
>
> From: Yoshinori Sato 
>
> This part only supported RXv1 instructions.
>
> Instruction manual:
>   
> https://www.renesas.com/us/en/doc/products/mpumcu/doc/rx_family/r01us0032ej0120_rxsm.pdf

Hi; Coverity points out a possible missing 'break' here
(CID 142):

> +static void rx_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> +{
> +DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +switch (ctx->base.is_jmp) {
> +case DISAS_NEXT:
> +case DISAS_TOO_MANY:
> +gen_goto_tb(ctx, 0, dcbase->pc_next);
> +break;
> +case DISAS_JUMP:
> +if (ctx->base.singlestep_enabled) {
> +gen_helper_debug(cpu_env);
> +} else {
> +tcg_gen_lookup_and_goto_ptr();
> +}
> +break;
> +case DISAS_UPDATE:
> +tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next);

Should this have a "break" or a "/* fall through */" comment ?

> +case DISAS_EXIT:
> +tcg_gen_exit_tb(NULL, 0);
> +break;
> +case DISAS_NORETURN:
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +}

thanks
-- PMM

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Maciej S. Szmigiero]

Hi Jon,

On 03.04.2020 16:23, Jon Doron wrote:
> Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
> entry to DSDT in case vmbus has been enabled.
> 
> Experimentally Windows guests were found to require this entry to
> include two IRQ resources, so this patch adds two semi-arbitrarily
> chosen ones (7 and 13).  This results, in particular, in parallel port
> conflicting with vmbus.
> 
> TODO: discover and use spare IRQs to avoid conflicts.
> 
> Signed-off-by: Evgeny Yakovlev 
> Signed-off-by: Roman Kagan 
> Signed-off-by: Jon Doron 

Nice work, thanks!

However, it seems to be based on the code version that was posted in 
February 2018, and not the latest version in OpenVZ qemu repository
dated October 2019:
https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus

This newer version has slightly different API here and there.
Any particular reason for selecting that older version for porting?

I have actually rebased this latest version on the top of the current
QEMU master, and it basically seems to work fine.
However, I haven't done extensive tests whether there isn't a memory leak
somewhere or so on.

Maciej

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Maciej S. Szmigiero]

Thanks Jon,

I will push out to what I have in a moment.

Maciej

On 03.04.2020 17:30, Jon Doron wrote:
>  Thank you Maciej it seems like your version is really ahead I'll do
> the required work and merge it so i can submit a v2 with the latest
> patchset from Roman
> 
> On Fri, Apr 3, 2020 at 6:06 PM Jon Doron  wrote:
>>
>> Thank you Maciej, I based it on top of what Denis (d...@openvz.org) gave me
>> which was this:
>> https://ftp.openvz.org/virtuozzo/releases/openvz-7.0.12-288/source/SRPMS/q/qemu-kvm-vz-2.12.0-33.vz7.14.4.src.rpm
>>
>> Do you think you have a more recent version I dont mind diffing and
>> resubmitting a new version of the patchset?
>>
>> Thanks,
>> -- Jon.
>>
>> On Fri, Apr 3, 2020 at 5:56 PM Maciej S. Szmigiero
>>  wrote:
>>>
>>> Hi Jon,
>>>
>>> On 03.04.2020 16:23, Jon Doron wrote:
 Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
 entry to DSDT in case vmbus has been enabled.

 Experimentally Windows guests were found to require this entry to
 include two IRQ resources, so this patch adds two semi-arbitrarily
 chosen ones (7 and 13).  This results, in particular, in parallel port
 conflicting with vmbus.

 TODO: discover and use spare IRQs to avoid conflicts.

 Signed-off-by: Evgeny Yakovlev 
 Signed-off-by: Roman Kagan 
 Signed-off-by: Jon Doron 
>>>
>>> Nice work, thanks!
>>>
>>> However, it seems to be based on the code version that was posted in
>>> February 2018, and not the latest version in OpenVZ qemu repository
>>> dated October 2019:
>>> https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus
>>>
>>> This newer version has slightly different API here and there.
>>> Any particular reason for selecting that older version for porting?
>>>
>>> I have actually rebased this latest version on the top of the current
>>> QEMU master, and it basically seems to work fine.
>>> However, I haven't done extensive tests whether there isn't a memory leak
>>> somewhere or so on.
>>>
>>> Maciej

Re: bdrv_drained_begin deadlock with io-threads

[Dietmar Maurer]

> On April 3, 2020 10:47 AM Kevin Wolf  wrote:
> 
>  
> Am 03.04.2020 um 10:26 hat Dietmar Maurer geschrieben:
> > > With the following patch, it seems to survive for now. I'll give it some
> > > more testing tomorrow (also qemu-iotests to check that I didn't
> > > accidentally break something else.)
> > 
> > Wow, that was fast! Seems your patch fixes the bug!
> > 
> > I wonder what commit introduced that problem, maybe:
> > 
> > https://github.com/qemu/qemu/commit/cf3129323f900ef5ddbccbe86e4fa801e88c566e#diff-7cb66df56045598b75a219eebc27efb6
> > 
> > If so, version 4.1.X in not affected by this bug, but 4.2.0 and later?
> 
> Yes, I'm pretty sure that's the one.

I also wonder if we can loose an aio_wait_kick() by directly accessing 
blk->in_flight.
I thought this should use atomic_mb_read()?

diff --git a/block/block-backend.c b/block/block-backend.c
index 8b8f2a80a0..48f3721505 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2275,7 +2275,7 @@ static bool blk_root_drained_poll(BdrvChild *child)
 {
 BlockBackend *blk = child->opaque;
 assert(blk->quiesce_counter);
-return !!blk->in_flight;
+return !!atomic_mb_read(>in_flight);
 }
 
 static void blk_root_drained_end(BdrvChild *child, int *drained_end_counter)

Re: [PATCH v2 13/22] intel_iommu: add PASID cache management infrastructure

[Peter Xu]

On Fri, Apr 03, 2020 at 03:05:57PM +, Liu, Yi L wrote:
> > From: Peter Xu 
> > Sent: Thursday, April 2, 2020 9:45 PM
> > To: Liu, Yi L 
> > Subject: Re: [PATCH v2 13/22] intel_iommu: add PASID cache management
> > infrastructure
> > 
> > On Thu, Apr 02, 2020 at 06:46:11AM +, Liu, Yi L wrote:
> > 
> > [...]
> > 
> > > > > +/**
> > > > > + * This function replay the guest pasid bindings to hots by
> > > > > + * walking the guest PASID table. This ensures host will have
> > > > > + * latest guest pasid bindings. Caller should hold iommu_lock.
> > > > > + */
> > > > > +static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> > > > > +VTDPASIDCacheInfo
> > > > > +*pc_info) {
> > > > > +VTDHostIOMMUContext *vtd_dev_icx;
> > > > > +int start = 0, end = VTD_HPASID_MAX;
> > > > > +vtd_pasid_table_walk_info walk_info = {.flags = 0};
> > > >
> > > > So vtd_pasid_table_walk_info is still used.  I thought we had
> > > > reached a consensus that this can be dropped?
> > >
> > > yeah, I did have considered your suggestion and plan to do it. But
> > > when I started coding, it looks a little bit weird to me:
> > > For one, there is an input VTDPASIDCacheInfo in this function. It may
> > > be nature to think about passing the parameter to further calling
> > > (vtd_replay_pasid_bind_for_dev()). But, we can't do that. The
> > > vtd_bus/devfn fields should be filled when looping the assigned
> > > devices, not the one passed by vtd_replay_guest_pasid_bindings() caller.
> > 
> > Hacky way is we can directly modify VTDPASIDCacheInfo* with bus/devfn for 
> > the
> > loop.  Otherwise we can duplicate the object when looping, so that we can 
> > avoid
> > introducing a new struct which seems to contain mostly the same information.
> 
> I see. Please see below reply.
> 
> > > For two, reusing the VTDPASIDCacheInfo for passing walk info may
> > > require the final user do the same thing as what the
> > > vtd_replay_guest_pasid_bindings() has done here.
> > 
> > I don't see it happen, could you explain?
> 
> my concern is around flags field in VTDPASIDCacheInfo. The flags not
> only indicates the invalidation granularity, but also indicates the
> field presence. e.g. VTD_PASID_CACHE_DEVSI indicates the vtd_bus/devfn
> fields are valid. If reuse it to pass walk info to 
> vtd_sm_pasid_table_walk_one,
> it would be meaningless as vtd_bus/devfn fields are always valid. But
> I'm fine to reuse it's more prefered. Instead of modifying the vtd_bus/devn
> in VTDPASIDCacheInfo*, I'd rather to define another VTDPASIDCacheInfo variable
> and pass it to vtd_sm_pasid_table_walk_one. This may not affect the future
> caller of vtd_replay_guest_pasid_bindings() as vtd_bus/devfn field are not
> designed to bring something back to caller.

Yeah, let's give it a shot.  I know it's not ideal, but IMHO it's
still better than defining the page_walk struct and that might confuse
readers on what's the difference between the two.  When duplicating
the object, we can add some comment explaining this.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Peter Xu]

On Fri, Apr 03, 2020 at 03:21:10PM +, Liu, Yi L wrote:
> > From: Peter Xu 
> > Sent: Friday, April 3, 2020 10:46 PM
> > To: Liu, Yi L 
> > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context 
> > cache
> > invalidation
> > 
> > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote:
> > > This patch replays guest pasid bindings after context cache
> > > invalidation. This is a behavior to ensure safety. Actually,
> > > programmer should issue pasid cache invalidation with proper
> > > granularity after issuing a context cache invalidation.
> > >
> > > Cc: Kevin Tian 
> > > Cc: Jacob Pan 
> > > Cc: Peter Xu 
> > > Cc: Yi Sun 
> > > Cc: Paolo Bonzini 
> > > Cc: Richard Henderson 
> > > Cc: Eduardo Habkost 
> > > Signed-off-by: Liu Yi L 
> > > ---
> > >  hw/i386/intel_iommu.c  | 51
> > ++
> > >  hw/i386/intel_iommu_internal.h |  6 -
> > >  hw/i386/trace-events   |  1 +
> > >  3 files changed, 57 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index d87f608..883aeac 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -68,6 +68,10 @@ static void
> > vtd_address_space_refresh_all(IntelIOMMUState *s);
> > >  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier 
> > > *n);
> > >
> > >  static void vtd_pasid_cache_reset(IntelIOMMUState *s);
> > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > > + VTDPASIDCacheInfo *pc_info);
> > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> > > +  VTDBus *vtd_bus, uint16_t devfn);
> > >
> > >  static void vtd_panic_require_caching_mode(void)
> > >  {
> > > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState
> > *s)
> > >
> > >  static void vtd_context_global_invalidate(IntelIOMMUState *s)
> > >  {
> > > +VTDPASIDCacheInfo pc_info;
> > > +
> > >  trace_vtd_inv_desc_cc_global();
> > > +
> > >  /* Protects context cache */
> > >  vtd_iommu_lock(s);
> > >  s->context_cache_gen++;
> > > @@ -1870,6 +1877,9 @@ static void
> > vtd_context_global_invalidate(IntelIOMMUState *s)
> > >   * VT-d emulation codes.
> > >   */
> > >  vtd_iommu_replay_all(s);
> > > +
> > > +pc_info.flags = VTD_PASID_CACHE_GLOBAL;
> > > +vtd_pasid_cache_sync(s, _info);
> > >  }
> > >
> > >  /**
> > > @@ -2005,6 +2015,22 @@ static void
> > vtd_context_device_invalidate(IntelIOMMUState *s,
> > >   * happened.
> > >   */
> > >  vtd_sync_shadow_page_table(vtd_as);
> > > +/*
> > > + * Per spec, context flush should also followed with 
> > > PASID
> > > + * cache and iotlb flush. Regards to a device selective
> > > + * context cache invalidation:
> > 
> > If context entry flush should also follow another pasid cache flush,
> > then this is still needed?  Shouldn't the pasid flush do the same
> > thing again?
> 
> yes, but how about guest software failed to follow it? It will do
> the same thing when pasid cache flush comes. But this only happens
> for the rid2pasid case (the IOVA page table).

Do you mean it will not happen when nested page table is used (so it's
required for nested tables)?

Yeah we can keep them for safe no matter what; at least I'm fine with
it (I believe most of the code we're discussing is not fast path).
Just want to be sure of it since if it's definitely duplicated then we
can instead drop it.

Thanks,

-- 
Peter Xu

Re: qemu-system-aarch64 windows binary run Arm64 defconfig kernel not working

[yoma sophian]

hi Peter:
> unfortunately, 32-bit Windows qemu binary still fail on booting
> upstream arm64 defconfig kernel.

Is there any debug info of qemu could I provide?

Appreciate ur kind help,

Re: [PATCH v3 0/3] hw/riscv: Add a serial property to sifive_u

[Palmer Dabbelt]

On Mon, 23 Mar 2020 19:08:19 PDT (-0700), bmeng...@gmail.com wrote:

Hi Palmer,

On Sat, Mar 7, 2020 at 5:45 AM Alistair Francis
 wrote:


At present the board serial number is hard-coded to 1, and passed
to OTP model during initialization. Firmware (FSBL, U-Boot) uses
the serial number to generate a unique MAC address for the on-chip
ethernet controller. When multiple QEMU 'sifive_u' instances are
created and connected to the same subnet, they all have the same
MAC address hence it creates a unusable network.

A new "serial" property is introduced to specify the board serial
number. When not given, the default serial number 1 is used.



Could you please take this for v5.0.0?


It's in the queue, sorry I missed them.

Re: [PATCH 2/5] ppc/pnv: Add support for NMI interface

[Cédric Le Goater]

On 4/3/20 3:12 PM, Nicholas Piggin wrote:
> Nicholas Piggin's on April 3, 2020 5:57 pm:
>> Cédric Le Goater's on March 26, 2020 2:38 am:
>>> [ Please use c...@kaod.org ! ]
>>>
>>> On 3/25/20 3:41 PM, Nicholas Piggin wrote:
 This implements the NMI interface for the PNV machine, similarly to
 commit 3431648272d ("spapr: Add support for new NMI interface") for
 SPAPR.

 Signed-off-by: Nicholas Piggin 
>>>
>>> one minor comment,
>>>
>>> Reviewed-by: Cédric Le Goater 
>>>
 ---
  hw/ppc/pnv.c | 30 +-
  1 file changed, 29 insertions(+), 1 deletion(-)

 diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
 index b75ad06390..671535ebe6 100644
 --- a/hw/ppc/pnv.c
 +++ b/hw/ppc/pnv.c
 @@ -27,6 +27,7 @@
  #include "sysemu/runstate.h"
  #include "sysemu/cpus.h"
  #include "sysemu/device_tree.h"
 +#include "sysemu/hw_accel.h"
  #include "target/ppc/cpu.h"
  #include "qemu/log.h"
  #include "hw/ppc/fdt.h"
 @@ -34,6 +35,7 @@
  #include "hw/ppc/pnv.h"
  #include "hw/ppc/pnv_core.h"
  #include "hw/loader.h"
 +#include "hw/nmi.h"
  #include "exec/address-spaces.h"
  #include "qapi/visitor.h"
  #include "monitor/monitor.h"
 @@ -1955,10 +1957,35 @@ static void pnv_machine_set_hb(Object *obj, bool 
 value, Error **errp)
  }
  }

 +static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, run_on_cpu_data arg)
 +{
 +PowerPCCPU *cpu = POWERPC_CPU(cs);
 +CPUPPCState *env = >env;
 +
 +cpu_synchronize_state(cs);
 +ppc_cpu_do_system_reset(cs);
 +/*
 + * SRR1[42:45] is set to 0100 which the ISA defines as implementation
>>>
>>> I see 'System Reset' in ISA 3.0
 + * dependent. POWER processors use this for xscom triggered 
 interrupts,
 + * which come from the BMC or NMI IPIs.
 + */
 +env->spr[SPR_SRR1] |= PPC_BIT(43);
>>>
>>> So we could have used the skiboot SPR_SRR1_PM_WAKE_SRESET define ? 
>>
>> Ah, that's only for power-saving wakeup. But you got me to dig further
>> and I think I've got a few things wrong here.
>>
>> The architectural power save wakeup due to sreset bit 43 needs to be
>> set, probably in excp_helper.c if (msr_pow) test.
>>
>> case POWERPC_EXCP_RESET: /* System reset exception   
>> */
>> /* A power-saving exception sets ME, otherwise it is unchanged */
>> if (msr_pow) {
>> /* indicate that we resumed from power save mode */
>> msr |= 0x1;
>> new_msr |= ((target_ulong)1 << MSR_ME);
>> }
> 
> Sorry I'm wrong, that's the old MSR_POW thing I guess G5 had it.
> 
> 'stop' state wakeup is powerpc_reset_wakeup of course, which seems to
> do the right thing with SRR1.
> 
> Something like this patch should fix this issue in the ppc-5.1 branch.
> This appears to do the right thing with SRR1 in testing with Linux.
> 
> ---
>  hw/ppc/pnv.c | 21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index ac83b8698b..596ccfd99e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1964,12 +1964,21 @@ static void pnv_cpu_do_nmi_on_cpu(CPUState *cs, 
> run_on_cpu_data arg)
>  
>  cpu_synchronize_state(cs);
>  ppc_cpu_do_system_reset(cs);
> -/*
> - * SRR1[42:45] is set to 0100 which the ISA defines as implementation
> - * dependent. POWER processors use this for xscom triggered interrupts,
> - * which come from the BMC or NMI IPIs.
> - */
> -env->spr[SPR_SRR1] |= PPC_BIT(43);
> +if (env->spr[SPR_SRR1] & PPC_BITMASK(46, 47)) {
> +/* system reset caused wake from power saving state */
> +if (!(env->spr[SPR_SRR1] & PPC_BIT(43))) {
> +warn_report("ppc_cpu_do_system_reset does not set system reset 
> wakeup reason");
> +env->spr[SPR_SRR1] |= PPC_BIT(43);
> +}
> +} else {
> +/*
> +  * For non-powersave wakeup system resets, SRR1[42:45] are defined to
> +  * be implementation dependent. Set to 0b0010, which POWER9 UM defines
> +  * to be interrupt caused by SCOM, which can come from the BMC or NMI
> +  * IPIs.
> + */
> +env->spr[SPR_SRR1] |= PPC_BIT(44);
> +}
>  }

That looks correct according to the UM.

Do we care about the other non-powersave wakeup system resets ? 

  0011 Interrupt caused by hypervisor door bell.
  0101 Interrupt caused by privileged door bell.

The reason is that I sometime see CPU lockups under load, or with a KVM guest, 
and I haven't found why yet.

Thanks,


C. 


[  259.069436] virbr0: port 2(tap0) entered forwarding state
[  259.069523] virbr0: topology change detected, propagating
[  384.427337] watchdog: CPU 1 detected hard LOCKUP on other CPUs 0
[  384.428566] watchdog: CPU 1 TB:255514422948, last SMP heartbeat 
TB:246648941120 (17315ms ago)
[  384.528958] watchdog: CPU 0 Hard LOCKUP
[ 

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

03.04.2020 18:05, Dr. David Alan Gilbert wrote:

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:

03.04.2020 14:29, Vladimir Sementsov-Ogievskiy wrote:

03.04.2020 14:23, Peter Krempa wrote:

On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:

19.12.2019 13:36, Peter Krempa wrote:

On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

It's a continuation for
"bitmap migration bug with -drive while block mirror runs"
<315cff78-dcdb-a3ce-2742-da3cc9f0c...@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html

The problem is that bitmaps migrated to node with same node-name or
blk-parent name. And currently only the latter actually work in libvirt.
And with mirror-top filter it doesn't work, because
bdrv_get_device_or_node_name don't go through filters.


I want to point out that since libvirt-5.10 we use -blockdev to
configure the backend of storage devices with qemu-4.2 and later. This
means unfortunately that the BlockBackend of the drive does not have a
name any more and thus the above will not work even if you make the
lookup code to see through filters.


Not that this series doesn't make things worse, as it loops through named
block backends when trying to use their name for migration. So with these
patches applied, qemu will just work in more possible scenarios.


Okay, if that's so it's fair enough in this case.

I'm just very firmly against baking in the assumption that
node names mean the same thing accross migration, because that will
create a precedent situation and more stuff may be baked in on top of
this in the future. It seems that it has already happened though and
it's wrong. And the worst part is that it's never mentioned that this
might occur. But again, don't do that and preferrably remove the
matching of node names for bitmaps altogether until we can control it
arbitrarily.

We've also seen this already before with the backend name of memory
devices being baked in to the migration stream which creates an unwanted
dependancy.



Hmm. Actually, matching by node-name never worked. May be just drop it now, and 
allow only matching by blk-name?

And then (in 5.1) implement special qmp commands for precise mapping.



Hmm, it may break someones setup... Bad idea. Probably we can forbid 
auto-generated node-names.


If we want to remove it I guess we have to go through a proper
deprecation; but that's OK.

The thing to keep in mind is that when people say 'the commandline
should match' on source/destination - that's just not true;
so we have to define what actually needs to stay the same for bitmap
migration to work.


Hmm. Let's add two qmp commands

1. migrate-set-outgoing-bitmap-mapping, which can set mapping (node-name, 
bitmap-name) -> (migration-node-name, migration-bitmap-name)
2. migrate-set-incoming-bitmap-mapping, which can set mapping 
(migration-node-name, migration-bitmap-name) -> (node-name, bitmap-name)

So, if we want to migrate bitmap B1 from node N1 on source to node M1 on 
target, we'll have three possibilities:

1. call on source migrate-set-outgoing-bitmap-mapping, to set mapping (N1, B1) 
-> (M1, B1) (and target will use 'M1' from migration stream to search the node)

2. call on destination migrate-set-incoming-bitmap-mapping, to set mapping (N1, 
B1) -> (M1, B1) (source will just put 'N1' to migration stream, and target will 
made transformation)

or even
3. Set mapping both on source and target, to make migration stream abstract, for example, 
(N1, B1) -> ('_abstract_bitmap_migration_node_', ) -> (M1, B1)

Using 1 and 2, it is possible to make any migration to/from older Qemu version..

And what should be deprecated is dirty-bitmaps migration capability, which is 
associated with old behavior. So, newer libvirt will call set-mapping commands 
both on source and target, instead of enabling capability.


--
Best regards,
Vladimir

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Jon Doron]

 Thank you Maciej it seems like your version is really ahead I'll do
the required work and merge it so i can submit a v2 with the latest
patchset from Roman

On Fri, Apr 3, 2020 at 6:06 PM Jon Doron  wrote:
>
> Thank you Maciej, I based it on top of what Denis (d...@openvz.org) gave me
> which was this:
> https://ftp.openvz.org/virtuozzo/releases/openvz-7.0.12-288/source/SRPMS/q/qemu-kvm-vz-2.12.0-33.vz7.14.4.src.rpm
>
> Do you think you have a more recent version I dont mind diffing and
> resubmitting a new version of the patchset?
>
> Thanks,
> -- Jon.
>
> On Fri, Apr 3, 2020 at 5:56 PM Maciej S. Szmigiero
>  wrote:
> >
> > Hi Jon,
> >
> > On 03.04.2020 16:23, Jon Doron wrote:
> > > Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
> > > entry to DSDT in case vmbus has been enabled.
> > >
> > > Experimentally Windows guests were found to require this entry to
> > > include two IRQ resources, so this patch adds two semi-arbitrarily
> > > chosen ones (7 and 13).  This results, in particular, in parallel port
> > > conflicting with vmbus.
> > >
> > > TODO: discover and use spare IRQs to avoid conflicts.
> > >
> > > Signed-off-by: Evgeny Yakovlev 
> > > Signed-off-by: Roman Kagan 
> > > Signed-off-by: Jon Doron 
> >
> > Nice work, thanks!
> >
> > However, it seems to be based on the code version that was posted in
> > February 2018, and not the latest version in OpenVZ qemu repository
> > dated October 2019:
> > https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus
> >
> > This newer version has slightly different API here and there.
> > Any particular reason for selecting that older version for porting?
> >
> > I have actually rebased this latest version on the top of the current
> > QEMU master, and it basically seems to work fine.
> > However, I haven't done extensive tests whether there isn't a memory leak
> > somewhere or so on.
> >
> > Maciej

RE: [PATCH v2 22/22] intel_iommu: modify x-scalable-mode to be string option

[Liu, Yi L]

> From:  Peter Xu < pet...@redhat.com>
> Sent: Friday, April 3, 2020 10:49 PM
> To: Liu, Yi L 
> Subject: Re: [PATCH v2 22/22] intel_iommu: modify x-scalable-mode to be string
> option
> 
> On Sun, Mar 29, 2020 at 09:25:01PM -0700, Liu Yi L wrote:
> > Intel VT-d 3.0 introduces scalable mode, and it has a bunch of
> > capabilities related to scalable mode translation, thus there are multiple
> combinations.
> > While this vIOMMU implementation wants simplify it for user by
> > providing typical combinations. User could config it by
> > "x-scalable-mode" option. The usage is as below:
> >
> > "-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"
> >
> >  - "legacy": gives support for SL page table
> >  - "modern": gives support for FL page table, pasid, virtual command
> >  - "off": no scalable mode support
> >  -  if not configured, means no scalable mode support, if not proper
> > configured, will throw error
> >
> > Note: this patch is supposed to be merged when  the whole vSVA patch
> > series were merged.
> >
> > Cc: Kevin Tian 
> > Cc: Jacob Pan 
> > Cc: Peter Xu 
> > Cc: Yi Sun 
> > Cc: Paolo Bonzini 
> > Cc: Richard Henderson 
> > Cc: Eduardo Habkost 
> > Signed-off-by: Liu Yi L 
> > Signed-off-by: Yi Sun 
> 
> Reviewed-by: Peter Xu 
thanks for your help. :-)

Regards,
Yi Liu

Re: [PATCH v2] s390x/pv: Retry ioctls on -EINTR

[Cornelia Huck]

On Fri, 27 Mar 2020 08:46:16 -0400
Christian Borntraeger  wrote:

> PV_ENABLE (and maybe others) might return -EINTR when a signal is
> pending. See the Linux kernel patch "s390/gmap: return proper error code
> on ksm unsharing" for details. Let us retry the ioctl in that case.
> 
> Fixes: 4d226deafc44 ("s390x: protvirt: Support unpack facility")

I'll update that when the commit ids are stable.

> Reported-by: Marc Hartmayer 
> Acked-by: Janosch Frank 
> Tested-by: Marc Hartmayer 
> Signed-off-by: Christian Borntraeger 
> ---
>  hw/s390x/pv.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Thanks, queued to s390-next.

RE: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Liu, Yi L]

> From: Peter Xu 
> Sent: Friday, April 3, 2020 10:46 PM
> To: Liu, Yi L 
> Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context 
> cache
> invalidation
> 
> On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote:
> > This patch replays guest pasid bindings after context cache
> > invalidation. This is a behavior to ensure safety. Actually,
> > programmer should issue pasid cache invalidation with proper
> > granularity after issuing a context cache invalidation.
> >
> > Cc: Kevin Tian 
> > Cc: Jacob Pan 
> > Cc: Peter Xu 
> > Cc: Yi Sun 
> > Cc: Paolo Bonzini 
> > Cc: Richard Henderson 
> > Cc: Eduardo Habkost 
> > Signed-off-by: Liu Yi L 
> > ---
> >  hw/i386/intel_iommu.c  | 51
> ++
> >  hw/i386/intel_iommu_internal.h |  6 -
> >  hw/i386/trace-events   |  1 +
> >  3 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index d87f608..883aeac 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -68,6 +68,10 @@ static void
> vtd_address_space_refresh_all(IntelIOMMUState *s);
> >  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
> >
> >  static void vtd_pasid_cache_reset(IntelIOMMUState *s);
> > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > + VTDPASIDCacheInfo *pc_info);
> > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> > +  VTDBus *vtd_bus, uint16_t devfn);
> >
> >  static void vtd_panic_require_caching_mode(void)
> >  {
> > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState
> *s)
> >
> >  static void vtd_context_global_invalidate(IntelIOMMUState *s)
> >  {
> > +VTDPASIDCacheInfo pc_info;
> > +
> >  trace_vtd_inv_desc_cc_global();
> > +
> >  /* Protects context cache */
> >  vtd_iommu_lock(s);
> >  s->context_cache_gen++;
> > @@ -1870,6 +1877,9 @@ static void
> vtd_context_global_invalidate(IntelIOMMUState *s)
> >   * VT-d emulation codes.
> >   */
> >  vtd_iommu_replay_all(s);
> > +
> > +pc_info.flags = VTD_PASID_CACHE_GLOBAL;
> > +vtd_pasid_cache_sync(s, _info);
> >  }
> >
> >  /**
> > @@ -2005,6 +2015,22 @@ static void
> vtd_context_device_invalidate(IntelIOMMUState *s,
> >   * happened.
> >   */
> >  vtd_sync_shadow_page_table(vtd_as);
> > +/*
> > + * Per spec, context flush should also followed with PASID
> > + * cache and iotlb flush. Regards to a device selective
> > + * context cache invalidation:
> 
> If context entry flush should also follow another pasid cache flush,
> then this is still needed?  Shouldn't the pasid flush do the same
> thing again?

yes, but how about guest software failed to follow it? It will do
the same thing when pasid cache flush comes. But this only happens
for the rid2pasid case (the IOVA page table).

> > + * if (emaulted_device)
> > + *modify the pasid cache gen and pasid-based iotlb gen
> > + *value (will be added in following patches)
> 
> Let's avoid using "following patches" because it'll be helpless after
> merged.  Also, the pasid cache gen is gone.

got it. will modify the description here.

> > + * else if (assigned_device)
> > + *check if the device has been bound to any pasid
> > + *invoke pasid_unbind regards to each bound pasid
> > + * Here, we have vtd_pasid_cache_devsi() to invalidate 
> > pasid
> > + * caches, while for piotlb in QEMU, we don't have it yet, 
> > so
> > + * no handling. For assigned device, host iommu driver 
> > would
> > + * flush piotlb when a pasid unbind is pass down to it.
> > + */
> > + vtd_pasid_cache_devsi(s, vtd_bus, devfn_it);
> >  }
> >  }
> >  }
> > @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, 
> > gpointer
> value,
> >  /* Fall through */
> >  case VTD_PASID_CACHE_GLOBAL:
> >  break;
> > +case VTD_PASID_CACHE_DEVSI:
> > +if (pc_info->vtd_bus != vtd_bus ||
> > +pc_info->devfn == devfn) {
> 
> Do you mean "!="?

exactly. thanks for catching it.

Regards,
Yi Liu

> > +return false;
> > +}
> > +break;
> >  default:
> >  error_report("invalid pc_info->flags");
> >  abort();
> > @@ -2827,6 +2859,11 @@ static void
> vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> >  walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK;
> >  /* loop all assigned devices */
> >  break;
> > +case VTD_PASID_CACHE_DEVSI:
> > +walk_info.vtd_bus = pc_info->vtd_bus;
> > +walk_info.devfn = pc_info->devfn;
> > +

RE: [PATCH v2 19/22] intel_iommu: process PASID-based iotlb invalidation

[Liu, Yi L]

> From: Peter Xu 
> Sent: Friday, April 3, 2020 10:47 PM
> To: Liu, Yi L 
> Subject: Re: [PATCH v2 19/22] intel_iommu: process PASID-based iotlb 
> invalidation
> 
> On Sun, Mar 29, 2020 at 09:24:58PM -0700, Liu Yi L wrote:
> > This patch adds the basic PASID-based iotlb (piotlb) invalidation
> > support. piotlb is used during walking Intel VT-d 1st level page
> > table. This patch only adds the basic processing. Detailed handling
> > will be added in next patch.
> >
> > Cc: Kevin Tian 
> > Cc: Jacob Pan 
> > Cc: Peter Xu 
> > Cc: Yi Sun 
> > Cc: Paolo Bonzini 
> > Cc: Richard Henderson 
> > Cc: Eduardo Habkost 
> > Signed-off-by: Liu Yi L 
> 
> Reviewed-by: Peter Xu 
> 
thanks for your help. :-)

Regards,
Yi Liu

Re: [PULL for-5.0 0/1] Block patches

[Peter Maydell]

On Fri, 3 Apr 2020 at 12:52, Stefan Hajnoczi  wrote:
>
> The following changes since commit 5142ca078d1cbc0f77b0f385d28cdb3e504e62bd:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into 
> staging (2020-04-02 20:18:25 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to ae60ab7eb20715fa63cca1b0bb4493e160da51ce:
>
>   aio-posix: fix test-aio /aio/event/wait with fdmon-io_uring (2020-04-03 
> 12:42:40 +0100)
>
> 
> Pull request
>
> 


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: [PATCH v4 4/4] target/ppc: Add support for Radix partition-scoped translation

[Greg Kurz]

On Fri,  3 Apr 2020 16:00:56 +0200
Cédric Le Goater  wrote:

> The Radix tree translation model currently supports process-scoped
> translation for the PowerNV machine (Hypervisor mode) and for the
> pSeries machine (Guest mode). Guests running under an emulated
> Hypervisor (PowerNV machine) require a new type of Radix translation,
> called partition-scoped, which is missing today.
> 
> The Radix tree translation is a 2 steps process. The first step,
> process-scoped translation, converts an effective Address to a guest
> real address, and the second step, partition-scoped translation,
> converts a guest real address to a host real address.
> 
> There are difference cases to covers :
> 
> * Hypervisor real mode access: no Radix translation.
> 
> * Hypervisor or host application access (quadrant 0 and 3) with
>   relocation on: process-scoped translation.
> 
> * Guest OS real mode access: only partition-scoped translation.
> 
> * Guest OS real or guest application access (quadrant 0 and 3) with
>   relocation on: both process-scoped translation and partition-scoped
>   translations.
> 
> * Hypervisor access in quadrant 1 and 2 with relocation on: both
>   process-scoped translation and partition-scoped translations.
> 
> The radix tree partition-scoped translation is performed using tables
> pointed to by the first double-word of the Partition Table Entries and
> process-scoped translation uses tables pointed to by the Process Table
> Entries (second double-word of the Partition Table Entries).
> 
> Both partition-scoped and process-scoped translations process are
> identical and thus the radix tree traversing code is largely reused.
> However, errors in partition-scoped translations generate hypervisor
> exceptions.
> 
> Signed-off-by: Suraj Jitindar Singh 
> Signed-off-by: Greg Kurz 
> Signed-off-by: Cédric Le Goater 
> ---
>  target/ppc/cpu.h |   3 +
>  target/ppc/excp_helper.c |   3 +-
>  target/ppc/mmu-radix64.c | 188 +++
>  3 files changed, 175 insertions(+), 19 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f4a5304d4356..6b6dd7e483f1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -463,6 +463,9 @@ typedef struct ppc_v3_pate_t {
>  #define DSISR_AMR0x0020
>  /* Unsupported Radix Tree Configuration */
>  #define DSISR_R_BADCONFIG0x0008
> +#define DSISR_ATOMIC_RC  0x0004
> +/* Unable to translate address of (guest) pde or process/page table entry */
> +#define DSISR_PRTABLE_FAULT  0x0002
>  
>  /* SRR1 error code fields */
>  
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 1acc3786de0e..f05297966472 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -506,9 +506,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>  case POWERPC_EXCP_ISEG:  /* Instruction segment exception
> */
>  case POWERPC_EXCP_TRACE: /* Trace exception  
> */
>  break;
> +case POWERPC_EXCP_HISI:  /* Hypervisor instruction storage exception 
> */
> +msr |= env->error_code;
>  case POWERPC_EXCP_HDECR: /* Hypervisor decrementer exception 
> */
>  case POWERPC_EXCP_HDSI:  /* Hypervisor data storage exception
> */
> -case POWERPC_EXCP_HISI:  /* Hypervisor instruction storage exception 
> */
>  case POWERPC_EXCP_HDSEG: /* Hypervisor data segment exception
> */
>  case POWERPC_EXCP_HISEG: /* Hypervisor instruction segment exception 
> */
>  case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt
> */
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 2400da41e06c..d473dc742e11 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -103,6 +103,27 @@ static void ppc_radix64_raise_si(PowerPCCPU *cpu, int 
> rwx, vaddr eaddr,
>  }
>  }
>  
> +static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, int rwx, vaddr eaddr,
> +  hwaddr g_raddr, uint32_t cause)
> +{
> +CPUState *cs = CPU(cpu);
> +CPUPPCState *env = >env;
> +
> +if (rwx == 2) { /* H Instruction Storage Interrupt */
> +cs->exception_index = POWERPC_EXCP_HISI;
> +env->spr[SPR_ASDR] = g_raddr;
> +env->error_code = cause;
> +} else { /* H Data Storage Interrupt */
> +cs->exception_index = POWERPC_EXCP_HDSI;
> +if (rwx == 1) { /* Write -> Store */
> +cause |= DSISR_ISSTORE;
> +}
> +env->spr[SPR_HDSISR] = cause;
> +env->spr[SPR_HDAR] = eaddr;
> +env->spr[SPR_ASDR] = g_raddr;
> +env->error_code = 0;
> +}
> +}
>  
>  static bool ppc_radix64_check_prot(PowerPCCPU *cpu, int rwx, uint64_t pte,
> int *fault_cause, int *prot,
> @@ -243,6 +264,37 @@ static bool validate_pate(PowerPCCPU *cpu, 

RE: [PATCH v2 13/22] intel_iommu: add PASID cache management infrastructure

[Liu, Yi L]

> From: Peter Xu 
> Sent: Thursday, April 2, 2020 9:45 PM
> To: Liu, Yi L 
> Subject: Re: [PATCH v2 13/22] intel_iommu: add PASID cache management
> infrastructure
> 
> On Thu, Apr 02, 2020 at 06:46:11AM +, Liu, Yi L wrote:
> 
> [...]
> 
> > > > +/**
> > > > + * This function replay the guest pasid bindings to hots by
> > > > + * walking the guest PASID table. This ensures host will have
> > > > + * latest guest pasid bindings. Caller should hold iommu_lock.
> > > > + */
> > > > +static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> > > > +VTDPASIDCacheInfo
> > > > +*pc_info) {
> > > > +VTDHostIOMMUContext *vtd_dev_icx;
> > > > +int start = 0, end = VTD_HPASID_MAX;
> > > > +vtd_pasid_table_walk_info walk_info = {.flags = 0};
> > >
> > > So vtd_pasid_table_walk_info is still used.  I thought we had
> > > reached a consensus that this can be dropped?
> >
> > yeah, I did have considered your suggestion and plan to do it. But
> > when I started coding, it looks a little bit weird to me:
> > For one, there is an input VTDPASIDCacheInfo in this function. It may
> > be nature to think about passing the parameter to further calling
> > (vtd_replay_pasid_bind_for_dev()). But, we can't do that. The
> > vtd_bus/devfn fields should be filled when looping the assigned
> > devices, not the one passed by vtd_replay_guest_pasid_bindings() caller.
> 
> Hacky way is we can directly modify VTDPASIDCacheInfo* with bus/devfn for the
> loop.  Otherwise we can duplicate the object when looping, so that we can 
> avoid
> introducing a new struct which seems to contain mostly the same information.

I see. Please see below reply.

> > For two, reusing the VTDPASIDCacheInfo for passing walk info may
> > require the final user do the same thing as what the
> > vtd_replay_guest_pasid_bindings() has done here.
> 
> I don't see it happen, could you explain?

my concern is around flags field in VTDPASIDCacheInfo. The flags not
only indicates the invalidation granularity, but also indicates the
field presence. e.g. VTD_PASID_CACHE_DEVSI indicates the vtd_bus/devfn
fields are valid. If reuse it to pass walk info to vtd_sm_pasid_table_walk_one,
it would be meaningless as vtd_bus/devfn fields are always valid. But
I'm fine to reuse it's more prefered. Instead of modifying the vtd_bus/devn
in VTDPASIDCacheInfo*, I'd rather to define another VTDPASIDCacheInfo variable
and pass it to vtd_sm_pasid_table_walk_one. This may not affect the future
caller of vtd_replay_guest_pasid_bindings() as vtd_bus/devfn field are not
designed to bring something back to caller.

struct VTDPASIDCacheInfo {
#define VTD_PASID_CACHE_FORCE_RESET(1ULL << 0)
#define VTD_PASID_CACHE_GLOBAL (1ULL << 1)
#define VTD_PASID_CACHE_DOMSI  (1ULL << 2)
#define VTD_PASID_CACHE_PASIDSI(1ULL << 3)
#define VTD_PASID_CACHE_DEVSI  (1ULL << 4)
uint32_t flags;
uint16_t domain_id;
uint32_t pasid;
VTDBus *vtd_bus;
uint16_t devfn;
}; 

> >
> > So kept the vtd_pasid_table_walk_info.
> 
> [...]
> 
> > > > +/**
> > > > + * This function syncs the pasid bindings between guest and host.
> > > > + * It includes updating the pasid cache in vIOMMU and updating
> > > > +the
> > > > + * pasid bindings per guest's latest pasid entry presence.
> > > > + */
> > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > > > + VTDPASIDCacheInfo *pc_info) {
> > > > +/*
> > > > + * Regards to a pasid cache invalidation, e.g. a PSI.
> > > > + * it could be either cases of below:
> > > > + * a) a present pasid entry moved to non-present
> > > > + * b) a present pasid entry to be a present entry
> > > > + * c) a non-present pasid entry moved to present
> > > > + *
> > > > + * Different invalidation granularity may affect different device
> > > > + * scope and pasid scope. But for each invalidation granularity,
> > > > + * it needs to do two steps to sync host and guest pasid binding.
> > > > + *
> > > > + * Here is the handling of a PSI:
> > > > + * 1) loop all the existing vtd_pasid_as instances to update them
> > > > + *according to the latest guest pasid entry in pasid table.
> > > > + *this will make sure affected existing vtd_pasid_as instances
> > > > + *cached the latest pasid entries. Also, during the loop, the
> > > > + *host should be notified if needed. e.g. pasid unbind or pasid
> > > > + *update. Should be able to cover case a) and case b).
> > > > + *
> > > > + * 2) loop all devices to cover case c)
> > > > + *- For devices which have HostIOMMUContext instances,
> > > > + *  we loop them and check if guest pasid entry exists. If yes,
> > > > + *  it is case c), we update the pasid cache and also notify
> > > > + *  host.
> > > > + *- For devices which have no HostIOMMUContext, it is not
> > > > +   

Re: [PATCH v1 5/5] i386: Hyper-V VMBus ACPI DSDT entry

[Jon Doron]

Thank you Maciej, I based it on top of what Denis (d...@openvz.org) gave me
which was this:
https://ftp.openvz.org/virtuozzo/releases/openvz-7.0.12-288/source/SRPMS/q/qemu-kvm-vz-2.12.0-33.vz7.14.4.src.rpm

Do you think you have a more recent version I dont mind diffing and
resubmitting a new version of the patchset?

Thanks,
-- Jon.

On Fri, Apr 3, 2020 at 5:56 PM Maciej S. Szmigiero
 wrote:
>
> Hi Jon,
>
> On 03.04.2020 16:23, Jon Doron wrote:
> > Guest OS uses ACPI to discover vmbus presence.  Add a corresponding
> > entry to DSDT in case vmbus has been enabled.
> >
> > Experimentally Windows guests were found to require this entry to
> > include two IRQ resources, so this patch adds two semi-arbitrarily
> > chosen ones (7 and 13).  This results, in particular, in parallel port
> > conflicting with vmbus.
> >
> > TODO: discover and use spare IRQs to avoid conflicts.
> >
> > Signed-off-by: Evgeny Yakovlev 
> > Signed-off-by: Roman Kagan 
> > Signed-off-by: Jon Doron 
>
> Nice work, thanks!
>
> However, it seems to be based on the code version that was posted in
> February 2018, and not the latest version in OpenVZ qemu repository
> dated October 2019:
> https://src.openvz.org/projects/UP/repos/qemu/commits?until=refs%2Fheads%2Fvmbus
>
> This newer version has slightly different API here and there.
> Any particular reason for selecting that older version for porting?
>
> I have actually rebased this latest version on the top of the current
> QEMU master, and it basically seems to work fine.
> However, I haven't done extensive tests whether there isn't a memory leak
> somewhere or so on.
>
> Maciej

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> 03.04.2020 14:29, Vladimir Sementsov-Ogievskiy wrote:
> > 03.04.2020 14:23, Peter Krempa wrote:
> > > On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy 
> > > wrote:
> > > > 19.12.2019 13:36, Peter Krempa wrote:
> > > > > On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy 
> > > > > wrote:
> > > > > > Hi all!
> > > > > > 
> > > > > > It's a continuation for
> > > > > > "bitmap migration bug with -drive while block mirror runs"
> > > > > > <315cff78-dcdb-a3ce-2742-da3cc9f0c...@redhat.com>
> > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
> > > > > > 
> > > > > > The problem is that bitmaps migrated to node with same node-name or
> > > > > > blk-parent name. And currently only the latter actually work in 
> > > > > > libvirt.
> > > > > > And with mirror-top filter it doesn't work, because
> > > > > > bdrv_get_device_or_node_name don't go through filters.
> > > > > 
> > > > > I want to point out that since libvirt-5.10 we use -blockdev to
> > > > > configure the backend of storage devices with qemu-4.2 and later. This
> > > > > means unfortunately that the BlockBackend of the drive does not have a
> > > > > name any more and thus the above will not work even if you make the
> > > > > lookup code to see through filters.
> > > > 
> > > > Not that this series doesn't make things worse, as it loops through 
> > > > named
> > > > block backends when trying to use their name for migration. So with 
> > > > these
> > > > patches applied, qemu will just work in more possible scenarios.
> > > 
> > > Okay, if that's so it's fair enough in this case.
> > > 
> > > I'm just very firmly against baking in the assumption that
> > > node names mean the same thing accross migration, because that will
> > > create a precedent situation and more stuff may be baked in on top of
> > > this in the future. It seems that it has already happened though and
> > > it's wrong. And the worst part is that it's never mentioned that this
> > > might occur. But again, don't do that and preferrably remove the
> > > matching of node names for bitmaps altogether until we can control it
> > > arbitrarily.
> > > 
> > > We've also seen this already before with the backend name of memory
> > > devices being baked in to the migration stream which creates an unwanted
> > > dependancy.
> > > 
> > 
> > Hmm. Actually, matching by node-name never worked. May be just drop it now, 
> > and allow only matching by blk-name?
> > 
> > And then (in 5.1) implement special qmp commands for precise mapping.
> > 
> 
> Hmm, it may break someones setup... Bad idea. Probably we can forbid 
> auto-generated node-names.

If we want to remove it I guess we have to go through a proper
deprecation; but that's OK.

The thing to keep in mind is that when people say 'the commandline
should match' on source/destination - that's just not true;
so we have to define what actually needs to stay the same for bitmap
migration to work.

Dave

> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v4 for-5.0] configure: warn if not using a separate build directory

[BALATON Zoltan]

On Fri, 3 Apr 2020, Eric Blake wrote:

On 4/3/20 8:53 AM, Daniel P. Berrangé wrote:

Running configure directly from the source directory is a build
configuration that will go away in future. It is also not currently
covered by any automated testing. Display a deprecation warning if
the user attempts to use an in-srcdir build setup, so that they are
aware that they're building QEMU in an undesirable manner.

Reviewed-by: Eric Blake 
Reviewed-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---

Changed in v4:
   - Adopted Eric's suggested wording



+if test "$in_srcdir" = "yes"; then
+echo
+echo "WARNING: SUPPORT FOR BUILDING IN THE SOURCE DIR IS DEPRECATED"
+echo
+echo "Support for running the 'configure' script directly from the"
+echo "source directory is deprecated. In-tree builds are not covered"
+echo "by automated testing and thus may not correctly build QEMU."
+echo "Users are recommended to use a separate build directory:"
+echo
+echo "  $ mkdir build"
+echo "  $ cd build"
+echo "  $ ../configure"
+echo "  $ make"


Late question, but:

Since this is just a warning, we still manage to complete the ./configure 
run, including whatever generated files it leaves in-tree. Is there any 
additional step we need to recommend prior to 'mkdir build' that will clean 
up the in-tree artifacts, so that the user then attempting the VPATH build 
won't still have a broken build due to the leftovers from the in-tree 
attempt?  'make distclean', perhaps?


/me starts testing; I'll reply back once it finishes...


You proabably also need make distclean before going to use build dir. 
Since in-tree build continues to work as before and hopefully even after 
it won't some convenience functions will take care of it without the user 
having to do it by hand I wonder if such a long warning is really needed 
in configure now. Other than uselessly annoying users, what does this 
patch achieve? The recommended way to build may change again when build 
system is replaced and I won't change my usual way until it works and 
going to just ignore this warning and I guess others who like in-tree 
builds will do the same. But I've already said that so probably won't 
mention it again.


Regards,
BALATON Zoltan

Re: [PATCH v4 1/4] target/ppc: Introduce ppc_radix64_xlate() for Radix tree translation

[Greg Kurz]

On Fri,  3 Apr 2020 16:00:53 +0200
Cédric Le Goater  wrote:

> This is moving code under a new ppc_radix64_xlate() routine shared by
> the MMU Radix page fault handler and the 'get_phys_page_debug' PPC
> callback. The difference being that 'get_phys_page_debug' does not
> generate exceptions.
> 
> The specific part of process-scoped Radix translation is moved under
> ppc_radix64_process_scoped_xlate() in preparation of the future support
> for partition-scoped Radix translation. Routines raising the exceptions
> now take a 'cause_excp' bool to cover the 'get_phys_page_debug' case.
> 
> It should be functionally equivalent.
> 
> Signed-off-by: Suraj Jitindar Singh 
> Signed-off-by: Cédric Le Goater 
> ---
>  target/ppc/mmu-radix64.c | 219 ++-
>  1 file changed, 123 insertions(+), 96 deletions(-)
> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index d2422d1c54c9..4b0d0ff50a3c 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -219,17 +219,127 @@ static bool validate_pate(PowerPCCPU *cpu, uint64_t 
> lpid, ppc_v3_pate_t *pate)
>  return true;
>  }
>  
> +static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> +vaddr eaddr, uint64_t pid,
> +ppc_v3_pate_t pate, hwaddr 
> *g_raddr,
> +int *g_prot, int *g_page_size,
> +bool cause_excp)
> +{
> +CPUState *cs = CPU(cpu);
> +uint64_t offset, size, prtbe_addr, prtbe0, pte;
> +int fault_cause = 0;
> +hwaddr pte_addr;
> +
> +/* Index Process Table by PID to Find Corresponding Process Table Entry 
> */
> +offset = pid * sizeof(struct prtb_entry);
> +size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
> +if (offset >= size) {
> +/* offset exceeds size of the process table */
> +if (cause_excp) {
> +ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> +}
> +return 1;
> +}
> +prtbe_addr = (pate.dw1 & PATE1_R_PRTB) + offset;
> +prtbe0 = ldq_phys(cs->as, prtbe_addr);
> +
> +/* Walk Radix Tree from Process Table Entry to Convert EA to RA */
> +*g_page_size = PRTBE_R_GET_RTS(prtbe0);
> +pte = ppc_radix64_walk_tree(cpu, eaddr & R_EADDR_MASK,
> +prtbe0 & PRTBE_R_RPDB, prtbe0 & PRTBE_R_RPDS,
> +g_raddr, g_page_size, _cause, 
> _addr);
> +
> +if (!(pte & R_PTE_VALID) ||

As previously discussed, this could have remained "if (!pte ||" but checking
the valid bit works too and anyway this goes away in a later patch in this
series.

Reviewed-by: Greg Kurz 


> +ppc_radix64_check_prot(cpu, rwx, pte, _cause, g_prot)) {
> +/* No valid pte or access denied due to protection */
> +if (cause_excp) {
> +ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
> +}
> +return 1;
> +}
> +
> +ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> +
> +return 0;
> +}
> +
> +static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> + bool relocation,
> + hwaddr *raddr, int *psizep, int *protp,
> + bool cause_excp)
> +{
> +uint64_t lpid = 0, pid = 0;
> +ppc_v3_pate_t pate;
> +int psize, prot;
> +hwaddr g_raddr;
> +
> +/* Virtual Mode Access - get the fully qualified address */
> +if (!ppc_radix64_get_fully_qualified_addr(>env, eaddr, , 
> )) {
> +if (cause_excp) {
> +ppc_radix64_raise_segi(cpu, rwx, eaddr);
> +}
> +return 1;
> +}
> +
> +/* Get Process Table */
> +if (cpu->vhyp) {
> +PPCVirtualHypervisorClass *vhc;
> +vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +vhc->get_pate(cpu->vhyp, );
> +} else {
> +if (!ppc64_v3_get_pate(cpu, lpid, )) {
> +if (cause_excp) {
> +ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> +}
> +return 1;
> +}
> +if (!validate_pate(cpu, lpid, )) {
> +if (cause_excp) {
> +ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_R_BADCONFIG);
> +}
> +return 1;
> +}
> +/* We don't support guest mode yet */
> +if (lpid != 0) {
> +error_report("PowerNV guest support Unimplemented");
> +exit(1);
> +}
> +}
> +
> +*psizep = INT_MAX;
> +*protp = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +
> +/*
> + * Perform process-scoped translation if relocation enabled.
> + *
> + * - Translates an effective address to a host real address in
> + *   quadrants 0 and 3 when HV=1.
> + */
> +if (relocation) {
> +int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, pid,
> +

Re: [PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Kevin Wolf]

Am 03.04.2020 um 14:42 hat Max Reitz geschrieben:
> On 03.04.20 12:44, Kevin Wolf wrote:
> > Calling blk_wait_while_drained() while blk->in_flight is increased for
> > the current request is wrong because it will cause the drain operation
> > to deadlock.
> > 
> > Many callers of blk_wait_while_drained() have already increased
> > blk->in_flight when called in a blk_aio_*() path, but can also be called
> > in synchonous code paths where blk->in_flight isn't increased. This
> > means that these calls of blk_wait_while_drained() are wrong at least in
> > some cases.
> > 
> > In order to fix this, increase blk->in_flight even for synchronous
> > operations and temporarily decrease the counter again in
> > blk_wait_while_drained().
> > 
> > Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/block-backend.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> blk_co_pdiscard() and blk_co_flush() are called from outside of
> block-backend.c (namely from mirror.c and nbd/server.c).  Is that OK?

Hm... I think you're right that the NBD server has a problem now because
we might now decrease blk->in_flight without having increased it.
(Mirror should be fine anyway because it sets disable_request_queuing.)

At first I was going to suggest that we could do the opposite of this
patch and just move the dec/wait/inc sequence (which this patch removes
for read/write) to all coroutine entry functions, so direct calls
wouldn't incorrectly decrease the counter.

But this is not what we want either, we do want to queue requests for
drained BlockBackends even in the blk_co_*() API.

Do you have another idea or do we have to turn blk_co_*() into wrappers
around the existing functions, which would gain an additional bool
parameter that tells whether we need to dec/inc or not?

Kevin


signature.asc
Description: PGP signature

Re: [PATCH v2 22/22] intel_iommu: modify x-scalable-mode to be string option

[Peter Xu]

On Sun, Mar 29, 2020 at 09:25:01PM -0700, Liu Yi L wrote:
> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
> related to scalable mode translation, thus there are multiple combinations.
> While this vIOMMU implementation wants simplify it for user by providing
> typical combinations. User could config it by "x-scalable-mode" option. The
> usage is as below:
> 
> "-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"
> 
>  - "legacy": gives support for SL page table
>  - "modern": gives support for FL page table, pasid, virtual command
>  - "off": no scalable mode support
>  -  if not configured, means no scalable mode support, if not proper
> configured, will throw error
> 
> Note: this patch is supposed to be merged when  the whole vSVA patch series
> were merged.
> 
> Cc: Kevin Tian 
> Cc: Jacob Pan 
> Cc: Peter Xu 
> Cc: Yi Sun 
> Cc: Paolo Bonzini 
> Cc: Richard Henderson 
> Cc: Eduardo Habkost 
> Signed-off-by: Liu Yi L 
> Signed-off-by: Yi Sun 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v2 19/22] intel_iommu: process PASID-based iotlb invalidation

[Peter Xu]

On Sun, Mar 29, 2020 at 09:24:58PM -0700, Liu Yi L wrote:
> This patch adds the basic PASID-based iotlb (piotlb) invalidation
> support. piotlb is used during walking Intel VT-d 1st level page
> table. This patch only adds the basic processing. Detailed handling
> will be added in next patch.
> 
> Cc: Kevin Tian 
> Cc: Jacob Pan 
> Cc: Peter Xu 
> Cc: Yi Sun 
> Cc: Paolo Bonzini 
> Cc: Richard Henderson 
> Cc: Eduardo Habkost 
> Signed-off-by: Liu Yi L 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Peter Xu]

On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote:
> This patch replays guest pasid bindings after context cache
> invalidation. This is a behavior to ensure safety. Actually,
> programmer should issue pasid cache invalidation with proper
> granularity after issuing a context cache invalidation.
> 
> Cc: Kevin Tian 
> Cc: Jacob Pan 
> Cc: Peter Xu 
> Cc: Yi Sun 
> Cc: Paolo Bonzini 
> Cc: Richard Henderson 
> Cc: Eduardo Habkost 
> Signed-off-by: Liu Yi L 
> ---
>  hw/i386/intel_iommu.c  | 51 
> ++
>  hw/i386/intel_iommu_internal.h |  6 -
>  hw/i386/trace-events   |  1 +
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index d87f608..883aeac 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -68,6 +68,10 @@ static void vtd_address_space_refresh_all(IntelIOMMUState 
> *s);
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
>  
>  static void vtd_pasid_cache_reset(IntelIOMMUState *s);
> +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> + VTDPASIDCacheInfo *pc_info);
> +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> +  VTDBus *vtd_bus, uint16_t devfn);
>  
>  static void vtd_panic_require_caching_mode(void)
>  {
> @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s)
>  
>  static void vtd_context_global_invalidate(IntelIOMMUState *s)
>  {
> +VTDPASIDCacheInfo pc_info;
> +
>  trace_vtd_inv_desc_cc_global();
> +
>  /* Protects context cache */
>  vtd_iommu_lock(s);
>  s->context_cache_gen++;
> @@ -1870,6 +1877,9 @@ static void 
> vtd_context_global_invalidate(IntelIOMMUState *s)
>   * VT-d emulation codes.
>   */
>  vtd_iommu_replay_all(s);
> +
> +pc_info.flags = VTD_PASID_CACHE_GLOBAL;
> +vtd_pasid_cache_sync(s, _info);
>  }
>  
>  /**
> @@ -2005,6 +2015,22 @@ static void 
> vtd_context_device_invalidate(IntelIOMMUState *s,
>   * happened.
>   */
>  vtd_sync_shadow_page_table(vtd_as);
> +/*
> + * Per spec, context flush should also followed with PASID
> + * cache and iotlb flush. Regards to a device selective
> + * context cache invalidation:

If context entry flush should also follow another pasid cache flush,
then this is still needed?  Shouldn't the pasid flush do the same
thing again?

> + * if (emaulted_device)
> + *modify the pasid cache gen and pasid-based iotlb gen
> + *value (will be added in following patches)

Let's avoid using "following patches" because it'll be helpless after
merged.  Also, the pasid cache gen is gone.

> + * else if (assigned_device)
> + *check if the device has been bound to any pasid
> + *invoke pasid_unbind regards to each bound pasid
> + * Here, we have vtd_pasid_cache_devsi() to invalidate pasid
> + * caches, while for piotlb in QEMU, we don't have it yet, so
> + * no handling. For assigned device, host iommu driver would
> + * flush piotlb when a pasid unbind is pass down to it.
> + */
> + vtd_pasid_cache_devsi(s, vtd_bus, devfn_it);
>  }
>  }
>  }
> @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, gpointer 
> value,
>  /* Fall through */
>  case VTD_PASID_CACHE_GLOBAL:
>  break;
> +case VTD_PASID_CACHE_DEVSI:
> +if (pc_info->vtd_bus != vtd_bus ||
> +pc_info->devfn == devfn) {

Do you mean "!="?

> +return false;
> +}
> +break;
>  default:
>  error_report("invalid pc_info->flags");
>  abort();
> @@ -2827,6 +2859,11 @@ static void 
> vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
>  walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK;
>  /* loop all assigned devices */
>  break;
> +case VTD_PASID_CACHE_DEVSI:
> +walk_info.vtd_bus = pc_info->vtd_bus;
> +walk_info.devfn = pc_info->devfn;
> +vtd_replay_pasid_bind_for_dev(s, start, end, _info);
> +return;
>  case VTD_PASID_CACHE_FORCE_RESET:
>  /* For force reset, no need to go further replay */
>  return;
> @@ -2912,6 +2949,20 @@ static void vtd_pasid_cache_sync(IntelIOMMUState *s,
>  vtd_iommu_unlock(s);
>  }
>  
> +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> +  VTDBus *vtd_bus, uint16_t devfn)
> +{
> +VTDPASIDCacheInfo pc_info;
> +
> +trace_vtd_pasid_cache_devsi(devfn);
> +
> +pc_info.flags = VTD_PASID_CACHE_DEVSI;
> +pc_info.vtd_bus = vtd_bus;
> +pc_info.devfn = devfn;
> +
> +vtd_pasid_cache_sync(s, 

Re: [PATCH v4 for-5.0] configure: warn if not using a separate build directory

[Eric Blake]

On 4/3/20 9:02 AM, Eric Blake wrote:

On 4/3/20 8:53 AM, Daniel P. Berrangé wrote:

Running configure directly from the source directory is a build
configuration that will go away in future. It is also not currently
covered by any automated testing. Display a deprecation warning if
the user attempts to use an in-srcdir build setup, so that they are
aware that they're building QEMU in an undesirable manner.

Reviewed-by: Eric Blake 
Reviewed-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---

Changed in v4:
   - Adopted Eric's suggested wording



+if test "$in_srcdir" = "yes"; then
+    echo
+    echo "WARNING: SUPPORT FOR BUILDING IN THE SOURCE DIR IS DEPRECATED"
+    echo
+    echo "Support for running the 'configure' script directly from the"
+    echo "source directory is deprecated. In-tree builds are not 
covered"

+    echo "by automated testing and thus may not correctly build QEMU."
+    echo "Users are recommended to use a separate build directory:"
+    echo
+    echo "  $ mkdir build"
+    echo "  $ cd build"
+    echo "  $ ../configure"
+    echo "  $ make"


Late question, but:

Since this is just a warning, we still manage to complete the 
./configure run, including whatever generated files it leaves in-tree. 
Is there any additional step we need to recommend prior to 'mkdir build' 
that will clean up the in-tree artifacts, so that the user then 
attempting the VPATH build won't still have a broken build due to the 
leftovers from the in-tree attempt?  'make distclean', perhaps?


/me starts testing; I'll reply back once it finishes...


tl;dr: 'make distclean' isn't perfect (it still leaves 2 directories 
behind), but does clean up a lot of directories and .mak files, and IS 
necessary before you can build in the subdirectory; but at least make 
warns you.  Still, I'd prefer adding that step in the warning, rather 
than getting an error several steps later.


On a fresh git checkout:

$ ./configure
...
$ git clean -dfxn
Would remove aarch64-linux-user/
Would remove aarch64-softmmu/
Would remove aarch64_be-linux-user/
Would remove alpha-linux-user/
Would remove alpha-softmmu/
Would remove arm-linux-user/
Would remove arm-softmmu/
Would remove armeb-linux-user/
Would remove config-all-disas.mak
Would remove config-host.mak
Would remove config.log
Would remove config.status
Would remove cris-linux-user/
Would remove cris-softmmu/
Would remove docs/sphinx/__pycache__/
Would remove hppa-linux-user/
Would remove hppa-softmmu/
Would remove i386-linux-user/
Would remove i386-softmmu/
Would remove linux-headers/asm
Would remove lm32-softmmu/
Would remove m68k-linux-user/
Would remove m68k-softmmu/
Would remove microblaze-linux-user/
Would remove microblaze-softmmu/
Would remove microblazeel-linux-user/
Would remove microblazeel-softmmu/
Would remove mips-linux-user/
Would remove mips-softmmu/
Would remove mips64-linux-user/
Would remove mips64-softmmu/
Would remove mips64el-linux-user/
Would remove mips64el-softmmu/
Would remove mipsel-linux-user/
Would remove mipsel-softmmu/
Would remove mipsn32-linux-user/
Would remove mipsn32el-linux-user/
Would remove moxie-softmmu/
Would remove nios2-linux-user/
Would remove nios2-softmmu/
Would remove or1k-linux-user/
Would remove or1k-softmmu/
Would remove ppc-linux-user/
Would remove ppc-softmmu/
Would remove ppc64-linux-user/
Would remove ppc64-softmmu/
Would remove ppc64abi32-linux-user/
Would remove ppc64le-linux-user/
Would remove riscv32-linux-user/
Would remove riscv32-softmmu/
Would remove riscv64-linux-user/
Would remove riscv64-softmmu/
Would remove rx-softmmu/
Would remove s390x-linux-user/
Would remove s390x-softmmu/
Would remove sh4-linux-user/
Would remove sh4-softmmu/
Would remove sh4eb-linux-user/
Would remove sh4eb-softmmu/
Would remove sparc-linux-user/
Would remove sparc-softmmu/
Would remove sparc32plus-linux-user/
Would remove sparc64-linux-user/
Would remove sparc64-softmmu/
Would remove tests/qemu-iotests/common.env
Would remove tests/qgraph/
Would remove tests/tcg/config-aarch64-linux-user.mak
Would remove tests/tcg/config-aarch64-softmmu.mak
Would remove tests/tcg/config-aarch64_be-linux-user.mak
Would remove tests/tcg/config-alpha-linux-user.mak
Would remove tests/tcg/config-alpha-softmmu.mak
Would remove tests/tcg/config-arm-linux-user.mak
Would remove tests/tcg/config-arm-softmmu.mak
Would remove tests/tcg/config-armeb-linux-user.mak
Would remove tests/tcg/config-cris-linux-user.mak
Would remove tests/tcg/config-cris-softmmu.mak
Would remove tests/tcg/config-hppa-linux-user.mak
Would remove tests/tcg/config-hppa-softmmu.mak
Would remove tests/tcg/config-i386-linux-user.mak
Would remove tests/tcg/config-i386-softmmu.mak
Would remove tests/tcg/config-lm32-softmmu.mak
Would remove tests/tcg/config-m68k-linux-user.mak
Would remove tests/tcg/config-m68k-softmmu.mak
Would remove tests/tcg/config-mips-linux-user.mak
Would remove 

[PATCH v2] nvdimm-utils: clean up headers and add license comment

[Igor Mammedov]

Fixes: 3f350f6bb36233be50fc2bc18dc78b6a948a5dbe
Reported-by: Peter Maydell 
Signed-off-by: Igor Mammedov 
---
v2:
  - add license blob to header as well
  - trim comment a litle bit to remove unrelated NFIT/PMEM sentences
---
 include/qemu/nvdimm-utils.h | 23 +--
 util/nvdimm-utils.c | 23 +++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/qemu/nvdimm-utils.h b/include/qemu/nvdimm-utils.h
index 4b8b198ba7..2d8cde085c 100644
--- a/include/qemu/nvdimm-utils.h
+++ b/include/qemu/nvdimm-utils.h
@@ -1,7 +1,26 @@
 #ifndef NVDIMM_UTILS_H
 #define NVDIMM_UTILS_H
-
-#include "qemu/osdep.h"
+/*
+ * NVDIMM utilities
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */
 
 GSList *nvdimm_get_device_list(void);
 #endif
diff --git a/util/nvdimm-utils.c b/util/nvdimm-utils.c
index 5cc768ca47..3d570c3c3e 100644
--- a/util/nvdimm-utils.c
+++ b/util/nvdimm-utils.c
@@ -1,3 +1,26 @@
+/*
+ * NVDIMM utilities
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */
+
+#include "qemu/osdep.h"
 #include "qemu/nvdimm-utils.h"
 #include "hw/mem/nvdimm.h"
 
-- 
2.18.1