Re: [Qemu-devel] [PATCH v2 1/1] migration: calculate expected_downtime with ram_bytes_remaining()

[David Gibson]

On Thu, Apr 19, 2018 at 12:24:04PM +0100, Dr. David Alan Gilbert wrote:
> * Balamuruhan S (bal...@linux.vnet.ibm.com) wrote:
> > On Wed, Apr 18, 2018 at 09:36:33AM +0100, Dr. David Alan Gilbert wrote:
> > > * Balamuruhan S (bal...@linux.vnet.ibm.com) wrote:
> > > > On Wed, Apr 18, 2018 at 10:57:26AM +1000, David Gibson wrote:
> > > > > On Wed, Apr 18, 2018 at 10:55:50AM +1000, David Gibson wrote:
> > > > > > On Tue, Apr 17, 2018 at 06:53:17PM +0530, Balamuruhan S wrote:
> > > > > > > expected_downtime value is not accurate with dirty_pages_rate * 
> > > > > > > page_size,
> > > > > > > using ram_bytes_remaining would yeild it correct.
> > > > > > 
> > > > > > This commit message hasn't been changed since v1, but the patch is
> > > > > > doing something completely different.  I think most of the info from
> > > > > > your cover letter needs to be in here.
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Balamuruhan S 
> > > > > > > ---
> > > > > > >  migration/migration.c | 6 +++---
> > > > > > >  migration/migration.h | 1 +
> > > > > > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > > > > index 52a5092add..4d866bb920 100644
> > > > > > > --- a/migration/migration.c
> > > > > > > +++ b/migration/migration.c
> > > > > > > @@ -614,7 +614,7 @@ static void populate_ram_info(MigrationInfo 
> > > > > > > *info, MigrationState *s)
> > > > > > >  }
> > > > > > >  
> > > > > > >  if (s->state != MIGRATION_STATUS_COMPLETED) {
> > > > > > > -info->ram->remaining = ram_bytes_remaining();
> > > > > > > +info->ram->remaining = s->ram_bytes_remaining;
> > > > > > >  info->ram->dirty_pages_rate = 
> > > > > > > ram_counters.dirty_pages_rate;
> > > > > > >  }
> > > > > > >  }
> > > > > > > @@ -2227,6 +2227,7 @@ static void 
> > > > > > > migration_update_counters(MigrationState *s,
> > > > > > >  transferred = qemu_ftell(s->to_dst_file) - 
> > > > > > > s->iteration_initial_bytes;
> > > > > > >  time_spent = current_time - s->iteration_start_time;
> > > > > > >  bandwidth = (double)transferred / time_spent;
> > > > > > > +s->ram_bytes_remaining = ram_bytes_remaining();
> > > > > > >  s->threshold_size = bandwidth * s->parameters.downtime_limit;
> > > > > > >  
> > > > > > >  s->mbps = (((double) transferred * 8.0) /
> > > > > > > @@ -2237,8 +2238,7 @@ static void 
> > > > > > > migration_update_counters(MigrationState *s,
> > > > > > >   * recalculate. 1 is a small enough number for our 
> > > > > > > purposes
> > > > > > >   */
> > > > > > >  if (ram_counters.dirty_pages_rate && transferred > 1) {
> > > > > > > -s->expected_downtime = ram_counters.dirty_pages_rate *
> > > > > > > -qemu_target_page_size() / bandwidth;
> > > > > > > +s->expected_downtime = s->ram_bytes_remaining / 
> > > > > > > bandwidth;
> > > > > > >  }
> > > > > 
> > > > > ..but more importantly, I still think this change is bogus.  expected
> > > > > downtime is not the same thing as remaining ram / bandwidth.
> > > > 
> > > > I tested precopy migration of 16M HP backed P8 guest from P8 to 1G P9 
> > > > host
> > > > and observed precopy migration was infinite with expected_downtime set 
> > > > as
> > > > downtime-limit.
> > > 
> > > Did you debug why it was infinite? Which component of the calculation
> > > had gone wrong and why?
> > > 
> > > > During the discussion for Bug RH1560562, Michael Roth quoted that
> > > > 
> > > > One thing to note: in my testing I found that the "expected downtime" 
> > > > value
> > > > seems inaccurate in this scenario. To find a max downtime that allowed
> > > > migration to complete I had to divide "remaining ram" by "throughput" 
> > > > from
> > > > "info migrate" (after the initial pre-copy pass through ram, i.e. once
> > > > "dirty pages" value starts getting reported and we're just sending 
> > > > dirtied
> > > > pages).
> > > > 
> > > > Later by trying it precopy migration could able to complete with this
> > > > approach.
> > > > 
> > > > adding Michael Roth in cc.
> > > 
> > > We should try and _understand_ the rational for the change, not just go
> > > with it.  Now, remember that whatever we do is just an estimate and
> > 
> > I have made the change based on my understanding,
> > 
> > Currently the calculation is,
> > 
> > expected_downtime = (dirty_pages_rate * qemu_target_page_size) / bandwidth
> > 
> > dirty_pages_rate = No of dirty pages / time =>  its unit (1 / seconds)
> > qemu_target_page_size => its unit (bytes)
> > 
> > dirty_pages_rate * qemu_target_page_size => bytes/seconds
> > 
> > bandwidth = bytes transferred / time => bytes/seconds
> > 
> > dividing this would not be a measurement of time.
> 
> OK, that argument makes sense to me about why it feels broken; but see
> below.
> 
> > > there will be lots of cases where it's bad - so be careful what 

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[David Gibson]

On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
> Igor Mammedov  writes:
> 
> [...]
> > Series allows to configure NUMA mapping at runtime using QMP
> > interface. For that to happen it introduces a new '-preconfig' CLI option
> > which allows to pause QEMU before machine_init() is run and
> > adds new set-numa-node QMP command which in conjunction with
> > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> >
> > Later we can modify other commands to run early, for example device_add.
> > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > stage it's considered hotplug already), so SPAPR had to work around the 
> > issue.
> 
> That instance is just stupidity / laziness, I think: we consider any
> plug after machine creation a hot plug.  Real machines remain cold until
> you press the power button.  Our virtual machines should remain cold
> until they start running, i.e. with -S until the first "cont".

Makes sense to me.  As I recall, the chief problem I had here with
PAPR hotplug was that -S stopped *after* the machine_reset, which
meant the only mechanisms we really had to inform the guest of the
hardware were the hotplug mechanisms.  Sort of.  It got complicated
because of the feature negotiation system that PAPR guests have.

At present -S doesn't really operate like a "stop before hitting the
power", it instead acts more like a breakpoint on the first
instruction of the firmware.

If we were to move the -S stop to before the machine_reset, I believe
that might simplify several things in our hotplug handling code for
papr.

> I vaguely remember me asking this before, but your answer didn't make it
> into this cover letter, which gives me a pretext to ask again instead of
> looking it up in the archives: what exactly prevents us from keeping the
> machine cold enough for numa configuration until the first "cont"?
> 
> >
> > Example of configuration session:
> > $QEMU -smp 2 -preconfig ...
> >
> > QMP:
> > # get CPUs layout for current target/machine/CLI
> > -> {'execute': 'query-hotpluggable-cpus' }  
> > <- {'return': [
> >{'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
> >{'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
> >]}
> >
> > # configure 1st node
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 
> > } }  
> > <- {'return': {}}
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',   
> >'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
> >}
> > <- {'return': {}}
> >
> > # configure 2nd node
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 
> > } }
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',  
> >'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
> >}
> > <- {'return': {}}
> >
> > # [optional] verify configuration
> > -> {'execute': 'query-hotpluggable-cpus' }  
> > <- {'return': [
> >{'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 
> > 1}, ... },
> >{'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 
> > 0}, ... }
> >]}
> >
> >
> > Git tree:
> > https://github.com/imammedo/qemu.git qmp_preconfig_v3
> >
> > Ref to v1:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
> > Message-Id: <1508170976-96869-1-git-send-email-imamm...@redhat.com>
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] intel-iommu: send PSI always when notify_unmap set

[Peter Xu]

On Fri, Apr 20, 2018 at 12:57:13PM +0800, Jason Wang wrote:
> 
> 
> On 2018年04月18日 12:51, Peter Xu wrote:
> > During IOVA page table walk, there is a special case when:
> > 
> > - notify_unmap is set, meanwhile
> > - entry is invalid
> > 
> > In the past, we skip the entry always.  This is not correct.  We should
> > send UNMAP notification to registered notifiers in this case.  Otherwise
> > some stall pages will still be mapped in the host even if L1 guest
> 
> You mean 'stale' here?

Ah, yes.

> 
> > unmapped them already.
> 
> It looks like some IOTLB invalidation were missed? If not, the page should
> be unmaped during invalidation.

I'm afraid it's not the same problem, and that's why I think we need
two fixes.

Even if the guest sends the correct invalidations, it's still possible
that QEMU skips the PSI with current code.

> 
> > 
> > Without this patch, nested device assignment to L2 guests might dump
> > some errors like:
> > 
> > qemu-system-x86_64: VFIO_MAP_DMA: -17
> > qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
> >  0x7f89a920d000) = -17 (File exists)
> > 
> > To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not
> > affected by this problem).
> > 
> > Signed-off-by: Peter Xu 
> > ---
> > 
> > To test nested assignment, one also needs to apply below patchset:
> > https://lkml.org/lkml/2018/4/18/5
> 
> Have a quick glance at it, looks like there's no need for this patch is we
> had that kernel patch applied.

Even if guest kernel applies above fix, QEMU might still drop some of
the PSIs.  Above error messages were captured with kernel patches
applied already (otherwise we see no error messages, we directly miss
them from guest).

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] intel-iommu: send PSI always when notify_unmap set

[Jason Wang]

On 2018年04月18日 12:51, Peter Xu wrote:

During IOVA page table walk, there is a special case when:

- notify_unmap is set, meanwhile
- entry is invalid

In the past, we skip the entry always.  This is not correct.  We should
send UNMAP notification to registered notifiers in this case.  Otherwise
some stall pages will still be mapped in the host even if L1 guest


You mean 'stale' here?


unmapped them already.


It looks like some IOTLB invalidation were missed? If not, the page 
should be unmaped during invalidation.




Without this patch, nested device assignment to L2 guests might dump
some errors like:

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
 0x7f89a920d000) = -17 (File exists)

To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not
affected by this problem).

Signed-off-by: Peter Xu 
---

To test nested assignment, one also needs to apply below patchset:
https://lkml.org/lkml/2018/4/18/5


Have a quick glance at it, looks like there's no need for this patch is 
we had that kernel patch applied.


Thanks


---
  hw/i386/intel_iommu.c | 42 ++
  1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fb31de9416..b359efd6f9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -722,6 +722,15 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t 
iova, bool is_write,
  
  typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
  
+static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,

+ vtd_page_walk_hook hook_fn, void *private)
+{
+assert(hook_fn);
+trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
+entry->addr_mask, entry->perm);
+return hook_fn(entry, private);
+}
+
  /**
   * vtd_page_walk_level - walk over specific level for IOVA range
   *
@@ -781,28 +790,37 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t 
start,
   */
  entry_valid = read_cur | write_cur;
  
+entry.target_as = _space_memory;

+entry.iova = iova & subpage_mask;
+entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
+entry.addr_mask = ~subpage_mask;
+
  if (vtd_is_last_slpte(slpte, level)) {
-entry.target_as = _space_memory;
-entry.iova = iova & subpage_mask;
  /* NOTE: this is only meaningful if entry_valid == true */
  entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-entry.addr_mask = ~subpage_mask;
-entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
  if (!entry_valid && !notify_unmap) {
  trace_vtd_page_walk_skip_perm(iova, iova_next);
  goto next;
  }
-trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
-entry.addr_mask, entry.perm);
-if (hook_fn) {
-ret = hook_fn(, private);
-if (ret < 0) {
-return ret;
-}
+ret = vtd_page_walk_one(, level, hook_fn, private);
+if (ret < 0) {
+return ret;
  }
  } else {
  if (!entry_valid) {
-trace_vtd_page_walk_skip_perm(iova, iova_next);
+if (notify_unmap) {
+/*
+ * The whole entry is invalid; unmap it all.
+ * Translated address is meaningless, zero it.
+ */
+entry.translated_addr = 0x0;
+ret = vtd_page_walk_one(, level, hook_fn, private);
+if (ret < 0) {
+return ret;
+}
+} else {
+trace_vtd_page_walk_skip_perm(iova, iova_next);
+}
  goto next;
  }
  ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,

[Qemu-devel] [PATCH v3 3/3] configure: enable debug-mutex if debug enabled

[Peter Xu]

Signed-off-by: Peter Xu 
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index a80af735b2..87453edd88 100755
--- a/configure
+++ b/configure
@@ -1005,6 +1005,7 @@ for opt do
   --enable-debug)
   # Enable debugging options that aren't excessively noisy
   debug_tcg="yes"
+  debug_mutex="yes"
   debug="yes"
   strip_opt="no"
   fortify_source="no"
-- 
2.14.3

[Qemu-devel] [PATCH v3 2/3] QemuMutex: support --enable-debug-mutex

[Peter Xu]

We have had some tracing tools for mutex but it's not easy to use them
for e.g. dead locks.  Let's provide "--enable-debug-mutex" parameter
when configure to allow QemuMutex to store the last owner that took
specific lock.  It will be easy to use this tool to debug deadlocks
since we can directly know who took the lock then as long as we can have
a debugger attached to the process.

Signed-off-by: Peter Xu 
---
 configure   | 10 ++
 include/qemu/thread-posix.h |  4 
 include/qemu/thread-win32.h |  4 
 util/qemu-thread-common.c   |  8 
 4 files changed, 26 insertions(+)

diff --git a/configure b/configure
index 0a19b033bc..a80af735b2 100755
--- a/configure
+++ b/configure
@@ -451,6 +451,7 @@ jemalloc="no"
 replication="yes"
 vxhs=""
 libxml2=""
+debug_mutex="no"
 
 supported_cpu="no"
 supported_os="no"
@@ -1374,6 +1375,10 @@ for opt do
   ;;
   --disable-git-update) git_update=no
   ;;
+  --enable-debug-mutex) debug_mutex=yes
+  ;;
+  --disable-debug-mutex) debug_mutex=no
+  ;;
   *)
   echo "ERROR: unknown option $opt"
   echo "Try '$0 --help' for more information"
@@ -1631,6 +1636,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   crypto-afalgLinux AF_ALG crypto backend driver
   vhost-user  vhost-user support
   capstonecapstone disassembler support
+  debug-mutex mutex debugging support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5874,6 +5880,7 @@ echo "avx2 optimization $avx2_opt"
 echo "replication support $replication"
 echo "VxHS block device $vxhs"
 echo "capstone  $capstone"
+echo "mutex debugging   $debug_mutex"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -6602,6 +6609,9 @@ fi
 if test "$capstone" != "no" ; then
   echo "CONFIG_CAPSTONE=y" >> $config_host_mak
 fi
+if test "$debug_mutex" = "yes" ; then
+  echo "CONFIG_DEBUG_MUTEX=y" >> $config_host_mak
+fi
 
 # Hold two types of flag:
 #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name on
diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index f3f47e426f..fd27b34128 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -12,6 +12,10 @@ typedef QemuMutex QemuRecMutex;
 
 struct QemuMutex {
 pthread_mutex_t lock;
+#ifdef CONFIG_DEBUG_MUTEX
+const char *file;
+int line;
+#endif
 bool initialized;
 };
 
diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
index 3a05e3b3aa..d668d789b4 100644
--- a/include/qemu/thread-win32.h
+++ b/include/qemu/thread-win32.h
@@ -5,6 +5,10 @@
 
 struct QemuMutex {
 SRWLOCK lock;
+#ifdef CONFIG_DEBUG_MUTEX
+const char *file;
+int line;
+#endif
 bool initialized;
 };
 
diff --git a/util/qemu-thread-common.c b/util/qemu-thread-common.c
index fc1f1aa969..fb88bf111c 100644
--- a/util/qemu-thread-common.c
+++ b/util/qemu-thread-common.c
@@ -21,10 +21,18 @@ void qemu_mutex_pre_lock(QemuMutex *mutex, const char 
*file, int line)
 
 void qemu_mutex_post_lock(QemuMutex *mutex, const char *file, int line)
 {
+#ifdef CONFIG_DEBUG_MUTEX
+mutex->file = file;
+mutex->line = line;
+#endif
 trace_qemu_mutex_locked(mutex, file, line);
 }
 
 void qemu_mutex_pre_unlock(QemuMutex *mutex, const char *file, int line)
 {
+#ifdef CONFIG_DEBUG_MUTEX
+mutex->file = NULL;
+mutex->line = 0;
+#endif
 trace_qemu_mutex_unlock(mutex, file, line);
 }
-- 
2.14.3

[Qemu-devel] [PATCH v3 1/3] qemu-thread: introduce qemu-thread-common.[ch]

[Peter Xu]

Put all the shared qemu-thread implementations into these files.  The
header should be internal to qemu-thread but not for qemu-thread users.

Introduce some hooks correspondingly for the shared part.  Note that in
qemu_mutex_unlock_impl() we moved the call before unlock operation which
should make more sense.  And we don't need qemu_mutex_post_unlock() hook.

Currently the hooks only calls the tracepoints.

Signed-off-by: Peter Xu 
---
 util/qemu-thread-common.h | 23 +++
 util/qemu-thread-common.c | 30 ++
 util/qemu-thread-posix.c  | 17 +++--
 util/qemu-thread-win32.c  | 15 +++
 util/Makefile.objs|  4 ++--
 5 files changed, 69 insertions(+), 20 deletions(-)
 create mode 100644 util/qemu-thread-common.h
 create mode 100644 util/qemu-thread-common.c

diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
new file mode 100644
index 00..f3f66613e9
--- /dev/null
+++ b/util/qemu-thread-common.h
@@ -0,0 +1,23 @@
+/*
+ * Common qemu-thread implementation header file.
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Authors:
+ *  Peter Xu ,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef __QEMU_THREAD_COMMON_H__
+#define __QEMU_THREAD_COMMON_H__
+
+#include "qemu/typedefs.h"
+#include "qemu/thread.h"
+
+void qemu_mutex_pre_lock(QemuMutex *mutex, const char *file, int line);
+void qemu_mutex_post_lock(QemuMutex *mutex, const char *file, int line);
+void qemu_mutex_pre_unlock(QemuMutex *mutex, const char *file, int line);
+
+#endif
diff --git a/util/qemu-thread-common.c b/util/qemu-thread-common.c
new file mode 100644
index 00..fc1f1aa969
--- /dev/null
+++ b/util/qemu-thread-common.c
@@ -0,0 +1,30 @@
+/*
+ * Common qemu-thread implementation shared for all platforms.
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Authors:
+ *  Peter Xu ,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-thread-common.h"
+#include "trace.h"
+
+void qemu_mutex_pre_lock(QemuMutex *mutex, const char *file, int line)
+{
+trace_qemu_mutex_lock(mutex, file, line);
+}
+
+void qemu_mutex_post_lock(QemuMutex *mutex, const char *file, int line)
+{
+trace_qemu_mutex_locked(mutex, file, line);
+}
+
+void qemu_mutex_pre_unlock(QemuMutex *mutex, const char *file, int line)
+{
+trace_qemu_mutex_unlock(mutex, file, line);
+}
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index b789cf32e9..b0e7008db3 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -14,7 +14,7 @@
 #include "qemu/thread.h"
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
-#include "trace.h"
+#include "qemu-thread-common.h"
 
 static bool name_threads;
 
@@ -62,13 +62,11 @@ void qemu_mutex_lock_impl(QemuMutex *mutex, const char 
*file, const int line)
 int err;
 
 assert(mutex->initialized);
-trace_qemu_mutex_lock(mutex, file, line);
-
+qemu_mutex_pre_lock(mutex, file, line);
 err = pthread_mutex_lock(>lock);
 if (err)
 error_exit(err, __func__);
-
-trace_qemu_mutex_locked(mutex, file, line);
+qemu_mutex_post_lock(mutex, file, line);
 }
 
 int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
@@ -78,7 +76,7 @@ int qemu_mutex_trylock_impl(QemuMutex *mutex, const char 
*file, const int line)
 assert(mutex->initialized);
 err = pthread_mutex_trylock(>lock);
 if (err == 0) {
-trace_qemu_mutex_locked(mutex, file, line);
+qemu_mutex_post_lock(mutex, file, line);
 return 0;
 }
 if (err != EBUSY) {
@@ -92,11 +90,10 @@ void qemu_mutex_unlock_impl(QemuMutex *mutex, const char 
*file, const int line)
 int err;
 
 assert(mutex->initialized);
+qemu_mutex_pre_unlock(mutex, file, line);
 err = pthread_mutex_unlock(>lock);
 if (err)
 error_exit(err, __func__);
-
-trace_qemu_mutex_unlock(mutex, file, line);
 }
 
 void qemu_rec_mutex_init(QemuRecMutex *mutex)
@@ -160,9 +157,9 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, 
const char *file, con
 int err;
 
 assert(cond->initialized);
-trace_qemu_mutex_unlock(mutex, file, line);
+qemu_mutex_pre_unlock(mutex, file, line);
 err = pthread_cond_wait(>cond, >lock);
-trace_qemu_mutex_locked(mutex, file, line);
+qemu_mutex_post_lock(mutex, file, line);
 if (err)
 error_exit(err, __func__);
 }
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index ab60c0d557..f9fb0581f2 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -19,7 +19,7 @@
 #include "qemu-common.h"
 #include "qemu/thread.h"
 #include "qemu/notify.h"
-#include "trace.h"
+#include "qemu-thread-common.h"
 #include 
 
 static bool 

[Qemu-devel] [PATCH v3 0/3] qemu-thread: support --enable-debug-mutex

[Peter Xu]

v3:
- note down owner for every locking operations [Emilio]
- let Windows use it too [Emilio]
- added two more patches: patch 1 & 3.

Patch 1 generalize some common hooks for qemu-thread implementation
for both POSIX and Windows.

Patch 2 introduces the new debugging facility.

Patch 3 turns on mutex debugging automatically for "--enable-debug". I
suppose most developers are with that so we naturally benefit from it.

Please review.  Thanks.

Peter Xu (3):
  qemu-thread: introduce qemu-thread-common.[ch]
  QemuMutex: support --enable-debug-mutex
  configure: enable debug-mutex if debug enabled

 configure   | 11 +++
 include/qemu/thread-posix.h |  4 
 include/qemu/thread-win32.h |  4 
 util/qemu-thread-common.h   | 23 +++
 util/qemu-thread-common.c   | 38 ++
 util/qemu-thread-posix.c| 17 +++--
 util/qemu-thread-win32.c| 15 +++
 util/Makefile.objs  |  4 ++--
 8 files changed, 96 insertions(+), 20 deletions(-)
 create mode 100644 util/qemu-thread-common.h
 create mode 100644 util/qemu-thread-common.c

-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 0/7] avocado: Add acceptance tests parsing the Linux boot console

[Philippe Mathieu-Daudé]

Cross-posting qemu-devel + avocado-devel.

> While previously working on a Super I/O refactor, I encountered some problems
> at runtime, after building the codebase successfully and running qtests.
> I had to manually start to boot different guests and check the bootlog.
> 
> I wanted to give a try at Avocado which seems designed to simplify that kind
> of functional tests.
> 
> I applied Amador Pahim work following Cleber Rosa documentation from
> http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg03891.html,
> however I had to modify few things to parse the boot console.
> Since his work is not merged, I included it in this series.
> 
> The tests simply expect to find a string reported by Linux printk when a
> device is detected/initialized, such "ttyS0 at I/O 0x3f8 (irq = 4) is a 
> 16550A"
> and "i8042 KBD port at 0x60,0x64 irq 1" for the Super I/O chip, or such
> "registered as PCnet/PCI II 79C970A" to confirms the PCI subsystem and network
> device are correctly detected:
> 
> 
> self.assertIn(u'ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A', bootlog)
> self.assertIn(u'ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A', bootlog)
> self.assertIn(u'i8042 KBD port at 0x60,0x64 irq 1', bootlog)
> self.assertIn(u'i8042 AUX port at 0x60,0x64 irq 12', bootlog)
> 
> Example of the tests output:
> 
> $ avocado run test_linux-boot-console.py -m 
> test_linux-boot-console.py.data/parameters.yaml 
> JOB ID : 695094c9bbe8f6011226da7c2031c2c53e949910
> JOB LOG: 
> /home/phil/avocado/job-results/job-2018-04-19T13.36-695094c/job.log
>  (1/6) 
> test_linux-boot-console.py:TestAlphaClipperBoot2_6.test_boot_console;alpha-2582:
>  PASS (4.76 s)
>  (2/6) 
> test_linux-boot-console.py:TestAlphaClipperBoot2_6.test_boot_console;mips-4a72:
>  PASS (0.00 s)
>  (3/6) 
> test_linux-boot-console.py:TestMips4kcMaltaBoot2_6.test_boot_console;alpha-2582:
>  PASS (0.00 s)
>  (4/6) 
> test_linux-boot-console.py:TestMips4kcMaltaBoot2_6.test_boot_console;mips-4a72:
>  PASS (3.92 s)
>  (5/6) 
> test_linux-boot-console.py:TestMips4kcMaltaBoot3_2.test_boot_console;alpha-2582:
>  PASS (0.00 s)
>  (6/6) 
> test_linux-boot-console.py:TestMips4kcMaltaBoot3_2.test_boot_console;mips-4a72:
>  PASS (4.08 s)
> RESULTS: PASS 6 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
> CANCEL 0
> JOB TIME   : 13.31 s
> 
> Please apologize my ugly Python, this series is just a proof-of-concept :)
> I couldn't figure out how to use the @skipUnless(correct arch) decorator.

Eduardo asked me to share my first impressions after trying this
framework with QEMU.

So far it seems more designed to run tests _inside_ a qemu instance
(think user-space), eventually testing packages/scripts.

My tests are targeting the machine model itself, if the devices are
correctly instantiated and so.
The fastest approach was to check the Linux kernel bootlog, but if you
look at the TestAlphaClipperBoot console you'll see a boot "firmware" is
executed before the kernel. Casually this firmware also send information
on the console, but what if the console is not accessible?
We can use a chardev for the ioport80 POST, but some firmwares post boot
events via I2C, SPI, CAN...

To improve the testing, my idea is to use the Tracing framework.
The machine trace events would be logged in a db and avogado would
verify some of the trace events, did they occur? in the correct sequence
order? with the correct arguments? We can also check relative
timestamps, is this sequence timing fast enough?

I plan to use the gdb API to insert breakpoints and follow part
execution flow, eventually injecting (faulty) events.

The multi-arch/machines support is a bit weak yet.

I'm personally more interested in automatically testing [real world]
firmware or full machine boot process, the kind of integration testing I
can not do with qtests.

I liked the possibility to generate coredumps, or the replay function.

I also liked to be able to write a test on how a machine boots, in ~20
LOC (see TestAlphaClipperBoot2_6).

The storage API is OK to fetch a full VM image, but not a single file
like a kernel or a flash image.

Enough for my second try, good work :)

I hope this was helpful.

Regards,

Phil.



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Andrew Waterman]

On Thu, Apr 19, 2018 at 5:05 PM, Michael Clark  wrote:

>
>
> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>
>> 2018-04-19 12:43 GMT+08:00 Michael Clark :
>> > Hi Zong,
>> >
>> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
>> >>
>> >> Hi all,
>> >>
>> >> For BBL part, in fp_init at machine/minit.c,
>> >> it will clear the D and F bits of misa register, and assertion that
>> >> the bits is cleared.
>> >> But the misa is WARL register, so there is no effect for writing it,
>> >> and the assertion not be true.
>> >> So is there has necessary to do that if toolchain not support D and F
>> extension?
>> >>
>> >> For QEMU part, when writing misa, it will trigger the illegal
>> >> instruction exception, but I think that the WARL allow write behavior?
>> >
>> > QEMU in the riscv-all branch should have WARL behavior.
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>> >
>> > There is a bug in upstream. We have submitted patches to fix the issue
>> for review on the qemu-devel mailing list. The patch series will be
>> submitted for upstream review again shortly. We were holding off on the
>> series as we didn’t classify it as a “critical bug” as QEMU was in
>> soft-freeze for 2.12 and we weren’t able to get review in time to include
>> this fix in the 2.12 release.
>> >
>> > See “No traps on writes to misa,minstret,mcycle"
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
>> >
>> > The history is that there were several unimplemented CSRs that had
>> printf followed by exit. Richard Henderson said we should fix this. I
>> changed several CSRs to cause illegal instruction traps instead of calling
>> exit. That was a mistake as CSRs that don’t support write are WARL (Write
>> Any Read Legal). It was certainly better than having the simulation exit as
>> a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
>> trapping was wrong. My mistake. See here for the history:
>> >
>> > - https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211
>> c63bfe1707ec057b12f7d/target-riscv/op_helper.c
>> >
>> > The implementation in the current tree is quite different. We have
>> recently made the CSR system more modular so that with minor changes,
>> custom CPUs will be able to hook their own control and status registers.
>> >
>> > - https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstr
>> eam/target/riscv/csr.c#L780-L867
>> >
>> > See these changes:
>> >
>> > - https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a
>> 35bd3f8c0ed2e14cc96bbb7
>> > - https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb6
>> 5fdcf078fb9a8391da1d6b1
>> >
>> > We know have a flexible system that will allow implementations to hook
>> per-cpu control and status registers, and we have predicates that make CSRs
>> appear on some processor but not on others. i.e. if misa.S is not present,
>> then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but
>> sometimes trapping is the correct behaviour i.e. if the processor does not
>> implement S-mode.
>> >
>> > misa traps on write should only affect bootloaders as Supervisor’s like
>> Linux don’t yet have access to the isa register. It’s not a major issuse.
>> >
>> > Michael.
>>
>> Hi Michael,
>>
>> Thanks for the information. The new CSR system is helpful for custom
>> CPU such as ours. Thanks.
>>
>> In the future, maybe we can do something like this in BBL for flexible
>> custom platform which has own device to control the timer, ipi and so
>> on.
>>
>> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
>> assertion will has problem because the bits of misa will not be
>> cleared.
>>
>> the code piece like below:
>> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
>> clear_csr(misa, fd_mask);
>> assert(!(read_csr(misa) & fd_mask));
>>
>> I think that the assertion is not necessary even the clear misa.
>>
>
> I agree. The specification makes no guarantee that misa writes are not
> ignored so it is legal for a processor that supports FD to drop misa writes
> and the assertion will trigger on legal RISC-V implementations. That code
> piece does not support legal RISC-V implementations that can't disable F
> and D. Disabling F and D should not be asserted because it is harmless if
> an unused extension is present.
>

The problem is that BBL cannot cope with this inconsistent scenario.  If pk
is compiled with to assume no floating-point, there had better be no
floating-point.  If you remove the assertion, it will break in other ways
later during in execution.

If you don't want the assertion to fire, compile BBL to match the ISA.


> This assertion will always trigger in QEMU until we support the 'optional'
> feature to allow changes to 'misa'.
>
> Just noting this is not QEMU specifc so we should drop qemu-devel if we
> continue to discuss misa on RISC-V in bbl.
>
> Nevertheless, we do plan to 

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Andrew Waterman]

On Thu, Apr 19, 2018 at 5:11 PM, Michael Clark  wrote:

>
>
> On Fri, Apr 20, 2018 at 12:05 PM, Michael Clark  wrote:
>
>>
>>
>> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>>
>>> 2018-04-19 12:43 GMT+08:00 Michael Clark :
>>> > Hi Zong,
>>> >
>>> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
>>> >>
>>> >> Hi all,
>>> >>
>>> >> For BBL part, in fp_init at machine/minit.c,
>>> >> it will clear the D and F bits of misa register, and assertion that
>>> >> the bits is cleared.
>>> >> But the misa is WARL register, so there is no effect for writing it,
>>> >> and the assertion not be true.
>>> >> So is there has necessary to do that if toolchain not support D and F
>>> extension?
>>> >>
>>> >> For QEMU part, when writing misa, it will trigger the illegal
>>> >> instruction exception, but I think that the WARL allow write behavior?
>>> >
>>> > QEMU in the riscv-all branch should have WARL behavior.
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>>> >
>>> > There is a bug in upstream. We have submitted patches to fix the issue
>>> for review on the qemu-devel mailing list. The patch series will be
>>> submitted for upstream review again shortly. We were holding off on the
>>> series as we didn’t classify it as a “critical bug” as QEMU was in
>>> soft-freeze for 2.12 and we weren’t able to get review in time to include
>>> this fix in the 2.12 release.
>>> >
>>> > See “No traps on writes to misa,minstret,mcycle"
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
>>> >
>>> > The history is that there were several unimplemented CSRs that had
>>> printf followed by exit. Richard Henderson said we should fix this. I
>>> changed several CSRs to cause illegal instruction traps instead of calling
>>> exit. That was a mistake as CSRs that don’t support write are WARL (Write
>>> Any Read Legal). It was certainly better than having the simulation exit as
>>> a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
>>> trapping was wrong. My mistake. See here for the history:
>>> >
>>> > - https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211
>>> c63bfe1707ec057b12f7d/target-riscv/op_helper.c
>>> >
>>> > The implementation in the current tree is quite different. We have
>>> recently made the CSR system more modular so that with minor changes,
>>> custom CPUs will be able to hook their own control and status registers.
>>> >
>>> > - https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstr
>>> eam/target/riscv/csr.c#L780-L867
>>> >
>>> > See these changes:
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a
>>> 35bd3f8c0ed2e14cc96bbb7
>>> > - https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb6
>>> 5fdcf078fb9a8391da1d6b1
>>> >
>>> > We know have a flexible system that will allow implementations to hook
>>> per-cpu control and status registers, and we have predicates that make CSRs
>>> appear on some processor but not on others. i.e. if misa.S is not present,
>>> then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but
>>> sometimes trapping is the correct behaviour i.e. if the processor does not
>>> implement S-mode.
>>> >
>>> > misa traps on write should only affect bootloaders as Supervisor’s
>>> like Linux don’t yet have access to the isa register. It’s not a major
>>> issuse.
>>> >
>>> > Michael.
>>>
>>> Hi Michael,
>>>
>>> Thanks for the information. The new CSR system is helpful for custom
>>> CPU such as ours. Thanks.
>>>
>>> In the future, maybe we can do something like this in BBL for flexible
>>> custom platform which has own device to control the timer, ipi and so
>>> on.
>>>
>>> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
>>> assertion will has problem because the bits of misa will not be
>>> cleared.
>>>
>>> the code piece like below:
>>> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
>>> clear_csr(misa, fd_mask);
>>> assert(!(read_csr(misa) & fd_mask));
>>>
>>> I think that the assertion is not necessary even the clear misa.
>>>
>>
>> I agree. The specification makes no guarantee that misa writes are not
>> ignored so it is legal for a processor that supports FD to drop misa writes
>> and the assertion will trigger on legal RISC-V implementations. That code
>> piece does not support legal RISC-V implementations that can't disable F
>> and D. Disabling F and D should not be asserted because it is harmless if
>> an unused extension is present.
>>
>> This assertion will always trigger in QEMU until we support the
>> 'optional' feature to allow changes to 'misa'.
>>
>> Just noting this is not QEMU specifc so we should drop qemu-devel if we
>> continue to discuss misa on RISC-V in bbl.
>>
>> Nevertheless, we do plan to support 'misa' writes however we need to do
>> some work in translate.c to make sure that cached translations match the
>> 

Re: [Qemu-devel] [PATCH v2 00/43] fix building of tests/tcg

[Max Filippov]

On Thu, Apr 19, 2018 at 6:58 AM, Alex Bennée  wrote:
> We still have quite a bit of test code that is not built. These are:
[...]
>   * tests/tcg/xtensa
>   Needs a cross-compiler

FWIW static bare-metal cross-compiler w/o libc for xtensa core
used in test/tcg/xtensa is available here:

https://github.com/foss-xtensa/toolchain/releases/download/2018.02/x86_64-2018.02-xtensa-dc232b-elf.tar.gz

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH] docker: Rename the amd64 image as amd64-cross

[Philippe Mathieu-Daudé]

On 04/19/2018 11:57 PM, Fam Zheng wrote:
> On Thu, 04/19 23:40, Philippe Mathieu-Daudé wrote:
>> Like the other images, this one is also used to cross-compile.
>> Name it accordingly, matching directory pattern.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  tests/docker/Makefile.include   | 2 +-
>>  .../{debian-amd64.docker => debian-amd64-cross.docker}  | 0
>>  2 files changed, 1 insertion(+), 1 deletion(-)
>>  rename tests/docker/dockerfiles/{debian-amd64.docker => 
>> debian-amd64-cross.docker} (100%)
>>
>> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
>> index de87341528..a113a15f77 100644
>> --- a/tests/docker/Makefile.include
>> +++ b/tests/docker/Makefile.include
>> @@ -49,7 +49,7 @@ docker-image-debian-powerpc-cross: 
>> EXTRA_FILES:=$(SRC_PATH)/tests/docker/dockerf
>>  # Enforce dependancies for composite images
>>  docker-image-debian: docker-image-debian9
>>  docker-image-debian8-mxe: docker-image-debian8
>> -docker-image-debian-amd64: docker-image-debian9
>> +docker-image-debian-amd64-cross: docker-image-debian9
>>  docker-image-debian-armel-cross: docker-image-debian9
>>  docker-image-debian-armhf-cross: docker-image-debian9
>>  docker-image-debian-arm64-cross: docker-image-debian9
>> diff --git a/tests/docker/dockerfiles/debian-amd64.docker 
>> b/tests/docker/dockerfiles/debian-amd64-cross.docker
>> similarity index 100%
>> rename from tests/docker/dockerfiles/debian-amd64.docker
>> rename to tests/docker/dockerfiles/debian-amd64-cross.docker
> 
> But the dockerfile is written differently: it doesn't use the --cross-prefix
> configure option, plus, amd64 is usually the "native" arch making it non-cross
> build.

Indeed, the goal of this dockerfile is to let a developer build QEMU
with as much dependencies as possible, without having to install them on
his own workstation.

> What do you need this patch for beside the consistency?

I was thinking it might be easier for Alex to use these images in his
last series "fix building of tests/tcg" (having the same pattern).

On another hand I'm trying to cross-build on a arm32v7 host to be able
to natively run 32-bit qtests [1]. But for that this patch is incomplete.

[1]:
https://github.com/docker-library/official-images#architectures-other-than-amd64

Regards,

Phil.

Re: [Qemu-devel] [RFC 1/2] block/file-posix: implement bdrv_co_invalidate_cache() on Linux

[Fam Zheng]

On Fri, 04/20 11:15, Stefan Hajnoczi wrote:
> On Thu, Apr 19, 2018 at 04:13:44PM +0800, Fam Zheng wrote:
> > On Thu, 04/19 15:52, Stefan Hajnoczi wrote:
> > > On Linux posix_fadvise(POSIX_FADV_DONTNEED) invalidates pages*.  Use
> > > this to drop page cache on the destination host during shared storage
> > > migration.  This way the destination host will read the latest copy of
> > > the data and will not use stale data from the page cache.
> > > 
> > > The flow is as follows:
> > > 
> > > 1. Source host writes out all dirty pages and inactivates drives.
> > > 2. QEMU_VM_EOF is sent on migration stream.
> > > 3. Destination host invalidates caches before accessing drives.
> > > 
> > > This patch enables live migration even with -drive cache.direct=off.
> > > 
> > > * Terms and conditions may apply, please see patch for details.
> > > 
> > > Signed-off-by: Stefan Hajnoczi 
> > > ---
> > >  block/file-posix.c | 39 +++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 3794c0007a..df4f52919f 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -2236,6 +2236,42 @@ static int coroutine_fn 
> > > raw_co_block_status(BlockDriverState *bs,
> > >  return ret | BDRV_BLOCK_OFFSET_VALID;
> > >  }
> > >  
> > > +static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
> > > + Error **errp)
> > > +{
> > > +BDRVRawState *s = bs->opaque;
> > > +int ret;
> > > +
> > > +ret = fd_open(bs);
> > > +if (ret < 0) {
> > > +error_setg_errno(errp, -ret, "The file descriptor is not open");
> > > +return;
> > > +}
> > > +
> > > +if (s->open_flags & O_DIRECT) {
> > > +return; /* No host kernel page cache */
> > > +}
> > > +
> > > +#if defined(__linux__)
> > > +/* This sets the scene for the next syscall... */
> > > +ret = bdrv_co_flush(bs);
> > > +if (ret < 0) {
> > > +error_setg_errno(errp, -ret, "flush failed");
> > > +return;
> > > +}
> > > +
> > > +/* Linux does not invalidate pages that are dirty, locked, or 
> > > mmapped by a
> > > + * process.  These limitations are okay because we just fsynced the 
> > > file,
> > > + * we don't use mmap, and the file should not be in use by other 
> > > processes.
> > > + */
> > > +ret = posix_fadvise(s->fd, 0, 0, POSIX_FADV_DONTNEED);
> > > +if (ret != 0) { /* the return value is a positive errno */
> > > +error_setg_errno(errp, ret, "fadvise failed");
> > > +return;
> > > +}
> > > +#endif /* __linux__ */
> > 
> > What about the #else branch? It doesn't automatically work, I guess?
> 
> Right, no error is reported.  This is existing QEMU behavior.
> 
> If we want to change behavior then it must be done consistently (i.e. by
> auditing the other block drivers) and we need to be prepared for bug
> reports (just like file locking, it may expose interesting use cases
> that we cannot easily dismiss as wrong).  I didn't want to go there.
> 
> If there is consensus then I will change the behavior.

No need to change behavior (reporting error), at least not in this patch. But a

#else
/* TODO: ... */
#endif

to remember adding similar code to invalidate system cache on other *nix systems
cannot hurt.  E.g BSDes have posix_fadvise() too, though I have no idea if
POSIX_FADV_DONTNEED works the same.

(I'm sure there are some tricks on Windows to but do we care? :)

Fam

Re: [Qemu-devel] [RFC 1/2] block/file-posix: implement bdrv_co_invalidate_cache() on Linux

[Stefan Hajnoczi]

On Thu, Apr 19, 2018 at 10:18:33AM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > On Linux posix_fadvise(POSIX_FADV_DONTNEED) invalidates pages*.  Use
> > this to drop page cache on the destination host during shared storage
> > migration.  This way the destination host will read the latest copy of
> > the data and will not use stale data from the page cache.
> > 
> > The flow is as follows:
> > 
> > 1. Source host writes out all dirty pages and inactivates drives.
> > 2. QEMU_VM_EOF is sent on migration stream.
> > 3. Destination host invalidates caches before accessing drives.
> > 
> > This patch enables live migration even with -drive cache.direct=off.
> > 
> > * Terms and conditions may apply, please see patch for details.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  block/file-posix.c | 39 +++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 3794c0007a..df4f52919f 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2236,6 +2236,42 @@ static int coroutine_fn 
> > raw_co_block_status(BlockDriverState *bs,
> >  return ret | BDRV_BLOCK_OFFSET_VALID;
> >  }
> >  
> > +static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
> > + Error **errp)
> > +{
> > +BDRVRawState *s = bs->opaque;
> > +int ret;
> > +
> > +ret = fd_open(bs);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "The file descriptor is not open");
> > +return;
> > +}
> > +
> > +if (s->open_flags & O_DIRECT) {
> > +return; /* No host kernel page cache */
> > +}
> > +
> > +#if defined(__linux__)
> > +/* This sets the scene for the next syscall... */
> > +ret = bdrv_co_flush(bs);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "flush failed");
> > +return;
> > +}
> > +
> > +/* Linux does not invalidate pages that are dirty, locked, or mmapped 
> > by a
> > + * process.  These limitations are okay because we just fsynced the 
> > file,
> > + * we don't use mmap, and the file should not be in use by other 
> > processes.
> > + */
> > +ret = posix_fadvise(s->fd, 0, 0, POSIX_FADV_DONTNEED);
> 
> What happens if I try a migrate between two qemu's on the same host?
> (Which I, and avocado, both use for testing; I think think users
> occasionally do for QEMU updates).

The steps quoted from the commit description:

  1. Source host writes out all dirty pages and inactivates drives.
  2. QEMU_VM_EOF is sent on migration stream.
  3. Destination host invalidates caches before accessing drives.

When we reach Step 3 the source QEMU is not doing I/O (no pages are
locked).  The destination QEMU does bdrv_co_flush() so even if pages are
still dirty (that shouldn't happen since the source already drained and
flushed) they will be written out and pages will be clean.  Therefore
fadvise really invalidates all resident pages.

FWIW when writing this patch I tested with both QEMUs on the same host.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC 1/2] block/file-posix: implement bdrv_co_invalidate_cache() on Linux

[Stefan Hajnoczi]

On Thu, Apr 19, 2018 at 04:13:44PM +0800, Fam Zheng wrote:
> On Thu, 04/19 15:52, Stefan Hajnoczi wrote:
> > On Linux posix_fadvise(POSIX_FADV_DONTNEED) invalidates pages*.  Use
> > this to drop page cache on the destination host during shared storage
> > migration.  This way the destination host will read the latest copy of
> > the data and will not use stale data from the page cache.
> > 
> > The flow is as follows:
> > 
> > 1. Source host writes out all dirty pages and inactivates drives.
> > 2. QEMU_VM_EOF is sent on migration stream.
> > 3. Destination host invalidates caches before accessing drives.
> > 
> > This patch enables live migration even with -drive cache.direct=off.
> > 
> > * Terms and conditions may apply, please see patch for details.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  block/file-posix.c | 39 +++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 3794c0007a..df4f52919f 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2236,6 +2236,42 @@ static int coroutine_fn 
> > raw_co_block_status(BlockDriverState *bs,
> >  return ret | BDRV_BLOCK_OFFSET_VALID;
> >  }
> >  
> > +static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
> > + Error **errp)
> > +{
> > +BDRVRawState *s = bs->opaque;
> > +int ret;
> > +
> > +ret = fd_open(bs);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "The file descriptor is not open");
> > +return;
> > +}
> > +
> > +if (s->open_flags & O_DIRECT) {
> > +return; /* No host kernel page cache */
> > +}
> > +
> > +#if defined(__linux__)
> > +/* This sets the scene for the next syscall... */
> > +ret = bdrv_co_flush(bs);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "flush failed");
> > +return;
> > +}
> > +
> > +/* Linux does not invalidate pages that are dirty, locked, or mmapped 
> > by a
> > + * process.  These limitations are okay because we just fsynced the 
> > file,
> > + * we don't use mmap, and the file should not be in use by other 
> > processes.
> > + */
> > +ret = posix_fadvise(s->fd, 0, 0, POSIX_FADV_DONTNEED);
> > +if (ret != 0) { /* the return value is a positive errno */
> > +error_setg_errno(errp, ret, "fadvise failed");
> > +return;
> > +}
> > +#endif /* __linux__ */
> 
> What about the #else branch? It doesn't automatically work, I guess?

Right, no error is reported.  This is existing QEMU behavior.

If we want to change behavior then it must be done consistently (i.e. by
auditing the other block drivers) and we need to be prepared for bug
reports (just like file locking, it may expose interesting use cases
that we cannot easily dismiss as wrong).  I didn't want to go there.

If there is consensus then I will change the behavior.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 31/43] tests/tcg: enable building for MIPS

[Fam Zheng]

On Thu, 04/19 23:57, Philippe Mathieu-Daudé wrote:
> On 04/19/2018 02:58 PM, Peter Maydell wrote:
> > On 19 April 2018 at 18:49, Alex Bennée  wrote:
> >> Philippe Mathieu-Daudé  writes:
> >>> On 04/19/2018 10:58 AM, Alex Bennée wrote:
>  This doesn't add any additional tests but enables building the
>  multiarch tests for MIPS using docker cross compilers. We don't have a
>  cross compiler for mips64 big endian though.
> >>>
> >>> Oh we have one, using CFLAGS+=-EB
> >>>
> >>> we don't have cross libraries although.
> >>
> >> Yeah I thought the same with the ARM compilers (-mbig-endian) but it's
> >> the libraries that let us down. If, as you say, newlib gets this right I
> >> suspect we should use that to build the compilers not supported out of
> >> the box by Debian.
> > 
> > I don't think we really want to get into the business of
> > building our own cross compilers if we can avoid it...it's harder
> > than it looks and we would be essentially reinventing the wheel.
> 
> I totally agree.
> 
> I'v been looking at existing maintained images we can use to
> cross-build, so we only have to pull (download) and image and use it,
> not wasting cpu time building it.
> 
> However some images might be huge, full of things we don't need. But I
> prefer that rather than having to maintain cross toolchains.
> 
> An example I like to use is the coreboot-sdk, designed to build
> i386/amd64 binaries, it fulfills our needs. But takes 2GB...
> 
> $ docker pull coreboot/coreboot-sdk:1.47
> 1.47: Pulling from coreboot/coreboot-sdk
> 3d6aedfc3e47: Downloading [>  ]  538.2kB/1.201GB
> d23c9a72f1e5: Waiting
> 
> Another candidate is crossbuild, ~1GB
> $ docker pull multiarch/crossbuild

Makes sense to me. It is not insanely big. It might take some time to download
depending on the network but it's more likely a one-off thing compared to
building one from a handcraft dockerfile we make, which probably will involve
even more downloading time and disk usage.

Fam

Re: [Qemu-devel] [RFC for-2.13 0/7] spapr: Clean up pagesize handling

[David Gibson]

On Thu, Apr 19, 2018 at 05:30:04PM +0200, Andrea Bolognani wrote:
> On Thu, 2018-04-19 at 16:29 +1000, David Gibson wrote:
> > Currently the "pseries" machine type will (usually) advertise
> > different pagesizes to the guest when running under KVM and TCG, which
> > is not how things are supposed to work.
> > 
> > This comes from poor handling of hardware limitations which mean that
> > under KVM HV the guest is unable to use pagesizes larger than those
> > backing the guest's RAM on the host side.
> > 
> > The new scheme turns things around by having an explicit machine
> > parameter controlling the largest page size that the guest is allowed
> > to use.  This limitation applies regardless of accelerator.  When
> > we're running on KVM HV we ensure that our backing pages are adequate
> > to supply the requested guest page sizes, rather than adjusting the
> > guest page sizes based on what KVM can supply.
> > 
> > This means that in order to use hugepages in a PAPR guest it's
> > necessary to add a "cap-hpt-mps=24" machine parameter as well as
> > setting the mem-path correctly.  This is a bit more work on the user
> > and/or management side, but results in consistent behaviour so I think
> > it's worth it.
> 
> libvirt guests already need to explicitly opt-in to hugepages, so
> adding this new option automagically based on that shouldn't be too
> difficult.

Right.  We have to be a bit careful with automagic though, because
treating hugepage as a boolean is one of the problems that this
parameter is there to address.

If libvirt were to set the parameter based on the pagesize of the
hugepage mount, then it might not be consistent across a migration
(e.g. p8 to p9).  Now the new code would at least catch that and
safely fail the migration, but that might be confusing to users.

> A couple of questions:
> 
>   * I see the option accepts values 12, 16, 24 and 34, with 16
> being the default.

In fact it should accept any value >= 12, though the ones that you
list are the interesting ones.  This does mean, for example, that if
it was just set to the hugepage size on a p9, 21 (2MiB) things should
work correctly (in practice it would act identically to setting it to
16).

> I guess 34 corresponds to 1 GiB hugepages?

No, 16GiB hugepages, which is the "colossal page" size on HPT POWER
machines.  It's a simple shift, (1 << 34) == 16 GiB, 1GiB pages would
be 30 (but wouldn't let the guest do any more than 24 ~ 16 MiB in
practice).

> Also, in what scenario would 12 be used?

So RHEL, at least, generally configures ppc64 kernels to use 64kiB
pages, but 4kiB pages are still supported upstream (not sure if there
are any distros that still use that mode).  If your host uses 4kiB
pages you wouldn't be able to start a (KVM HV) guest without setting
this to 12 (or using a 64kiB hugepage mount).

>   * The name of the property suggests this setting is only relevant
> for HPT guests. libvirt doesn't really have the notion of HPT
> and RPT, and I'm not really itching to introduce it. Can we
> safely use this option for all guests, even RPT ones?

Yes.  The "hpt" in the main is meant to imply that its restriction
only applies when the guest is in HPT mode, but it can be safely set
in any mode.  In RPT mode guest and host pagesizes are independent of
each other, so we don't have to deal with this mess.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [qemu-s390x] [PATCH for-2.13] Clear mem_path if we fall back to anonymous RAM allocation

[David Gibson]

On Thu, Apr 19, 2018 at 06:08:51PM +0200, Greg Kurz wrote:
> On Thu, 19 Apr 2018 16:11:37 +0200
> David Hildenbrand  wrote:
> 
> > On 19.04.2018 15:34, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 04/19/2018 02:58 PM, Cornelia Huck wrote:  
> > >> On Thu, 19 Apr 2018 14:33:18 +0200
> > >> Igor Mammedov  wrote:
> > >>  
> > >>> On Thu, 19 Apr 2018 17:21:23 +1000
> > >>> David Gibson  wrote:
> > >>>  
> >  If the -mem-path option is set, we attempt to map the guest's RAM from 
> >  a
> >  file in the given path; it's usually used to back guest RAM with 
> >  hugepages.
> >  If we're unable to (e.g. not enough free hugepages) then we fall back 
> >  to
> >  allocating normal anonymous pages.  This behaviour can be surprising, 
> >  but a
> >  comment in allocate_system_memory_nonnuma() suggests it's legacy 
> >  behaviour
> >  we can't change.
> > 
> >  What really isn't ok, though, is that in this case we leave mem_path 
> >  set.
> >  That means functions which attempt to determine the pagesize of main 
> >  RAM
> >  can erroneously think it is hugepage based on the requested path, even
> >  though it's not.
> > 
> >  This is particular bad for the pseries machine type.  KVM HV 
> >  limitations
> >  mean the guest can't use pagesizes larger than the host page size used 
> >  to
> >  back RAM.  That means that such a fallback, rather than merely giving
> >  poorer performance that expected will cause the guest to freeze up 
> >  early in
> >  boot as it attempts to use large page mappings that can't work.
> > 
> >  This patch addresses the problem by clearing the mem_path variable 
> >  when we
> >  fall back to anonymous pages, meaning that subsequent attempts to
> >  determine the RAM page size will get an accurate result.
> > 
> >  Signed-off-by: David Gibson 
> >  ---
> >   numa.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> >  Paolo et al, as with my earlier patches adding some extensions to the
> >  helpers for determining backing page sizes, if there are no objections
> >  can I get an ack to merge this via my ppc tree?
> > 
> >  diff --git a/numa.c b/numa.c
> >  index 1116c90af9..78a869e598 100644
> >  --- a/numa.c
> >  +++ b/numa.c
> >  @@ -469,6 +469,7 @@ static void 
> >  allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> >   /* Legacy behavior: if allocation failed, fall back to
> >    * regular RAM allocation.
> >    */
> >  +mem_path = NULL;
> >   memory_region_init_ram_nomigrate(mr, owner, name, 
> >  ram_size, _fatal);
> >   }
> >   #else
> > >>>
> > >>> mem_path is also used by kvm_s390_apply_cpu_model(),
> > >>> and in ccw_init() memory is initialized before CPUs are
> 
> Something similar happens with spapr: kvm_fixup_page_sizes() calls
> qemu_getrampagesize() during CPU start, which happens before the machine
> init calls allocate_system_memory_nonnuma(). Shouldn't we allocate memory
> before calling spapr_init_cpus() in spapr_machine_init() then ?

Note that the way kvm_fixup_page_sizes() works is broken in it's own
right - this patch was actually written as a prliminary to fixing
that.

> > >>> so if QEM was started with -mem-path, then before patch
> > >>> created CPU won't have CMM enabled and print warning:
> > >>>   
> > >>>  "CMM will not be enabled because it is not compatible with hugetlbfs."
> > >>>
> > >>> and after patch it might enable CMM if we clear mem_path.
> > >>> So question is do we care about this?  
> > >>
> > >> I don't quite remember the cmm semantics here -- Christian?  
> > > 
> > > The CMMA interface does not work on large pages. I think the kernel will 
> > > react
> > > with EFAULT in some cases (cmma migration and others) so qemu will 
> > > probably fail
> > > unexpectedly. 
> > > 
> > > But this patch seems to only clear mem-path if we do not allocate at all 
> > > from
> > > hugetlbfs. So things should be ok, no?
> > > 
> > >   
> > 
> > This even looks like the right thing to me, as hugetlbfs was never
> > supported.
> > 
> 
> Unrelated to this patch, -mem-path can be passed something that doesn't sit
> in a hugetlbfs, in which case we use getpagesize()... is there a reason for
> kvm_s390_enable_cmma() to filter out this case as well ? Or should we rather
> check mem_path isn't NULL and points to a hugetlbfs ?
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.13] Clear mem_path if we fall back to anonymous RAM allocation

[David Gibson]

On Thu, Apr 19, 2018 at 06:30:37PM +0200, Greg Kurz wrote:
> On Thu, 19 Apr 2018 17:21:23 +1000
> David Gibson  wrote:
> 
> > If the -mem-path option is set, we attempt to map the guest's RAM from a
> > file in the given path; it's usually used to back guest RAM with hugepages.
> > If we're unable to (e.g. not enough free hugepages) then we fall back to
> > allocating normal anonymous pages.  This behaviour can be surprising, but a
> > comment in allocate_system_memory_nonnuma() suggests it's legacy behaviour
> > we can't change.
> > 
> > What really isn't ok, though, is that in this case we leave mem_path set.
> > That means functions which attempt to determine the pagesize of main RAM
> > can erroneously think it is hugepage based on the requested path, even
> > though it's not.
> > 
> > This is particular bad for the pseries machine type.  KVM HV limitations
> > mean the guest can't use pagesizes larger than the host page size used to
> > back RAM.  That means that such a fallback, rather than merely giving
> > poorer performance that expected will cause the guest to freeze up early in
> 
> s/that expected/than expected/

Adjusted, thanks.

> 
> > boot as it attempts to use large page mappings that can't work.
> > 
> > This patch addresses the problem by clearing the mem_path variable when we
> > fall back to anonymous pages, meaning that subsequent attempts to
> > determine the RAM page size will get an accurate result.
> > 
> > Signed-off-by: David Gibson 
> > ---
> >  numa.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > Paolo et al, as with my earlier patches adding some extensions to the
> > helpers for determining backing page sizes, if there are no objections
> > can I get an ack to merge this via my ppc tree?
> > 
> > diff --git a/numa.c b/numa.c
> > index 1116c90af9..78a869e598 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -469,6 +469,7 @@ static void allocate_system_memory_nonnuma(MemoryRegion 
> > *mr, Object *owner,
> >  /* Legacy behavior: if allocation failed, fall back to
> >   * regular RAM allocation.
> >   */
> > +mem_path = NULL;
> >  memory_region_init_ram_nomigrate(mr, owner, name, ram_size, 
> > _fatal);
> >  }
> >  #else
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC 0/2] block/file-posix: allow -drive cache.direct=off live migration

[Stefan Hajnoczi]

On Thu, Apr 19, 2018 at 11:09:53AM -0500, Eric Blake wrote:
> On 04/19/2018 02:52 AM, Stefan Hajnoczi wrote:
> > file-posix.c only supports shared storage live migration with -drive
> > cache.direct=off due to cache consistency issues.  There are two main shared
> > storage configurations: files on NFS and host block devices on SAN LUNs.
> > 
> > The problem is that QEMU starts on the destination host before the source 
> > host
> > has written everything out to the disk.  The page cache on the destination 
> > host
> > may contain stale data read when QEMU opened the image file (before 
> > migration
> > handover).  Using O_DIRECT avoids this problem but prevents users from 
> > taking
> > advantage of the host page cache.
> > 
> > Although cache=none is the recommended setting for virtualization use cases,
> > there are scenarios where cache=writeback makes sense.  If the guest has 
> > much
> > less RAM than the host or many guests share the same backing file, then the
> > host page cache can significantly improve disk I/O performance.
> > 
> > This patch series implements .bdrv_co_invalidate_cache() for 
> > block/file-posix.c
> > on Linux so that shared storage live migration works.  I have sent it as an 
> > RFC
> > because cache consistency is not binary, there are corner cases which I've
> > described in the actual patch, and this may require more discussion.
> 
> Interesting, in that the NBD list is also discussing the possible
> standardization of a NBD_CMD_CACHE command (based on existing practice
> in the xNBD implementation), and covering whether that MIGHT be worth
> doing as a thin wrapper that corresponds to posix_fadvise() semantics.
> Thus, if NBD_CMD_CACHE learns flags, we could support
> .bdrv_co_invalidate_cache() through the NBD protocol driver, in addition
> to the POSIX file driver.  Obviously, your usage invalidates the cache
> of the entire file; but does it also make sense to expose a start/length
> subset invalidation, for better exposure to posix_fadvise() semantics?

bdrv_co_invalidate_cache() is currently only used by migration before
using a file that may have been touched by the other host.  I don't
think start/length will be needed for that use case.

Can you describe how will NBD use cache invalidation?  Maybe this will
help me understand other use cases.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC 2/2] block/file-posix: verify page cache is not used

[Stefan Hajnoczi]

On Thu, Apr 19, 2018 at 10:05:47AM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > This commit is for debugging only.  Do not merge it.
> > 
> > mincore(2) checks whether pages are resident.  Use it to verify that
> > page cache has been dropped.
> > 
> > You can trigger a verification failure by mmapping the image file from
> > another process and loading a byte from a page so that it becomes
> > resident.  bdrv_co_invalidate_cache() will fail while the process is
> > alive.
> 
> It doesn't seem a bad diagnostic to keep in (with a switch to activate)
> for when we're faced with some weird corruption on some weird storage
> system.

Okay.  It's very slow to mmap an entire image file and query mincore(2)
so it needs to be off by default.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [virtio-dev] RE: [PATCH v3 6/6] vhost-user: support registering external host notifiers

[Liang, Cunming]

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, April 20, 2018 12:56 AM
> To: Liang, Cunming 
> Cc: Paolo Bonzini ; Bie, Tiwei ;
> jasow...@redhat.com; alex.william...@redhat.com; stefa...@redhat.com;
> qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Daly, Dan
> ; Tan, Jianfeng ; Wang, Zhihong
> ; Wang, Xiao W 
> Subject: Re: [virtio-dev] RE: [PATCH v3 6/6] vhost-user: support registering
> external host notifiers
> 
> On Thu, Apr 19, 2018 at 04:24:29PM +, Liang, Cunming wrote:
> >
> >
> > > -Original Message-
> > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > Sent: Thursday, April 19, 2018 11:19 PM
> > > To: Paolo Bonzini 
> > > Cc: Liang, Cunming ; Bie, Tiwei
> > > ; jasow...@redhat.com;
> > > alex.william...@redhat.com; stefa...@redhat.com;
> > > qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Daly, Dan
> > > ; Tan, Jianfeng ; Wang,
> > > Zhihong ; Wang, Xiao W
> > > 
> > > Subject: Re: [virtio-dev] RE: [PATCH v3 6/6] vhost-user: support
> > > registering external host notifiers
> > >
> > > On Thu, Apr 19, 2018 at 03:02:40PM +0200, Paolo Bonzini wrote:
> > > > On 19/04/2018 14:43, Liang, Cunming wrote:
> > > > >> 2. Memory barriers. Right now after updating the avail idx,
> > > > >> virtio does smp_wmb() and then the MMIO write. Normal hardware
> > > > >> drivers do
> > > > >> wmb() which is an sfence. Can a PCI device read bypass index
> > > > >> write and see a stale index value?
> > > > >
> > > > > A compiler barrier is enough on strongly-ordered memory
> > > > > platform. As it doesn't re-order store, PCI device won't see a stale 
> > > > > index
> value.
> > > > > But a weakly-ordered memory needs sfence.
> > > >
> > > > That is complicated then.  We need to define a feature bit and (in
> > > > the Linux driver) propagate it to vring_create_virtqueue's
> > > > weak_barrier argument.  However:
> > > >
> > > > - if we make it 1 when weak barriers are needed, the device also
> > > > needs to nack feature negotiation (not allow setting the
> > > > FEATURES_OK) if the bit is not set by the driver.
> > > >  However, that is not enough.  Live migration assumes that it is
> > > > okay to migrate a virtual machine from a source that doesn't
> > > > support a feature to a destination that supports it.
> > > >  In this case, it would assume that it is okay to migrate from
> > > > software virtio to hardware virtio.  This is wrong because the
> > > > destination would use weak barriers
> > >
> > > You can't migrate between systems with different sets of device
> > > features right now.
> > >
> > > > - if we make it 1 when strong barriers are enough, software virtio
> > > > devices needs to be updated to expose the bit.  This works,
> > > > including live migration, but updated drivers will now go slower
> > > > when run against an old device that doesn't know the feature bit.
> > > >
> > > > Maybe bump the PCI revision, so that only the new revision has the bit?
> > > >
> > > > Thanks,
> > > >
> > > > Paolo
> > >
> > > As a first step, if you want to migrate to a HW offloaded solution
> > > then you need to enable the feature.
> >
> > > It does mean it will go a bit slower when run with software, so it's
> > > only good if most systems in your cluster do have the HW offload.
> > To clarify a bit more, it's suboptimal to always use mandatory barriers for 
> > MMIO.
> Per strongly-order memory, 'weak barriers' (smp_wmb) is pretty good for MMIO.
> The tradeoff doesn't always happen, software and HW offload can align on the
> same page.
> 
> I agree to all of the above except where you say smp_wmb.
> 
> smp_wmb is for controlling SMP effects on Linux, and I suspect it will not do 
> the
> right thing on some non-Intel architectures.
> 
> The claim is I think correct for Intel/AMD platforms, and probably other 
> strongly
> ordered ones. I suspect it's incorrect for ARM and power.
> 
> Replace smp_wmb with 'asm volatile ("") on Intel' and I'll agree.

Yeah, that's more accurate. 

> 
> 
> 
> > > I think we can start by getting that working and think about ways to
> > > improve down the road.
> > >
> > >
> > > That's the usecase we designed FEATURES_OK for though, so I do
> > > think/hope it's enough and we don't need to play with revisions.
> > >
> > >
> > > --
> > > MST

Re: [Qemu-devel] [PATCH] docker: Rename the amd64 image as amd64-cross

[Fam Zheng]

On Thu, 04/19 23:40, Philippe Mathieu-Daudé wrote:
> Like the other images, this one is also used to cross-compile.
> Name it accordingly, matching directory pattern.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/docker/Makefile.include   | 2 +-
>  .../{debian-amd64.docker => debian-amd64-cross.docker}  | 0
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename tests/docker/dockerfiles/{debian-amd64.docker => 
> debian-amd64-cross.docker} (100%)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index de87341528..a113a15f77 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -49,7 +49,7 @@ docker-image-debian-powerpc-cross: 
> EXTRA_FILES:=$(SRC_PATH)/tests/docker/dockerf
>  # Enforce dependancies for composite images
>  docker-image-debian: docker-image-debian9
>  docker-image-debian8-mxe: docker-image-debian8
> -docker-image-debian-amd64: docker-image-debian9
> +docker-image-debian-amd64-cross: docker-image-debian9
>  docker-image-debian-armel-cross: docker-image-debian9
>  docker-image-debian-armhf-cross: docker-image-debian9
>  docker-image-debian-arm64-cross: docker-image-debian9
> diff --git a/tests/docker/dockerfiles/debian-amd64.docker 
> b/tests/docker/dockerfiles/debian-amd64-cross.docker
> similarity index 100%
> rename from tests/docker/dockerfiles/debian-amd64.docker
> rename to tests/docker/dockerfiles/debian-amd64-cross.docker

But the dockerfile is written differently: it doesn't use the --cross-prefix
configure option, plus, amd64 is usually the "native" arch making it non-cross
build.

What do you need this patch for beside the consistency?

Fam

Re: [Qemu-devel] [PATCH v2 31/43] tests/tcg: enable building for MIPS

[Philippe Mathieu-Daudé]

On 04/19/2018 02:58 PM, Peter Maydell wrote:
> On 19 April 2018 at 18:49, Alex Bennée  wrote:
>> Philippe Mathieu-Daudé  writes:
>>> On 04/19/2018 10:58 AM, Alex Bennée wrote:
 This doesn't add any additional tests but enables building the
 multiarch tests for MIPS using docker cross compilers. We don't have a
 cross compiler for mips64 big endian though.
>>>
>>> Oh we have one, using CFLAGS+=-EB
>>>
>>> we don't have cross libraries although.
>>
>> Yeah I thought the same with the ARM compilers (-mbig-endian) but it's
>> the libraries that let us down. If, as you say, newlib gets this right I
>> suspect we should use that to build the compilers not supported out of
>> the box by Debian.
> 
> I don't think we really want to get into the business of
> building our own cross compilers if we can avoid it...it's harder
> than it looks and we would be essentially reinventing the wheel.

I totally agree.

I'v been looking at existing maintained images we can use to
cross-build, so we only have to pull (download) and image and use it,
not wasting cpu time building it.

However some images might be huge, full of things we don't need. But I
prefer that rather than having to maintain cross toolchains.

An example I like to use is the coreboot-sdk, designed to build
i386/amd64 binaries, it fulfills our needs. But takes 2GB...

$ docker pull coreboot/coreboot-sdk:1.47
1.47: Pulling from coreboot/coreboot-sdk
3d6aedfc3e47: Downloading [>  ]  538.2kB/1.201GB
d23c9a72f1e5: Waiting

Another candidate is crossbuild, ~1GB
$ docker pull multiarch/crossbuild

Re: [Qemu-devel] [PATCH v2 09/43] docker: extend "cc" command to accept compiler

[Philippe Mathieu-Daudé]

On 04/19/2018 10:58 AM, Alex Bennée wrote:
> When calling our cross-compilation images we want to call something
> other than the default cc.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Philippe Mathieu-Daudé 

> 
> ---
> v2
>   - use arg.cc default to simplify logic
> ---
>  tests/docker/docker.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index bcc3ee2dee..59bce9f19a 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -415,6 +415,8 @@ class CcCommand(SubCommand):
>  def args(self, parser):
>  parser.add_argument("--image", "-i", required=True,
>  help="The docker image in which to run cc")
> +parser.add_argument("--cc", default="cc",
> +help="The compiler executable to call")
>  parser.add_argument("--source-path", "-s", nargs="*", dest="paths",
>  help="""Extra paths to (ro) mount into container 
> for
>  reading sources""")
> @@ -428,7 +430,7 @@ class CcCommand(SubCommand):
>  if args.paths:
>  for p in args.paths:
>  cmd += ["-v", "%s:%s:ro,z" % (p, p)]
> -cmd += [args.image, "cc"]
> +cmd += [args.image, args.cc]
>  cmd += argv
>  return Docker().command("run", cmd, args.quiet)
>  
> 

[Qemu-devel] [PATCH] docker: Rename the amd64 image as amd64-cross

[Philippe Mathieu-Daudé]

Like the other images, this one is also used to cross-compile.
Name it accordingly, matching directory pattern.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/docker/Makefile.include   | 2 +-
 .../{debian-amd64.docker => debian-amd64-cross.docker}  | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename tests/docker/dockerfiles/{debian-amd64.docker => 
debian-amd64-cross.docker} (100%)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index de87341528..a113a15f77 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -49,7 +49,7 @@ docker-image-debian-powerpc-cross: 
EXTRA_FILES:=$(SRC_PATH)/tests/docker/dockerf
 # Enforce dependancies for composite images
 docker-image-debian: docker-image-debian9
 docker-image-debian8-mxe: docker-image-debian8
-docker-image-debian-amd64: docker-image-debian9
+docker-image-debian-amd64-cross: docker-image-debian9
 docker-image-debian-armel-cross: docker-image-debian9
 docker-image-debian-armhf-cross: docker-image-debian9
 docker-image-debian-arm64-cross: docker-image-debian9
diff --git a/tests/docker/dockerfiles/debian-amd64.docker 
b/tests/docker/dockerfiles/debian-amd64-cross.docker
similarity index 100%
rename from tests/docker/dockerfiles/debian-amd64.docker
rename to tests/docker/dockerfiles/debian-amd64-cross.docker
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 00/43] fix building of tests/tcg

[Philippe Mathieu-Daudé]

> It seems I can't even build fp-test for x86_64.
> 
>   CROSS-BUILD x86_64 guest-tests with cc
> In file included from /home/rth/work/qemu/qemu/tests/fp/fp-test.c:14:0:
> /home/rth/work/qemu/qemu/include/qemu/osdep.h:30:10: fatal error:
> config-host.h: No such file or directory
>  #include "config-host.h"
>   ^~~
> compilation terminated.

Cross builds when target==host arch are often broken because the less
tested. Probably because it is easier to directly use the host, yet
fully configured.

Re: [Qemu-devel] [PATCH v2] QemuMutex: support --enable-debug-mutex

[Peter Xu]

On Thu, Apr 19, 2018 at 03:43:01PM -0400, Emilio G. Cota wrote:
> On Thu, Apr 19, 2018 at 11:13:35 +0800, Peter Xu wrote:
> > On Thu, Apr 19, 2018 at 09:56:31AM +0800, Fam Zheng wrote:
> (snip)
> > > > @@ -12,6 +12,10 @@ typedef QemuMutex QemuRecMutex;
> > > >  
> > > >  struct QemuMutex {
> > > >  pthread_mutex_t lock;
> > > > +#ifdef CONFIG_DEBUG_MUTEX
> > > > +const char *file;
> > > > +int line;
> > > > +#endif
> > > 
> > > These look quite cheap, why we need a configure option to enable it?
> > 
> > Yeah; I am not 100% sure about whether it's cheap or not, hence with
> > that.  I can remove that part if we are sure we want it always.
> 
> I can think of a few good reasons not to enable these by default.
> 
> * Adds 12 bytes to struct QemuMutex on 64-bit hosts.

Yes, I worried about this too.  Mutex is still widely used, and there
can be a large amount of locks even not used quite often but will
still consume the space.

> * Increases slightly the critical region (the assignment happens
>   once the lock has been acquired)
>   + This is measurable for a single-thread with a microbenchmark:
> Before (no --enable-debug-mutex):
> $ taskset -c 0 tests/atomic_add-bench -n 1 -m -d 10
> Parameters:
>  # of threads:  1
>  duration:  10
>  ops' range:1024
> Results:
> Duration:10 s
>  Throughput: 57.59 Mops/s
>  Throughput/thread:  57.59 Mops/s/thread
> 
> After (with --enable-debug-mutex):
> $ taskset -c 0 tests/atomic_add-bench -n 1 -m -d 10
> Parameters:
>  # of threads:  1
>  duration:  10
>  ops' range:1024
> Results:
> Duration:10 s
>  Throughput: 56.25 Mops/s
>  Throughput/thread:  56.25 Mops/s/thread
> 
> NB. The -m option for atomic_add-bench is not upstream yet, but
> feel free to cherry-pick this commit: 
>   https://github.com/cota/qemu/commit/f04f34df
> 
>   + A longer critical section can impact scalability, especially
> for large core counts.

Thanks for the testing report.  Then I think I'll keep the configure
part, and keep it off by default.

> 
> Also note that there are some alternatives to this.
> On POSIX systems when I need to debug mutexes I just revert
> 24fa904 ("qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK",
> 2015-03-10). Note that this doesn't work well with fork() in
> linux-user, but I rarely need that.

Note that this patch can even be helpful to debug unlock missing paths
(when owner of a lock forgot to release after use).  AFAIU
PTHREAD_MUTEX_ERRORCHECK won't be able to, since it only provides the
thread ID that has taken the lock (and the lock type) then we can't
tell where the lock is explicitly taken.  Or is there a way that I
missed?

> 
> Another alternative is to trace mutex_lock, that will give
> you the same info although at higher overhead (in both runtime
> and disk usage).
> 
> So I'm not against this, but please keep it configured out.
> BTW you might also want to add the file/line pair to
> qemu-thread-win32.c, or hide the configure option to Windows
> users.

Yes, I missed some other paths that will take the lock, and it won't
be hard to support Windows too.  Thanks for your input.

-- 
Peter Xu

Re: [Qemu-devel] vnc: heap-use-after-free in vnc_client_io() after vnc_disconnect_finish()

[Philippe Mathieu-Daudé]

On 04/19/2018 09:39 PM, Philippe Mathieu-Daudé wrote:
> On 04/19/2018 01:40 PM, Daniel P. Berrangé wrote:
>> On Thu, Apr 19, 2018 at 06:37:12PM +0200, Marc-André Lureau wrote:
>>> On Tue, Apr 17, 2018 at 7:52 PM, Philippe Mathieu-Daudé  
>>> wrote:
 running commit ce8d4082054519f2eaac39958edde502860a7fc6:

 qemu-system-mips -M malta -m 512 \
   -kernel vmlinux-3.2.0-4-4kc-malta \
   -append 'root=/dev/sda1 console=ttyS0' \
   -hda debian_wheezy_mips_standard.qcow2 \
   -redir tcp:5556::22 -serial stdio

 and connecting with "xtightvncviewer :0" I get when closing vnc:
>>>
>>> This is also true with other targets, ex:
>>> x86_64-softmmu/qemu-system-x86_64 -vnc :0
>>>
>>> Daniel, Gerd, are you looking at it? this looks like a 2.12 regression.
>>
>> I've not had toime to look at it - would be useful if someone can bisect
>> it at least if its a regression since 2.11
> 
> My worst bisect so far, anyway here we go:
> 
> 13e1d0e71e78a925848258391a6e616b6b5ae219 is the first bad commit
> commit 13e1d0e71e78a925848258391a6e616b6b5ae219
> Author: Daniel P. Berrange 
> Date:   Thu Feb 1 16:45:14 2018 +
> 
> ui: convert VNC server to QIONetListener
> 
> The VNC server already has the ability to listen on multiple sockets.
> Converting it to use the QIONetListener APIs though, will reduce the
> amount of code in the VNC server and improve the clarity of what is
> left.
> 
> Signed-off-by: Daniel P. Berrange 
> Message-id: 20180201164514.10330-1-berra...@redhat.com
> Signed-off-by: Gerd Hoffmann 
> 
> 
 ==27686==ERROR: AddressSanitizer: heap-use-after-free on address
 0x6313c814 at pc 0x55eed918362a bp 0x7ffdf5c36c80 sp 0x7ffdf5c36c78
 READ of size 4 at 0x6313c814 thread T0
 #0 0x55eed9183629 in vnc_client_io /source/qemu/ui/vnc.c:1549
 #1 0x55eed94ae26c in qio_channel_fd_source_dispatch
 /source/qemu/io/channel-watch.c:84
 #2 0x7f3e181860f4 in g_main_context_dispatch
 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
 #3 0x55eed95b9799 in glib_pollfds_poll 
 /source/qemu/util/main-loop.c:215
 #4 0x55eed95b9989 in os_host_main_loop_wait
 /source/qemu/util/main-loop.c:263
 #5 0x55eed95b9b5f in main_loop_wait /source/qemu/util/main-loop.c:522
 #6 0x55eed898f7dd in main_loop /source/qemu/vl.c:1943
 #7 0x55eed89a1f0b in main /source/qemu/vl.c:4734
 #8 0x7f3dfe094a86 in __libc_start_main
 (/lib/x86_64-linux-gnu/libc.so.6+0x21a86)
 #9 0x55eed8518db9 in _start
 (/build/qemu/mips-softmmu/qemu-system-mips+0x14f3db9)

 0x6313c814 is located 20 bytes inside of 75744-byte region
 [0x6313c800,0x6314efe0)
 freed by thread T0 here:
 #0 0x7f3e18b878c8 in __interceptor_free
 (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd98c8)
 #1 0x55eed9181bb0 in vnc_disconnect_finish /source/qemu/ui/vnc.c:1278
 #2 0x55eed9183225 in vnc_client_read /source/qemu/ui/vnc.c:1511
 #3 0x55eed91835a9 in vnc_client_io /source/qemu/ui/vnc.c:1541
 #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
 /source/qemu/io/channel-watch.c:84
 #5 0x7f3e181860f4 in g_main_context_dispatch
 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)

 previously allocated by thread T0 here:
 #0 0x7f3e18b87df8 in calloc
 (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9df8)
 #1 0x7f3e1818b8b0 in g_malloc0
 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518b0)
 #2 0x55eed9192aa0 in vnc_listen_io /source/qemu/ui/vnc.c:3186
 #3 0x55eed94b9fd7 in qio_net_listener_channel_func
 /source/qemu/io/net-listener.c:57
 #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
 /source/qemu/io/channel-watch.c:84
 #5 0x7f3e181860f4 in g_main_context_dispatch
 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)

 SUMMARY: AddressSanitizer: heap-use-after-free
 /source/qemu/ui/vnc.c:1549 in vnc_client_io
 Shadow bytes around the buggy address:
   0x0c6278b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
   0x0c6278c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
   0x0c6278d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
   0x0c6278e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
   0x0c6278f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
 =>0x0c627900: fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x0c627910: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x0c627920: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x0c627930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x0c627940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x0c627950: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd

 Shadow byte legend (one shadow byte 

Re: [Qemu-devel] [PATCH v2 00/43] fix building of tests/tcg

[Fam Zheng]

On Thu, 04/19 14:58, Alex Bennée wrote:
> Hi,
> 
> This is the second revision of my attempt to revive the tests/tcg
> directory. You can find the first iteration here:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg01361.html

Thanks for taking over! I cannot review all the patches, but for the Docker
part:

Acked-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2 01/43] docker: add "probe" command for configure

[Fam Zheng]

On Thu, 04/19 14:58, Alex Bennée wrote:
> From: Peter Maydell 
> 
> This is a helper function for the configure script. It replies yes,
> sudo or no to inform the user if non-interactive docker support is
> available. We trap the Exception to fail gracefully.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/docker/docker.py | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index 1246ba9578..f8267586eb 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -390,6 +390,24 @@ class ImagesCommand(SubCommand):
>  def run(self, args, argv):
>  return Docker().command("images", argv, args.quiet)
>  
> +
> +class ProbeCommand(SubCommand):
> +"""Probe if we can run docker automatically"""
> +name = "probe"
> +
> +def run(self, args, argv):
> +try:
> +docker = Docker()
> +if docker._command[0] == "docker":
> +print "yes"
> +elif docker._command[0] == "sudo":
> +print "sudo"
> +except Exception:
> +print "no"

Non-zero exit code is better in 'no' case, but I guess it's not required by
configure. Either way:

Reviewed-by: Fam Zheng 


> +
> +return
> +
> +
>  def main():
>  parser = argparse.ArgumentParser(description="A Docker helper",
>  usage="%s  ..." % os.path.basename(sys.argv[0]))
> -- 
> 2.17.0
> 

Re: [Qemu-devel] [PATCH v2 09/43] docker: extend "cc" command to accept compiler

[Fam Zheng]

On Thu, 04/19 14:58, Alex Bennée wrote:
> When calling our cross-compilation images we want to call something
> other than the default cc.
> 
> Signed-off-by: Alex Bennée 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Zong Li]

2018-04-20 8:05 GMT+08:00 Michael Clark :
>
>
> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>>
>> 2018-04-19 12:43 GMT+08:00 Michael Clark :
>> > Hi Zong,
>> >
>> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
>> >>
>> >> Hi all,
>> >>
>> >> For BBL part, in fp_init at machine/minit.c,
>> >> it will clear the D and F bits of misa register, and assertion that
>> >> the bits is cleared.
>> >> But the misa is WARL register, so there is no effect for writing it,
>> >> and the assertion not be true.
>> >> So is there has necessary to do that if toolchain not support D and F
>> >> extension?
>> >>
>> >> For QEMU part, when writing misa, it will trigger the illegal
>> >> instruction exception, but I think that the WARL allow write behavior?
>> >
>> > QEMU in the riscv-all branch should have WARL behavior.
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>> >
>> > There is a bug in upstream. We have submitted patches to fix the issue
>> > for review on the qemu-devel mailing list. The patch series will be
>> > submitted for upstream review again shortly. We were holding off on the
>> > series as we didn’t classify it as a “critical bug” as QEMU was in
>> > soft-freeze for 2.12 and we weren’t able to get review in time to include
>> > this fix in the 2.12 release.
>> >
>> > See “No traps on writes to misa,minstret,mcycle"
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
>> >
>> > The history is that there were several unimplemented CSRs that had
>> > printf followed by exit. Richard Henderson said we should fix this. I
>> > changed several CSRs to cause illegal instruction traps instead of calling
>> > exit. That was a mistake as CSRs that don’t support write are WARL (Write
>> > Any Read Legal). It was certainly better than having the simulation exit as
>> > a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
>> > trapping was wrong. My mistake. See here for the history:
>> >
>> > -
>> > https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211c63bfe1707ec057b12f7d/target-riscv/op_helper.c
>> >
>> > The implementation in the current tree is quite different. We have
>> > recently made the CSR system more modular so that with minor changes, 
>> > custom
>> > CPUs will be able to hook their own control and status registers.
>> >
>> > -
>> > https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstream/target/riscv/csr.c#L780-L867
>> >
>> > See these changes:
>> >
>> > -
>> > https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a35bd3f8c0ed2e14cc96bbb7
>> > -
>> > https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb65fdcf078fb9a8391da1d6b1
>> >
>> > We know have a flexible system that will allow implementations to hook
>> > per-cpu control and status registers, and we have predicates that make CSRs
>> > appear on some processor but not on others. i.e. if misa.S is not present,
>> > then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but
>> > sometimes trapping is the correct behaviour i.e. if the processor does not
>> > implement S-mode.
>> >
>> > misa traps on write should only affect bootloaders as Supervisor’s like
>> > Linux don’t yet have access to the isa register. It’s not a major issuse.
>> >
>> > Michael.
>>
>> Hi Michael,
>>
>> Thanks for the information. The new CSR system is helpful for custom
>> CPU such as ours. Thanks.
>>
>> In the future, maybe we can do something like this in BBL for flexible
>> custom platform which has own device to control the timer, ipi and so
>> on.
>>
>> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
>> assertion will has problem because the bits of misa will not be
>> cleared.
>>
>> the code piece like below:
>> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
>> clear_csr(misa, fd_mask);
>> assert(!(read_csr(misa) & fd_mask));
>>
>> I think that the assertion is not necessary even the clear misa.
>
>
> I agree. The specification makes no guarantee that misa writes are not
> ignored so it is legal for a processor that supports FD to drop misa writes
> and the assertion will trigger on legal RISC-V implementations. That code
> piece does not support legal RISC-V implementations that can't disable F and
> D. Disabling F and D should not be asserted because it is harmless if an
> unused extension is present.
>
> This assertion will always trigger in QEMU until we support the 'optional'
> feature to allow changes to 'misa'.
>
> Just noting this is not QEMU specifc so we should drop qemu-devel if we
> continue to discuss misa on RISC-V in bbl.
>
> Nevertheless, we do plan to support 'misa' writes however we need to do some
> work in translate.c to make sure that cached translations match the current
> state of misa. We may want to perform a tb_flush when we implement writable
> misa. We also want writable misa to be a CPU feature so we can emulate CPUs

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Zong Li]

2018-04-20 8:11 GMT+08:00 Andrew Waterman :
>
>
> On Thu, Apr 19, 2018 at 5:05 PM, Michael Clark  wrote:
>>
>>
>>
>> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>>>
>>> 2018-04-19 12:43 GMT+08:00 Michael Clark :
>>> > Hi Zong,
>>> >
>>> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
>>> >>
>>> >> Hi all,
>>> >>
>>> >> For BBL part, in fp_init at machine/minit.c,
>>> >> it will clear the D and F bits of misa register, and assertion that
>>> >> the bits is cleared.
>>> >> But the misa is WARL register, so there is no effect for writing it,
>>> >> and the assertion not be true.
>>> >> So is there has necessary to do that if toolchain not support D and F
>>> >> extension?
>>> >>
>>> >> For QEMU part, when writing misa, it will trigger the illegal
>>> >> instruction exception, but I think that the WARL allow write behavior?
>>> >
>>> > QEMU in the riscv-all branch should have WARL behavior.
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>>> >
>>> > There is a bug in upstream. We have submitted patches to fix the issue
>>> > for review on the qemu-devel mailing list. The patch series will be
>>> > submitted for upstream review again shortly. We were holding off on the
>>> > series as we didn’t classify it as a “critical bug” as QEMU was in
>>> > soft-freeze for 2.12 and we weren’t able to get review in time to include
>>> > this fix in the 2.12 release.
>>> >
>>> > See “No traps on writes to misa,minstret,mcycle"
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
>>> >
>>> > The history is that there were several unimplemented CSRs that had
>>> > printf followed by exit. Richard Henderson said we should fix this. I
>>> > changed several CSRs to cause illegal instruction traps instead of calling
>>> > exit. That was a mistake as CSRs that don’t support write are WARL (Write
>>> > Any Read Legal). It was certainly better than having the simulation exit 
>>> > as
>>> > a cpu doesn’t typically have a way to ”exit” like a C program, 
>>> > nevertheless
>>> > trapping was wrong. My mistake. See here for the history:
>>> >
>>> > -
>>> > https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211c63bfe1707ec057b12f7d/target-riscv/op_helper.c
>>> >
>>> > The implementation in the current tree is quite different. We have
>>> > recently made the CSR system more modular so that with minor changes, 
>>> > custom
>>> > CPUs will be able to hook their own control and status registers.
>>> >
>>> > -
>>> > https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstream/target/riscv/csr.c#L780-L867
>>> >
>>> > See these changes:
>>> >
>>> > -
>>> > https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a35bd3f8c0ed2e14cc96bbb7
>>> > -
>>> > https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb65fdcf078fb9a8391da1d6b1
>>> >
>>> > We know have a flexible system that will allow implementations to hook
>>> > per-cpu control and status registers, and we have predicates that make 
>>> > CSRs
>>> > appear on some processor but not on others. i.e. if misa.S is not present,
>>> > then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, 
>>> > but
>>> > sometimes trapping is the correct behaviour i.e. if the processor does not
>>> > implement S-mode.
>>> >
>>> > misa traps on write should only affect bootloaders as Supervisor’s like
>>> > Linux don’t yet have access to the isa register. It’s not a major issuse.
>>> >
>>> > Michael.
>>>
>>> Hi Michael,
>>>
>>> Thanks for the information. The new CSR system is helpful for custom
>>> CPU such as ours. Thanks.
>>>
>>> In the future, maybe we can do something like this in BBL for flexible
>>> custom platform which has own device to control the timer, ipi and so
>>> on.
>>>
>>> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
>>> assertion will has problem because the bits of misa will not be
>>> cleared.
>>>
>>> the code piece like below:
>>> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
>>> clear_csr(misa, fd_mask);
>>> assert(!(read_csr(misa) & fd_mask));
>>>
>>> I think that the assertion is not necessary even the clear misa.
>>
>>
>> I agree. The specification makes no guarantee that misa writes are not
>> ignored so it is legal for a processor that supports FD to drop misa writes
>> and the assertion will trigger on legal RISC-V implementations. That code
>> piece does not support legal RISC-V implementations that can't disable F and
>> D. Disabling F and D should not be asserted because it is harmless if an
>> unused extension is present.
>
>
> The problem is that BBL cannot cope with this inconsistent scenario.  If pk
> is compiled with to assume no floating-point, there had better be no
> floating-point.  If you remove the assertion, it will break in other ways
> later during in execution.
>
> If you don't want the assertion to fire, compile BBL to match the ISA.

It 

Re: [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[David Gibson]

On Thu, 19 Apr 2018 10:09:30 +0200
Laszlo Ersek  wrote:

> On 04/19/18 02:09, David Gibson wrote:
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> 
> Thank you -- I will replace SLOF with "openfirmware".
> 
> This also implies I shouldn't add "openbios" separately, which was
> suggested earlier by Gerd -- according to
> , OpenBIOS is another
> implementation of OFW.

Right.  Although I think OpenBIOS and SLOF support a disjoint set of
machines.  Openhackware which is (was?) used on some machines is yet
another (very partial) openfirmware implementation.

> 
>  [...]  
> 
> Right, this is about interfaces, so I'll keep "uboot".

Ok.  More specifically I guess this is about firmware <-> guest OS, and
firmware <-> interactive user interfaces.  Firmware <-> qemu interfaces
(fw_cfg, device tree) are a different question.

-- 
David Gibson 
Principal Software Engineer, Virtualization, Red Hat


pgp7OGdkmQpfl.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v1 2/2] riscv: htif: increase the priority of the htif subregion

[Michael Clark]

On Wed, Apr 18, 2018 at 10:16 PM, KONRAD Frederic <
frederic.kon...@adacore.com> wrote:

> The htif device is supposed to be mapped over an other subregion. So
> increase
> its priority to one to avoid any conflict.
>
> Here is the output of info mtree:
>
> Before:
> (qemu) info mtree
>  address-space: memory
>- (prio 0, i/o): system
>  -000f (prio 0, i/o): riscv.htif.uart
>  -00011fff (prio 0, ram): riscv.spike.bootrom
>  0200-0200 (prio 0, i/o): riscv.sifive.clint
>  8000-87ff (prio 0, ram): riscv.spike.ram
>
>  address-space: I/O
>- (prio 0, i/o): io
>
>  address-space: cpu-memory-0
>- (prio 0, i/o): system
>  -000f (prio 0, i/o): riscv.htif.uart
>  -00011fff (prio 0, ram): riscv.spike.bootrom
>  0200-0200 (prio 0, i/o): riscv.sifive.clint
>  8000-87ff (prio 0, ram): riscv.spike.ram
>
> After:
>  (qemu) info mtree
>  address-space: memory
>- (prio 0, i/o): system
>  -000f (prio 1, i/o): riscv.htif.uart
>  -00011fff (prio 0, ram): riscv.spike.bootrom
>  0200-0200 (prio 0, i/o): riscv.sifive.clint
>  8000-87ff (prio 0, ram): riscv.spike.ram
>
>  address-space: I/O
>- (prio 0, i/o): io
>
>  address-space: cpu-memory-0
>- (prio 0, i/o): system
>  -000f (prio 1, i/o): riscv.htif.uart
>  -00011fff (prio 0, ram): riscv.spike.bootrom
>  0200-0200 (prio 0, i/o): riscv.sifive.clint
>  8000-87ff (prio 0, ram): riscv.spike.ram
>
> Signed-off-by: KONRAD Frederic 
>

Reviewed-by: Michael Clark 

BTW if you like I can incorporate these into the riscv tree. e.g. here,
feel free to make a PR in GitHub:

https://github.com/riscv/riscv-qemu/tree/qemu-2.13-for-upstream

(however the riscv.org tree has a lot of changes in it so you might be
better off getting these changes merged in upstream as the review backlog
is large)

It's just that i'd like to keep the trees in sync with upstream while at
the same time incorporating as many fixes as I can in the riscv tree
(before they are in upstream) so that the riscv tree is complete for users
who want to pull a branch that includes the outstanding riscv fixes. I have
scripts that can merge mutliple branches into 'riscv-all' in the riscv
github so if you make a PR against the riscv github I can merge them in
that tree, and drop them if they get accepted upstream independently. I'm
pulling from master frequently so its fine either way.

Just i'd like the riscv tree to accumulate all of the fixes that haven't
yet made it upstream.

---
>  hw/riscv/riscv_htif.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
> index 6e687f2..48e5452 100644
> --- a/hw/riscv/riscv_htif.c
> +++ b/hw/riscv/riscv_htif.c
> @@ -253,8 +253,9 @@ HTIFState *htif_mm_init(MemoryRegion *address_space,
> MemoryRegion *main_mem,
>  htif_be_change, s, NULL, true);
>  if (address_symbol_set) {
>  memory_region_init_io(>mmio, NULL, _mm_ops, s,
> -TYPE_HTIF_UART, size);
> -memory_region_add_subregion(address_space, base, >mmio);
> +  TYPE_HTIF_UART, size);
> +memory_region_add_subregion_overlap(address_space, base,
> +>mmio, 1);
>  }
>
>  return s;
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] vnc: heap-use-after-free in vnc_client_io() after vnc_disconnect_finish()

[Philippe Mathieu-Daudé]

On 04/19/2018 01:40 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 19, 2018 at 06:37:12PM +0200, Marc-André Lureau wrote:
>> On Tue, Apr 17, 2018 at 7:52 PM, Philippe Mathieu-Daudé  
>> wrote:
>>> running commit ce8d4082054519f2eaac39958edde502860a7fc6:
>>>
>>> qemu-system-mips -M malta -m 512 \
>>>   -kernel vmlinux-3.2.0-4-4kc-malta \
>>>   -append 'root=/dev/sda1 console=ttyS0' \
>>>   -hda debian_wheezy_mips_standard.qcow2 \
>>>   -redir tcp:5556::22 -serial stdio
>>>
>>> and connecting with "xtightvncviewer :0" I get when closing vnc:
>>
>> This is also true with other targets, ex:
>> x86_64-softmmu/qemu-system-x86_64 -vnc :0
>>
>> Daniel, Gerd, are you looking at it? this looks like a 2.12 regression.
> 
> I've not had toime to look at it - would be useful if someone can bisect
> it at least if its a regression since 2.11

My worst bisect so far, anyway here we go:

13e1d0e71e78a925848258391a6e616b6b5ae219 is the first bad commit
commit 13e1d0e71e78a925848258391a6e616b6b5ae219
Author: Daniel P. Berrange 
Date:   Thu Feb 1 16:45:14 2018 +

ui: convert VNC server to QIONetListener

The VNC server already has the ability to listen on multiple sockets.
Converting it to use the QIONetListener APIs though, will reduce the
amount of code in the VNC server and improve the clarity of what is
left.

Signed-off-by: Daniel P. Berrange 
Message-id: 20180201164514.10330-1-berra...@redhat.com
Signed-off-by: Gerd Hoffmann 


>>> ==27686==ERROR: AddressSanitizer: heap-use-after-free on address
>>> 0x6313c814 at pc 0x55eed918362a bp 0x7ffdf5c36c80 sp 0x7ffdf5c36c78
>>> READ of size 4 at 0x6313c814 thread T0
>>> #0 0x55eed9183629 in vnc_client_io /source/qemu/ui/vnc.c:1549
>>> #1 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>> /source/qemu/io/channel-watch.c:84
>>> #2 0x7f3e181860f4 in g_main_context_dispatch
>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>> #3 0x55eed95b9799 in glib_pollfds_poll /source/qemu/util/main-loop.c:215
>>> #4 0x55eed95b9989 in os_host_main_loop_wait
>>> /source/qemu/util/main-loop.c:263
>>> #5 0x55eed95b9b5f in main_loop_wait /source/qemu/util/main-loop.c:522
>>> #6 0x55eed898f7dd in main_loop /source/qemu/vl.c:1943
>>> #7 0x55eed89a1f0b in main /source/qemu/vl.c:4734
>>> #8 0x7f3dfe094a86 in __libc_start_main
>>> (/lib/x86_64-linux-gnu/libc.so.6+0x21a86)
>>> #9 0x55eed8518db9 in _start
>>> (/build/qemu/mips-softmmu/qemu-system-mips+0x14f3db9)
>>>
>>> 0x6313c814 is located 20 bytes inside of 75744-byte region
>>> [0x6313c800,0x6314efe0)
>>> freed by thread T0 here:
>>> #0 0x7f3e18b878c8 in __interceptor_free
>>> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd98c8)
>>> #1 0x55eed9181bb0 in vnc_disconnect_finish /source/qemu/ui/vnc.c:1278
>>> #2 0x55eed9183225 in vnc_client_read /source/qemu/ui/vnc.c:1511
>>> #3 0x55eed91835a9 in vnc_client_io /source/qemu/ui/vnc.c:1541
>>> #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>> /source/qemu/io/channel-watch.c:84
>>> #5 0x7f3e181860f4 in g_main_context_dispatch
>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>>
>>> previously allocated by thread T0 here:
>>> #0 0x7f3e18b87df8 in calloc
>>> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9df8)
>>> #1 0x7f3e1818b8b0 in g_malloc0
>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518b0)
>>> #2 0x55eed9192aa0 in vnc_listen_io /source/qemu/ui/vnc.c:3186
>>> #3 0x55eed94b9fd7 in qio_net_listener_channel_func
>>> /source/qemu/io/net-listener.c:57
>>> #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>> /source/qemu/io/channel-watch.c:84
>>> #5 0x7f3e181860f4 in g_main_context_dispatch
>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>>
>>> SUMMARY: AddressSanitizer: heap-use-after-free
>>> /source/qemu/ui/vnc.c:1549 in vnc_client_io
>>> Shadow bytes around the buggy address:
>>>   0x0c6278b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>   0x0c6278c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>   0x0c6278d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>   0x0c6278e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>   0x0c6278f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>> =>0x0c627900: fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>   0x0c627910: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>   0x0c627920: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>   0x0c627930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>   0x0c627940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>   0x0c627950: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>
>>> Shadow byte legend (one shadow byte represents 8 application bytes):
>>>   Heap left redzone:   fa
>>>   Freed heap region:   fd
>>> ==26606==ABORTING

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Michael Clark]

On Fri, Apr 20, 2018 at 12:12 PM, Andrew Waterman  wrote:

>
>
> On Thu, Apr 19, 2018 at 5:11 PM, Michael Clark  wrote:
>
>>
>>
>> On Fri, Apr 20, 2018 at 12:05 PM, Michael Clark  wrote:
>>
>>>
>>>
>>> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>>>
 2018-04-19 12:43 GMT+08:00 Michael Clark :
 > Hi Zong,
 >
 >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
 >>
 >> Hi all,
 >>
 >> For BBL part, in fp_init at machine/minit.c,
 >> it will clear the D and F bits of misa register, and assertion that
 >> the bits is cleared.
 >> But the misa is WARL register, so there is no effect for writing it,
 >> and the assertion not be true.
 >> So is there has necessary to do that if toolchain not support D and
 F extension?
 >>
 >> For QEMU part, when writing misa, it will trigger the illegal
 >> instruction exception, but I think that the WARL allow write
 behavior?
 >
 > QEMU in the riscv-all branch should have WARL behavior.
 >
 > - https://github.com/riscv/riscv-qemu/commits/riscv-all
 >
 > There is a bug in upstream. We have submitted patches to fix the
 issue for review on the qemu-devel mailing list. The patch series will be
 submitted for upstream review again shortly. We were holding off on the
 series as we didn’t classify it as a “critical bug” as QEMU was in
 soft-freeze for 2.12 and we weren’t able to get review in time to include
 this fix in the 2.12 release.
 >
 > See “No traps on writes to misa,minstret,mcycle"
 >
 > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
 >
 > The history is that there were several unimplemented CSRs that had
 printf followed by exit. Richard Henderson said we should fix this. I
 changed several CSRs to cause illegal instruction traps instead of calling
 exit. That was a mistake as CSRs that don’t support write are WARL (Write
 Any Read Legal). It was certainly better than having the simulation exit as
 a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
 trapping was wrong. My mistake. See here for the history:
 >
 > - https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211
 c63bfe1707ec057b12f7d/target-riscv/op_helper.c
 >
 > The implementation in the current tree is quite different. We have
 recently made the CSR system more modular so that with minor changes,
 custom CPUs will be able to hook their own control and status registers.
 >
 > - https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstr
 eam/target/riscv/csr.c#L780-L867
 >
 > See these changes:
 >
 > - https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a
 35bd3f8c0ed2e14cc96bbb7
 > - https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb6
 5fdcf078fb9a8391da1d6b1
 >
 > We know have a flexible system that will allow implementations to
 hook per-cpu control and status registers, and we have predicates that make
 CSRs appear on some processor but not on others. i.e. if misa.S is not
 present, then S-mode s* CSRs will trap. Sometimes WARL is the correct
 behaviour, but sometimes trapping is the correct behaviour i.e. if the
 processor does not implement S-mode.
 >
 > misa traps on write should only affect bootloaders as Supervisor’s
 like Linux don’t yet have access to the isa register. It’s not a major
 issuse.
 >
 > Michael.

 Hi Michael,

 Thanks for the information. The new CSR system is helpful for custom
 CPU such as ours. Thanks.

 In the future, maybe we can do something like this in BBL for flexible
 custom platform which has own device to control the timer, ipi and so
 on.

 Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
 assertion will has problem because the bits of misa will not be
 cleared.

 the code piece like below:
 uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
 clear_csr(misa, fd_mask);
 assert(!(read_csr(misa) & fd_mask));

 I think that the assertion is not necessary even the clear misa.

>>>
>>> I agree. The specification makes no guarantee that misa writes are not
>>> ignored so it is legal for a processor that supports FD to drop misa writes
>>> and the assertion will trigger on legal RISC-V implementations. That code
>>> piece does not support legal RISC-V implementations that can't disable F
>>> and D. Disabling F and D should not be asserted because it is harmless if
>>> an unused extension is present.
>>>
>>> This assertion will always trigger in QEMU until we support the
>>> 'optional' feature to allow changes to 'misa'.
>>>
>>> Just noting this is not QEMU specifc so we should drop qemu-devel if we
>>> 

Re: [Qemu-devel] [PATCH v2 00/43] fix building of tests/tcg

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> I did start playing with crosstool-ng and Linaro's own ABE scripts but
> realised this could end up a massive time sync. What would be really
> helpful is if the respective maintainers could encode their EXACT
> STEPS for building their cross compilers into some docker recipes.

Building a cross-compiler with an existing libc binary is easy.
Building a cross-compiler without a libc is harder, and involves
a bit of two-stepping to get things right.

For someone who has never used docker, what's a recipe look like?
Just a shell script that gets run within a container?
Is there an opportunity to wget or local copy an existing libc
tarball/package to put us into the easy case?

> Finally the end of the series has me adding Emilio's fp-test to the
> per-target builds. Unfortunately although some osdep.h and softfloat.c
> fiddling allows us to build in most cases I still can't build for
> example an i386 fp-test on an x86_64 host using the cross compiler as
> it triggers incompatibilities with config-host.h - in this case Int128
> support. Currently I just hackily disable fp-test for non-64 bit
> platforms.

It seems I can't even build fp-test for x86_64.

  CROSS-BUILD x86_64 guest-tests with cc
In file included from /home/rth/work/qemu/qemu/tests/fp/fp-test.c:14:0:
/home/rth/work/qemu/qemu/include/qemu/osdep.h:30:10: fatal error:
config-host.h: No such file or directory
 #include "config-host.h"
  ^~~
compilation terminated.


r~

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Michael Clark]

On Fri, Apr 20, 2018 at 12:05 PM, Michael Clark  wrote:

>
>
> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:
>
>> 2018-04-19 12:43 GMT+08:00 Michael Clark :
>> > Hi Zong,
>> >
>> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
>> >>
>> >> Hi all,
>> >>
>> >> For BBL part, in fp_init at machine/minit.c,
>> >> it will clear the D and F bits of misa register, and assertion that
>> >> the bits is cleared.
>> >> But the misa is WARL register, so there is no effect for writing it,
>> >> and the assertion not be true.
>> >> So is there has necessary to do that if toolchain not support D and F
>> extension?
>> >>
>> >> For QEMU part, when writing misa, it will trigger the illegal
>> >> instruction exception, but I think that the WARL allow write behavior?
>> >
>> > QEMU in the riscv-all branch should have WARL behavior.
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>> >
>> > There is a bug in upstream. We have submitted patches to fix the issue
>> for review on the qemu-devel mailing list. The patch series will be
>> submitted for upstream review again shortly. We were holding off on the
>> series as we didn’t classify it as a “critical bug” as QEMU was in
>> soft-freeze for 2.12 and we weren’t able to get review in time to include
>> this fix in the 2.12 release.
>> >
>> > See “No traps on writes to misa,minstret,mcycle"
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
>> >
>> > The history is that there were several unimplemented CSRs that had
>> printf followed by exit. Richard Henderson said we should fix this. I
>> changed several CSRs to cause illegal instruction traps instead of calling
>> exit. That was a mistake as CSRs that don’t support write are WARL (Write
>> Any Read Legal). It was certainly better than having the simulation exit as
>> a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
>> trapping was wrong. My mistake. See here for the history:
>> >
>> > - https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211
>> c63bfe1707ec057b12f7d/target-riscv/op_helper.c
>> >
>> > The implementation in the current tree is quite different. We have
>> recently made the CSR system more modular so that with minor changes,
>> custom CPUs will be able to hook their own control and status registers.
>> >
>> > - https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstr
>> eam/target/riscv/csr.c#L780-L867
>> >
>> > See these changes:
>> >
>> > - https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a
>> 35bd3f8c0ed2e14cc96bbb7
>> > - https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb6
>> 5fdcf078fb9a8391da1d6b1
>> >
>> > We know have a flexible system that will allow implementations to hook
>> per-cpu control and status registers, and we have predicates that make CSRs
>> appear on some processor but not on others. i.e. if misa.S is not present,
>> then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but
>> sometimes trapping is the correct behaviour i.e. if the processor does not
>> implement S-mode.
>> >
>> > misa traps on write should only affect bootloaders as Supervisor’s like
>> Linux don’t yet have access to the isa register. It’s not a major issuse.
>> >
>> > Michael.
>>
>> Hi Michael,
>>
>> Thanks for the information. The new CSR system is helpful for custom
>> CPU such as ours. Thanks.
>>
>> In the future, maybe we can do something like this in BBL for flexible
>> custom platform which has own device to control the timer, ipi and so
>> on.
>>
>> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
>> assertion will has problem because the bits of misa will not be
>> cleared.
>>
>> the code piece like below:
>> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
>> clear_csr(misa, fd_mask);
>> assert(!(read_csr(misa) & fd_mask));
>>
>> I think that the assertion is not necessary even the clear misa.
>>
>
> I agree. The specification makes no guarantee that misa writes are not
> ignored so it is legal for a processor that supports FD to drop misa writes
> and the assertion will trigger on legal RISC-V implementations. That code
> piece does not support legal RISC-V implementations that can't disable F
> and D. Disabling F and D should not be asserted because it is harmless if
> an unused extension is present.
>
> This assertion will always trigger in QEMU until we support the 'optional'
> feature to allow changes to 'misa'.
>
> Just noting this is not QEMU specifc so we should drop qemu-devel if we
> continue to discuss misa on RISC-V in bbl.
>
> Nevertheless, we do plan to support 'misa' writes however we need to do
> some work in translate.c to make sure that cached translations match the
> current state of misa. We may want to perform a tb_flush when we implement
> writable misa. We also want writable misa to be a CPU feature so we can
> emulate CPUs that don't support writable misa. 

Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Michael Clark]

On Thu, Apr 19, 2018 at 9:28 PM, Zong Li  wrote:

> 2018-04-19 12:43 GMT+08:00 Michael Clark :
> > Hi Zong,
> >
> >> On 19/04/2018, at 2:40 PM, Zong Li  wrote:
> >>
> >> Hi all,
> >>
> >> For BBL part, in fp_init at machine/minit.c,
> >> it will clear the D and F bits of misa register, and assertion that
> >> the bits is cleared.
> >> But the misa is WARL register, so there is no effect for writing it,
> >> and the assertion not be true.
> >> So is there has necessary to do that if toolchain not support D and F
> extension?
> >>
> >> For QEMU part, when writing misa, it will trigger the illegal
> >> instruction exception, but I think that the WARL allow write behavior?
> >
> > QEMU in the riscv-all branch should have WARL behavior.
> >
> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
> >
> > There is a bug in upstream. We have submitted patches to fix the issue
> for review on the qemu-devel mailing list. The patch series will be
> submitted for upstream review again shortly. We were holding off on the
> series as we didn’t classify it as a “critical bug” as QEMU was in
> soft-freeze for 2.12 and we weren’t able to get review in time to include
> this fix in the 2.12 release.
> >
> > See “No traps on writes to misa,minstret,mcycle"
> >
> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream
> >
> > The history is that there were several unimplemented CSRs that had
> printf followed by exit. Richard Henderson said we should fix this. I
> changed several CSRs to cause illegal instruction traps instead of calling
> exit. That was a mistake as CSRs that don’t support write are WARL (Write
> Any Read Legal). It was certainly better than having the simulation exit as
> a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless
> trapping was wrong. My mistake. See here for the history:
> >
> > - https://github.com/riscv/riscv-qemu/blob/
> ff36f2f77ec3e6a6211c63bfe1707ec057b12f7d/target-riscv/op_helper.c
> >
> > The implementation in the current tree is quite different. We have
> recently made the CSR system more modular so that with minor changes,
> custom CPUs will be able to hook their own control and status registers.
> >
> > - https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-
> upstream/target/riscv/csr.c#L780-L867
> >
> > See these changes:
> >
> > - https://github.com/riscv/riscv-qemu/commit/
> 9d9c1bfef911c520a35bd3f8c0ed2e14cc96bbb7
> > - https://github.com/riscv/riscv-qemu/commit/
> b5a4cd79ce6c7fbb65fdcf078fb9a8391da1d6b1
> >
> > We know have a flexible system that will allow implementations to hook
> per-cpu control and status registers, and we have predicates that make CSRs
> appear on some processor but not on others. i.e. if misa.S is not present,
> then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but
> sometimes trapping is the correct behaviour i.e. if the processor does not
> implement S-mode.
> >
> > misa traps on write should only affect bootloaders as Supervisor’s like
> Linux don’t yet have access to the isa register. It’s not a major issuse.
> >
> > Michael.
>
> Hi Michael,
>
> Thanks for the information. The new CSR system is helpful for custom
> CPU such as ours. Thanks.
>
> In the future, maybe we can do something like this in BBL for flexible
> custom platform which has own device to control the timer, ipi and so
> on.
>
> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the
> assertion will has problem because the bits of misa will not be
> cleared.
>
> the code piece like below:
> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A'));
> clear_csr(misa, fd_mask);
> assert(!(read_csr(misa) & fd_mask));
>
> I think that the assertion is not necessary even the clear misa.
>

I agree. The specification makes no guarantee that misa writes are not
ignored so it is legal for a processor that supports FD to drop misa writes
and the assertion will trigger on legal RISC-V implementations. That code
piece does not support legal RISC-V implementations that can't disable F
and D. Disabling F and D should not be asserted because it is harmless if
an unused extension is present.

This assertion will always trigger in QEMU until we support the 'optional'
feature to allow changes to 'misa'.

Just noting this is not QEMU specifc so we should drop qemu-devel if we
continue to discuss misa on RISC-V in bbl.

Nevertheless, we do plan to support 'misa' writes however we need to do
some work in translate.c to make sure that cached translations match the
current state of misa. We may want to perform a tb_flush when we implement
writable misa. We also want writable misa to be a CPU feature so we can
emulate CPUs that don't support writable misa. eg add this to the CPU model.

set_feature(env, RISCV_FEATURE_MISA_WRITABLE)

Thanks for raising this because the new modular CSR implementation only
implemented 'existential' predicates for CSRs. We 

[Qemu-devel] [PATCH] hw/char/cmsdk-apb-uart.c: Accept more input after character read

[Patrick Oppenlander]

The character frontend needs to be notified that the uart receive buffer
is empty and ready to handle another character.

Previously, the uart only worked correctly when receiving one character
at a time.

Signed-off-by: Patrick Oppenlander 
---
 hw/char/cmsdk-apb-uart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
index 9c0929d8a2..ddfbb25c24 100644
--- a/hw/char/cmsdk-apb-uart.c
+++ b/hw/char/cmsdk-apb-uart.c
@@ -157,6 +157,7 @@ static uint64_t uart_read(void *opaque, hwaddr
offset, unsigned size)
 r = s->rxbuf;
 s->state &= ~R_STATE_RXFULL_MASK;
 cmsdk_apb_uart_update(s);
+qemu_chr_fe_accept_input(>chr);
 break;
 case A_STATE:
 r = s->state;
-- 
2.17.0

Re: [Qemu-devel] [PATCH] riscv: requires libfdt

[Michael Clark]

On Fri, Apr 20, 2018 at 4:56 AM, Philippe Mathieu-Daudé 
wrote:

> > On 04/19/2018 12:51 PM, KONRAD Frederic wrote:
> >> When compiling on a machine without libfdt installed the configure
> script
> >> should try to get libfdt from the git or should die because otherwise
> >> CONFIG_LIBFDT is not set and the build process end in an error in the
> >> link
> >> phase.. eg:
> >>
> >> hw/riscv/virt.o: In function `riscv_virt_board_init':
> >> qemu/src/hw/riscv/virt.c:317: undefined reference to
> >> `qemu_fdt_setprop_cell'
> >> qemu/src/hw/riscv/virt.c:319: undefined reference to
> >> `qemu_fdt_setprop_cell'
> >> qemu/src/hw/riscv/virt.c:345: undefined reference to `qemu_fdt_dumpdtb'
> >> collect2: error: ld returned 1 exit status
> >> make[1]: *** [qemu-system-riscv64] Error 1
> >> make: *** [subdir-riscv64-softmmu] Error 2
> >>
> >> Signed-off-by: KONRAD Frederic 
> >> ---
> >>   configure | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/configure b/configure
> >> index 0a19b03..1587f08 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -3732,7 +3732,7 @@ fi
> >>   fdt_required=no
> >>   for target in $target_list; do
> >> case $target in
> >> -
> >> aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-
> softmmu|mips64el-softmmu)
> >>
> >> +
> >> aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-
> softmmu|mips64el-softmmu|riscv64-softmmu)
> >>
> >
> > I just realized that riscv32 use it too so I'll correct with
> > riscv*-softmmu.
>
> Using "riscv*-softmmu":
>

Indeed.

We had this fix in our tree before one of the rebases against upstream:

https://github.com/riscv/riscv-qemu/commit/90cdfb86e81c54c1df42412b10b86fd83a6dee82

It must have somehow got dropped when we forward ported to QEMU master in
December 2017. My apologies.

Reviewed-by: Philippe Mathieu-Daudé 
>

Reviewed-by: Michael Clark 

>> fdt_required=yes
> >>   ;;
> >> esac
> >>
> >
>

Re: [Qemu-devel] [PATCH v1 5/5] target-microblaze: mmu: Make the TLBX MISS bit read-only

[Alistair Francis]

On Thu, Apr 19, 2018 at 4:21 AM, Edgar E. Iglesias
 wrote:
> From: "Edgar E. Iglesias" 
>
> Make the TLBX MISS bit read-only.
>
> Signed-off-by: Edgar E. Iglesias 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/microblaze/mmu.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 8391811900..9d5e6aa8a5 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -273,6 +273,10 @@ void mmu_write(CPUMBState *env, uint32_t rn, uint32_t v)
>  env->mmu.regs[rn] = v;
>  }
>  break;
> +case MMU_R_TLBX:
> +/* Bit 31 is read-only.  */
> +env->mmu.regs[rn] = deposit32(env->mmu.regs[rn], 0, 31, v);
> +break;
>  case MMU_R_TLBSX:
>  {
>  struct microblaze_mmu_lookup lu;
> --
> 2.14.1
>
>

Re: [Qemu-devel] [PATCH v1 4/5] target-microblaze: mmu: Make TLBSX write-only

[Alistair Francis]

On Thu, Apr 19, 2018 at 4:21 AM, Edgar E. Iglesias
 wrote:
> From: "Edgar E. Iglesias" 
>
> Make TLBSX write-only and guest-error log reads from it.
>
> Signed-off-by: Edgar E. Iglesias 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/microblaze/mmu.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index a0f06758f8..8391811900 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -182,7 +182,7 @@ done:
>  uint32_t mmu_read(CPUMBState *env, uint32_t rn)
>  {
>  unsigned int i;
> -uint32_t r;
> +uint32_t r = 0;
>
>  if (env->mmu.c_mmu < 2 || !env->mmu.c_mmu_tlb_access) {
>  qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
> @@ -211,6 +211,9 @@ uint32_t mmu_read(CPUMBState *env, uint32_t rn)
>  }
>  r = env->mmu.regs[rn];
>  break;
> +case MMU_R_TLBSX:
> +qemu_log_mask(LOG_GUEST_ERROR, "TLBSX is write-only.\n");
> +break;
>  default:
>  r = env->mmu.regs[rn];
>  break;
> --
> 2.14.1
>
>

Re: [Qemu-devel] [PATCH v3 6/6] vhost-user: support registering external host notifiers

[Liang, Cunming]

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, April 20, 2018 1:01 AM
> To: Liang, Cunming 
> Cc: Paolo Bonzini ; Bie, Tiwei ;
> jasow...@redhat.com; alex.william...@redhat.com; stefa...@redhat.com;
> qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Daly, Dan
> ; Tan, Jianfeng ; Wang, Zhihong
> ; Wang, Xiao W 
> Subject: Re: [PATCH v3 6/6] vhost-user: support registering external host
> notifiers
> 
> On Thu, Apr 19, 2018 at 04:52:20PM +, Liang, Cunming wrote:
> >
> >
> > > -Original Message-
> > > From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> > > Sent: Thursday, April 19, 2018 11:52 PM
> > > To: Michael S. Tsirkin ; Liang, Cunming
> > > 
> > > Cc: Bie, Tiwei ; jasow...@redhat.com;
> > > alex.william...@redhat.com; stefa...@redhat.com;
> > > qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Daly, Dan
> > > ; Tan, Jianfeng ; Wang,
> > > Zhihong ; Wang, Xiao W
> > > 
> > > Subject: Re: [PATCH v3 6/6] vhost-user: support registering external
> > > host notifiers
> > >
> > > On 19/04/2018 17:42, Michael S. Tsirkin wrote:
> > > >> A compiler barrier is enough on strongly-ordered memory platform.
> > > >> As it doesn't re-order store, PCI device won't see a stale index
> > > >> value. But a weakly-ordered memory needs sfence.
> > > >
> > > > Oh you are right.
> > > >
> > > > So it's only needed for non-intel platforms or when packets are in
> > > > WC memory then. And I don't know whether dpdk ever puts packets in
> > > > WC memory.
> > > >
> > > > I guess we'll cross this bridge when we get to it.
> > >
> > > Non-TSO architectures seem important...
> >
> > I'm not familiar with Non-TSO, trying to understand the difference
> > according to the feature set. Let's say non-TSO architectures do not
> > set 'weak_barriers'. Then mandatory barrier is used for software. HW
> > offload on that platform would choose different feature set against
> > software?
> 
> Right. We'll need a flag for this feature for starters. It doesn't exist
> :) Paolo also points out that we should then add code to disallow migration
> between setups with and without the feature.
I see. Thanks.

> 
> > If it's not, essentially we're worried about live migration from a TSO to a 
> > non-
> TSO architectures platform?
> 
> Probably not.
> 
> > >
> > > Paolo

Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
> commit, avoiding any rollback.
> 
> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

Sorry if it was my idea entirely that led you on this wild goose chase,
I had forgotten :(

Re: [Qemu-devel] [PATCH v2 6/7] block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop parameter "HBitmap **out" which is unused now, all callers set
> it to NULL.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

And now this patch is in question too.

Re: [Qemu-devel] [PATCH v2] QemuMutex: support --enable-debug-mutex

[Emilio G. Cota]

On Thu, Apr 19, 2018 at 15:43:01 -0400, Emilio G. Cota wrote:
> BTW you might also want to add the file/line pair to
> qemu-thread-win32.c, or hide the configure option to Windows
> users.

A few other call sites are missing as well, e.g. when returning from
pthread_cond_wait or with _trylock.

If you grep for trace_qemu_mutex_locked you'll find all the
places where the file/line update should occur.

Emilio

Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
> commit, avoiding any rollback.
> 
> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
> 

I'm trying to remember why we ever bothered doing it the other way. Is
it because of timings with respect to other operations and we wanted the
"clear" to take effect sooner rather than later to co-operate with other
commands?

(You and Nikolay hinted about similar problems in the Libvirt PULL api
thread, too. It continues to be an issue.)

Hopping in my time machine:

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html

(Oh, I wrote it...)

Looks like it was a conscious decision to avoid ordering conflicts with
other block jobs.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block_int.h |  1 -
>  block/dirty-bitmap.c  |  9 -
>  blockdev.c| 16 +---
>  3 files changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 189666efa5..22059c8119 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
>  
>  void bdrv_inc_in_flight(BlockDriverState *bs);
>  void bdrv_dec_in_flight(BlockDriverState *bs);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 1812e17549..3c69a8eed9 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, 
> HBitmap **out)
>  bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
> -{
> -HBitmap *tmp = bitmap->bitmap;
> -assert(bdrv_dirty_bitmap_enabled(bitmap));
> -assert(!bdrv_dirty_bitmap_readonly(bitmap));
> -bitmap->bitmap = in;
> -hbitmap_free(tmp);
> -}
> -
>  uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>uint64_t offset, uint64_t 
> bytes)
>  {
> diff --git a/blockdev.c b/blockdev.c
> index c31bf3d98d..88eae60c1c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>  BlkActionState common;
>  BdrvDirtyBitmap *bitmap;
>  BlockDriverState *bs;
> -HBitmap *backup;
>  bool prepared;
>  } BlockDirtyBitmapState;
>  
> @@ -2129,18 +2128,6 @@ static void 
> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>  error_setg(errp, "Cannot clear a readonly bitmap");
>  return;
>  }
> -
> -bdrv_clear_dirty_bitmap(state->bitmap, >backup);
> -}
> -
> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
> -{
> -BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> - common, common);
> -
> -if (state->backup) {
> -bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
> -}
>  }
>  
>  static void block_dirty_bitmap_clear_commit(BlkActionState *common)
> @@ -2148,7 +2135,7 @@ static void 
> block_dirty_bitmap_clear_commit(BlkActionState *common)
>  BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>   common, common);
>  
> -hbitmap_free(state->backup);
> +bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>  .instance_size = sizeof(BlockDirtyBitmapState),
>  .prepare = block_dirty_bitmap_clear_prepare,
>  .commit = block_dirty_bitmap_clear_commit,
> -.abort = block_dirty_bitmap_clear_abort,
>  }
>  };
>  
> 

Re: [Qemu-devel] [PATCH RFC 14/14] qapi: Add #if conditions to commands, events, types, visitors

[Eric Blake]

On 02/23/2018 12:13 PM, Markus Armbruster wrote:

[reviving this RFC]

> 
> For what it's worth, I disliked the decorator magic enough to write this
> series.
> 
>>Furthermore, I don't fancy much having to redo and tune
>> the generation *again* to fix the inden, extra-spaces etc that were
>> fixed after several revisions (it takes hours to get there, it's
>> boring). Can't we go first with my approach and then look at replacing
>> it? Can't one add a "FIXME: replace the decorator with something less
>> magic" at ifcond_decorator definition for now? Is this code so
>> critical that it has to be the way you want in the first place? The
>> approach to take it first and improve it worked very well for
>> qapi2texi, it just took a few more days for you (and reviewers) to
>> improve it. I'd suggest we work that way instead of having me rewrite
>> and rewrite until you are happy (which is something I can't do right
>> without many painful iterations for you and me).
> 
> This is up to the backup QAPI maintainer now :)

Except my (cop-out?) decision was that it was not 2.12 material, so now
that 2.13 is opening up it's somewhat back to you...

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 0/2] QAPI file renames

[Eric Blake]

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02877.html

Since then: rebase to master, drop patch that has already been
incorporated

Eric Blake (2):
  qapi: Rename QMP and QGA schema files
  qapi: Rename .json to .qapi

 docs/devel/blkdebug.txt|   2 +-
 docs/devel/qapi-code-gen.txt   |   6 +-
 docs/devel/testing.rst |   4 +-
 docs/devel/writing-qmp-commands.txt|   6 +-
 docs/interop/live-block-operations.rst |   4 +-
 docs/interop/qmp-intro.txt |   6 +-
 Makefile   |  40 +--
 qapi/{block-core.json => block-core.qapi}  |   6 +-
 qapi/{block.json => block.qapi}|   2 +-
 qapi/{char.json => char.qapi}  |   2 +-
 qapi/{common.json => common.qapi}  |   0
 qapi/{crypto.json => crypto.qapi}  |   0
 qapi/{introspect.json => introspect.qapi}  |   0
 qapi/{migration.json => migration.qapi}|   2 +-
 qapi/{misc.json => misc.qapi}  |   0
 qapi/{net.json => net.qapi}|   2 +-
 qga/qapi-schema.json => qapi/qga-schema.qapi   |   0
 qapi/{qapi-schema.json => qmp-schema.qapi} |  30 +-
 qapi/{rocker.json => rocker.qapi}  |   0
 qapi/{run-state.json => run-state.qapi}|   0
 qapi/{sockets.json => sockets.qapi}|   2 +-
 qapi/{tpm.json => tpm.qapi}|   0
 qapi/{trace.json => trace.qapi}|   0
 qapi/{transaction.json => transaction.qapi}|   2 +-
 qapi/{ui.json => ui.qapi}  |   2 +-
 include/crypto/block.h |   2 +-
 include/crypto/cipher.h|   2 +-
 include/crypto/hash.h  |   2 +-
 include/crypto/ivgen.h |   2 +-
 MAINTAINERS|  26 +-
 scripts/git.orderfile  |   2 +-
 tests/Makefile.include | 336 ++---
 tests/qapi-schema/alternate-any.err|   2 +-
 .../{alternate-any.json => alternate-any.qapi} |   0
 tests/qapi-schema/alternate-array.err  |   2 +-
 .../{alternate-array.json => alternate-array.qapi} |   0
 tests/qapi-schema/alternate-base.err   |   2 +-
 .../{alternate-base.json => alternate-base.qapi}   |   0
 tests/qapi-schema/alternate-clash.err  |   2 +-
 .../{alternate-clash.json => alternate-clash.qapi} |   0
 .../qapi-schema/alternate-conflict-bool-string.err |   2 +-
 ...ng.json => alternate-conflict-bool-string.qapi} |   0
 tests/qapi-schema/alternate-conflict-dict.err  |   2 +-
 ...lict-dict.json => alternate-conflict-dict.qapi} |   0
 tests/qapi-schema/alternate-conflict-enum-bool.err |   2 +-
 ...bool.json => alternate-conflict-enum-bool.qapi} |   0
 tests/qapi-schema/alternate-conflict-enum-int.err  |   2 +-
 ...m-int.json => alternate-conflict-enum-int.qapi} |   0
 .../qapi-schema/alternate-conflict-num-string.err  |   2 +-
 ...ing.json => alternate-conflict-num-string.qapi} |   0
 tests/qapi-schema/alternate-conflict-string.err|   2 +-
 ...-string.json => alternate-conflict-string.qapi} |   0
 tests/qapi-schema/alternate-empty.err  |   2 +-
 .../{alternate-empty.json => alternate-empty.qapi} |   0
 tests/qapi-schema/alternate-nested.err |   2 +-
 ...alternate-nested.json => alternate-nested.qapi} |   0
 tests/qapi-schema/alternate-unknown.err|   2 +-
 ...ternate-unknown.json => alternate-unknown.qapi} |   0
 tests/qapi-schema/args-alternate.err   |   2 +-
 .../{args-alternate.json => args-alternate.qapi}   |   0
 tests/qapi-schema/args-any.err |   2 +-
 tests/qapi-schema/{args-any.json => args-any.qapi} |   0
 tests/qapi-schema/args-array-empty.err |   2 +-
 ...args-array-empty.json => args-array-empty.qapi} |   0
 tests/qapi-schema/args-array-unknown.err   |   2 +-
 ...-array-unknown.json => args-array-unknown.qapi} |   0
 tests/qapi-schema/args-bad-boxed.err   |   2 +-
 .../{args-bad-boxed.json => args-bad-boxed.qapi}   |   0
 tests/qapi-schema/args-boxed-anon.err  |   2 +-
 .../{args-boxed-anon.json => args-boxed-anon.qapi} |   0
 tests/qapi-schema/args-boxed-empty.err |   2 +-
 ...args-boxed-empty.json => args-boxed-empty.qapi} |   0
 tests/qapi-schema/args-boxed-string.err|   2 +-
 ...gs-boxed-string.json => args-boxed-string.qapi} |   0
 tests/qapi-schema/args-int.err |   2 +-
 tests/qapi-schema/{args-int.json => args-int.qapi} |   0
 tests/qapi-schema/args-invalid.err |   2 +-
 .../{args-invalid.json => args-invalid.qapi}   |   0
 tests/qapi-schema/args-member-array-bad.err|   2 +-
 

[Qemu-devel] [PATCH v2 1/2] qapi: Rename QMP and QGA schema files

[Eric Blake]

Having two files in the tree both named qapi-schema.json just adds
confusion.  Rename these files to {qmp,qga}-schema to make it
obvious which schema is in effect, and relocate qga into the common
qapi/ subdirectory.  Update all build rules that refer to the file
names, and adjust other documentation and comment references that
need to refer to the new file names.

Maintainer-wise, this means that qapi/qga-schema.json continues
to belong to Michael as QGA maintainer, but now also notifies
Markus and Eric as QAPI maintainers, alongside all the other
QMP QAPI files, matching how other .json QAPI modules belong
to multiple maintainer blurbs.  Also, fix a stale reference to
a file removed in commit eb815e248f.

Signed-off-by: Eric Blake 
---
v2: rebase on top of Markus' work that already moved
qapi-schema.json to qapi/
---
 docs/devel/writing-qmp-commands.txt  |  2 +-
 docs/interop/qmp-intro.txt   |  6 +++---
 Makefile | 12 ++--
 qga/qapi-schema.json => qapi/qga-schema.json |  0
 qapi/{qapi-schema.json => qmp-schema.json}   |  0
 MAINTAINERS  |  2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)
 rename qga/qapi-schema.json => qapi/qga-schema.json (100%)
 rename qapi/{qapi-schema.json => qmp-schema.json} (100%)

diff --git a/docs/devel/writing-qmp-commands.txt 
b/docs/devel/writing-qmp-commands.txt
index 9dfc62bf5a3..1cd3dbf48ac 100644
--- a/docs/devel/writing-qmp-commands.txt
+++ b/docs/devel/writing-qmp-commands.txt
@@ -13,7 +13,7 @@ start with docs/interop/qmp-intro.txt.
 == Overview ==

 Generally speaking, the following steps should be taken in order to write a
-new QMP command.
+new QMP command (similar steps for QGA).

 1. Define the command and any types it needs in the appropriate QAPI
schema module.
diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt
index 900d69d6128..19ac6c573b6 100644
--- a/docs/interop/qmp-intro.txt
+++ b/docs/interop/qmp-intro.txt
@@ -72,14 +72,14 @@ Escape character is '^]'.
 { "execute": "query-status" }
 {
 "return": {
-"status": "prelaunch", 
-"singlestep": false, 
+"status": "prelaunch",
+"singlestep": false,
 "running": false
 }
 }

 Please refer to docs/interop/qemu-qmp-ref.* for a complete command
-reference, generated from qapi/qapi-schema.json.
+reference, generated from qapi/qmp-schema.json.

 QMP wiki page
 -
diff --git a/Makefile b/Makefile
index d71dd5bea41..f5c2ace8b30 100644
--- a/Makefile
+++ b/Makefile
@@ -570,14 +570,14 @@ qga/qapi-generated/qga-qapi-types.c 
qga/qapi-generated/qga-qapi-types.h \
 qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \
 qga/qapi-generated/qga-qapi-commands.h qga/qapi-generated/qga-qapi-commands.c \
 qga/qapi-generated/qga-qapi-doc.texi: \
-qga/qapi-generated/qapi-gen-timestamp ;
-qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json 
$(qapi-py)
+qga/qapi-generated/qga-gen-timestamp ;
+qga/qapi-generated/qga-gen-timestamp: $(SRC_PATH)/qapi/qga-schema.json 
$(qapi-py)
$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
-o qga/qapi-generated -p "qga-" $<, \
"GEN","$(@:%-timestamp=%)")
@>$@

-qapi-modules = $(SRC_PATH)/qapi/qapi-schema.json $(SRC_PATH)/qapi/common.json \
+qmp-modules = $(SRC_PATH)/qapi/qmp-schema.json $(SRC_PATH)/qapi/common.json \
$(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
$(SRC_PATH)/qapi/char.json \
$(SRC_PATH)/qapi/crypto.json \
@@ -665,8 +665,8 @@ qapi/qapi-events-transaction.c 
qapi/qapi-events-transaction.h \
 qapi/qapi-events-ui.c qapi/qapi-events-ui.h \
 qapi/qapi-introspect.h qapi/qapi-introspect.c \
 qapi/qapi-doc.texi: \
-qapi-gen-timestamp ;
-qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
+qmp-gen-timestamp ;
+qmp-gen-timestamp: $(qmp-modules) $(qapi-py)
$(call quiet-command,$(PYTHON_UTF8) $(SRC_PATH)/scripts/qapi-gen.py \
-o "qapi" -b $<, \
"GEN","$(@:%-timestamp=%)")
@@ -730,7 +730,7 @@ clean:
rm -f trace/generated-tracers-dtrace.dtrace*
rm -f trace/generated-tracers-dtrace.h*
rm -f $(foreach f,$(GENERATED_FILES),$(f) $(f)-timestamp)
-   rm -f qapi-gen-timestamp
+   rm -f qmp-gen-timestamp
rm -rf qga/qapi-generated
for d in $(ALL_SUBDIRS); do \
if test -d $$d; then $(MAKE) -C $$d $@ || exit 1; fi; \
diff --git a/qga/qapi-schema.json b/qapi/qga-schema.json
similarity index 100%
rename from qga/qapi-schema.json
rename to qapi/qga-schema.json
diff --git a/qapi/qapi-schema.json b/qapi/qmp-schema.json
similarity index 100%
rename from qapi/qapi-schema.json
rename to qapi/qmp-schema.json
diff --git a/MAINTAINERS b/MAINTAINERS
index 24b70169bc3..4a4b7aa2a64 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1581,7 +1581,6 @@ QAPI Schema
 M: Eric 

Re: [Qemu-devel] [PATCH 2/2] Revert "Revert "mux: fix ctrl-a b again"" again

[Marc-André Lureau]

Hi

On Fri, Apr 20, 2018 at 12:09 AM, Philippe Mathieu-Daudé
 wrote:
> This reverts commit 6f660996f1623034344cc37a1d430099067b755b to
> reintroduce commit 1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7.
>
> Signed-off-by: Philippe Mathieu-Daudé 

I think we agreed this would go after 2.12.

I was going to send an updated patch with additional tests. I can take
your patch for the next series perhaps, instead of doing the double
revert ;)

> ---
>  chardev/char-mux.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 1b925c8dec..6055e76293 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
>  }
>
>  d->focus = focus;
> +chr->be = d->backends[focus];
>  mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
>  }
>
> --
> 2.17.0
>

[Qemu-devel] [PATCH 1/2] hw/isa/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

4c3119a6e3e and cd9526ab7c0 introduced an incorrect and inconsistent
use of Chardev->be. Also, this CharBackend member is private and is
not supposed to be accessible.

Fix it by removing the inconsistent check.

Reported-by: Marc-André Lureau 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/isa/isa-superio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index b95608a003..08afe44731 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -43,7 +43,7 @@ static void isa_superio_realize(DeviceState *dev, Error 
**errp)
 if (!k->parallel.is_enabled || k->parallel.is_enabled(sio, i)) {
 /* FIXME use a qdev chardev prop instead of parallel_hds[] */
 chr = parallel_hds[i];
-if (chr == NULL || chr->be) {
+if (chr == NULL) {
 name = g_strdup_printf("discarding-parallel%d", i);
 chr = qemu_chr_new(name, "null");
 } else {
@@ -83,7 +83,7 @@ static void isa_superio_realize(DeviceState *dev, Error 
**errp)
 if (!k->serial.is_enabled || k->serial.is_enabled(sio, i)) {
 /* FIXME use a qdev chardev prop instead of serial_hds[] */
 chr = serial_hds[i];
-if (chr == NULL || chr->be) {
+if (chr == NULL) {
 name = g_strdup_printf("discarding-serial%d", i);
 chr = qemu_chr_new(name, "null");
 } else {
-- 
2.17.0

[Qemu-devel] [PATCH 2/2] Revert "Revert "mux: fix ctrl-a b again"" again

[Philippe Mathieu-Daudé]

This reverts commit 6f660996f1623034344cc37a1d430099067b755b to
reintroduce commit 1b2503fcf7b5932c5a3779ca2ceb92bd403c4ee7.

Signed-off-by: Philippe Mathieu-Daudé 
---
 chardev/char-mux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 1b925c8dec..6055e76293 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -304,6 +304,7 @@ void mux_set_focus(Chardev *chr, int focus)
 }
 
 d->focus = focus;
+chr->be = d->backends[focus];
 mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
 }
 
-- 
2.17.0

[Qemu-devel] [PATCH 0/2] hw/superio: Fix inconsistent use of Chardev->be

[Philippe Mathieu-Daudé]

My commit cd9526ab7c0 break something in the chardev frontend (serial).
Also 4c3119a6e3e does the same with the parallel device.

Fix the broken code and restore Marc-André patch which fixes the ctrl-a+b.
Tested with my usual setup.

Philippe Mathieu-Daudé (2):
  hw/isa/superio: Fix inconsistent use of Chardev->be
  Revert "Revert "mux: fix ctrl-a b again"" again

 chardev/char-mux.c   | 1 +
 hw/isa/isa-superio.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 42/43] tests/tcg: disable fp-test for 32 bit (HACK!)

[Richard Henderson]

On 04/19/2018 03:59 AM, Alex Bennée wrote:
> +ifneq (,$(findstring 64,$(TARGET_NAME)))

You're only considering aarch64, x86_64, ppc64 and the like.
This fails for alpha, s390x and the like.


r~

Re: [Qemu-devel] [PATCH v2 38/43] osdep: disable glib-compat.h include with QEMU_NO_GLIB

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> From: "Emilio G. Cota" 
> 
> To ease the cross-compilation of tests that do not use glib.
> 
> Signed-off-by: Emilio G. Cota 
> ---
>  include/qemu/osdep.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate them in the header and clarify needed locking in comments.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/dirty-bitmap.h | 14 +-
>  block/dirty-bitmap.c |  5 +
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index c7e910016d..b7ccfd1363 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
> *bs,
>uint32_t granularity,
>const char *name,
>Error **errp);
> -void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -   int chunk_size);
> -void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> @@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t offset, int64_t bytes);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>   int64_t offset, int64_t bytes);
> -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>  
> @@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
>  void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> -int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>  bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
> @@ -99,4 +94,13 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>BdrvDirtyBitmap *bitmap,
>Error **errp);
>  
> +/*
> + * Unused for now meta-bitmaps related functions
> + */
> +

I assume you have plans to use them still? I thought these were useful
for storage migration -- but I guess we don't use these for the postcopy
mechanism?

> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap, int chunk_size);
> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
> +int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
> +
>  #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 6c00288fd7..1812e17549 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -149,6 +149,8 @@ BdrvDirtyBitmap 
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>   * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
>   * @chunk_size: how many bytes of bitmap data does each bit in the meta 
> bitmap
>   * track.
> + *
> + * Called with BQL taken.
>   */
>  void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int chunk_size)
> @@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap 
> *bitmap,
>  qemu_mutex_unlock(bitmap->mutex);
>  }
>  
> +/* Called with BQL taken. */
>  void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>  assert(bitmap->meta);
> @@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap 
> *bitmap)
>  return iter;
>  }
>  
> +/* Called with BQL and dirty_bitmap_mutex locked. */
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
>  {
>  BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> @@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>  return hbitmap_count(bitmap->bitmap);
>  }
>  
> +/* Called with BQL or dirty_bitmap_mutex locked */
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
>  return hbitmap_count(bitmap->meta);
> 

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Eduardo Habkost]

On Thu, Apr 19, 2018 at 11:32:16PM +0200, Paolo Bonzini wrote:
> On 19/04/2018 21:56, Eduardo Habkost wrote:
> > On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote:
> >> On 17/04/2018 22:59, Eduardo Habkost wrote:
>  +if (disable_exits) {
>  +disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
>  +  KVM_X86_DISABLE_EXITS_HLT |
>  +  KVM_X86_DISABLE_EXITS_PAUSE);
>  +if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) {
>  +disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT;
>  +}
> >>>
> >>> In the future, if we decide to enable kvm-pv-unhalt by default,
> >>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt
> >>> automatically, or should we require an explicit
> >>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option?
> >>
> >> It should be automatic.
> >>
> >>> For today's defaults, this patch solves the problem, only one
> >>> thing is missing before I give my R-b: we need to clearly
> >>> document what exactly are the consequences and requirements of
> >>> setting kvm-hint-dedicated=on (I'm not sure if the best place for
> >>> this is qemu-options.hx, x86_cpu_list(), or somewhere else).
> >>
> >> I don't think we have a good place for this kind of documentation,
> >> unfortunately.  Right now it is mentioned in
> >> Documentation/virtual/kvm/cpuid.txt.
> > 
> > With this patch, the QEMU option will do more than just setting
> > the CPUID bit, that's why I miss more detailed documentation on
> > the QEMU side.  But I agree we have no obvious place for that
> > documentation.
> > 
> > In the worst case we can just add a code comment on top of
> > feature_word_info[FEAT_KVM_HINTS].feat_names warning that
> > kvm-hint-dedicated won't just enable the flag on CPUID and has
> > other side-effects.
> 
> Maybe we should use "-realtime dedicated=on" instead of, or in addition
> to kvm-hint-dedicated=on?

Maybe it's a better idea than overloading an option that is only
expected to control a CPUID bit.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 39/43] softfloat: do not include glib headers

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> From: "Emilio G. Cota" 
> 
> To ease the cross-compilation of softfloat.o.
> 
> Signed-off-by: Emilio G. Cota 
> ---
>  fpu/softfloat.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 03/43] configure: add support for --cross-cc-FOO

[Eric Blake]

On 04/19/2018 08:58 AM, Alex Bennée wrote:
> This allows us to specify cross compilers for our guests. This is
> useful for building test images/programs. Currently we re-run the
> compile test for each target. I couldn't think of a way to cache the
> value for a given arch without getting messier configure code.
> 
> The cross compiler for the guest is visible to each target as
> CROSS_CC_GUEST in config-target.mak.
> 
> Signed-off-by: Alex Bennée 
> ---
>  configure | 50 ++
>  1 file changed, 50 insertions(+)
> 

> @@ -483,6 +490,11 @@ for opt do
>;;
>--disable-debug-info) debug_info="no"
>;;
> +  --cross-cc-*[!a-zA-Z0-9_0]=*) error_exit "Passed bad --cross-cc-FOO option"

Not quite right; it looks like you intended to have a trailing - instead
of 0; and if you are trying to filter out bad characters, then you need
* on both sides of the [!...] list:

--cross-cc-*[!a-zA-Z0-9_-]*=*)

otherwise you are only filtering out bad characters immediately before
the first =.  There's also the question of whether I can spell both
'--cross-cc-FOO=BAR' as one argument and '--cross-cc-FOO BAR' as two
arguments; this is filtering out only the one-argument case.

> +  ;;
> +  --cross-cc-*) cc_arch=${opt#--cross-cc-}
> +eval "cross_cc_${cc_arch}=\$optarg"
> +  ;;


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 3/4] qdev: Simplify the SysBusDeviceClass::init path

[Philippe Mathieu-Daudé]

On 04/19/2018 06:27 PM, Philippe Mathieu-Daudé wrote:
> The SysBusDevice is the last DeviceClass::init user.
> 
> Instead of using
>   SysBusDeviceClass::realize
>-> DeviceClass::realize
>-> DeviceClass::init
>-> sysbus_device_init
>   -> SysBusDeviceClass::init
> 
> Simplify the path by directly calling SysBusDeviceClass::init
> in SysBusDeviceClass::realize:
> 
>   SysBusDeviceClass::realize
>-> SysBusDeviceClass::init
> 
> Finally, remove the DeviceClass::init, there are no more users.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/qdev-core.h |  2 --
>  hw/core/qdev.c | 14 --
>  hw/core/sysbus.c   | 15 ++-
>  3 files changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9453588160..6f60748043 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -29,7 +29,6 @@ typedef enum DeviceCategory {
>  DEVICE_CATEGORY_MAX
>  } DeviceCategory;
>  
> -typedef int (*qdev_initfn)(DeviceState *dev);
>  typedef int (*qdev_event)(DeviceState *dev);
>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
> @@ -124,7 +123,6 @@ typedef struct DeviceClass {
>  const struct VMStateDescription *vmsd;
>  
>  /* Private to qdev / bus.  */
> -qdev_initfn init; /* TODO remove, once users are converted to realize */

The DeviceClass documentation (top of this file) is now out of date :(

If the maintainer taking this series prefer a respin, I plan to only
remove the 'init' references in the big comment.

>  qdev_event exit; /* TODO remove, once users are converted to unrealize */
>  const char *bus_type;
>  } DeviceClass;
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f6f92473b8..4153b733c8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -208,19 +208,6 @@ void device_listener_unregister(DeviceListener *listener)
>  QTAILQ_REMOVE(_listeners, listener, link);
>  }
>  
> -static void device_realize(DeviceState *dev, Error **errp)
> -{
> -DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -
> -if (dc->init) {
> -int rc = dc->init(dev);
> -if (rc < 0) {
> -error_setg(errp, "Device initialization failed.");
> -return;
> -}
> -}
> -}
> -
>  static void device_unrealize(DeviceState *dev, Error **errp)
>  {
>  DeviceClass *dc = DEVICE_GET_CLASS(dev);
> @@ -1065,7 +1052,6 @@ static void device_class_init(ObjectClass *class, void 
> *data)
>  DeviceClass *dc = DEVICE_CLASS(class);
>  
>  class->unparent = device_unparent;
> -dc->realize = device_realize;
>  dc->unrealize = device_unrealize;
>  
>  /* by default all devices were considered as hotpluggable,
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 5d0887f499..11f6d14b84 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "monitor/monitor.h"
>  #include "exec/address-spaces.h"
> @@ -200,15 +201,19 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
> ioport, uint32_t size)
>  }
>  }
>  
> -static int sysbus_device_init(DeviceState *dev)
> +static void sysbus_realize(DeviceState *dev, Error **errp)
>  {
>  SysBusDevice *sd = SYS_BUS_DEVICE(dev);
>  SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
>  
> -if (!sbc->init) {
> -return 0;
> +/* TODO remove, once users are converted to realize */
> +if (sbc->init) {
> +int rc = sbc->init(sd);
> +if (rc < 0) {
> +error_setg(errp, "Device initialization failed.");
> +return;
> +}
>  }
> -return sbc->init(sd);
>  }
>  
>  DeviceState *sysbus_create_varargs(const char *name,
> @@ -324,7 +329,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
>  static void sysbus_device_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *k = DEVICE_CLASS(klass);
> -k->init = sysbus_device_init;
> +k->realize = sysbus_realize;
>  k->bus_type = TYPE_SYSTEM_BUS;
>  /*
>   * device_add plugs devices into a suitable bus.  For "real" buses,
> 

Re: [Qemu-devel] [PATCH v3 0/4] qdev: remove DeviceClass::init/exit()

[Philippe Mathieu-Daudé]

On 04/19/2018 06:27 PM, Philippe Mathieu-Daudé wrote:
> Since v2:
>   - rebased for 2.13 (Markus)
>   - dropped 2 patches already merged (Gerd)
>   - start sentences with a capital letter and end with a full stop (Peter)

I forgot to put the backport-diff, in case someone already reviewed v2:

[] : patches are identical
The flags [FC] indicate (F)unctional and (C)ontextual differences,
respectively

001/4:[] [--] 'hw/i2c/smbus: use DeviceClass::realize instead of
SMBusDeviceClass::init'
002/4:[] [-C] 'hw/i2c: use DeviceClass::realize instead of
I2CSlaveClass::init'
003/4:[] [-C] 'qdev: simplify the SysBusDeviceClass::init path'
004/4:[] [-C] 'qdev: remove DeviceClass::exit'


> since v1:
>   - fix format string on 32-bit host (patchew)
>   - do not add smbus_eeprom_reset() (Eduardo)
>   - directly use DeviceClass::realize (Eduardo)
>   - squashed 2 patches (Eduardo)

Re: [Qemu-devel] [PATCH v2 26/43] tests/tcg/arm: fix up test-arm-iwmmxt test

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> +test-arm-iwmmxt: CFLAGS+=-marm -march=iwmmxt -mabi=aapcs
> +test-arm-iwmmxt: test-arm-iwmmxt.S

This appears to be insufficient.
If I begin with armv7l-linux-gnueabihf-gcc, then I get

  CROSS-BUILD arm guest-tests with armv7l-linux-gnueabihf-gcc
cc1: error: iWMMXt and NEON are incompatible



r~

Re: [Qemu-devel] [PATCH v2 03/43] configure: add support for --cross-cc-FOO

[Eric Blake]

On 04/19/2018 03:47 PM, Richard Henderson wrote:
> On 04/19/2018 10:41 AM, Richard Henderson wrote:
>> On 04/19/2018 03:58 AM, Alex Bennée wrote:
>>> +  --cross-cc-*) cc_arch=${opt#--cross-cc-}
>>> +eval "cross_cc_${cc_arch}=\$optarg"
>>
>> This doesn't work as intended.
>>
>> Given e.g. --cross-cc-aarch64=aarch64-linux-gcc
>>
>> + cc_arch=aarch64=aarch64-linux-gcc
>> + eval 'cross_cc_aarch64=aarch64-linux-gcc=$optarg'
>> ++ cross_cc_aarch64=aarch64-linux-gcc=aarch64-linux-gcc
>>
>> Which sets the variable "cross_cc_aarch64"
>> to "aarch64-linux-gcc=aarch64-linux-gcc".
>> Which of course won't exist to execute.
> 
> -  --cross-cc-*) cc_arch=${opt#--cross-cc-}
> +  --cross-cc-*) cc_arch=$(expr "$opt" : '--cross-cc-\([^=]*\)')

Forking an expr subshell may not be needed; how about:

--cross-cc-*) cc_arch=${opt#--cross-cc-}; cc_arch=${cc_arch%%=*}

to strip both the --cross-cc- prefix, and any suffix starting at the
first =.

> 
> seems to do the trick.  Obviously a similar change will be needed for
> --cross-cc-flags-*.
> 
> 
> r~
> 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 25/43] tests/tcg: move ARM specific tests into subdir

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> These only need to be built for ARM guests.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Thomas Huth 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> ---
> v2
>   - do VPATH manipulation in tests/tcg/arm/Makefile.target
>   - merge with fix hello-arm test
> ---

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 24/43] tests/tcg/i386/test-i386: fix printf format

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> Signed-off-by: Alex Bennée 
> ---
>  tests/tcg/i386/test-i386.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH v3 4/4] qdev: Remove DeviceClass::exit

[Philippe Mathieu-Daudé]

Since no devices use it, we can safely remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/qdev-core.h |  2 --
 hw/core/qdev.c | 14 --
 2 files changed, 16 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 6f60748043..88b52be8fb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -29,7 +29,6 @@ typedef enum DeviceCategory {
 DEVICE_CATEGORY_MAX
 } DeviceCategory;
 
-typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceReset)(DeviceState *dev);
@@ -123,7 +122,6 @@ typedef struct DeviceClass {
 const struct VMStateDescription *vmsd;
 
 /* Private to qdev / bus.  */
-qdev_event exit; /* TODO remove, once users are converted to unrealize */
 const char *bus_type;
 } DeviceClass;
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4153b733c8..ffec461791 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -208,19 +208,6 @@ void device_listener_unregister(DeviceListener *listener)
 QTAILQ_REMOVE(_listeners, listener, link);
 }
 
-static void device_unrealize(DeviceState *dev, Error **errp)
-{
-DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-if (dc->exit) {
-int rc = dc->exit(dev);
-if (rc < 0) {
-error_setg(errp, "Device exit failed.");
-return;
-}
-}
-}
-
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
  int required_for_version)
 {
@@ -1052,7 +1039,6 @@ static void device_class_init(ObjectClass *class, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(class);
 
 class->unparent = device_unparent;
-dc->unrealize = device_unrealize;
 
 /* by default all devices were considered as hotpluggable,
  * so with intent to check it in generic qdev_unplug() /
-- 
2.17.0

Re: [Qemu-devel] [PATCH RESEND v2] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Paolo Bonzini]

On 19/04/2018 21:56, Eduardo Habkost wrote:
> On Thu, Apr 19, 2018 at 05:48:57PM +0200, Paolo Bonzini wrote:
>> On 17/04/2018 22:59, Eduardo Habkost wrote:
 +if (disable_exits) {
 +disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
 +  KVM_X86_DISABLE_EXITS_HLT |
 +  KVM_X86_DISABLE_EXITS_PAUSE);
 +if (env->user_features[FEAT_KVM] & KVM_PV_UNHALT) {
 +disable_exits &= ~KVM_X86_DISABLE_EXITS_HLT;
 +}
>>>
>>> In the future, if we decide to enable kvm-pv-unhalt by default,
>>> should "-cpu ...,kvm-hint-dedicated=on" disable kvm-pv-unhalt
>>> automatically, or should we require an explicit
>>> "kvm-hint-dedicated=on,kvm-pv-unhalt=off" option?
>>
>> It should be automatic.
>>
>>> For today's defaults, this patch solves the problem, only one
>>> thing is missing before I give my R-b: we need to clearly
>>> document what exactly are the consequences and requirements of
>>> setting kvm-hint-dedicated=on (I'm not sure if the best place for
>>> this is qemu-options.hx, x86_cpu_list(), or somewhere else).
>>
>> I don't think we have a good place for this kind of documentation,
>> unfortunately.  Right now it is mentioned in
>> Documentation/virtual/kvm/cpuid.txt.
> 
> With this patch, the QEMU option will do more than just setting
> the CPUID bit, that's why I miss more detailed documentation on
> the QEMU side.  But I agree we have no obvious place for that
> documentation.
> 
> In the worst case we can just add a code comment on top of
> feature_word_info[FEAT_KVM_HINTS].feat_names warning that
> kvm-hint-dedicated won't just enable the flag on CPUID and has
> other side-effects.

Maybe we should use "-realtime dedicated=on" instead of, or in addition
to kvm-hint-dedicated=on?

Thanks,

Paolo

[Qemu-devel] [PATCH v3 3/4] qdev: Simplify the SysBusDeviceClass::init path

[Philippe Mathieu-Daudé]

The SysBusDevice is the last DeviceClass::init user.

Instead of using
  SysBusDeviceClass::realize
   -> DeviceClass::realize
   -> DeviceClass::init
   -> sysbus_device_init
  -> SysBusDeviceClass::init

Simplify the path by directly calling SysBusDeviceClass::init
in SysBusDeviceClass::realize:

  SysBusDeviceClass::realize
   -> SysBusDeviceClass::init

Finally, remove the DeviceClass::init, there are no more users.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/qdev-core.h |  2 --
 hw/core/qdev.c | 14 --
 hw/core/sysbus.c   | 15 ++-
 3 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9453588160..6f60748043 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -29,7 +29,6 @@ typedef enum DeviceCategory {
 DEVICE_CATEGORY_MAX
 } DeviceCategory;
 
-typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
@@ -124,7 +123,6 @@ typedef struct DeviceClass {
 const struct VMStateDescription *vmsd;
 
 /* Private to qdev / bus.  */
-qdev_initfn init; /* TODO remove, once users are converted to realize */
 qdev_event exit; /* TODO remove, once users are converted to unrealize */
 const char *bus_type;
 } DeviceClass;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f6f92473b8..4153b733c8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -208,19 +208,6 @@ void device_listener_unregister(DeviceListener *listener)
 QTAILQ_REMOVE(_listeners, listener, link);
 }
 
-static void device_realize(DeviceState *dev, Error **errp)
-{
-DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-if (dc->init) {
-int rc = dc->init(dev);
-if (rc < 0) {
-error_setg(errp, "Device initialization failed.");
-return;
-}
-}
-}
-
 static void device_unrealize(DeviceState *dev, Error **errp)
 {
 DeviceClass *dc = DEVICE_GET_CLASS(dev);
@@ -1065,7 +1052,6 @@ static void device_class_init(ObjectClass *class, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(class);
 
 class->unparent = device_unparent;
-dc->realize = device_realize;
 dc->unrealize = device_unrealize;
 
 /* by default all devices were considered as hotpluggable,
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 5d0887f499..11f6d14b84 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
@@ -200,15 +201,19 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
ioport, uint32_t size)
 }
 }
 
-static int sysbus_device_init(DeviceState *dev)
+static void sysbus_realize(DeviceState *dev, Error **errp)
 {
 SysBusDevice *sd = SYS_BUS_DEVICE(dev);
 SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
 
-if (!sbc->init) {
-return 0;
+/* TODO remove, once users are converted to realize */
+if (sbc->init) {
+int rc = sbc->init(sd);
+if (rc < 0) {
+error_setg(errp, "Device initialization failed.");
+return;
+}
 }
-return sbc->init(sd);
 }
 
 DeviceState *sysbus_create_varargs(const char *name,
@@ -324,7 +329,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
 static void sysbus_device_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
-k->init = sysbus_device_init;
+k->realize = sysbus_realize;
 k->bus_type = TYPE_SYSTEM_BUS;
 /*
  * device_add plugs devices into a suitable bus.  For "real" buses,
-- 
2.17.0

[Qemu-devel] [PATCH v3 1/4] hw/i2c/smbus: Use DeviceClass::realize instead of SMBusDeviceClass::init

[Philippe Mathieu-Daudé]

SMBusDeviceClass::init is no more used, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/i2c/smbus.h | 1 -
 hw/i2c/smbus.c | 9 -
 hw/i2c/smbus_eeprom.c  | 5 ++---
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 544bbc1957..cfe3fa69f3 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -38,7 +38,6 @@
 typedef struct SMBusDeviceClass
 {
 I2CSlaveClass parent_class;
-int (*init)(SMBusDevice *dev);
 void (*quick_cmd)(SMBusDevice *dev, uint8_t read);
 void (*send_byte)(SMBusDevice *dev, uint8_t val);
 uint8_t (*receive_byte)(SMBusDevice *dev);
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 2d1b79a689..587ce1ab7f 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -202,14 +202,6 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
 return 0;
 }
 
-static int smbus_device_init(I2CSlave *i2c)
-{
-SMBusDevice *dev = SMBUS_DEVICE(i2c);
-SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
-
-return sc->init(dev);
-}
-
 /* Master device commands.  */
 int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
 {
@@ -350,7 +342,6 @@ static void smbus_device_class_init(ObjectClass *klass, 
void *data)
 {
 I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
 
-sc->init = smbus_device_init;
 sc->event = smbus_i2c_event;
 sc->recv = smbus_i2c_recv;
 sc->send = smbus_i2c_send;
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index b13ec0fe7a..125c887d1f 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -97,12 +97,11 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t 
cmd, int n)
 return eeprom_receive_byte(dev);
 }
 
-static int smbus_eeprom_initfn(SMBusDevice *dev)
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
 SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
 
 eeprom->offset = 0;
-return 0;
 }
 
 static Property smbus_eeprom_properties[] = {
@@ -115,7 +114,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, 
void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
 
-sc->init = smbus_eeprom_initfn;
+dc->realize = smbus_eeprom_realize;
 sc->quick_cmd = eeprom_quick_cmd;
 sc->send_byte = eeprom_send_byte;
 sc->receive_byte = eeprom_receive_byte;
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 23/43] tests/tcg/i386/test-i386: use modern vector_size attributes

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> The compiler complains about the old __mode__ style attributes.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/tcg/i386/test-i386.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH v3 2/4] hw/i2c: Use DeviceClass::realize instead of I2CSlaveClass::init

[Philippe Mathieu-Daudé]

I2CSlaveClass::init is no more used, remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/i2c/i2c.h|  3 ---
 hw/audio/wm8750.c   |  8 +++-
 hw/display/ssd0303.c|  9 -
 hw/gpio/max7310.c   |  9 -
 hw/i2c/core.c   | 13 -
 hw/input/lm832x.c   |  9 -
 hw/misc/tmp105.c|  7 +++
 hw/misc/tmp421.c|  8 +++-
 hw/nvram/eeprom_at24c.c | 24 +++-
 hw/timer/twl92230.c | 11 ---
 10 files changed, 36 insertions(+), 65 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index d727379b48..5dc166158b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -28,9 +28,6 @@ typedef struct I2CSlave I2CSlave;
 typedef struct I2CSlaveClass {
 DeviceClass parent_class;
 
-/* Callbacks provided by the device.  */
-int (*init)(I2CSlave *dev);
-
 /* Master to slave. Returns non-zero for a NAK, 0 for success. */
 int (*send)(I2CSlave *s, uint8_t data);
 
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index 416a78e869..f4aa838f62 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -617,14 +617,12 @@ static const VMStateDescription vmstate_wm8750 = {
 }
 };
 
-static int wm8750_init(I2CSlave *i2c)
+static void wm8750_realize(DeviceState *dev, Error **errp)
 {
-WM8750State *s = WM8750(i2c);
+WM8750State *s = WM8750(dev);
 
 AUD_register_card(CODEC, >card);
 wm8750_reset(I2C_SLAVE(s));
-
-return 0;
 }
 
 #if 0
@@ -707,7 +705,7 @@ static void wm8750_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
 
-sc->init = wm8750_init;
+dc->realize = wm8750_realize;
 sc->event = wm8750_event;
 sc->recv = wm8750_rx;
 sc->send = wm8750_tx;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index 68a80b9d64..eb90ba26be 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -297,13 +297,12 @@ static const GraphicHwOps ssd0303_ops = {
 .gfx_update  = ssd0303_update_display,
 };
 
-static int ssd0303_init(I2CSlave *i2c)
+static void ssd0303_realize(DeviceState *dev, Error **errp)
 {
-ssd0303_state *s = SSD0303(i2c);
+ssd0303_state *s = SSD0303(dev);
 
-s->con = graphic_console_init(DEVICE(i2c), 0, _ops, s);
+s->con = graphic_console_init(dev, 0, _ops, s);
 qemu_console_resize(s->con, 96 * MAGNIFY, 16 * MAGNIFY);
-return 0;
 }
 
 static void ssd0303_class_init(ObjectClass *klass, void *data)
@@ -311,7 +310,7 @@ static void ssd0303_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
-k->init = ssd0303_init;
+dc->realize = ssd0303_realize;
 k->event = ssd0303_event;
 k->recv = ssd0303_recv;
 k->send = ssd0303_send;
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index 4c203ef5c6..a560e3afd2 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -182,14 +182,13 @@ static void max7310_gpio_set(void *opaque, int line, int 
level)
 
 /* MAX7310 is SMBus-compatible (can be used with only SMBus protocols),
  * but also accepts sequences that are not SMBus so return an I2C device.  */
-static int max7310_init(I2CSlave *i2c)
+static void max7310_realize(DeviceState *dev, Error **errp)
 {
-MAX7310State *s = MAX7310(i2c);
+I2CSlave *i2c = I2C_SLAVE(dev);
+MAX7310State *s = MAX7310(dev);
 
 qdev_init_gpio_in(>qdev, max7310_gpio_set, 8);
 qdev_init_gpio_out(>qdev, s->handler, 8);
-
-return 0;
 }
 
 static void max7310_class_init(ObjectClass *klass, void *data)
@@ -197,7 +196,7 @@ static void max7310_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
-k->init = max7310_init;
+dc->realize = max7310_realize;
 k->event = max7310_event;
 k->recv = max7310_rx;
 k->send = max7310_tx;
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index cfccefca3d..ab72d5bf2b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -258,18 +258,6 @@ const VMStateDescription vmstate_i2c_slave = {
 }
 };
 
-static int i2c_slave_qdev_init(DeviceState *dev)
-{
-I2CSlave *s = I2C_SLAVE(dev);
-I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
-
-if (sc->init) {
-return sc->init(s);
-}
-
-return 0;
-}
-
 DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
 {
 DeviceState *dev;
@@ -283,7 +271,6 @@ DeviceState *i2c_create_slave(I2CBus *bus, const char 
*name, uint8_t addr)
 static void i2c_slave_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
-k->init = i2c_slave_qdev_init;
 set_bit(DEVICE_CATEGORY_MISC, k->categories);
 k->bus_type = TYPE_I2C_BUS;
 k->props = i2c_props;
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index d39953126b..74da30d9ca 100644
--- a/hw/input/lm832x.c
+++ 

[Qemu-devel] [PATCH v3 0/4] qdev: remove DeviceClass::init/exit()

[Philippe Mathieu-Daudé]

Since v2:
  - rebased for 2.13 (Markus)
  - dropped 2 patches already merged (Gerd)
  - start sentences with a capital letter and end with a full stop (Peter)

since v1:
  - fix format string on 32-bit host (patchew)
  - do not add smbus_eeprom_reset() (Eduardo)
  - directly use DeviceClass::realize (Eduardo)
  - squashed 2 patches (Eduardo)

Hi,

This series finalize the qdev QOMification.

We first convert the I2CSlave/SMBusDevice,
then the usb-ccid and virtio-ccw,
and finally the SysBusDevice.

At the end we removed *TWO* TODO :)

/* TODO remove, once users are converted to realize */
/* TODO remove, once users are converted to unrealize */

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/i2c/smbus: Use DeviceClass::realize instead of SMBusDeviceClass::init
  hw/i2c: Use DeviceClass::realize instead of I2CSlaveClass::init
  qdev: Simplify the SysBusDeviceClass::init path
  qdev: Remove DeviceClass::exit

 include/hw/i2c/i2c.h|  3 ---
 include/hw/i2c/smbus.h  |  1 -
 include/hw/qdev-core.h  |  4 
 hw/audio/wm8750.c   |  8 +++-
 hw/core/qdev.c  | 28 
 hw/core/sysbus.c| 15 ++-
 hw/display/ssd0303.c|  9 -
 hw/gpio/max7310.c   |  9 -
 hw/i2c/core.c   | 13 -
 hw/i2c/smbus.c  |  9 -
 hw/i2c/smbus_eeprom.c   |  5 ++---
 hw/input/lm832x.c   |  9 -
 hw/misc/tmp105.c|  7 +++
 hw/misc/tmp421.c|  8 +++-
 hw/nvram/eeprom_at24c.c | 24 +++-
 hw/timer/twl92230.c | 11 ---
 16 files changed, 48 insertions(+), 115 deletions(-)

-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 19/43] tests/tcg/i386: fix test-i386

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> The test-i386 test case is a little special as it includes assembler
> files. Add the additional compile magic to assemble these bits and
> link them to the final binary.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Thomas Huth 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> ---
>  tests/tcg/i386/test-i386.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 18/43] tests/tcg/i386: Build fix for hello-i386

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> From: Fam Zheng 
> 
> We have -Werror=missing-prototype, add a dummy prototype to avoid that
> warning.
> 
> Signed-off-by: Fam Zheng 
> Reviewed-by: Thomas Huth 
> ---
>  tests/tcg/i386/hello-i386.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 14/43] tests/tcg/multiarch: enable additional linux-test tests

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> Un-comment the remaining tests. I removed the itimer value tests
> because I'm fairly sure a re-arming timer will always have a different
> value in it when you grab it.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/tcg/multiarch/linux-test.c | 20 
>  1 file changed, 8 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 3/7] dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/dirty-bitmap.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 1ff8949b1b..c7e910016d 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -88,7 +88,6 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>  bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> 

Assuming you checked for others as well.

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v2 2/7] dirty-bitmaps: fix comment about dirty_bitmap_mutex

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Clarify first two cases and fix Modify -> Any access in third case.
> Also, drop 'only' from third case, as it a bit confuses, when thinking
> about case where we modify BdrvDirtyBitmap and access HBitmap.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block_int.h | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index c4dd1d4bb8..189666efa5 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -709,10 +709,14 @@ struct BlockDriverState {
>  uint64_t write_threshold_offset;
>  NotifierWithReturn write_threshold_notifier;
>  
> -/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
> - * Reading from the list can be done with either the BQL or the
> - * dirty_bitmap_mutex.  Modifying a bitmap only requires
> - * dirty_bitmap_mutex.  */
> +/* Writing to the list (i.e. to any field of BdrvDirtyBitmap or to the
> + * list-head) requires both the BQL _and_ the dirty_bitmap_mutex.
> + *
> + * Reading from the list (from any field of BdrvDirtyBitmap or from the
> + * list-head) can be done with either the BQL or the dirty_bitmap_mutex.
> + *
> + * Any access to underlying HBitmap requires dirty_bitmap_mutex.

"to the underlying HBitmap," probably.

> + */
>  QemuMutex dirty_bitmap_mutex;
>  QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
>  
> 

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Edgar E. Iglesias]

On Thu, Apr 19, 2018 at 11:17:58AM -1000, Richard Henderson wrote:
> On 04/19/2018 10:33 AM, Edgar E. Iglesias wrote:
> > On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
> >> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
> >>>  static inline void msr_write(DisasContext *dc, TCGv v)
> >>>  {
> >>> -TCGv t;
> >>> -
> >>> -t = tcg_temp_new();
> >>>  dc->cpustate_changed = 1;
> >>>  /* PVR bit is not writable.  */
> >>> -tcg_gen_andi_tl(t, v, ~MSR_PVR);
> >>> -tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
> >>> -tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
> >>> -tcg_temp_free(t);
> >>> +tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 
> >>> 1);
> >>>  }
> >>
> >> Um... the old code was correct, but the new code isn't.
> >>
> >> The new code sets msr to v, with bit 10 set to the old msr bit 0.
> >>
> >> Why do you believe the old code to be wrong?
> > 
> > The old code was incorrectly ORing v instead of t...
> 
> Ah, yes.
> 
> > What about the following?
> > tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
> > tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
> 
> *shrug* While that would be functional, I don't think it's any better than 
> just
> fixing the typo in the OR.

Allright, I'll fix the typo and send out a v2.

Thanks for catching this.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v2 12/43] tests/tcg: move architecture independent tests into subdir

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> We will want to build these for all supported guest architectures so
> lets move them all into one place. We also drop test_path at this
> point because it needs qemu utils and glib bits which is hard to
> support for cross compiling.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Thomas Huth 
> 
> ---
> v2
>   - move VPATH and TESTs setup into multiarch/Makefile.target
>   - remove moved bits from tests/tcg/Makefile
> ---

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 13/43] tests/tcg/multiarch: Build fix for linux-test

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> From: Fam Zheng 
> 
> To keep the compiler happy, and to fit in our buildsys flags:
> 
> - Make local functions "static"
> - #ifdef out unused functions
> - drop cutils/osdep dependencies
> 
> Signed-off-by: Fam Zheng 
> [AJB: drop cutils/osdep dependencies]
> Signed-off-by: Alex Bennée 
> Reviewed-by: Thomas Huth 
> ---
>  tests/tcg/multiarch/linux-test.c | 68 ++--
>  1 file changed, 21 insertions(+), 47 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Richard Henderson]

On 04/19/2018 10:33 AM, Edgar E. Iglesias wrote:
> On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
>> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
>>>  static inline void msr_write(DisasContext *dc, TCGv v)
>>>  {
>>> -TCGv t;
>>> -
>>> -t = tcg_temp_new();
>>>  dc->cpustate_changed = 1;
>>>  /* PVR bit is not writable.  */
>>> -tcg_gen_andi_tl(t, v, ~MSR_PVR);
>>> -tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
>>> -tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
>>> -tcg_temp_free(t);
>>> +tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 
>>> 1);
>>>  }
>>
>> Um... the old code was correct, but the new code isn't.
>>
>> The new code sets msr to v, with bit 10 set to the old msr bit 0.
>>
>> Why do you believe the old code to be wrong?
> 
> The old code was incorrectly ORing v instead of t...

Ah, yes.

> What about the following?
> tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
> tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);

*shrug* While that would be functional, I don't think it's any better than just
fixing the typo in the OR.


r~

Re: [Qemu-devel] [PATCH v2 1/7] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions write to BdrvDirtyBitmap field, so the should take the lock.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Looks like this won't introduce any recursive locking that I can spot,
so this looks correct.

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v2 11/43] docker: Makefile.include introduce DOCKER_SCRIPT

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> Define this in one place to make it easy to re-use.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/docker/Makefile.include | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 07/43] Makefile: Rename TARGET_DIRS to TARGET_LIST

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> From: Fam Zheng 
> 
> To be more accurate on its purpose and make code that looks for a certain
> target out of this variable more readable.
> 
> Signed-off-by: Fam Zheng 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> ---
>  Makefile   | 20 ++--
>  configure  |  2 +-
>  scripts/create_config  |  2 +-
>  tests/Makefile.include |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 06/43] configure: set cross_cc_FOO for host compiler

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
>  i386)
> CPU_CFLAGS="-m32"
> LDFLAGS="-m32 $LDFLAGS"
> +   cross_cc_i386=$cc
> +   cross_cc_cflags_i386=$CPU_CFLAGS
> ;;
>  x86_64)
> # ??? Only extremely old AMD cpus do not have cmpxchg16b.
> @@ -1454,7 +1468,7 @@ case "$cpu" in
> CPU_CFLAGS="-mx32"
> LDFLAGS="-mx32 $LDFLAGS"
> cross_cc_i386=$cc
> -   cross_cc_cflags_i386="-m32"
> +   cross_cc_cflags_i386=$CPU_CFLAGS
> ;;

Partially redundant with patch 4?  Anyway, this is what I was expecting to see
there.

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 04/43] configure: move i386_cc to cross_cc_i386

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> -cc_i386=i386-pc-linux-gnu-gcc
>  libs_qga=""
>  debug_info="yes"
>  stack_protector=""
> @@ -457,6 +456,8 @@ docker="no"
>  cross_cc_aarch64="aarch64-linux-gnu-gcc"
>  cross_cc_arm="arm-linux-gnueabihf-gcc"
>  cross_cc_powerpc="powerpc-linux-gnu-gcc"
> +cross_cc_i386="i386-pc-linux-gnu-gcc"
> +cross_cc_cflags_i386=""
...
>  
>  enabled_cross_compilers=""
>  
> @@ -687,12 +688,10 @@ case "$cpu" in
>i386|i486|i586|i686|i86pc|BePC)
>  cpu="i386"
>  supported_cpu="yes"
> -cross_cc_i386=gcc
>;;
>x86_64|amd64)
>  cpu="x86_64"
>  supported_cpu="yes"
> -cross_cc_x86_64=gcc
>;;
>armv*b|armv*l|arm)
>  cpu="arm"
> @@ -1435,7 +1434,6 @@ case "$cpu" in
>  i386)
> CPU_CFLAGS="-m32"
> LDFLAGS="-m32 $LDFLAGS"
> -   cc_i386='$(CC) -m32'

Why is the i386 case not handled like...

> ;;
>  x86_64)
> # ??? Only extremely old AMD cpus do not have cmpxchg16b.
> @@ -1443,12 +1441,14 @@ case "$cpu" in
> # runtime and generate the fallback to serial emulation.
> CPU_CFLAGS="-m64 -mcx16"
> LDFLAGS="-m64 $LDFLAGS"
> -   cc_i386='$(CC) -m32'
> +   cross_cc_x86_64=$cc
> +   cross_cc_cflags_x86_64=$CPU_CFLAGS

... the x86_64 case?

Also, does cross_cc_cflags_foo really have value over including the flags
directly in the cross_cc_foo variable?


r~

Re: [Qemu-devel] [Qemu-block] [PATCH v2] block: simplify code around releasing bitmaps

[John Snow]

On 03/26/2018 06:40 AM, Paolo Bonzini wrote:
> QLIST_REMOVE does not require walking the list, and once the "bitmap"
> argument is removed from bdrv_do_release_matching_dirty_bitmap_locked
> the code simplifies a lot and it is worth inlining everything in the
> callers of bdrv_do_release_matching_dirty_bitmap.
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: John Snow 

> ---
> 
> Notes:
> v1->v2: improve locking comments [Vladimir]
> 


Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory

[John Snow]

On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> If it appropriate for 2.12, let's push it. If not - then for 2.13.
> 

I wonder if I can make the case that this should be in 2.12.1; arguably
it is important to prevent corruption no matter how unlikely it is to
ever happen.

Moving it into stable increases the likelihood it shows up in
downstreams, so maybe let's see what we can get away with.

> 
> v2: - squash 02 (indentation fix) to 01
> - drop comment from qcow2_check_metadata_overlap()
> - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
>   bitmap_list_store. I don't think non-inplace case should be changed,
>   as it don't touch active bitmap directory.
> 
>  block/qcow2.h  | 45 -
>  block/qcow2-bitmap.c   |  7 ++-
>  block/qcow2-refcount.c | 10 ++
>  block/qcow2.c  | 22 ++
>  4 files changed, 54 insertions(+), 30 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6f0ff15dd0..896ad08e5b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,7 @@
>  #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
>  #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>  #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
>  #define QCOW2_OPT_CACHE_SIZE "cache-size"
>  #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>  #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
>  } QCow2ClusterType;
>  
>  typedef enum QCow2MetadataOverlap {
> -QCOW2_OL_MAIN_HEADER_BITNR= 0,
> -QCOW2_OL_ACTIVE_L1_BITNR  = 1,
> -QCOW2_OL_ACTIVE_L2_BITNR  = 2,
> -QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> -QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> -QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> -QCOW2_OL_INACTIVE_L1_BITNR= 6,
> -QCOW2_OL_INACTIVE_L2_BITNR= 7,
> -
> -QCOW2_OL_MAX_BITNR= 8,
> -
> -QCOW2_OL_NONE   = 0,
> -QCOW2_OL_MAIN_HEADER= (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> -QCOW2_OL_ACTIVE_L1  = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> -QCOW2_OL_ACTIVE_L2  = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> -QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> -QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> -QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> -QCOW2_OL_INACTIVE_L1= (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> +QCOW2_OL_MAIN_HEADER_BITNR  = 0,
> +QCOW2_OL_ACTIVE_L1_BITNR= 1,
> +QCOW2_OL_ACTIVE_L2_BITNR= 2,
> +QCOW2_OL_REFCOUNT_TABLE_BITNR   = 3,
> +QCOW2_OL_REFCOUNT_BLOCK_BITNR   = 4,
> +QCOW2_OL_SNAPSHOT_TABLE_BITNR   = 5,
> +QCOW2_OL_INACTIVE_L1_BITNR  = 6,
> +QCOW2_OL_INACTIVE_L2_BITNR  = 7,
> +QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
> +

A bit hard to read due to the formatting, but you've added #8 here, and

> +QCOW2_OL_MAX_BITNR  = 9,
> +> +QCOW2_OL_NONE = 0,
> +QCOW2_OL_MAIN_HEADER  = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> +QCOW2_OL_ACTIVE_L1= (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> +QCOW2_OL_ACTIVE_L2= (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> +QCOW2_OL_REFCOUNT_TABLE   = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> +QCOW2_OL_REFCOUNT_BLOCK   = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> +QCOW2_OL_SNAPSHOT_TABLE   = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> +QCOW2_OL_INACTIVE_L1  = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>  /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
>   * reads. */
> -QCOW2_OL_INACTIVE_L2= (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> +QCOW2_OL_INACTIVE_L2  = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> +QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),

and this one down here.

>  } QCow2MetadataOverlap;
>  
>  /* Perform all overlap checks which can be done in constant time */
>  #define QCOW2_OL_CONSTANT \
>  (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
> - QCOW2_OL_SNAPSHOT_TABLE)
> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>  
>  /* Perform all overlap checks which don't require disk access */
>  #define QCOW2_OL_CACHED \
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index f45e46cfbd..fb750ba8d3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, 
> Qcow2BitmapList *bm_list,
>  }
>  }
>  
> -ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
> +/* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is 
> not
> + * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when 

Re: [Qemu-devel] [PATCH v2 02/43] configure: add test for docker availability

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> This tests for a working docker installation without sudo and sets up
> config-host.mak accordingly. This will be useful from cross compiling
> things in the future.
> 
> Signed-off-by: Alex Bennée 
> ---
>  configure | 17 +
>  1 file changed, 17 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 03/43] configure: add support for --cross-cc-FOO

[Richard Henderson]

On 04/19/2018 10:41 AM, Richard Henderson wrote:
> On 04/19/2018 03:58 AM, Alex Bennée wrote:
>> +  --cross-cc-*) cc_arch=${opt#--cross-cc-}
>> +eval "cross_cc_${cc_arch}=\$optarg"
> 
> This doesn't work as intended.
> 
> Given e.g. --cross-cc-aarch64=aarch64-linux-gcc
> 
> + cc_arch=aarch64=aarch64-linux-gcc
> + eval 'cross_cc_aarch64=aarch64-linux-gcc=$optarg'
> ++ cross_cc_aarch64=aarch64-linux-gcc=aarch64-linux-gcc
> 
> Which sets the variable "cross_cc_aarch64"
> to "aarch64-linux-gcc=aarch64-linux-gcc".
> Which of course won't exist to execute.

-  --cross-cc-*) cc_arch=${opt#--cross-cc-}
+  --cross-cc-*) cc_arch=$(expr "$opt" : '--cross-cc-\([^=]*\)')

seems to do the trick.  Obviously a similar change will be needed for
--cross-cc-flags-*.


r~

Re: [Qemu-devel] [PATCH v2 03/43] configure: add support for --cross-cc-FOO

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> +  --cross-cc-*) cc_arch=${opt#--cross-cc-}
> +eval "cross_cc_${cc_arch}=\$optarg"

This doesn't work as intended.

Given e.g. --cross-cc-aarch64=aarch64-linux-gcc

+ cc_arch=aarch64=aarch64-linux-gcc
+ eval 'cross_cc_aarch64=aarch64-linux-gcc=$optarg'
++ cross_cc_aarch64=aarch64-linux-gcc=aarch64-linux-gcc

Which sets the variable "cross_cc_aarch64"
to "aarch64-linux-gcc=aarch64-linux-gcc".
Which of course won't exist to execute.


r~

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Edgar E. Iglesias]

On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
> >  static inline void msr_write(DisasContext *dc, TCGv v)
> >  {
> > -TCGv t;
> > -
> > -t = tcg_temp_new();
> >  dc->cpustate_changed = 1;
> >  /* PVR bit is not writable.  */
> > -tcg_gen_andi_tl(t, v, ~MSR_PVR);
> > -tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
> > -tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
> > -tcg_temp_free(t);
> > +tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 
> > 1);
> >  }
> 
> Um... the old code was correct, but the new code isn't.
> 
> The new code sets msr to v, with bit 10 set to the old msr bit 0.
> 
> Why do you believe the old code to be wrong?

The old code was incorrectly ORing v instead of t...

What about the following?
tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Philippe Mathieu-Daudé]

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> perror() is defined to fprintf(stderr,...).  HACKING says
> fprintf(stderr,...) is wrong.  So perror() is too.
> 
> Signed-off-by: Ian Jackson 
> CC: Paolo Bonzini 
> CC: Markus Armbruster 
> CC: Daniel P. Berrange 
> CC: Michael Tokarev 
> CC: Alistair Francis 
> ---
> v7: New patch
> ---
>  os-posix.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index d4cf466..0108028 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
>  /* Could rewrite argv[0] too, but that's a bit more complicated.
> This simple way is enough for `top'. */
>  if (prctl(PR_SET_NAME, name)) {
> -perror("unable to change process name");
> +error_report("unable to change process name: %s", strerror(errno));
>  exit(1);
>  }
>  #else
> @@ -247,7 +247,7 @@ static void change_root(void)
>  exit(1);
>  }
>  if (chdir("/")) {
> -perror("not able to chdir to /");
> +error_report("not able to chdir to /: %s", strerror(errno));
>  exit(1);
>  }
>  }
> @@ -309,7 +309,7 @@ void os_setup_post(void)
>  
>  if (daemonize) {
>  if (chdir("/")) {
> -perror("not able to chdir to /");
> +error_report("not able to chdir to /: %s", strerror(errno));
>  exit(1);
>  }
>  TFR(fd = qemu_open("/dev/null", O_RDWR));
> @@ -383,7 +383,7 @@ int os_mlock(void)
>  
>  ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>  if (ret < 0) {
> -perror("mlockall");
> +error_report("mlockall: %s", strerror(errno));
>  }
>  
>  return ret;

Thinking loudly, maybe we can refactor as error_report_errno(const char
*desc)...

Anyway:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Philippe Mathieu-Daudé]

Hi Ian,

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson 
> CC: Paolo Bonzini 
> CC: Markus Armbruster 
> CC: Daniel P. Berrange 
> CC: Michael Tokarev 
> ---
> v7: New patch
> ---
>  os-posix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 0f59566..d4cf466 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -129,7 +129,7 @@ void os_set_proc_name(const char *s)
>  exit(1);
>  }
>  #else
> -fprintf(stderr, "Change of process name not supported by your OS\n");
> +error_report("Change of process name not supported by your OS\n");

removing the trailing "\n":
Reviewed-by: Philippe Mathieu-Daudé 

>  exit(1);
>  #endif
>  }
> @@ -243,7 +243,7 @@ static void change_root(void)
>  {
>  if (chroot_dir) {
>  if (chroot(chroot_dir) < 0) {
> -fprintf(stderr, "chroot failed\n");
> +error_report("chroot failed");
>  exit(1);
>  }
>  if (chdir("/")) {
> 

Re: [Qemu-devel] [PATCH v2 03/43] configure: add support for --cross-cc-FOO

[Richard Henderson]

On 04/19/2018 03:58 AM, Alex Bennée wrote:
> @@ -6805,6 +6823,7 @@ case "$target_name" in
>  bflt="yes"
>  mttcg="yes"
>  gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml"
> +target_compiler=$cross_cc_arm
>;;
>aarch64|aarch64_be)
>  TARGET_ARCH=aarch64
> @@ -6812,6 +6831,7 @@ case "$target_name" in
>  bflt="yes"
>  mttcg="yes"
>  gdb_xml_files="aarch64-core.xml aarch64-fpu.xml arm-core.xml arm-vfp.xml 
> arm-vfp3.xml arm-neon.xml"
> +target_compiler=$cross_cc_aarch64
>;;
>cris)
>;;

Is there any reason not to fill in the reset of the cases within the switch at
the same time?


r~