Re: [Qemu-devel] [PATCH RESEND v15 08/10] target-arm: kvm64: inject synchronous External Abort

[gengdongjiu]

Hi Peter,

On 2018/11/24 2:45, Peter Maydell wrote:
> On Wed, 21 Nov 2018 at 14:34, gengdongjiu  wrote:
>>
>> Hi Peter,
>>   Thanks for the review and comments.
>>
>>>
>>> On 8 November 2018 at 10:29, Dongjiu Geng  wrote:
 +bool write_part_cpustate_to_list(ARMCPU *cpu, ptrdiff_t fieldoffset)
>>>
>>> What is this about? Nothing else in QEMU needs to mess with the cpustate 
>>> synchronization. My first assumption is that you should not
>>> need to do so either.
>>
>> We should change the guest CP15 ESR_EL1's value, the only method is to 
>> change the cpu->cpreg_values[] in QEMU, then QEMU call 
>> write_list_to_kvmstate()
>> to set the cpu->cpreg_values[] to KVM which include the specified ESR_EL1 
>> value, KVM do world switch, and then set the specified ESR_EL1's value to 
>> guest kernel.
> 
> Ah, I see. This is a bug in our current handling of the register
> state, where we implicitly assume that nothing in QEMU will ever
> want to change any system register values. This assumption is
> now false -- kvm_arm_handle_debug() broke it -- so we need to
> fix the code that does kvm_arch_put_registers(). There is a comment
> in the kvm32.c version of that function about this. (The kvm64.c
> version has the same assumption but doesn't comment on it.)
> 
> We should (ideally) fix this bug in the code that does register
> syncing, without requiring places in QEMU that update system
> registers to have to manually indicate which registers they have
> changed. I'll have a think about how best to do this.

Ok, it is great that you will think about it, waiting for your wonderful 
solution

> 
>> About the detailed explanation, as shown in [2].
>>
>> kvm_arm_handle_debug() does not need to do this because QEMU does not need 
>> to change CP15 registers, such as ESR_EL1.
> 
> kvm_arm_handle_debug does change ESR_EL1: it is injecting an exception
> and so should set the exception register. This happens when it
> calls the do_interrupt() hook, because arm_cpu_do_interrupt_aarch64()
> writes to env->cp15.esr_el[new_el].

Yes, I see it, but the env->cp15.esr_el[new_el] shouldn't be successfully set 
to KVM when call kvm_arch_put_registers()

> 
> I'm not entirely sure why this is working today, in fact.
> Alex, did you test whether our debug-exception-injection
> reports the correct ESR_EL1 to the guest ?

Alex?

> 
 +/* Inject synchronous external abort */ static void
 +kvm_inject_arm_sea(CPUState *c) {
 +ARMCPU *cpu = ARM_CPU(c);
 +CPUARMState *env = >env;
 +CPUClass *cc = CPU_GET_CLASS(c);
 +uint32_t esr;
 +int ret;
 +
 +/* This exception is synchronous data abort */
 +c->exception_index = EXCP_DATA_ABORT;
 +/* Inject the exception to guest EL1 */
 +env->exception.target_el = 1;
>>>
>>> These comments don't tell us anything that the code does not.
>>
>>  Thanks, do you mean I need to remove it or add more detailed comments to it?
> 
> As a rule of thumb, comments should provide information to
> the reader which they wouldn't get if they only had the code.
> Comments often answer the "why do we do this" question, or
> provide an overall summary of what the code is going to do,
> or refer to an external source (a datasheet, an algorithm)
> that is necessary to understand the code. It's better to
> avoid comments that say "what the code is doing" at a line-by-line
> level, because the code itself already answers the "what"
> question at that level of detail.

sure, got it, thanks for the explanation and guidelines.

> 
> thanks
> -- PMM
> 
> .
> 

[Qemu-devel] [Bug 1498144] Re: Failure booting hurd with qemu-system-i386 on ARM

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

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

Title:
   Failure booting hurd with qemu-system-i386 on ARM

Status in QEMU:
  Expired

Bug description:
  Trying to boot debian-hurd-20150320.img ends with:

  qemu-system-i386: qemu-coroutine-lock.c:91: qemu_co_queue_restart_all:
  Assertion `qemu_in_coroutine()' failed.

  Program received signal SIGABRT, Aborted.
  __libc_do_syscall ()
  at ../ports/sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:44
  44  ../ports/sysdeps/unix/sysv/linux/arm/libc-do-syscall.S: No such file 
or directory.
  (gdb) bt
  #0  __libc_do_syscall ()
  at ../ports/sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:44
  #1  0xb6ef8f0e in __GI_raise (sig=sig@entry=6)
  at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
  #2  0xb6efb766 in __GI_abort () at abort.c:89
  #3  0xb6ef4150 in __assert_fail_base (
  fmt=0x1 ,
  assertion=0x7f89a234 "qemu_in_coroutine()", assertion@entry=0x0,
  file=0x7f89da58 "qemu-coroutine-lock.c", file@entry=0xb566 "\001",
  line=91, line@entry=3069931692,
  function=function@entry=0x7f89ab78 "qemu_co_queue_restart_all")
  at assert.c:92
  #4  0xb6ef41e6 in __GI___assert_fail (assertion=0x0, file=0xb566 "\001",
  line=3069931692, function=0x7f89ab78 "qemu_co_queue_restart_all")
  at assert.c:101
  #5  0x7f59a6b4 in ?? ()

  I was using the same setup as in Bug 893208 (i.e git checkout from
  2015-09-15, armv7 Odroid C1)

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

Re: [Qemu-devel] [PATCH v3 2/5] util: introduce threaded workqueue

[Emilio G. Cota]

On Thu, Nov 22, 2018 at 15:20:25 +0800, guangrong.x...@gmail.com wrote:
> +static uint64_t get_free_request_bitmap(Threads *threads, ThreadLocal 
> *thread)
> +{
> +uint64_t request_fill_bitmap, request_done_bitmap, result_bitmap;
> +
> +request_fill_bitmap = atomic_rcu_read(>request_fill_bitmap);
> +request_done_bitmap = atomic_rcu_read(>request_done_bitmap);
> +bitmap_xor(_bitmap, _fill_bitmap, _done_bitmap,
> +   threads->thread_requests_nr);

This is not wrong, but it's a big ugly. Instead, I would:

- Introduce bitmap_xor_atomic in a previous patch
- Use bitmap_xor_atomic here, getting rid of the rcu reads

Thanks,

Emilio

Re: [Qemu-devel] [PATCH v3 2/5] util: introduce threaded workqueue

[Emilio G. Cota]

On Thu, Nov 22, 2018 at 15:20:25 +0800, guangrong.x...@gmail.com wrote:
> +   /*
> + * the bit in these two bitmaps indicates the index of the ＠requests

This @ is not ASCII, is it?

> + * respectively. If it's the same, the corresponding request is free
> + * and owned by the user, i.e, where the user fills a request. Otherwise,
> + * it is valid and owned by the thread, i.e, where the thread fetches
> + * the request and write the result.
> + */
> +
> +/* after the user fills the request, the bit is flipped. */
> +uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
> +/* after handles the request, the thread flips the bit. */
> +uint64_t request_done_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);

Use DECLARE_BITMAP, otherwise you'll get type errors as David
pointed out.

Thanks,

Emilio

Re: [Qemu-devel] [RFC 12/48] atomic_template: define pre/post macros

[Emilio G. Cota]

On Thu, Nov 22, 2018 at 17:12:34 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > In preparation for plugin support.
> >
> > Signed-off-by: Emilio G. Cota 
> 
> More macros for the macro-god. I guess this works but I wonder if it's
> possible to do a clean-up ala softfloat and the experimental softmmu
> re-factor that makes this less a mess of macros?

Heh, point taken.

I'll try to fix this once (and if!) the series graduates from RFC
to proper patchset. BTW I'm planning to pick rth's softfloat
cleanup soon, so that should help.

Thanks,

Emilio

Re: [Qemu-devel] [RFC 21/48] *-user: plugin syscalls

[Emilio G. Cota]

On Fri, Nov 23, 2018 at 17:04:28 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > Signed-off-by: Emilio G. Cota 
> > ---
> >  bsd-user/syscall.c   | 9 +
> >  linux-user/syscall.c | 3 +++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
> > index b7818af450..4993f81b2b 100644
> > --- a/bsd-user/syscall.c
> > +++ b/bsd-user/syscall.c
> > @@ -323,6 +323,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, 
> > abi_long arg1,
> >  gemu_log("freebsd syscall %d\n", num);
> >  #endif
> >  trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> > arg7, arg8);
> > +qemu_plugin_vcpu_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> > arg7,
> > + arg8);
> 
> I think we discussed this on my series about avoiding this sort of
> duplication by providing a wrapper for trace points that are also plugin
> hooks. So something like:
> 
>trace_and_plugin_guest_user_syscall(...)
> 
> Although it's probably worth keeping the trace names grep-able so maybe:
> 
>plug_trace_guest_user_syscall(...)
> 
> ?

Yes, I remember that discussion and agree that a common function here
would be best.

Emilio

Re: [Qemu-devel] [RFC 16/48] tcg: add plugin_mask to TB hash

[Emilio G. Cota]

On Fri, Nov 23, 2018 at 16:52:31 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > Signed-off-by: Emilio G. Cota 
> 
> This commit message needs more description. What is this plugin mask
> for? Are we extending the TB flags with a mask of loaded plugins so we
> can correctly identify what code was generated under what plugins?

This is similar to the bitmap for trace events, which we added to the
TB hash so that when the TCG events have no subscribers left, we just
regenerate the TBs.

It's true though that in this RFC there are no good use cases for this,
because the callbacks that affect performance are all unconditionally
embedded in the generated code. So at least for now we could drop
this patch.

Thanks,

Emilio

Re: [Qemu-devel] [RFC 22/48] cpu: hook plugin vcpu events

[Emilio G. Cota]

On Fri, Nov 23, 2018 at 17:10:53 +, Alex Bennée wrote:
> Emilio G. Cota  writes:
(snip)
> > @@ -1322,12 +1323,21 @@ static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
> >
> >  static void qemu_wait_io_event(CPUState *cpu)
> >  {
> > +bool asleep = false;
> > +
> 
> slept?

Changed to slept, thanks.

> >  g_assert(cpu_mutex_locked(cpu));
> >  g_assert(!qemu_mutex_iothread_locked());
> >
> >  while (cpu_thread_is_idle(cpu)) {
> > +if (!asleep) {
> > +asleep = true;
> > +qemu_plugin_vcpu_idle_cb(cpu);
> > +}
> >  qemu_cond_wait(>halt_cond, >lock);
> >  }
> > +if (asleep) {
> > +qemu_plugin_vcpu_resume_cb(cpu);
> > +}
> 
> I wonder if having two hooks is too much? What might a plugin want to do
> before we go into idle sleep?
> 
> It feels like we are exposing too much of the guts of TCG to the plugin
> here as wait_io could be for any number of internal reasons other than
> the actual emulation blocking for IO through a WFI or something. If a
> plugin really wants to track such things shouldn't it be hooking to the
> guest sleep points?
> 
> If idle sleeps really are that important maybe we could just report our
> sleep time on resume - so a single hook but passing a bit more
> information?

I don't have all the use cases for this figured out. For now I'm using
this in plugins as a way to know when a vCPU is for sure idle, which
is used in memory reclamation schemes such as RCU.

What are the "guest sleep points" you mention? Are they target-agnostic?

Thanks,

Emilio

Re: [Qemu-devel] [RFC 10/48] exec: export do_tb_flush

[Emilio G. Cota]

On Thu, Nov 22, 2018 at 17:09:22 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > This will be used by plugin code to flush the code cache as well
> > as doing other bookkeeping in a safe work environment.
> 
> This seems a little excessive given the plugin code could just call
> tb_flush() directly. Wouldn't calling tb_flush after scheduling the
> plugin_destroy be enough?
> 
> If there is a race condition here maybe we could build some sort of
> awareness into tb_flush as to the current run state. But having two
> entry points to this rather fundamental action seems likely to either be
> misused or misunderstood.

We have to make sure that no callback left in the generated code is
called once a plugin has been uninstalled. To me, using the same safe
work window to both flush the TB and uninstall the plugin seems the
simplest way to do this.

Thanks,

Emilio

Re: [Qemu-devel] [RFC 19/48] translate-all: notify plugin code of tb_flush

[Emilio G. Cota]

On Fri, Nov 23, 2018 at 17:00:59 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > Signed-off-by: Emilio G. Cota 
> > ---
> >  accel/tcg/translate-all.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index 3423cf74db..1690e3fd5b 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, 
> > gpointer value, gpointer data)
> >  /* flush all the translation blocks */
> >  void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
> >  {
> > +bool did_flush = false;
> > +
> >  mmap_lock();
> >  /* If it is already been done on request of another CPU,
> >   * just retry.
> > @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data 
> > tb_flush_count)
> >  if (tb_ctx.tb_flush_count != tb_flush_count.host_int) {
> >  goto done;
> >  }
> > +did_flush = true;
> >
> >  if (DEBUG_TB_FLUSH_GATE) {
> >  size_t nb_tbs = tcg_nb_tbs();
> > @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data 
> > tb_flush_count)
> >
> >  done:
> >  mmap_unlock();
> > +if (did_flush) {
> > +qemu_plugin_flush_cb();
> > +}
> 
> Are we introducing a race here?

A race, how? We're in an async safe environment here, i.e. no other
vCPU is running.

> What is the purpose of letting the plugin know a flush has occurred?

Plugins might allocate per-TB data that then they get passed each
time the TB is executed (via the *userdata pointer). For example,
in a simulator we'd allocate a per-TB struct that describes the
guest instructions, after having disassembled them at translate time.

It is therefore useful for plugins to know when all TB's have been
flushed, so that they can then free that per-TB data.

> It shouldn't have any knowledge of the details of liveliness of the
> translated code and if it still exits or not. If all it wants to do is
> look at the counts then I think we can provide a simpler less abuse-able
> way to do this.

I'm confused. What does "look at the counts" mean here?

To reiterate, plugins should have a way to know when a TB doesn't
exist any longer, so that they can reclaim memory.

Thanks,

Emilio

[Qemu-devel] [PATCH 0/4] xxhash patches for 4.0

[Emilio G. Cota]

(Plus a qht-bench trivial patch.)

Note that these apply on top of rth's tcg-next-for-4.0.

Thanks,

Emilio

[Qemu-devel] [PATCH 3/4] include: move exec/tb-hash-xx.h to qemu/xxhash.h

[Emilio G. Cota]

Reviewed-by: Alex Bennée 
Signed-off-by: Emilio G. Cota 
---
 include/exec/tb-hash.h   | 2 +-
 include/{exec/tb-hash-xx.h => qemu/xxhash.h} | 6 +++---
 tests/qht-bench.c| 2 +-
 util/qsp.c   | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)
 rename include/{exec/tb-hash-xx.h => qemu/xxhash.h} (97%)

diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
index 731ba4c272..4f3a37d927 100644
--- a/include/exec/tb-hash.h
+++ b/include/exec/tb-hash.h
@@ -20,7 +20,7 @@
 #ifndef EXEC_TB_HASH_H
 #define EXEC_TB_HASH_H
 
-#include "exec/tb-hash-xx.h"
+#include "qemu/xxhash.h"
 
 #ifdef CONFIG_SOFTMMU
 
diff --git a/include/exec/tb-hash-xx.h b/include/qemu/xxhash.h
similarity index 97%
rename from include/exec/tb-hash-xx.h
rename to include/qemu/xxhash.h
index 98ce4b628a..fe35dde328 100644
--- a/include/exec/tb-hash-xx.h
+++ b/include/qemu/xxhash.h
@@ -31,8 +31,8 @@
  * - xxHash source repository : https://github.com/Cyan4973/xxHash
  */
 
-#ifndef EXEC_TB_HASH_XX_H
-#define EXEC_TB_HASH_XX_H
+#ifndef QEMU_XXHASH_H
+#define QEMU_XXHASH_H
 
 #include "qemu/bitops.h"
 
@@ -119,4 +119,4 @@ static inline uint32_t qemu_xxhash6(uint64_t ab, uint64_t 
cd, uint32_t e,
 return qemu_xxhash7(ab, cd, e, f, 0);
 }
 
-#endif /* EXEC_TB_HASH_XX_H */
+#endif /* QEMU_XXHASH_H */
diff --git a/tests/qht-bench.c b/tests/qht-bench.c
index 0278f4da04..ab4e708180 100644
--- a/tests/qht-bench.c
+++ b/tests/qht-bench.c
@@ -9,7 +9,7 @@
 #include "qemu/atomic.h"
 #include "qemu/qht.h"
 #include "qemu/rcu.h"
-#include "exec/tb-hash-xx.h"
+#include "qemu/xxhash.h"
 
 struct thread_stats {
 size_t rd;
diff --git a/util/qsp.c b/util/qsp.c
index dc29c41fde..410f1ba004 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -61,7 +61,7 @@
 #include "qemu/timer.h"
 #include "qemu/qht.h"
 #include "qemu/rcu.h"
-#include "exec/tb-hash-xx.h"
+#include "qemu/xxhash.h"
 
 enum QSPType {
 QSP_MUTEX,
-- 
2.17.1

[Qemu-devel] [PATCH 2/4] exec: introduce qemu_xxhash{2,4,5,6,7}

[Emilio G. Cota]

Before moving them all to include/qemu/xxhash.h.

Reviewed-by: Alex Bennée 
Signed-off-by: Emilio G. Cota 
---
 include/exec/tb-hash-xx.h | 41 +--
 include/exec/tb-hash.h|  2 +-
 tests/qht-bench.c |  2 +-
 util/qsp.c| 12 ++--
 4 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/include/exec/tb-hash-xx.h b/include/exec/tb-hash-xx.h
index 747a9a612c..98ce4b628a 100644
--- a/include/exec/tb-hash-xx.h
+++ b/include/exec/tb-hash-xx.h
@@ -42,23 +42,23 @@
 #define PRIME32_4668265263U
 #define PRIME32_5374761393U
 
-#define TB_HASH_XX_SEED 1
+#define QEMU_XXHASH_SEED 1
 
 /*
  * xxhash32, customized for input variables that are not guaranteed to be
  * contiguous in memory.
  */
 static inline uint32_t
-tb_hash_func7(uint64_t a0, uint64_t b0, uint32_t e, uint32_t f, uint32_t g)
+qemu_xxhash7(uint64_t ab, uint64_t cd, uint32_t e, uint32_t f, uint32_t g)
 {
-uint32_t v1 = TB_HASH_XX_SEED + PRIME32_1 + PRIME32_2;
-uint32_t v2 = TB_HASH_XX_SEED + PRIME32_2;
-uint32_t v3 = TB_HASH_XX_SEED + 0;
-uint32_t v4 = TB_HASH_XX_SEED - PRIME32_1;
-uint32_t a = a0 >> 32;
-uint32_t b = a0;
-uint32_t c = b0 >> 32;
-uint32_t d = b0;
+uint32_t v1 = QEMU_XXHASH_SEED + PRIME32_1 + PRIME32_2;
+uint32_t v2 = QEMU_XXHASH_SEED + PRIME32_2;
+uint32_t v3 = QEMU_XXHASH_SEED + 0;
+uint32_t v4 = QEMU_XXHASH_SEED - PRIME32_1;
+uint32_t a = ab >> 32;
+uint32_t b = ab;
+uint32_t c = cd >> 32;
+uint32_t d = cd;
 uint32_t h32;
 
 v1 += a * PRIME32_2;
@@ -98,4 +98,25 @@ tb_hash_func7(uint64_t a0, uint64_t b0, uint32_t e, uint32_t 
f, uint32_t g)
 return h32;
 }
 
+static inline uint32_t qemu_xxhash2(uint64_t ab)
+{
+return qemu_xxhash7(ab, 0, 0, 0, 0);
+}
+
+static inline uint32_t qemu_xxhash4(uint64_t ab, uint64_t cd)
+{
+return qemu_xxhash7(ab, cd, 0, 0, 0);
+}
+
+static inline uint32_t qemu_xxhash5(uint64_t ab, uint64_t cd, uint32_t e)
+{
+return qemu_xxhash7(ab, cd, e, 0, 0);
+}
+
+static inline uint32_t qemu_xxhash6(uint64_t ab, uint64_t cd, uint32_t e,
+uint32_t f)
+{
+return qemu_xxhash7(ab, cd, e, f, 0);
+}
+
 #endif /* EXEC_TB_HASH_XX_H */
diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
index 0526c4f678..731ba4c272 100644
--- a/include/exec/tb-hash.h
+++ b/include/exec/tb-hash.h
@@ -61,7 +61,7 @@ static inline
 uint32_t tb_hash_func(tb_page_addr_t phys_pc, target_ulong pc, uint32_t flags,
   uint32_t cf_mask, uint32_t trace_vcpu_dstate)
 {
-return tb_hash_func7(phys_pc, pc, flags, cf_mask, trace_vcpu_dstate);
+return qemu_xxhash7(phys_pc, pc, flags, cf_mask, trace_vcpu_dstate);
 }
 
 #endif
diff --git a/tests/qht-bench.c b/tests/qht-bench.c
index 636750d39f..0278f4da04 100644
--- a/tests/qht-bench.c
+++ b/tests/qht-bench.c
@@ -105,7 +105,7 @@ static bool is_equal(const void *ap, const void *bp)
 
 static uint32_t h(unsigned long v)
 {
-return tb_hash_func7(v, 0, 0, 0, 0);
+return qemu_xxhash2(v);
 }
 
 static uint32_t hval(unsigned long v)
diff --git a/util/qsp.c b/util/qsp.c
index a848b09c6d..dc29c41fde 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -135,13 +135,13 @@ QemuCondWaitFunc qemu_cond_wait_func = 
qemu_cond_wait_impl;
  * without it we still get a pretty unique hash.
  */
 static inline
-uint32_t do_qsp_callsite_hash(const QSPCallSite *callsite, uint64_t a)
+uint32_t do_qsp_callsite_hash(const QSPCallSite *callsite, uint64_t ab)
 {
-uint64_t b = (uint64_t)(uintptr_t)callsite->obj;
+uint64_t cd = (uint64_t)(uintptr_t)callsite->obj;
 uint32_t e = callsite->line;
 uint32_t f = callsite->type;
 
-return tb_hash_func7(a, b, e, f, 0);
+return qemu_xxhash6(ab, cd, e, f);
 }
 
 static inline
@@ -169,11 +169,11 @@ static uint32_t qsp_entry_no_thread_hash(const QSPEntry 
*entry)
 static uint32_t qsp_entry_no_thread_obj_hash(const QSPEntry *entry)
 {
 const QSPCallSite *callsite = entry->callsite;
-uint64_t a = g_str_hash(callsite->file);
-uint64_t b = callsite->line;
+uint64_t ab = g_str_hash(callsite->file);
+uint64_t cd = callsite->line;
 uint32_t e = callsite->type;
 
-return tb_hash_func7(a, b, e, 0, 0);
+return qemu_xxhash5(ab, cd, e);
 }
 
 static bool qsp_callsite_cmp(const void *ap, const void *bp)
-- 
2.17.1

[Qemu-devel] [PATCH 1/4] qht-bench: document -p flag

[Emilio G. Cota]

Which we forgot to do in bd224fce60 ("qht-bench: add -p flag
to precompute hash values", 2018-09-26).

Signed-off-by: Emilio G. Cota 
---
 tests/qht-bench.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qht-bench.c b/tests/qht-bench.c
index 2089e2bed1..636750d39f 100644
--- a/tests/qht-bench.c
+++ b/tests/qht-bench.c
@@ -72,6 +72,7 @@ static const char commands_string[] =
 " -n = number of threads\n"
 "\n"
 " -o = offset at which keys start\n"
+" -p = precompute hashes\n"
 "\n"
 " -g = set -s,-k,-K,-l,-r to the same value\n"
 " -s = initial size hint\n"
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] xxhash: match output against the original xxhash32

[Emilio G. Cota]

Change the order in which we extract a/b and c/d to
match the output of the upstream xxhash32.

Tested with:
  https://github.com/cota/xxhash/tree/qemu

Signed-off-by: Emilio G. Cota 
---
 include/qemu/xxhash.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
index fe35dde328..076f1f6054 100644
--- a/include/qemu/xxhash.h
+++ b/include/qemu/xxhash.h
@@ -55,10 +55,10 @@ qemu_xxhash7(uint64_t ab, uint64_t cd, uint32_t e, uint32_t 
f, uint32_t g)
 uint32_t v2 = QEMU_XXHASH_SEED + PRIME32_2;
 uint32_t v3 = QEMU_XXHASH_SEED + 0;
 uint32_t v4 = QEMU_XXHASH_SEED - PRIME32_1;
-uint32_t a = ab >> 32;
-uint32_t b = ab;
-uint32_t c = cd >> 32;
-uint32_t d = cd;
+uint32_t a = ab;
+uint32_t b = ab >> 32;
+uint32_t c = cd;
+uint32_t d = cd >> 32;
 uint32_t h32;
 
 v1 += a * PRIME32_2;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Eduardo Habkost]

On Fri, Nov 23, 2018 at 06:53:07PM +, Peter Maydell wrote:
> On Fri, 23 Nov 2018 at 18:24, Eduardo Habkost  wrote:
> >
> > On Fri, Nov 23, 2018 at 06:14:28PM +, Peter Maydell wrote:
> > > One thing I would like to do with this new "cpu cluster"
> > > concept is to use it to handle a problem we have at the
> > > moment with TCG, where we assume all CPUs have the same
> > > view of physical memory (and so if CPU A executes from physical
> > > address X it can share translated code with CPU B executing
> > > from physical address X). The idea is that we should include
> > > the CPU cluster number in the TCG hash key that we use to
> > > look up cached translation blocks, so that only CPUs in
> > > the same cluster (assumed to have the same view of memory
> > > and to be identical) share TBs.
> > >
> > > If we don't have a unique integer key for the cluster, what
> > > should we use instead ?
> >
> > This sounds like a reasonable use of cluster_id as implemented in
> > this patch.  The ID would be only used internally and not exposed
> > to the outside, right?
> 
> It would be internal to QEMU (not exposed to the guest or
> to the user), yes.
> 
> > I'm more worried about cases where we could end up exposing the
> > ID in an external interface (either to guests, or through QMP or
> > the command-line).  This happened to cpu_index and we took a long
> > time to fix the mess.
> 
> I see, thanks.
> 
> My other question about this code was a slightly different one -- are
> we guaranteed to be holding the iothread lock when we create
> new QOM objects? (ie that we won't have races between two threads
> which both try to create new objects and increment the variable)

I assume we are, because type_initialize() (called by
object_new()) isn't thread-safe.

-- 
Eduardo

Re: [Qemu-devel] [RFC 13/48] xxhash: add qemu_xxhash8

[Emilio G. Cota]

On Thu, Nov 22, 2018 at 17:15:20 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > It will be used for TB hashing soon.
> >
> > Signed-off-by: Emilio G. Cota 
> > ---
> >  include/qemu/xxhash.h | 40 +++-
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
> > index fe35dde328..450427eeaa 100644
> > --- a/include/qemu/xxhash.h
> > +++ b/include/qemu/xxhash.h
> > @@ -49,7 +49,8 @@
> >   * contiguous in memory.
> >   */
> >  static inline uint32_t
> > -qemu_xxhash7(uint64_t ab, uint64_t cd, uint32_t e, uint32_t f, uint32_t g)
> > +qemu_xxhash8(uint64_t ab, uint64_t cd, uint32_t e, uint32_t f, uint32_t g,
> > + uint32_t h)
> 
> As we've expanded to bigger and bigger hashes why are everything after
> cd passed as 32 bit values? Isn't this just generating extra register
> pressure or is the compiler smart enough to figure it out?

The latter -- the compiler does a good job with constant propagation.

> >  {
> >  uint32_t v1 = QEMU_XXHASH_SEED + PRIME32_1 + PRIME32_2;
> >  uint32_t v2 = QEMU_XXHASH_SEED + PRIME32_2;
> > @@ -77,17 +78,24 @@ qemu_xxhash7(uint64_t ab, uint64_t cd, uint32_t e, 
> > uint32_t f, uint32_t g)
> >  v4 = rol32(v4, 13);
> >  v4 *= PRIME32_1;
> >
> > -h32 = rol32(v1, 1) + rol32(v2, 7) + rol32(v3, 12) + rol32(v4, 18);
> > -h32 += 28;
> > +v1 += e * PRIME32_2;
> > +v1 = rol32(v1, 13);
> > +v1 *= PRIME32_1;
(snip)
> How do we validate we haven't broken the distribution of the original
> xxhash as we've extended it?

We weren't testing for that, so this is a valid concern.

I just added a test to my xxhash repo to compare our version against
the original:
  https://github.com/cota/xxhash/tree/qemu

Turns out that to exactly match the original we just have to swap our
a/b and c/d pairs. I don't see how this could cause a loss of randomness,
but just to be safe I'll send a for-4.0 patch so that our output matches
the original one.

Thanks,

Emilio

Re: [Qemu-devel] KVM Forum VFIO BoF summary

[Felipe Franciosi]

Hi Alex,

I'm also terribly sorry for the delay in responding to this. I'm only now 
having the time resources to come back to this topic and figure out a way 
forward with my proposal. Please see my notes below (quoting only the relevant 
sections).

> On Nov 6, 2018, at 9:32 PM, Alex Williamson  
> wrote:
> 
> There were mostly two main threads of discussion, the first was Filipe's
> discussion of a socket interface making use of the VFIO ABI to
> implement a userspace device model.  For a VM use case, QEMU would be
> modified to use a socket for the VFIO ABI, including shared memory for
> DMA.  Ideally this would be transparent for much of the QEMU vfio code
> outside of setup.  Effectively this becomes a userspace implementation
> of mdev as this could already be done using mdev, but it requires a
> vendor driver to expose the userspace interface and likely a longer
> round trip through the kernel and back to userspace. Either path
> allows device models in userspace that can be open or proprietary, but
> this new proposal eliminates the vendor kernel driver component.  Of
> course non-upstream kernel mdev vendor drivers do taint the host
> kernel, so they at least leave a breadcrumb.

Spot on. This can be done today via mdev, but it would unnecessarily involve 
the kernel in the setup of devices. Also, since vfio is the only existing bus 
driver for mdev, it would make little sense to implement an mdev "physical" 
driver (which wouldn't have a physical backing device) just to talk a vfio-like 
abi back to userspace.

This "vfio-user" proposal is a perfect fit for Qemu. I think the correct design 
is to slightly rearchitect the vfio implementation in Qemu to allow for 
userspace backends through unix sockets, very much like vhost-user does it for 
virtio devices today.

As a matter of fact, with something like this in place, I think the Qemu code 
could potentially be simplified by moving some of the existing vhost-user 
offloading code underneath it.

> I believe Filipe
> mentioned a conversation after the BoF with Alex Graf who had a related
> concern about using the socket approach to use this as a non-GPL
> backdoor for device models in QEMU and a suggested approach was a GPL
> handshake via the socket interface.

Not exactly. The GPL-violating concerns came from Paolo and Stefan (cc'd). Alex 
Graf (cc'd) came up with a solution for the concern which involves adding a 
copyrighted "poem" to the protocol handshake. Qemu would then grant the 
copyright for GPL applications, hence limiting who can use the protocol.

Vendors can obviously bypass this by using a GPL proxy application that talks 
to Qemu, but then uses a separate mechanism (possibly another unix socket) to 
talk to other non-GPL applications. Vendors which distribute Qemu can possibly 
also bypass this by modifying the protocol not to include the copyrighted 
messages. Using either legal loophole approach, however, can lead to a lot of 
public point and shame.

The point I would like to discuss more widely (before going forwards with code) 
is whether vendors can actually make use of a GPL-only protocol. I understand 
the community desire of gathering efforts into GPL software, Qemu code and 
Virtio. A non-GPL implementation means that maybe some vendors cannot use this.

I envision this protocol being used by userspace applications which _need_ to 
emulate a device outside Qemu for performance reasons, not license reasons (eg. 
a separate process can efficiently poll VQs from multiple VMs from a single 
core). Applications that already do that today, albeit open-source, are not GPL 
(eg. OVS, DPDK, SPDK).

This is why I'd like to hear more from Paolo and Stefan on why making this 
non-GPL is so bad. It is already possible today (via mdev), and similar 
approaches already exist for a large number of device implementations via 
vhost-user. Additionally, I would also like to hear from willing vendors that 
feel they could _only_ benefit from this if it does not enforce GPL.

Finally, I want to clarify I am not opposing making this GPL-only. I just want 
to make sure the effort is justifiable (ie. there is reason for this to enforce 
GPL peer applications given the existing vhost-user and mdev alternatives) and 
not in vain (ie. there are GPL use cases for this out there).

Thanks,
Felipe

Re: [Qemu-devel] [PATCH for-4.0 v2 00/37] tcg: Assorted cleanups

[no-reply]

Hi,

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

Message-id: 20181123144558.5048-1-richard.hender...@linaro.org
Type: series
Subject: [Qemu-devel] [PATCH for-4.0 v2 00/37] tcg: Assorted cleanups

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
fc046e8 tcg/i386: Remove L constraint
25a6e0e tcg/i386: Require segment syscalls to succeed
5e0fd5d tcg/i386: Add setup_guest_base_seg for FreeBSD
d68fe20 tcg/i386: Restrict user-only qemu_st_i32 values to q-regs
85fcd2d tcg/i386: Propagate is64 to tcg_out_qemu_ld_direct
a3178a9 tcg/arm: Set TCG_TARGET_HAS_MEMORY_BSWAP to false for user-only
4d0c5fc tcg/aarch64: Set TCG_TARGET_HAS_MEMORY_BSWAP to false
676c67e tcg/i386: Adjust TCG_TARGET_HAS_MEMORY_BSWAP
38c85b7 tcg: Add TCG_TARGET_HAS_MEMORY_BSWAP
d3a1899 tcg/optimize: Optimize bswap
3ad8388 tcg: Clean up generic bswap64
affd2d8 tcg: Clean up generic bswap32
fd89430 tcg/ppc: Use TCG_TARGET_NEED_LDST_OOL_LABELS
8aa5784 tcg/ppc: Force qemu_ld/st arguments into fixed registers
af59cb6 tcg/ppc: Change TCG_TARGET_CALL_ALIGN_ARGS to bool
26bfc9a tcg/ppc: Add constraints for R7-R8
6cd8567 tcg/ppc: Split out tcg_out_call_int
d40e505 tcg/ppc: Parameterize the temps for tcg_out_tlb_read
acfcb89 tcg/arm: Use TCG_TARGET_NEED_LDST_OOL_LABELS
5516052 tcg/arm: Force qemu_ld/st arguments into fixed registers
46ad1f6 tcg/arm: Reduce the number of temps for tcg_out_tlb_read
24abc30 tcg/arm: Add constraints for R0-R5
94b24c4 tcg/arm: Parameterize the temps for tcg_out_tlb_read
91b0db1 tcg/aarch64: Use TCG_TARGET_NEED_LDST_OOL_LABELS
7129f8e tcg/aarch64: Use B not BL for tcg_out_goto_long
c770cee tcg/aarch64: Parameterize the temp for tcg_out_goto_long
412bf17 tcg/aarch64: Parameterize the temps for tcg_out_tlb_read
2ffa3ee tcg/aarch64: Add constraints for x0, x1, x2
a403293 tcg/i386: Use TCG_TARGET_NEED_LDST_OOL_LABELS
a65b8a9 tcg/i386: Force qemu_ld/st arguments into fixed registers
4053d4c tcg/i386: Change TCG_REG_L[01] to not overlap function arguments
c9e6cd5 tcg/i386: Return a base register from tcg_out_tlb_load
a601058 tcg/i386: Add constraints for r8 and r9
41701df tcg: Add TCG_TARGET_NEED_LDST_OOL_LABELS
fdb09e0 tcg: Return success from patch_reloc
8dab265 tcg/i386: Move TCG_REG_CALL_STACK from define to enum
58990b6 tcg/i386: Always use %ebp for TCG_AREG0

=== OUTPUT BEGIN ===
Checking PATCH 1/37: tcg/i386: Always use %ebp for TCG_AREG0...
Checking PATCH 2/37: tcg/i386: Move TCG_REG_CALL_STACK from define to enum...
Checking PATCH 3/37: tcg: Return success from patch_reloc...
Checking PATCH 4/37: tcg: Add TCG_TARGET_NEED_LDST_OOL_LABELS...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#47: 
new file mode 100644

total: 0 errors, 1 warnings, 192 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/37: tcg/i386: Add constraints for r8 and r9...
Checking PATCH 6/37: tcg/i386: Return a base register from tcg_out_tlb_load...
Checking PATCH 7/37: tcg/i386: Change TCG_REG_L[01] to not overlap function 
arguments...
Checking PATCH 8/37: tcg/i386: Force qemu_ld/st arguments into fixed 
registers...
Checking PATCH 9/37: tcg/i386: Use TCG_TARGET_NEED_LDST_OOL_LABELS...
Checking PATCH 10/37: tcg/aarch64: Add constraints for x0, x1, x2...
Checking PATCH 11/37: tcg/aarch64: Parameterize the temps for 
tcg_out_tlb_read...
Checking PATCH 12/37: tcg/aarch64: Parameterize the temp for 
tcg_out_goto_long...
Checking PATCH 13/37: tcg/aarch64: Use B not BL for tcg_out_goto_long...
Checking PATCH 14/37: tcg/aarch64: Use TCG_TARGET_NEED_LDST_OOL_LABELS...
Checking PATCH 15/37: tcg/arm: Parameterize the temps for tcg_out_tlb_read...
Checking PATCH 16/37: tcg/arm: Add constraints for R0-R5...
Checking PATCH 17/37: tcg/arm: Reduce the number of temps for 
tcg_out_tlb_read...
Checking PATCH 18/37: tcg/arm: Force qemu_ld/st arguments into fixed 
registers...
Checking PATCH 19/37: tcg/arm: Use TCG_TARGET_NEED_LDST_OOL_LABELS...
ERROR: externs should be avoided in .c files
#169: FILE: tcg/arm/tcg-target.inc.c:1483:
+TCGReg addrlo __attribute__((unused));

ERROR: externs should be avoided in .c files
#170: FILE: tcg/arm/tcg-target.inc.c:1484:
+TCGReg addrhi __attribute__((unused));

ERROR: externs should be avoided in .c files
#171: FILE: tcg/arm/tcg-target.inc.c:1485:
+   

Re: [Qemu-devel] [PATCH v5 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:35, Anton Nefedov wrote:
> A block driver can provide a callback to report driver-specific
> statistics.
> 
> file-posix driver now reports discard statistics
> 
> Signed-off-by: Anton Nefedov 
> ---
>   qapi/block-core.json  | 39 +++
>   include/block/block.h |  1 +
>   include/block/block_int.h |  1 +
>   block.c   |  9 +
>   block/file-posix.c| 17 +
>   block/qapi.c  |  5 +
>   6 files changed, 72 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 01da84cb61..cd0344435e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -877,6 +877,42 @@
>  '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
>  '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>   
> +##
> +# @BlockStatsSpecificFile:
> +#
> +# File driver statistics
> +#
> +# @discard-nb-ok: The number of succeeded discard operations performed by
> +# the driver.
> +#
> +# @discard-nb-failed: The number of failed discard operations performed by
> +# the driver.
> +#
> +# @discard-bytes-ok: The number of bytes discarded by the driver.
> +#
> +# Since 3.1

colon after Since:
Since: 3.1

> +##
> +{ 'struct': 'BlockStatsSpecificFile',
> +  'data': {
> +  'discard-nb-ok': 'int',
> +  'discard-nb-failed': 'int',
> +  'discard-bytes-ok': 'int'
> +  } }

the common stile here is no extra \n around braces, like:

{ 'struct': 'BlockStatsSpecificFile',
   'data': { 'discard-nb-ok': 'int',
 'discard-nb-failed': 'int',
 'discard-bytes-ok': 'int' } }


> +
> +##
> +# @BlockStatsSpecific:
> +#
> +# Block driver specific statistics
> +#
> +# Since: 3.1
> +##
> +{ 'union': 'BlockStatsSpecific',
> +  'base': { 'driver': 'BlockdevDriver' },
> +  'discriminator': 'driver',
> +  'data': {
> +  'file': 'BlockStatsSpecificFile'
> +  } }

and here.

> +
>   ##
>   # @BlockStats:
>   #
> @@ -892,6 +928,8 @@
>   #
>   # @stats:  A @BlockDeviceStats for the device.
>   #
> +# @driver-specific: Optional driver-specific stats. (Since 3.1)
> +#
>   # @parent: This describes the file block device if it has one.
>   #  Contains recursively the statistics of the underlying
>   #  protocol (e.g. the host file for a qcow2 image). If there is
> @@ -905,6 +943,7 @@
>   { 'struct': 'BlockStats',
> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>  'stats': 'BlockDeviceStats',
> +   '*driver-specific': 'BlockStatsSpecific',

[offtopic]
hmm, do anyone know, why thunderbird when quoting patches adds "> " between all 
lines except ones starting with a space, and for lines, starting with space, it 
adds ">  " (two extra spaces, not one), which leads to wrong indenting in 
quotes? And how to fix that?
[/offtopic]

>  '*parent': 'BlockStats',
>  '*backing': 'BlockStats'} }
>   
> diff --git a/include/block/block.h b/include/block/block.h
> index b189cf422e..07a3b31386 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -475,6 +475,7 @@ const char *bdrv_get_device_or_node_name(const 
> BlockDriverState *bs);
>   int bdrv_get_flags(BlockDriverState *bs);
>   int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>   ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
> +BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
>   void bdrv_round_to_clusters(BlockDriverState *bs,
>   int64_t offset, int64_t bytes,
>   int64_t *cluster_offset,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f605622216..236d4aceef 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -320,6 +320,7 @@ struct BlockDriver {
> Error **errp);
>   int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>   ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
> +BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs);
>   
>   int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
> QEMUIOVector *qiov,
> diff --git a/block.c b/block.c
> index 95d8635aa1..1e5bba4ac6 100644
> --- a/block.c
> +++ b/block.c
> @@ -4244,6 +4244,15 @@ ImageInfoSpecific 
> *bdrv_get_specific_info(BlockDriverState *bs)
>   return NULL;
>   }
>   
> +BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs)
> +{
> +BlockDriver *drv = bs->drv;
> +if (!drv || !drv->bdrv_get_specific_stats) {
> +return NULL;
> +}
> +return drv->bdrv_get_specific_stats(bs);
> +}
> +
>   void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
>   {
>   if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 

Re: [Qemu-devel] [Qemu-arm] [PATCH RESEND v15 10/10] target-arm: kvm64: handle SIGBUS signal from kernel or KVM

[Peter Maydell]

On Fri, 23 Nov 2018 at 14:31, gengdongjiu  wrote:
>
> Hi Peter,
>   Thanks for the comments and mail.
>
> >
> > On 22 November 2018 at 10:28, Peter Maydell  
> > wrote:
> > > On 22 November 2018 at 03:05, gengdongjiu  wrote:
> > >>> >
> > >>> Shouldn't there be something in here to say "only report this error to 
> > >>> the guest if we are actually reporting RAS errors to the guest" ?
> > >>
> > >> Yes, We can say something that such as "report this error to the guest", 
> > >> because this error is indeed triggered by guest, which is guest
> > error.
> > >
> > > I'm afraid I don't really understand what you mean. Could you try
> > > rephrasing it?
> > >
> > > My understanding was:
> > >  * we get this signal if there is a RAS error in the host memory
> > >  * if we are exposing RAS errors to the guest (ie we have
> > >told it that in the ACPI table we passed it at startup)
> > >then we should pass on this error to the guest
> > >
> > > but that these are two different conditions.
> > >
> > > If the host hardware detects a RAS error in memory used by the guest
> > > but the guest is not being told about RAS errors, then we cannot
> > > report the error: we have no mechanism to do so, and the guest is not
> > > expecting it.
> >
> > If you look at the x86 version of this function you can see that it tests 
> > (env->mcg_cap & MCG_SER_P), which I think is the equivalent x86 "is
> > the guest CPU/config one we can report these errors to" test.
>
> MCG_SER_P (software error recovery support present) flag indicates (when set) 
> that the processor supports software error recovery.
> env->mcg_cap 's value should be got from KVM as shown in the QEMU code[1], it 
> indicates whether the KVM support software error recovery.
>
> [1]:
> -
>   ret = kvm_get_mce_cap_supported(cs->kvm_state, _cap, );
>   if (ret < 0) {
>fprintf(stderr, "kvm_get_mce_cap_supported: %s", strerror(-ret));
>return ret;
>}
> -

Yes, but if you look at the code which calls that, it
goes on to do:
env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;

which means that if the host kernel does not support this
feature then we will clear those bits in the env->mcg_cap
field, so we do not advertise it to the guest. But we might
be not advertising it to the guest at all, if env->mcg_cap
was 0 before this code was called. That happens if we
are presenting the guest with a guest CPU type which does
not have the feature.

--
> void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> {
> ...
>
> if (ram_addr != RAM_ADDR_INVALID &&
> kvm_physical_memory_addr_from_host(c->kvm_state, addr, )) {
>
>   If it got to here, it means the host hardware detects a RAS error in memory 
> used by the guest using above two judgments.
>   Maybe we can test/check whether KVM supports software error recovery in [3]
>

The question is not "does the host CPU / KVM support error
reporting". It is "does the *guest* CPU / system support
error reporting". These are distinct questions which may
not have the same answer.


thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Peter Maydell]

On Fri, 23 Nov 2018 at 18:24, Eduardo Habkost  wrote:
>
> On Fri, Nov 23, 2018 at 06:14:28PM +, Peter Maydell wrote:
> > One thing I would like to do with this new "cpu cluster"
> > concept is to use it to handle a problem we have at the
> > moment with TCG, where we assume all CPUs have the same
> > view of physical memory (and so if CPU A executes from physical
> > address X it can share translated code with CPU B executing
> > from physical address X). The idea is that we should include
> > the CPU cluster number in the TCG hash key that we use to
> > look up cached translation blocks, so that only CPUs in
> > the same cluster (assumed to have the same view of memory
> > and to be identical) share TBs.
> >
> > If we don't have a unique integer key for the cluster, what
> > should we use instead ?
>
> This sounds like a reasonable use of cluster_id as implemented in
> this patch.  The ID would be only used internally and not exposed
> to the outside, right?

It would be internal to QEMU (not exposed to the guest or
to the user), yes.

> I'm more worried about cases where we could end up exposing the
> ID in an external interface (either to guests, or through QMP or
> the command-line).  This happened to cpu_index and we took a long
> time to fix the mess.

I see, thanks.

My other question about this code was a slightly different one -- are
we guaranteed to be holding the iothread lock when we create
new QOM objects? (ie that we won't have races between two threads
which both try to create new objects and increment the variable)

thanks
-- PMM

Re: [Qemu-devel] [PATCH RESEND v15 08/10] target-arm: kvm64: inject synchronous External Abort

[Peter Maydell]

On Wed, 21 Nov 2018 at 14:34, gengdongjiu  wrote:
>
> Hi Peter,
>   Thanks for the review and comments.
>
> >
> > On 8 November 2018 at 10:29, Dongjiu Geng  wrote:
> > > +bool write_part_cpustate_to_list(ARMCPU *cpu, ptrdiff_t fieldoffset)
> >
> > What is this about? Nothing else in QEMU needs to mess with the cpustate 
> > synchronization. My first assumption is that you should not
> > need to do so either.
>
> We should change the guest CP15 ESR_EL1's value, the only method is to change 
> the cpu->cpreg_values[] in QEMU, then QEMU call write_list_to_kvmstate()
> to set the cpu->cpreg_values[] to KVM which include the specified ESR_EL1 
> value, KVM do world switch, and then set the specified ESR_EL1's value to 
> guest kernel.

Ah, I see. This is a bug in our current handling of the register
state, where we implicitly assume that nothing in QEMU will ever
want to change any system register values. This assumption is
now false -- kvm_arm_handle_debug() broke it -- so we need to
fix the code that does kvm_arch_put_registers(). There is a comment
in the kvm32.c version of that function about this. (The kvm64.c
version has the same assumption but doesn't comment on it.)

We should (ideally) fix this bug in the code that does register
syncing, without requiring places in QEMU that update system
registers to have to manually indicate which registers they have
changed. I'll have a think about how best to do this.

> About the detailed explanation, as shown in [2].
>
> kvm_arm_handle_debug() does not need to do this because QEMU does not need to 
> change CP15 registers, such as ESR_EL1.

kvm_arm_handle_debug does change ESR_EL1: it is injecting an exception
and so should set the exception register. This happens when it
calls the do_interrupt() hook, because arm_cpu_do_interrupt_aarch64()
writes to env->cp15.esr_el[new_el].

I'm not entirely sure why this is working today, in fact.
Alex, did you test whether our debug-exception-injection
reports the correct ESR_EL1 to the guest ?

> > > +/* Inject synchronous external abort */ static void
> > > +kvm_inject_arm_sea(CPUState *c) {
> > > +ARMCPU *cpu = ARM_CPU(c);
> > > +CPUARMState *env = >env;
> > > +CPUClass *cc = CPU_GET_CLASS(c);
> > > +uint32_t esr;
> > > +int ret;
> > > +
> > > +/* This exception is synchronous data abort */
> > > +c->exception_index = EXCP_DATA_ABORT;
> > > +/* Inject the exception to guest EL1 */
> > > +env->exception.target_el = 1;
> >
> > These comments don't tell us anything that the code does not.
>
>  Thanks, do you mean I need to remove it or add more detailed comments to it?

As a rule of thumb, comments should provide information to
the reader which they wouldn't get if they only had the code.
Comments often answer the "why do we do this" question, or
provide an overall summary of what the code is going to do,
or refer to an external source (a datasheet, an algorithm)
that is necessary to understand the code. It's better to
avoid comments that say "what the code is doing" at a line-by-line
level, because the code itself already answers the "what"
question at that level of detail.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 8/9] file-posix: account discard operations

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:35, Anton Nefedov wrote:
> This will help to identify how many of the user-issued discard operations
> (accounted on a device level) have actually suceeded down on the host file
> (even though the numbers will not be exactly the same if non-raw format
> driver is used (e.g. qcow2 sending metadata discards)).
> 
> Note that these numbers will not include discards triggered by
> write-zeroes + MAY_UNMAP calls.
> 
> Signed-off-by: Anton Nefedov 

Reviewed-by: Vladimir Sementsov-Ogievskiy 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 3/5] migration: use threaded workqueue for compression

[Dr. David Alan Gilbert]

* guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:
> From: Xiao Guangrong 
> 
> Adapt the compression code to the threaded workqueue
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  migration/ram.c | 308 
> 
>  1 file changed, 110 insertions(+), 198 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 7e7deec4d8..254c08f27b 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -57,6 +57,7 @@
>  #include "qemu/uuid.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
> +#include "qemu/threaded-workqueue.h"
>  
>  /***/
>  /* ram save/restore */
> @@ -349,22 +350,6 @@ typedef struct PageSearchStatus PageSearchStatus;
>  
>  CompressionStats compression_counters;
>  
> -struct CompressParam {
> -bool done;
> -bool quit;
> -bool zero_page;
> -QEMUFile *file;
> -QemuMutex mutex;
> -QemuCond cond;
> -RAMBlock *block;
> -ram_addr_t offset;
> -
> -/* internally used fields */
> -z_stream stream;
> -uint8_t *originbuf;
> -};
> -typedef struct CompressParam CompressParam;
> -
>  struct DecompressParam {
>  bool done;
>  bool quit;
> @@ -377,15 +362,6 @@ struct DecompressParam {
>  };
>  typedef struct DecompressParam DecompressParam;
>  
> -static CompressParam *comp_param;
> -static QemuThread *compress_threads;
> -/* comp_done_cond is used to wake up the migration thread when
> - * one of the compression threads has finished the compression.
> - * comp_done_lock is used to co-work with comp_done_cond.
> - */
> -static QemuMutex comp_done_lock;
> -static QemuCond comp_done_cond;
> -/* The empty QEMUFileOps will be used by file in CompressParam */
>  static const QEMUFileOps empty_ops = { };
>  
>  static QEMUFile *decomp_file;
> @@ -394,125 +370,6 @@ static QemuThread *decompress_threads;
>  static QemuMutex decomp_done_lock;
>  static QemuCond decomp_done_cond;
>  
> -static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
> *block,
> - ram_addr_t offset, uint8_t *source_buf);
> -
> -static void *do_data_compress(void *opaque)
> -{
> -CompressParam *param = opaque;
> -RAMBlock *block;
> -ram_addr_t offset;
> -bool zero_page;
> -
> -qemu_mutex_lock(>mutex);
> -while (!param->quit) {
> -if (param->block) {
> -block = param->block;
> -offset = param->offset;
> -param->block = NULL;
> -qemu_mutex_unlock(>mutex);
> -
> -zero_page = do_compress_ram_page(param->file, >stream,
> - block, offset, 
> param->originbuf);
> -
> -qemu_mutex_lock(_done_lock);
> -param->done = true;
> -param->zero_page = zero_page;
> -qemu_cond_signal(_done_cond);
> -qemu_mutex_unlock(_done_lock);
> -
> -qemu_mutex_lock(>mutex);
> -} else {
> -qemu_cond_wait(>cond, >mutex);
> -}
> -}
> -qemu_mutex_unlock(>mutex);
> -
> -return NULL;
> -}
> -
> -static void compress_threads_save_cleanup(void)
> -{
> -int i, thread_count;
> -
> -if (!migrate_use_compression() || !comp_param) {
> -return;
> -}
> -
> -thread_count = migrate_compress_threads();
> -for (i = 0; i < thread_count; i++) {
> -/*
> - * we use it as a indicator which shows if the thread is
> - * properly init'd or not
> - */
> -if (!comp_param[i].file) {
> -break;
> -}
> -
> -qemu_mutex_lock(_param[i].mutex);
> -comp_param[i].quit = true;
> -qemu_cond_signal(_param[i].cond);
> -qemu_mutex_unlock(_param[i].mutex);
> -
> -qemu_thread_join(compress_threads + i);
> -qemu_mutex_destroy(_param[i].mutex);
> -qemu_cond_destroy(_param[i].cond);
> -deflateEnd(_param[i].stream);
> -g_free(comp_param[i].originbuf);
> -qemu_fclose(comp_param[i].file);
> -comp_param[i].file = NULL;
> -}
> -qemu_mutex_destroy(_done_lock);
> -qemu_cond_destroy(_done_cond);
> -g_free(compress_threads);
> -g_free(comp_param);
> -compress_threads = NULL;
> -comp_param = NULL;
> -}
> -
> -static int compress_threads_save_setup(void)
> -{
> -int i, thread_count;
> -
> -if (!migrate_use_compression()) {
> -return 0;
> -}
> -thread_count = migrate_compress_threads();
> -compress_threads = g_new0(QemuThread, thread_count);
> -comp_param = g_new0(CompressParam, thread_count);
> -qemu_cond_init(_done_cond);
> -qemu_mutex_init(_done_lock);
> -for (i = 0; i < thread_count; i++) {
> -comp_param[i].originbuf = g_try_malloc(TARGET_PAGE_SIZE);
> -if (!comp_param[i].originbuf) {
> -goto exit;
> -}
> -
> -if (deflateInit(_param[i].stream,
> -

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Philippe Mathieu-Daudé]

On 23/11/18 18:06, Edgar E. Iglesias wrote:
> On Fri, Nov 23, 2018 at 06:02:25PM +0100, Edgar E. Iglesias wrote:
>> On Fri, Nov 23, 2018 at 05:59:45PM +0100, Edgar E. Iglesias wrote:
>>> On Fri, Nov 23, 2018 at 05:46:17PM +0100, Philippe Mathieu-Daudé wrote:
 Hi Edgar,
>>>
>>> Hi Philippe,
>>>

 On 23/11/18 14:54, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" 
>
> Don't assert on RX descriptor settings when the receiver is
> disabled. This fixes an issue with incoming packets on an
> unused GEM.
>
> Reported-by: mbilal 
> Signed-off-by: Edgar E. Iglesias 
> ---
>  hw/net/cadence_gem.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d95cc27f58..7f63411430 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
> uint8_t *buf, size_t size)
>  
>  /* Do nothing if receive is not enabled. */
>  if (!gem_can_receive(nc)) {
> -assert(!first_desc);

 Maybe worth:

trace_gem_receive_packet_drop(size);
>>>
>>> Or perhaps a generic tracepoint on packet drops for any device.
>>> Anyway this is probably something for after the release.
>>>
>>> Not sure if it's too late to even get the removal of the assert into this 
>>> release? Peter?
>>>

>  return -1;

 Shouldn't this be 'return 0'?

 The "net/net.h" doc is scarce...
>>>
>>> If we return 0 my understanding is that we later need to actively
>>> call qemu_flush_or_purge_queued_packets() to renable the rx
>>> path which the GEM model doesn't do. So that would mean
>>> refactoring the model a bit.
>>
>> Actually, the GEM model does do that, my bad, so yes return 0 seems to be 
>> the right thing to do here.
> 
> I take that back, the GEM model only handles some of the !can_receive cases
> with qemu_flush_queued_packets(). Not all, so return -1 is correct I think.

OK, thanks for checking this.

Reviewed-by: Philippe Mathieu-Daudé 

Regards,

Phil.

Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath

[Eduardo Habkost]

On Fri, Nov 23, 2018 at 11:10:40AM +0800, maozy wrote:
> Hi, Eduardo
> 
> On 11/20/18 7:31 AM, Eduardo Habkost wrote:
> > On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote:
> > > Currently, all sysbus devices have been converted to realize(),
> > > so remove this path.
> > > 
> > > Cc: ehabk...@redhat.com
> > > Cc: th...@redhat.com
> > > Cc: pbonz...@redhat.com
> > > Cc: arm...@redhat.com
> > > Cc: peter.mayd...@linaro.org
> > > Cc: richard.hender...@linaro.org
> > > Cc: alistair.fran...@wdc.com
> > > 
> > > Signed-off-by: Mao Zhongyi 
> > > Signed-off-by: Zhang Shengju 
> > > ---
> > >   hw/core/sysbus.c| 15 ---
> > >   include/hw/sysbus.h |  3 ---
> > >   2 files changed, 18 deletions(-)
> > > 
> > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> > > index 7ac36ad3e7..030ad426c1 100644
> > > --- a/hw/core/sysbus.c
> > > +++ b/hw/core/sysbus.c
> > > @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
> > > ioport, uint32_t size)
> > >   }
> > >   }
> > > -/* TODO remove once all sysbus devices have been converted to realize */
> > > -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;
> > > -}
> > > -if (sbc->init(sd) < 0) {
> > > -error_setg(errp, "Device initialization failed");
> > > -}
> > > -}
> > 
> > Nice.  :)
> > 
> > 
> > > -
> > >   DeviceState *sysbus_create_varargs(const char *name,
> > >  hwaddr addr, ...)
> > >   {
> > > @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
> > >   static void sysbus_device_class_init(ObjectClass *klass, void *data)
> > >   {
> > >   DeviceClass *k = DEVICE_CLASS(klass);
> > > -k->realize = sysbus_realize;
> > 
> > Have you ensured this won't break any subclasses that
> > saved the original realize function on a parent_realize field?
> 
> Thanks for the catch.
> 
> > Now they will have parent_realize set to NULL.
> 
> In order to void the subclasses whose parent_realize field is
> set to NULL, the k->realize function must be retained even
> though it doesn't do anything practical. Just like this:
> 
> 
> -/* TODO remove once all sysbus devices have been converted to realize*/
>  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;
> -}
> -if (sbc->init(sd) < 0) {
> -error_setg(errp, "Device initialization failed");
> -}
>  }
> 
> it doesn't look elegant, but I didn't think of a better way, if you
> can give me some hints, I really appreciate it. :)

I think this is good enough for now (as long as there's a comment
like Peter suggested).  Allowing parent_realize to be NULL would
be inconvenient to all code that uses parent_realize today.

Personally, I would love to get rid of parent_realize entirely.
We could simply provide a helper to let device subclasses call
the parent's realize function without the need to copy function
pointers around.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath

[Peter Maydell]

On Fri, 23 Nov 2018 at 18:16, Eduardo Habkost  wrote:
> I think this is good enough for now (as long as there's a comment
> like Peter suggested).  Allowing parent_realize to be NULL would
> be inconvenient to all code that uses parent_realize today.
>
> Personally, I would love to get rid of parent_realize entirely.
> We could simply provide a helper to let device subclasses call
> the parent's realize function without the need to copy function
> pointers around.

Agreed -- parent_realize is a hack that is working around
a deficiency in our object model, and it would be nice to
deal with that. But let's do our cleanups one at a time :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Peter Maydell]

On Fri, 23 Nov 2018 at 18:11, Eduardo Habkost  wrote:
> On Fri, Nov 23, 2018 at 10:17:14AM +0100, Luc Michel wrote:
> > This commit adds the cpu-cluster type. It aims at gathering CPUs from
> > the same cluster in a machine.
> >
> > For now it only has a `cluster-id` property.
> >
> > Signed-off-by: Luc Michel 
> > Reviewed-by: Alistair Francis 
> > Reviewed-by: Philippe Mathieu-Daudé 
> > Tested-by: Philippe Mathieu-Daudé 
> > Reviewed-by: Edgar E. Iglesias 
> [...]
> > +static void cpu_cluster_init(Object *obj)
> > +{
> > +static uint32_t cluster_id_auto_increment;
> > +CPUClusterState *cluster = CPU_CLUSTER(obj);
> > +
> > +cluster->cluster_id = cluster_id_auto_increment++;
>
> I see that you implemented this after a suggestion from Philippe,
> but I'm worried about this kind of side-effect on object/device
> code.  I'm afraid this will bite us back in the future.  We were
> bitten by problems caused by automatic cpu_index assignment on
> CPU instance_init, and we took a while to fix that.
>
> If you really want to do this and assign cluster_id
> automatically, please do it on realize, where it won't have
> unexpected side-effects after a simple `qom-list-properties` QMP
> command.
>
> I would also add a huge warning above the cluster_id field
> declaration, mentioning that the field is not supposed to be used
> for anything except debugging.  I think there's a large risk of
> people trying to reuse the field incorrectly, just like cpu_index
> was reused for multiple (conflicting) purposes in the past.

One thing I would like to do with this new "cpu cluster"
concept is to use it to handle a problem we have at the
moment with TCG, where we assume all CPUs have the same
view of physical memory (and so if CPU A executes from physical
address X it can share translated code with CPU B executing
from physical address X). The idea is that we should include
the CPU cluster number in the TCG hash key that we use to
look up cached translation blocks, so that only CPUs in
the same cluster (assumed to have the same view of memory
and to be identical) share TBs.

If we don't have a unique integer key for the cluster, what
should we use instead ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Eduardo Habkost]

On Fri, Nov 23, 2018 at 06:14:28PM +, Peter Maydell wrote:
> On Fri, 23 Nov 2018 at 18:11, Eduardo Habkost  wrote:
> > On Fri, Nov 23, 2018 at 10:17:14AM +0100, Luc Michel wrote:
> > > This commit adds the cpu-cluster type. It aims at gathering CPUs from
> > > the same cluster in a machine.
> > >
> > > For now it only has a `cluster-id` property.
> > >
> > > Signed-off-by: Luc Michel 
> > > Reviewed-by: Alistair Francis 
> > > Reviewed-by: Philippe Mathieu-Daudé 
> > > Tested-by: Philippe Mathieu-Daudé 
> > > Reviewed-by: Edgar E. Iglesias 
> > [...]
> > > +static void cpu_cluster_init(Object *obj)
> > > +{
> > > +static uint32_t cluster_id_auto_increment;
> > > +CPUClusterState *cluster = CPU_CLUSTER(obj);
> > > +
> > > +cluster->cluster_id = cluster_id_auto_increment++;
> >
> > I see that you implemented this after a suggestion from Philippe,
> > but I'm worried about this kind of side-effect on object/device
> > code.  I'm afraid this will bite us back in the future.  We were
> > bitten by problems caused by automatic cpu_index assignment on
> > CPU instance_init, and we took a while to fix that.
> >
> > If you really want to do this and assign cluster_id
> > automatically, please do it on realize, where it won't have
> > unexpected side-effects after a simple `qom-list-properties` QMP
> > command.
> >
> > I would also add a huge warning above the cluster_id field
> > declaration, mentioning that the field is not supposed to be used
> > for anything except debugging.  I think there's a large risk of
> > people trying to reuse the field incorrectly, just like cpu_index
> > was reused for multiple (conflicting) purposes in the past.
> 
> One thing I would like to do with this new "cpu cluster"
> concept is to use it to handle a problem we have at the
> moment with TCG, where we assume all CPUs have the same
> view of physical memory (and so if CPU A executes from physical
> address X it can share translated code with CPU B executing
> from physical address X). The idea is that we should include
> the CPU cluster number in the TCG hash key that we use to
> look up cached translation blocks, so that only CPUs in
> the same cluster (assumed to have the same view of memory
> and to be identical) share TBs.
> 
> If we don't have a unique integer key for the cluster, what
> should we use instead ?

This sounds like a reasonable use of cluster_id as implemented in
this patch.  The ID would be only used internally and not exposed
to the outside, right?

I'm more worried about cases where we could end up exposing the
ID in an external interface (either to guests, or through QMP or
the command-line).  This happened to cpu_index and we took a long
time to fix the mess.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 3/5] migration: use threaded workqueue for compression

[Paolo Bonzini]

On 23/11/18 19:17, Dr. David Alan Gilbert wrote:
> * guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:
>> From: Xiao Guangrong 
>>
>> Adapt the compression code to the threaded workqueue
>>
>> Signed-off-by: Xiao Guangrong 
>> ---
>>  migration/ram.c | 308 
>> 
>>  1 file changed, 110 insertions(+), 198 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 7e7deec4d8..254c08f27b 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -57,6 +57,7 @@
>>  #include "qemu/uuid.h"
>>  #include "savevm.h"
>>  #include "qemu/iov.h"
>> +#include "qemu/threaded-workqueue.h"
>>  
>>  /***/
>>  /* ram save/restore */
>> @@ -349,22 +350,6 @@ typedef struct PageSearchStatus PageSearchStatus;
>>  
>>  CompressionStats compression_counters;
>>  
>> -struct CompressParam {
>> -bool done;
>> -bool quit;
>> -bool zero_page;
>> -QEMUFile *file;
>> -QemuMutex mutex;
>> -QemuCond cond;
>> -RAMBlock *block;
>> -ram_addr_t offset;
>> -
>> -/* internally used fields */
>> -z_stream stream;
>> -uint8_t *originbuf;
>> -};
>> -typedef struct CompressParam CompressParam;
>> -
>>  struct DecompressParam {
>>  bool done;
>>  bool quit;
>> @@ -377,15 +362,6 @@ struct DecompressParam {
>>  };
>>  typedef struct DecompressParam DecompressParam;
>>  
>> -static CompressParam *comp_param;
>> -static QemuThread *compress_threads;
>> -/* comp_done_cond is used to wake up the migration thread when
>> - * one of the compression threads has finished the compression.
>> - * comp_done_lock is used to co-work with comp_done_cond.
>> - */
>> -static QemuMutex comp_done_lock;
>> -static QemuCond comp_done_cond;
>> -/* The empty QEMUFileOps will be used by file in CompressParam */
>>  static const QEMUFileOps empty_ops = { };
>>  
>>  static QEMUFile *decomp_file;
>> @@ -394,125 +370,6 @@ static QemuThread *decompress_threads;
>>  static QemuMutex decomp_done_lock;
>>  static QemuCond decomp_done_cond;
>>  
>> -static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
>> *block,
>> - ram_addr_t offset, uint8_t *source_buf);
>> -
>> -static void *do_data_compress(void *opaque)
>> -{
>> -CompressParam *param = opaque;
>> -RAMBlock *block;
>> -ram_addr_t offset;
>> -bool zero_page;
>> -
>> -qemu_mutex_lock(>mutex);
>> -while (!param->quit) {
>> -if (param->block) {
>> -block = param->block;
>> -offset = param->offset;
>> -param->block = NULL;
>> -qemu_mutex_unlock(>mutex);
>> -
>> -zero_page = do_compress_ram_page(param->file, >stream,
>> - block, offset, 
>> param->originbuf);
>> -
>> -qemu_mutex_lock(_done_lock);
>> -param->done = true;
>> -param->zero_page = zero_page;
>> -qemu_cond_signal(_done_cond);
>> -qemu_mutex_unlock(_done_lock);
>> -
>> -qemu_mutex_lock(>mutex);
>> -} else {
>> -qemu_cond_wait(>cond, >mutex);
>> -}
>> -}
>> -qemu_mutex_unlock(>mutex);
>> -
>> -return NULL;
>> -}
>> -
>> -static void compress_threads_save_cleanup(void)
>> -{
>> -int i, thread_count;
>> -
>> -if (!migrate_use_compression() || !comp_param) {
>> -return;
>> -}
>> -
>> -thread_count = migrate_compress_threads();
>> -for (i = 0; i < thread_count; i++) {
>> -/*
>> - * we use it as a indicator which shows if the thread is
>> - * properly init'd or not
>> - */
>> -if (!comp_param[i].file) {
>> -break;
>> -}
>> -
>> -qemu_mutex_lock(_param[i].mutex);
>> -comp_param[i].quit = true;
>> -qemu_cond_signal(_param[i].cond);
>> -qemu_mutex_unlock(_param[i].mutex);
>> -
>> -qemu_thread_join(compress_threads + i);
>> -qemu_mutex_destroy(_param[i].mutex);
>> -qemu_cond_destroy(_param[i].cond);
>> -deflateEnd(_param[i].stream);
>> -g_free(comp_param[i].originbuf);
>> -qemu_fclose(comp_param[i].file);
>> -comp_param[i].file = NULL;
>> -}
>> -qemu_mutex_destroy(_done_lock);
>> -qemu_cond_destroy(_done_cond);
>> -g_free(compress_threads);
>> -g_free(comp_param);
>> -compress_threads = NULL;
>> -comp_param = NULL;
>> -}
>> -
>> -static int compress_threads_save_setup(void)
>> -{
>> -int i, thread_count;
>> -
>> -if (!migrate_use_compression()) {
>> -return 0;
>> -}
>> -thread_count = migrate_compress_threads();
>> -compress_threads = g_new0(QemuThread, thread_count);
>> -comp_param = g_new0(CompressParam, thread_count);
>> -qemu_cond_init(_done_cond);
>> -qemu_mutex_init(_done_lock);
>> -for (i = 0; i < thread_count; i++) {
>> -

Re: [Qemu-devel] [PATCH v3 3/5] migration: use threaded workqueue for compression

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 23/11/18 19:17, Dr. David Alan Gilbert wrote:
> > * guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:
> >> From: Xiao Guangrong 
> >>
> >> Adapt the compression code to the threaded workqueue
> >>
> >> Signed-off-by: Xiao Guangrong 
> >> ---
> >>  migration/ram.c | 308 
> >> 
> >>  1 file changed, 110 insertions(+), 198 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 7e7deec4d8..254c08f27b 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -57,6 +57,7 @@
> >>  #include "qemu/uuid.h"
> >>  #include "savevm.h"
> >>  #include "qemu/iov.h"
> >> +#include "qemu/threaded-workqueue.h"
> >>  
> >>  /***/
> >>  /* ram save/restore */
> >> @@ -349,22 +350,6 @@ typedef struct PageSearchStatus PageSearchStatus;
> >>  
> >>  CompressionStats compression_counters;
> >>  
> >> -struct CompressParam {
> >> -bool done;
> >> -bool quit;
> >> -bool zero_page;
> >> -QEMUFile *file;
> >> -QemuMutex mutex;
> >> -QemuCond cond;
> >> -RAMBlock *block;
> >> -ram_addr_t offset;
> >> -
> >> -/* internally used fields */
> >> -z_stream stream;
> >> -uint8_t *originbuf;
> >> -};
> >> -typedef struct CompressParam CompressParam;
> >> -
> >>  struct DecompressParam {
> >>  bool done;
> >>  bool quit;
> >> @@ -377,15 +362,6 @@ struct DecompressParam {
> >>  };
> >>  typedef struct DecompressParam DecompressParam;
> >>  
> >> -static CompressParam *comp_param;
> >> -static QemuThread *compress_threads;
> >> -/* comp_done_cond is used to wake up the migration thread when
> >> - * one of the compression threads has finished the compression.
> >> - * comp_done_lock is used to co-work with comp_done_cond.
> >> - */
> >> -static QemuMutex comp_done_lock;
> >> -static QemuCond comp_done_cond;
> >> -/* The empty QEMUFileOps will be used by file in CompressParam */
> >>  static const QEMUFileOps empty_ops = { };
> >>  
> >>  static QEMUFile *decomp_file;
> >> @@ -394,125 +370,6 @@ static QemuThread *decompress_threads;
> >>  static QemuMutex decomp_done_lock;
> >>  static QemuCond decomp_done_cond;
> >>  
> >> -static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
> >> *block,
> >> - ram_addr_t offset, uint8_t *source_buf);
> >> -
> >> -static void *do_data_compress(void *opaque)
> >> -{
> >> -CompressParam *param = opaque;
> >> -RAMBlock *block;
> >> -ram_addr_t offset;
> >> -bool zero_page;
> >> -
> >> -qemu_mutex_lock(>mutex);
> >> -while (!param->quit) {
> >> -if (param->block) {
> >> -block = param->block;
> >> -offset = param->offset;
> >> -param->block = NULL;
> >> -qemu_mutex_unlock(>mutex);
> >> -
> >> -zero_page = do_compress_ram_page(param->file, >stream,
> >> - block, offset, 
> >> param->originbuf);
> >> -
> >> -qemu_mutex_lock(_done_lock);
> >> -param->done = true;
> >> -param->zero_page = zero_page;
> >> -qemu_cond_signal(_done_cond);
> >> -qemu_mutex_unlock(_done_lock);
> >> -
> >> -qemu_mutex_lock(>mutex);
> >> -} else {
> >> -qemu_cond_wait(>cond, >mutex);
> >> -}
> >> -}
> >> -qemu_mutex_unlock(>mutex);
> >> -
> >> -return NULL;
> >> -}
> >> -
> >> -static void compress_threads_save_cleanup(void)
> >> -{
> >> -int i, thread_count;
> >> -
> >> -if (!migrate_use_compression() || !comp_param) {
> >> -return;
> >> -}
> >> -
> >> -thread_count = migrate_compress_threads();
> >> -for (i = 0; i < thread_count; i++) {
> >> -/*
> >> - * we use it as a indicator which shows if the thread is
> >> - * properly init'd or not
> >> - */
> >> -if (!comp_param[i].file) {
> >> -break;
> >> -}
> >> -
> >> -qemu_mutex_lock(_param[i].mutex);
> >> -comp_param[i].quit = true;
> >> -qemu_cond_signal(_param[i].cond);
> >> -qemu_mutex_unlock(_param[i].mutex);
> >> -
> >> -qemu_thread_join(compress_threads + i);
> >> -qemu_mutex_destroy(_param[i].mutex);
> >> -qemu_cond_destroy(_param[i].cond);
> >> -deflateEnd(_param[i].stream);
> >> -g_free(comp_param[i].originbuf);
> >> -qemu_fclose(comp_param[i].file);
> >> -comp_param[i].file = NULL;
> >> -}
> >> -qemu_mutex_destroy(_done_lock);
> >> -qemu_cond_destroy(_done_cond);
> >> -g_free(compress_threads);
> >> -g_free(comp_param);
> >> -compress_threads = NULL;
> >> -comp_param = NULL;
> >> -}
> >> -
> >> -static int compress_threads_save_setup(void)
> >> -{
> >> -int i, thread_count;
> >> -
> >> -if (!migrate_use_compression()) {
> >> 

Re: [Qemu-devel] [PATCH v5 7/9] scsi: account unmap operations

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:34, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov 


Reviewed-by: Vladimir Sementsov-Ogievskiy 

but be careful: on git am, the chunk about read-only case goes into 
scsi_disk_emulate_write_same instead of scsi_disk_emulate_unmap (at least for 
me, with latest master branch and git 2.18

> ---
>   hw/scsi/scsi-disk.c | 12 +++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e132504913..dee71f9dde 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1663,10 +1663,16 @@ static void scsi_unmap_complete_noio(UnmapCBData 
> *data, int ret)
>   r->sector = ldq_be_p(>inbuf[0]);
>   r->sector_count = ldl_be_p(>inbuf[8]) & 0xULL;
>   if (!check_lba_range(s, r->sector, r->sector_count)) {
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
> +   BLOCK_ACCT_UNMAP);
>   scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>   goto done;
>   }
>   
> +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
> + r->sector_count * s->qdev.blocksize,
> + BLOCK_ACCT_UNMAP);
> +
>   r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>   r->sector * s->qdev.blocksize,
>   r->sector_count * s->qdev.blocksize,
> @@ -1693,10 +1699,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
>   r->req.aiocb = NULL;
>   
>   aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> -if (scsi_disk_req_check_error(r, ret, false)) {
> +if (scsi_disk_req_check_error(r, ret, true)) {
>   scsi_req_unref(>req);
>   g_free(data);
>   } else {
> +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
>   scsi_unmap_complete_noio(data, ret);
>   }
>   aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> @@ -1728,6 +1735,7 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, 
> uint8_t *inbuf)
>   }
>   
>   if (blk_is_read_only(s->qdev.conf.blk)) {
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), 
> BLOCK_ACCT_UNMAP);


this one goes to scsi_disk_emulate_write_same on automatic patch apply.


>   scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
>   return;
>   }
> @@ -1743,10 +1751,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, 
> uint8_t *inbuf)
>   return;
>   
>   invalid_param_len:
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>   scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
>   return;
>   
>   invalid_field:
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>   scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Eduardo Habkost]

Hi,

Sorry for not reviewing this series earlier.  I just stumbled
upon this part of the code:

On Fri, Nov 23, 2018 at 10:17:14AM +0100, Luc Michel wrote:
> This commit adds the cpu-cluster type. It aims at gathering CPUs from
> the same cluster in a machine.
> 
> For now it only has a `cluster-id` property.
> 
> Signed-off-by: Luc Michel 
> Reviewed-by: Alistair Francis 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Reviewed-by: Edgar E. Iglesias 
[...]
> +static void cpu_cluster_init(Object *obj)
> +{
> +static uint32_t cluster_id_auto_increment;
> +CPUClusterState *cluster = CPU_CLUSTER(obj);
> +
> +cluster->cluster_id = cluster_id_auto_increment++;

I see that you implemented this after a suggestion from Philippe,
but I'm worried about this kind of side-effect on object/device
code.  I'm afraid this will bite us back in the future.  We were
bitten by problems caused by automatic cpu_index assignment on
CPU instance_init, and we took a while to fix that.

If you really want to do this and assign cluster_id
automatically, please do it on realize, where it won't have
unexpected side-effects after a simple `qom-list-properties` QMP
command.

I would also add a huge warning above the cluster_id field
declaration, mentioning that the field is not supposed to be used
for anything except debugging.  I think there's a large risk of
people trying to reuse the field incorrectly, just like cpu_index
was reused for multiple (conflicting) purposes in the past.


> +}
> +
> +static Property cpu_cluster_properties[] = {
> +DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
> +DEFINE_PROP_END_OF_LIST()
> +};
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 4/9] ide: account UNMAP (TRIM) operations

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:34, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov 

Reviewed-by: Vladimir Sementsov-Ogievskiy 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] virtserialport/virtconsole: fix messy opening/closing port

[Paolo Bonzini]

On 01/11/18 16:11, Artem Pisarenko wrote:
> This fixes wrong interfacing between virtio serial port and bus
> models, and corresponding chardev backends, caused extra and incorrect
> activity during guest boot process (when virtserialport device used).
> 
> Signed-off-by: Artem Pisarenko 
> ---
> 
> Notes:
> Although this doesn't trigger any issue/bug (known to me), this may 
> prevent them in future.
> Also this patch optimizes emulation performance by avoiding extra 
> activity and fixes a case, when serial port wasn't closed if backend closed 
> while being disabled (even worse - serial port will open again!).

Marc-André, can you review this?

Thanks,

Paolo

>  hw/char/virtio-console.c  | 24 
>  hw/char/virtio-serial-bus.c   |  8 +++-
>  include/hw/virtio/virtio-serial.h |  8 
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 2cbe1d4..634e9bf 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -25,6 +25,7 @@
>  typedef struct VirtConsole {
>  VirtIOSerialPort parent_obj;
>  
> +bool backend_active;
>  CharBackend chr;
>  guint watch;
>  } VirtConsole;
> @@ -149,6 +150,11 @@ static void chr_event(void *opaque, int event)
>  VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
>  
>  trace_virtio_console_chr_event(port->id, event);
> +
> +if (!vcon->backend_active) {
> +return;
> +}
> +
>  switch (event) {
>  case CHR_EVENT_OPENED:
>  virtio_serial_open(port);
> @@ -187,17 +193,24 @@ static int chr_be_change(void *opaque)
>  return 0;
>  }
>  
> +static bool virtconsole_is_backend_enabled(VirtIOSerialPort *port)
> +{
> +VirtConsole *vcon = VIRTIO_CONSOLE(port);
> +
> +return vcon->backend_active;
> +}
> +
>  static void virtconsole_enable_backend(VirtIOSerialPort *port, bool enable)
>  {
>  VirtConsole *vcon = VIRTIO_CONSOLE(port);
>  
> -if (!qemu_chr_fe_backend_connected(>chr)) {
> -return;
> -}
> -
>  if (enable) {
>  VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>  
> +if (!k->is_console && virtio_serial_is_opened(port)
> +&& !qemu_chr_fe_backend_open(>chr)) {
> +virtio_serial_close(port);
> +}
>  qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
>   k->is_console ? NULL : chr_event,
>   chr_be_change, vcon, NULL, false);
> @@ -205,6 +218,7 @@ static void virtconsole_enable_backend(VirtIOSerialPort 
> *port, bool enable)
>  qemu_chr_fe_set_handlers(>chr, NULL, NULL, NULL,
>   NULL, NULL, NULL, false);
>  }
> +vcon->backend_active = enable;
>  }
>  
>  static void virtconsole_realize(DeviceState *dev, Error **errp)
> @@ -220,6 +234,7 @@ static void virtconsole_realize(DeviceState *dev, Error 
> **errp)
>  }
>  
>  if (qemu_chr_fe_backend_connected(>chr)) {
> +vcon->backend_active = true;
>  /*
>   * For consoles we don't block guest data transfer just
>   * because nothing is connected - we'll just let it go
> @@ -278,6 +293,7 @@ static void virtserialport_class_init(ObjectClass *klass, 
> void *data)
>  k->unrealize = virtconsole_unrealize;
>  k->have_data = flush_buf;
>  k->set_guest_connected = set_guest_connected;
> +k->is_backend_enabled = virtconsole_is_backend_enabled;
>  k->enable_backend = virtconsole_enable_backend;
>  k->guest_writable = guest_writable;
>  dc->props = virtserialport_properties;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 04e3ebe..d23d99d 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -258,6 +258,11 @@ static size_t send_control_event(VirtIOSerial *vser, 
> uint32_t port_id,
>  }
>  
>  /* Functions for use inside qemu to open and read from/write to ports */
> +bool virtio_serial_is_opened(VirtIOSerialPort *port)
> +{
> +return port->host_connected;
> +}
> +
>  int virtio_serial_open(VirtIOSerialPort *port)
>  {
>  /* Don't allow opening an already-open port */
> @@ -643,7 +648,8 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
>  
>  QTAILQ_FOREACH(port, >ports, next) {
>  VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> -if (vsc->enable_backend) {
> +if (vsc->is_backend_enabled && vsc->enable_backend
> +&& (vsc->is_backend_enabled(port) != vdev->vm_running)) {
>  vsc->enable_backend(port, vdev->vm_running);
>  }
>  }
> diff --git a/include/hw/virtio/virtio-serial.h 
> b/include/hw/virtio/virtio-serial.h
> index 12657a9..4b72562 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -58,6 +58,8 @@ typedef struct VirtIOSerialPortClass {
>  /* Guest 

Re: [Qemu-devel] [PATCH v5 1/9] qapi: group BlockDeviceStats fields

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:34, Anton Nefedov wrote:
> Make the stat fields definition slightly more readable.
> Also reorder total_time_ns stats read-write-flush as done elsewhere.
> Cosmetic change only.
> 
> Signed-off-by: Anton Nefedov 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   qapi/block-core.json | 26 +++---
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0fc1590c1b..2903b8dfc9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -784,12 +784,12 @@
>   # @flush_operations: The number of cache flush operations performed by the
>   #device (since 0.15.0)
>   #
> -# @flush_total_time_ns: Total time spend on cache flushes in nano-seconds
> -#   (since 0.15.0).
> +# @rd_total_time_ns: Total time spent on reads in nanoseconds (since 0.15.0).
>   #
> -# @wr_total_time_ns: Total time spend on writes in nano-seconds (since 
> 0.15.0).
> +# @wr_total_time_ns: Total time spent on writes in nanoseconds (since 
> 0.15.0).
>   #
> -# @rd_total_time_ns: Total_time_spend on reads in nano-seconds (since 
> 0.15.0).
> +# @flush_total_time_ns: Total time spent on cache flushes in nanoseconds
> +#   (since 0.15.0).
>   #
>   # @wr_highest_offset: The offset after the greatest byte written to the
>   # device.  The intended use of this information is for
> @@ -842,14 +842,18 @@
>   # Since: 0.14.0
>   ##
>   { 'struct': 'BlockDeviceStats',
> -  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
> -   'wr_operations': 'int', 'flush_operations': 'int',
> -   'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
> -   'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
> -   'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
> +  'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
> +   'rd_operations': 'int', 'wr_operations': 'int',
> +   'flush_operations': 'int',
> +   'rd_total_time_ns': 'int', 'wr_total_time_ns': 'int',
> +   'flush_total_time_ns': 'int',
> +   'wr_highest_offset': 'int',
> +   'rd_merged': 'int', 'wr_merged': 'int',
> +   '*idle_time_ns': 'int',
>  'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
> -   'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
> -   'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
> +   'failed_flush_operations': 'int',
> +   'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
> +   'invalid_flush_operations': 'int',
>  'account_invalid': 'bool', 'account_failed': 'bool',
>  'timed_stats': ['BlockDeviceTimedStats'],
>  '*x_rd_latency_histogram': 'BlockLatencyHistogramInfo',
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Edgar E. Iglesias]

On Fri, Nov 23, 2018 at 06:02:25PM +0100, Edgar E. Iglesias wrote:
> On Fri, Nov 23, 2018 at 05:59:45PM +0100, Edgar E. Iglesias wrote:
> > On Fri, Nov 23, 2018 at 05:46:17PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Edgar,
> > 
> > Hi Philippe,
> > 
> > > 
> > > On 23/11/18 14:54, Edgar E. Iglesias wrote:
> > > > From: "Edgar E. Iglesias" 
> > > > 
> > > > Don't assert on RX descriptor settings when the receiver is
> > > > disabled. This fixes an issue with incoming packets on an
> > > > unused GEM.
> > > > 
> > > > Reported-by: mbilal 
> > > > Signed-off-by: Edgar E. Iglesias 
> > > > ---
> > > >  hw/net/cadence_gem.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > > > index d95cc27f58..7f63411430 100644
> > > > --- a/hw/net/cadence_gem.c
> > > > +++ b/hw/net/cadence_gem.c
> > > > @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, 
> > > > const uint8_t *buf, size_t size)
> > > >  
> > > >  /* Do nothing if receive is not enabled. */
> > > >  if (!gem_can_receive(nc)) {
> > > > -assert(!first_desc);
> > > 
> > > Maybe worth:
> > > 
> > >trace_gem_receive_packet_drop(size);
> > 
> > Or perhaps a generic tracepoint on packet drops for any device.
> > Anyway this is probably something for after the release.
> > 
> > Not sure if it's too late to even get the removal of the assert into this 
> > release? Peter?
> > 
> > > 
> > > >  return -1;
> > > 
> > > Shouldn't this be 'return 0'?
> > > 
> > > The "net/net.h" doc is scarce...
> > 
> > If we return 0 my understanding is that we later need to actively
> > call qemu_flush_or_purge_queued_packets() to renable the rx
> > path which the GEM model doesn't do. So that would mean
> > refactoring the model a bit.
> 
> Actually, the GEM model does do that, my bad, so yes return 0 seems to be the 
> right thing to do here.

I take that back, the GEM model only handles some of the !can_receive cases
with qemu_flush_queued_packets(). Not all, so return -1 is correct I think.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Edgar E. Iglesias]

On Fri, Nov 23, 2018 at 05:59:45PM +0100, Edgar E. Iglesias wrote:
> On Fri, Nov 23, 2018 at 05:46:17PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Edgar,
> 
> Hi Philippe,
> 
> > 
> > On 23/11/18 14:54, Edgar E. Iglesias wrote:
> > > From: "Edgar E. Iglesias" 
> > > 
> > > Don't assert on RX descriptor settings when the receiver is
> > > disabled. This fixes an issue with incoming packets on an
> > > unused GEM.
> > > 
> > > Reported-by: mbilal 
> > > Signed-off-by: Edgar E. Iglesias 
> > > ---
> > >  hw/net/cadence_gem.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > > index d95cc27f58..7f63411430 100644
> > > --- a/hw/net/cadence_gem.c
> > > +++ b/hw/net/cadence_gem.c
> > > @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
> > > uint8_t *buf, size_t size)
> > >  
> > >  /* Do nothing if receive is not enabled. */
> > >  if (!gem_can_receive(nc)) {
> > > -assert(!first_desc);
> > 
> > Maybe worth:
> > 
> >trace_gem_receive_packet_drop(size);
> 
> Or perhaps a generic tracepoint on packet drops for any device.
> Anyway this is probably something for after the release.
> 
> Not sure if it's too late to even get the removal of the assert into this 
> release? Peter?
> 
> > 
> > >  return -1;
> > 
> > Shouldn't this be 'return 0'?
> > 
> > The "net/net.h" doc is scarce...
> 
> If we return 0 my understanding is that we later need to actively
> call qemu_flush_or_purge_queued_packets() to renable the rx
> path which the GEM model doesn't do. So that would mean
> refactoring the model a bit.

Actually, the GEM model does do that, my bad, so yes return 0 seems to be the 
right thing to do here.

Thanks,
Edgar


> 
> Cheers,
> Edgar
> 
> 
> > 
> > Regards,
> > 
> > Phil.
> > 
> > >  }
> > >  
> > > 

Re: [Qemu-devel] [RFC 20/48] *-user: notify plugin of exit

[Alex Bennée]

Emilio G. Cota  writes:

> Signed-off-by: Emilio G. Cota 

Reviewed-by: Alex Bennée 

> ---
>  bsd-user/syscall.c | 3 +++
>  linux-user/exit.c  | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
> index 66492aaf5d..b7818af450 100644
> --- a/bsd-user/syscall.c
> +++ b/bsd-user/syscall.c
> @@ -332,6 +332,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  _mcleanup();
>  #endif
>  gdb_exit(cpu_env, arg1);
> +qemu_plugin_atexit_cb();
>  /* XXX: should free thread stack and CPU env */
>  _exit(arg1);
>  ret = 0; /* avoid warning */
> @@ -430,6 +431,7 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  _mcleanup();
>  #endif
>  gdb_exit(cpu_env, arg1);
> +qemu_plugin_atexit_cb();
>  /* XXX: should free thread stack and CPU env */
>  _exit(arg1);
>  ret = 0; /* avoid warning */
> @@ -505,6 +507,7 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  _mcleanup();
>  #endif
>  gdb_exit(cpu_env, arg1);
> +qemu_plugin_atexit_cb();
>  /* XXX: should free thread stack and CPU env */
>  _exit(arg1);
>  ret = 0; /* avoid warning */
> diff --git a/linux-user/exit.c b/linux-user/exit.c
> index 14e94e28fa..768856483a 100644
> --- a/linux-user/exit.c
> +++ b/linux-user/exit.c
> @@ -32,4 +32,5 @@ void preexit_cleanup(CPUArchState *env, int code)
>  __gcov_dump();
>  #endif
>  gdb_exit(env, code);
> +qemu_plugin_atexit_cb();
>  }


--
Alex Bennée

Re: [Qemu-devel] [RFC 22/48] cpu: hook plugin vcpu events

[Alex Bennée]

Emilio G. Cota  writes:

> Signed-off-by: Emilio G. Cota 
> ---
>  cpus.c| 10 ++
>  exec.c|  2 ++
>  qom/cpu.c |  2 ++
>  3 files changed, 14 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index 28e39f045a..3efe89354d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -43,6 +43,7 @@
>  #include "exec/exec-all.h"
>
>  #include "qemu/thread.h"
> +#include "qemu/plugin.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/qtest.h"
>  #include "qemu/main-loop.h"
> @@ -1322,12 +1323,21 @@ static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
>
>  static void qemu_wait_io_event(CPUState *cpu)
>  {
> +bool asleep = false;
> +

slept?

>  g_assert(cpu_mutex_locked(cpu));
>  g_assert(!qemu_mutex_iothread_locked());
>
>  while (cpu_thread_is_idle(cpu)) {
> +if (!asleep) {
> +asleep = true;
> +qemu_plugin_vcpu_idle_cb(cpu);
> +}
>  qemu_cond_wait(>halt_cond, >lock);
>  }
> +if (asleep) {
> +qemu_plugin_vcpu_resume_cb(cpu);
> +}

I wonder if having two hooks is too much? What might a plugin want to do
before we go into idle sleep?

It feels like we are exposing too much of the guts of TCG to the plugin
here as wait_io could be for any number of internal reasons other than
the actual emulation blocking for IO through a WFI or something. If a
plugin really wants to track such things shouldn't it be hooking to the
guest sleep points?

If idle sleeps really are that important maybe we could just report our
sleep time on resume - so a single hook but passing a bit more
information?

>
>  #ifdef _WIN32
>  /* Eat dummy APC queued by qemu_cpu_kick_thread.  */
> diff --git a/exec.c b/exec.c
> index cd171adb93..71fc76f55e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -967,6 +967,8 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  }
>  tlb_init(cpu);
>
> +qemu_plugin_vcpu_init_hook(cpu);
> +
>  #ifndef CONFIG_USER_ONLY
>  if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>  vmstate_register(NULL, cpu->cpu_index, _cpu_common, cpu);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index d1e6ecae03..062817c03b 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -32,6 +32,7 @@
>  #include "hw/boards.h"
>  #include "hw/qdev-properties.h"
>  #include "trace-root.h"
> +#include "qemu/plugin.h"
>
>  CPUInterruptHandler cpu_interrupt_handler;
>
> @@ -353,6 +354,7 @@ static void cpu_common_unrealizefn(DeviceState *dev, 
> Error **errp)
>  CPUState *cpu = CPU(dev);
>  /* NOTE: latest generic point before the cpu is fully unrealized */
>  trace_fini_vcpu(cpu);
> +qemu_plugin_vcpu_exit_hook(cpu);
>  cpu_exec_unrealizefn(cpu);
>  }


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Peter Maydell]

On Fri, 23 Nov 2018 at 16:59, Edgar E. Iglesias
 wrote:
> Not sure if it's too late to even get the removal of the assert into this 
> release? Peter?

If you're happy that removing the assert is the correct fix,
yes, this could go in before rc3 next week.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Edgar E. Iglesias]

On Fri, Nov 23, 2018 at 05:46:17PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Edgar,

Hi Philippe,

> 
> On 23/11/18 14:54, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" 
> > 
> > Don't assert on RX descriptor settings when the receiver is
> > disabled. This fixes an issue with incoming packets on an
> > unused GEM.
> > 
> > Reported-by: mbilal 
> > Signed-off-by: Edgar E. Iglesias 
> > ---
> >  hw/net/cadence_gem.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index d95cc27f58..7f63411430 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
> > uint8_t *buf, size_t size)
> >  
> >  /* Do nothing if receive is not enabled. */
> >  if (!gem_can_receive(nc)) {
> > -assert(!first_desc);
> 
> Maybe worth:
> 
>trace_gem_receive_packet_drop(size);

Or perhaps a generic tracepoint on packet drops for any device.
Anyway this is probably something for after the release.

Not sure if it's too late to even get the removal of the assert into this 
release? Peter?

> 
> >  return -1;
> 
> Shouldn't this be 'return 0'?
> 
> The "net/net.h" doc is scarce...

If we return 0 my understanding is that we later need to actively
call qemu_flush_or_purge_queued_packets() to renable the rx
path which the GEM model doesn't do. So that would mean
refactoring the model a bit.

Cheers,
Edgar


> 
> Regards,
> 
> Phil.
> 
> >  }
> >  
> > 

Re: [Qemu-devel] [RFC 21/48] *-user: plugin syscalls

[Alex Bennée]

Emilio G. Cota  writes:

> Signed-off-by: Emilio G. Cota 
> ---
>  bsd-user/syscall.c   | 9 +
>  linux-user/syscall.c | 3 +++
>  2 files changed, 12 insertions(+)
>
> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
> index b7818af450..4993f81b2b 100644
> --- a/bsd-user/syscall.c
> +++ b/bsd-user/syscall.c
> @@ -323,6 +323,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  gemu_log("freebsd syscall %d\n", num);
>  #endif
>  trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> arg7, arg8);
> +qemu_plugin_vcpu_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> arg7,
> + arg8);

I think we discussed this on my series about avoiding this sort of
duplication by providing a wrapper for trace points that are also plugin
hooks. So something like:

   trace_and_plugin_guest_user_syscall(...)

Although it's probably worth keeping the trace names grep-able so maybe:

   plug_trace_guest_user_syscall(...)

?
>  if(do_strace)
>  print_freebsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
>
> @@ -404,6 +406,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  if (do_strace)
>  print_freebsd_syscall_ret(num, ret);
>  trace_guest_user_syscall_ret(cpu, num, ret);
> +qemu_plugin_vcpu_syscall_ret(cpu, num, ret);
>  return ret;
>   efault:
>  ret = -TARGET_EFAULT;
> @@ -422,6 +425,8 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  gemu_log("netbsd syscall %d\n", num);
>  #endif
>  trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> 0, 0);
> +qemu_plugin_vcpu_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 0,
> + 0);
>  if(do_strace)
>  print_netbsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
>
> @@ -480,6 +485,7 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  if (do_strace)
>  print_netbsd_syscall_ret(num, ret);
>  trace_guest_user_syscall_ret(cpu, num, ret);
> +qemu_plugin_vcpu_syscall_ret(cpu, num, ret);
>  return ret;
>   efault:
>  ret = -TARGET_EFAULT;
> @@ -498,6 +504,8 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  gemu_log("openbsd syscall %d\n", num);
>  #endif
>  trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> 0, 0);
> +qemu_plugin_vcpu_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 0,
> + 0);
>  if(do_strace)
>  print_openbsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
>
> @@ -556,6 +564,7 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, 
> abi_long arg1,
>  if (do_strace)
>  print_openbsd_syscall_ret(num, ret);
>  trace_guest_user_syscall_ret(cpu, num, ret);
> +qemu_plugin_vcpu_syscall_ret(cpu, num, ret);
>  return ret;
>   efault:
>  ret = -TARGET_EFAULT;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ae3c0dfef7..a6d17a9f37 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11232,6 +11232,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>
>  trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4,
>   arg5, arg6, arg7, arg8);
> +qemu_plugin_vcpu_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 
> arg7,
> + arg8);
>
>  if (unlikely(do_strace)) {
>  print_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
> @@ -11244,5 +11246,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  }
>
>  trace_guest_user_syscall_ret(cpu, num, ret);
> +qemu_plugin_vcpu_syscall_ret(cpu, num, ret);
>  return ret;
>  }


--
Alex Bennée

Re: [Qemu-devel] [RFC 19/48] translate-all: notify plugin code of tb_flush

[Alex Bennée]

Emilio G. Cota  writes:

> Signed-off-by: Emilio G. Cota 
> ---
>  accel/tcg/translate-all.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 3423cf74db..1690e3fd5b 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, 
> gpointer value, gpointer data)
>  /* flush all the translation blocks */
>  void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
>  {
> +bool did_flush = false;
> +
>  mmap_lock();
>  /* If it is already been done on request of another CPU,
>   * just retry.
> @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data 
> tb_flush_count)
>  if (tb_ctx.tb_flush_count != tb_flush_count.host_int) {
>  goto done;
>  }
> +did_flush = true;
>
>  if (DEBUG_TB_FLUSH_GATE) {
>  size_t nb_tbs = tcg_nb_tbs();
> @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data 
> tb_flush_count)
>
>  done:
>  mmap_unlock();
> +if (did_flush) {
> +qemu_plugin_flush_cb();
> +}

Are we introducing a race here?

What is the purpose of letting the plugin know a flush has occurred?

It shouldn't have any knowledge of the details of liveliness of the
translated code and if it still exits or not. If all it wants to do is
look at the counts then I think we can provide a simpler less abuse-able
way to do this.

>  }
>
>  void tb_flush(CPUState *cpu)


--
Alex Bennée

[Qemu-devel] [PATCH 00/11] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

Hi all!

The series brings threads to qcow2 encryption/decryption path,
like it is already done for compression.

Based-on: Kevin's block-next branch [d3db1496c5]

Performance gain is illustrated by the following test:

]# cat test.sh 
#!/bin/bash

size=1G
src=/ssd/src.raw
dst=/ssd/dst.enc.qcow2

echo create source for tests
./qemu-img create -f raw "$src" $size
./qemu-io -f raw -c "write -P 0xa 0 $size" "$src"

for w in "" "-W"; do
echo -e "\n\nTest with additional paramter for qemu-img: '$w'\n"

echo create target...
./qemu-img create -f qcow2 --object secret,id=sec0,data=test -o 
encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10 "$dst" $size
echo

echo test...
time ./qemu-img convert $w -f raw --object secret,id=sec0,data=test 
--target-image-opts -n "$src" 
"driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
done



before patches:

]# ./test.sh 
create source for tests
Formatting '/ssd/src.raw', fmt=raw size=1073741824
wrote 1073741824/1073741824 bytes at offset 0
1 GiB, 1 ops; 0:00:03.02 (338.734 MiB/sec and 0.3308 ops/sec)


Test with additional paramter for qemu-img: ''

create target...
Formatting '/ssd/dst.enc.qcow2', fmt=qcow2 size=1073741824 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 cluster_size=65536 
lazy_refcounts=off refcount_bits=16

test...

real0m12.014s
user0m11.299s
sys 0m0.928s


Test with additional paramter for qemu-img: '-W'

create target...
Formatting '/ssd/dst.enc.qcow2', fmt=qcow2 size=1073741824 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 cluster_size=65536 
lazy_refcounts=off refcount_bits=16

test...

real0m11.639s
user0m11.324s
sys 0m1.149s



after patches:

]# ./test.sh 
create source for tests
Formatting '/ssd/src.raw', fmt=raw size=1073741824
wrote 1073741824/1073741824 bytes at offset 0
1 GiB, 1 ops; 0:00:02.63 (388.900 MiB/sec and 0.3798 ops/sec)


Test with additional paramter for qemu-img: ''

create target...
Formatting '/ssd/dst.enc.qcow2', fmt=qcow2 size=1073741824 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 cluster_size=65536 
lazy_refcounts=off refcount_bits=16

test...

real0m12.113s
user0m11.433s
sys 0m0.878s


Test with additional paramter for qemu-img: '-W'

create target...
Formatting '/ssd/dst.enc.qcow2', fmt=qcow2 size=1073741824 encrypt.format=luks 
encrypt.key-secret=sec0 encrypt.iter-time=10 cluster_size=65536 
lazy_refcounts=off refcount_bits=16

test...

real0m3.436s
user0m13.429s
sys 0m1.183s


Vladimir Sementsov-Ogievskiy (11):
  qcow2.h: add missing include
  qcow2: add separate file for threaded data processing functions
  qcow2-threads: use thread_pool_submit_co
  qcow2: split out data processing threads state from BDRVQcow2State
  qcow2-threads: split out generic path
  qcow2-threads: add per-thread data
  qcow2-threads: add encryption
  qcow2: bdrv_co_preadv: improve locking
  qcow2: qcow2_co_preadv: skip using hd_qiov when possible
  qcow2: bdrv_co_pwritev: move encryption code out of the lock
  qcow2: do encryption in threads

 block/qcow2.h |  34 -
 block/qcow2-threads.c | 281 
 block/qcow2.c | 328 +-
 block/Makefile.objs   |   2 +-
 4 files changed, 412 insertions(+), 233 deletions(-)
 create mode 100644 block/qcow2-threads.c

-- 
2.18.0

[Qemu-devel] [PATCH 06/11] qcow2-threads: add per-thread data

[Vladimir Sementsov-Ogievskiy]

We'll need per-thread data (crypto blocks) for encryption threads.
Prepare threads infrastructure for it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |  6 ++
 block/qcow2-threads.c | 25 ++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index abe51a385c..7bef0393ce 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -257,9 +257,15 @@ typedef struct Qcow2BitmapHeaderExt {
 uint64_t bitmap_directory_offset;
 } QEMU_PACKED Qcow2BitmapHeaderExt;
 
+#define QCOW2_MAX_THREADS 4
+typedef struct Qcow2PerThreadData {
+bool in_use;
+} Qcow2PerThreadData;
+
 typedef struct Qcow2ThreadsState {
 CoQueue task_queue;
 int count;
+Qcow2PerThreadData per_thread[QCOW2_MAX_THREADS];
 } Qcow2ThreadsState;
 
 typedef struct BDRVQcow2State {
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a09f07e2f9..3ed990ef2f 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -31,24 +31,42 @@
 #include "qcow2.h"
 #include "block/thread-pool.h"
 
-#define QCOW2_MAX_THREADS 4
+typedef struct Qcow2ProcessData {
+Qcow2PerThreadData *self;
+void *arg;
+} Qcow2ProcessData;
 
 static int coroutine_fn
 qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func, void *arg)
 {
 int ret;
+int ind;
 BDRVQcow2State *s = bs->opaque;
 Qcow2ThreadsState *thr = >threads;
 ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+Qcow2ProcessData data = {
+.arg = arg
+};
 
 while (thr->count >= QCOW2_MAX_THREADS) {
 qemu_co_queue_wait(>task_queue, NULL);
 }
 
+for (ind = 0; ind < QCOW2_MAX_THREADS; ind++) {
+if (!thr->per_thread[ind].in_use) {
+data.self = >per_thread[ind];
+thr->per_thread[ind].in_use = true;
+break;
+}
+}
+assert(ind < QCOW2_MAX_THREADS);
+
 thr->count++;
-ret = thread_pool_submit_co(pool, func, arg);
+ret = thread_pool_submit_co(pool, func, );
 thr->count--;
 
+thr->per_thread[ind].in_use = false;
+
 qemu_co_queue_next(>task_queue);
 
 return ret;
@@ -158,7 +176,8 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
 
 static int qcow2_compress_pool_func(void *opaque)
 {
-Qcow2CompressData *data = opaque;
+Qcow2ProcessData *pdata = opaque;
+Qcow2CompressData *data = pdata->arg;
 
 data->ret = data->func(data->dest, data->dest_size,
data->src, data->src_size);
-- 
2.18.0

Re: [Qemu-devel] [RFC 18/48] tcg: add memory callbacks for plugins (WIP)

[Alex Bennée]

Emilio G. Cota  writes:

> XXX: store hostaddr from non-i386 TCG backends
> XXX: what hostaddr to return for I/O accesses?
> XXX: what hostaddr to return for cross-page accesses?
>
> Here the trickiest feature is passing the host address to
> memory callbacks that request it. Perhaps it would be more
> appropriate to pass a "physical" address to plugins, but since
> in QEMU host addr ~= guest physical, I'm going with that for
> simplicity.
>
> To keep the implementation simple we piggy-back on the TLB fast path,
> and thus can only provide the host address _after_ memory accesses
> have occurred. For the slow path, it's a bit tedious because there
> are many places to update, but it's fairly simple.
>
> However, note that cross-page accesses are tricky, since the
> access might be to non-contiguous host addresses. So I'm punting
> on that and just passing NULL.
>
> Signed-off-by: Emilio G. Cota 
> ---
>  accel/tcg/atomic_template.h   |  8 -
>  accel/tcg/softmmu_template.h  | 39 
>  include/exec/cpu-defs.h   |  2 ++
>  include/exec/cpu_ldst_template.h  | 43 +++
>  include/exec/cpu_ldst_useronly_template.h | 42 +++---
>  tcg/tcg-op.h  |  5 +++
>  tcg/tcg.h |  4 +++
>  tcg/i386/tcg-target.inc.c |  5 +++
>  tcg/tcg-op.c  | 37 ++-
>  tcg/tcg.c |  3 ++
>  10 files changed, 152 insertions(+), 36 deletions(-)
>
> diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
> index b13318c1ce..3de34dc462 100644
> --- a/accel/tcg/atomic_template.h
> +++ b/accel/tcg/atomic_template.h
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see 
> .
>   */
>
> +#include "qemu/plugin.h"
>  #include "trace/mem.h"
>
>  #if DATA_SIZE == 16
> @@ -66,17 +67,22 @@
>  trace_guest_mem_before_exec(ENV_GET_CPU(env), addr, info | 
> TRACE_MEM_ST); \
>  } while (0)
>
> -# define ATOMIC_TRACE_RMW_POST  \
> +# define ATOMIC_TRACE_RMW_POST do {  
>   \
> +  qemu_plugin_vcpu_mem_cb(ENV_GET_CPU(env), addr, haddr, info);  
>   \
> +  qemu_plugin_vcpu_mem_cb(ENV_GET_CPU(env), addr, haddr, info | 
> TRACE_MEM_ST); \
> +} while (0)
>
>  # define ATOMIC_TRACE_LD_PRE\
>  trace_guest_mem_before_exec(ENV_GET_CPU(env), addr, info)
>
>  # define ATOMIC_TRACE_LD_POST   \
> +qemu_plugin_vcpu_mem_cb(ENV_GET_CPU(env), addr, haddr, info)
>
>  # define ATOMIC_TRACE_ST_PRE\
>  trace_guest_mem_before_exec(ENV_GET_CPU(env), addr, info)
>
>  # define ATOMIC_TRACE_ST_POST   \
> +qemu_plugin_vcpu_mem_cb(ENV_GET_CPU(env), addr, haddr, info)
>
>  #endif /* ATOMIC_TRACE_RMW_PRE */
>
> diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
> index b0adea045e..f6d2f60b81 100644
> --- a/accel/tcg/softmmu_template.h
> +++ b/accel/tcg/softmmu_template.h
> @@ -103,6 +103,11 @@ static inline DATA_TYPE glue(io_read, 
> SUFFIX)(CPUArchState *env,
>MMUAccessType access_type)
>  {
>  CPUIOTLBEntry *iotlbentry = >iotlb[mmu_idx][index];
> +
> +/* XXX Any sensible choice other than NULL? */
> +if (tcg_ctx->plugin_mem_cb) {
> +env->hostaddr = NULL;
> +}

This is more argument for getting the softmmu de-macrofiction in first.


--
Alex Bennée

[Qemu-devel] [PATCH 10/11] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Vladimir Sementsov-Ogievskiy]

Encryption will be done in threads, to take benefit of it, we should
move it out of the lock first.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index bbd6df3614..3fa7e3ea27 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2118,11 +2118,20 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
 ret = qcow2_alloc_cluster_offset(bs, offset, _bytes,
  _offset, );
 if (ret < 0) {
-goto fail;
+goto out_locked;
 }
 
 assert((cluster_offset & 511) == 0);
 
+ret = qcow2_pre_write_overlap_check(bs, 0,
+cluster_offset + offset_in_cluster,
+cur_bytes);
+if (ret < 0) {
+goto out_locked;
+}
+
+qemu_co_mutex_unlock(>lock);
+
 qemu_iovec_reset(_qiov);
 qemu_iovec_concat(_qiov, qiov, bytes_done, cur_bytes);
 
@@ -2134,7 +2143,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
* s->cluster_size);
 if (cluster_data == NULL) {
 ret = -ENOMEM;
-goto fail;
+goto out_unlocked;
 }
 }
 
@@ -2149,40 +2158,34 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
   cluster_data,
   cur_bytes, NULL) < 0) {
 ret = -EIO;
-goto fail;
+goto out_unlocked;
 }
 
 qemu_iovec_reset(_qiov);
 qemu_iovec_add(_qiov, cluster_data, cur_bytes);
 }
 
-ret = qcow2_pre_write_overlap_check(bs, 0,
-cluster_offset + offset_in_cluster, cur_bytes);
-if (ret < 0) {
-goto fail;
-}
-
 /* If we need to do COW, check if it's possible to merge the
  * writing of the guest data together with that of the COW regions.
  * If it's not possible (or not necessary) then write the
  * guest data now. */
 if (!merge_cow(offset, cur_bytes, _qiov, l2meta)) {
-qemu_co_mutex_unlock(>lock);
 BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
 trace_qcow2_writev_data(qemu_coroutine_self(),
 cluster_offset + offset_in_cluster);
 ret = bdrv_co_pwritev(bs->file,
   cluster_offset + offset_in_cluster,
   cur_bytes, _qiov, 0);
-qemu_co_mutex_lock(>lock);
 if (ret < 0) {
-goto fail;
+goto out_unlocked;
 }
 }
 
+qemu_co_mutex_lock(>lock);
+
 ret = qcow2_handle_l2meta(bs, , true);
 if (ret) {
-goto fail;
+goto out_locked;
 }
 
 bytes -= cur_bytes;
@@ -2191,8 +2194,12 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
 trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_bytes);
 }
 ret = 0;
+goto out_locked;
 
-fail:
+out_unlocked:
+qemu_co_mutex_lock(>lock);
+
+out_locked:
 qcow2_handle_l2meta(bs, , false);
 
 qemu_co_mutex_unlock(>lock);
-- 
2.18.0

[Qemu-devel] [PATCH 07/11] qcow2-threads: add encryption

[Vladimir Sementsov-Ogievskiy]

Add thread-based encrypt/decrypt. QCrypto don't support parallel
operations with one block, so we need QCryptoBlock for each thread.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h | 12 +
 block/qcow2-threads.c | 62 +++
 block/qcow2.c | 57 ---
 3 files changed, 122 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 7bef0393ce..351ad8d3e7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -260,6 +260,12 @@ typedef struct Qcow2BitmapHeaderExt {
 #define QCOW2_MAX_THREADS 4
 typedef struct Qcow2PerThreadData {
 bool in_use;
+
+/* QCryptoBlock doesn't support parallel operations in threads, so we can't
+ * use BDRVQcow2State.crypto and instead we need separate crypto block for
+ * each thread.
+ */
+QCryptoBlock *crypto;
 } Qcow2PerThreadData;
 
 typedef struct Qcow2ThreadsState {
@@ -711,5 +717,11 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t 
dest_size,
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size);
+int coroutine_fn
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+ uint64_t offset, void *buf, size_t len);
+int coroutine_fn
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+ uint64_t offset, void *buf, size_t len);
 
 #endif
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3ed990ef2f..0a75c1aead 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -30,6 +30,7 @@
 
 #include "qcow2.h"
 #include "block/thread-pool.h"
+#include "crypto.h"
 
 typedef struct Qcow2ProcessData {
 Qcow2PerThreadData *self;
@@ -217,3 +218,64 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
 qcow2_decompress);
 }
+
+
+/*
+ * Encryption
+ */
+
+typedef int (*Qcow2EncryptFunc)(QCryptoBlock *block, uint64_t offset,
+uint8_t *buf, size_t len, Error **errp);
+/*
+ * encrypt functions are qcrypto_block_encrypt() and qcrypto_block_decrypt()
+ */
+
+typedef struct Qcow2EncryptData {
+uint64_t offset;
+uint8_t *buf;
+size_t len;
+
+Qcow2EncryptFunc func;
+} Qcow2EncryptData;
+
+static int qcow2_encrypt_pool_func(void *opaque)
+{
+Qcow2ProcessData *pdata = opaque;
+Qcow2EncryptData *data = pdata->arg;
+
+return data->func(pdata->self->crypto,
+  data->offset, data->buf, data->len, NULL);
+}
+
+static int coroutine_fn
+qcow2_co_do_crypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+  uint64_t offset, void *buf, size_t len, Qcow2EncryptFunc 
func)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2EncryptData arg = {
+.offset = s->crypt_physical_offset ?
+  file_cluster_offset + offset_into_cluster(s, offset) :
+  offset,
+.buf = buf,
+.len = len,
+.func = func,
+};
+
+return qcow2_co_process(bs, qcow2_encrypt_pool_func, );
+}
+
+int coroutine_fn
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+ uint64_t offset, void *buf, size_t len)
+{
+return qcow2_co_do_crypt(bs, file_cluster_offset, offset, buf, len,
+ qcrypto_block_encrypt);
+}
+
+int coroutine_fn
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+ uint64_t offset, void *buf, size_t len)
+{
+return qcow2_co_do_crypt(bs, file_cluster_offset, offset, buf, len,
+ qcrypto_block_decrypt);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 295ae926ee..1e28f17373 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -170,6 +170,47 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock 
*block, size_t offset,
 return ret;
 }
 
+static void qcow2_crypto_blocks_free(BDRVQcow2State *s)
+{
+int i;
+
+qcrypto_block_free(s->crypto);
+s->crypto = NULL;
+
+for (i = 0; i < QCOW2_MAX_THREADS; i++) {
+qcrypto_block_free(s->threads.per_thread[i].crypto);
+s->threads.per_thread[i].crypto = NULL;
+}
+}
+
+static int qcow2_crypto_blocks_open(BDRVQcow2State *s,
+const char *optprefix,
+QCryptoBlockReadFunc readfunc,
+void *opaque,
+unsigned int flags,
+Error **errp)
+{
+int i;
+
+s->crypto = qcrypto_block_open(s->crypto_opts, optprefix,
+   readfunc, opaque, flags, errp);
+if (!s->crypto) {
+qcrypto_block_free(s->crypto);
+return -EINVAL;
+}
+
+for (i = 0; i < QCOW2_MAX_THREADS; i++) {
+

[Qemu-devel] [PATCH 01/11] qcow2.h: add missing include

[Vladimir Sementsov-Ogievskiy]

qcow2.h depends on block_int.h. Compilation isn't broken currently only
due to block_int.h always included before qcow2.h. Though, it seems
better to directly include block_int.h in qcow2.h.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index a98d24500b..5095f893a0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -28,6 +28,7 @@
 #include "crypto/block.h"
 #include "qemu/coroutine.h"
 #include "qemu/units.h"
+#include "block/block_int.h"
 
 //#define DEBUG_ALLOC
 //#define DEBUG_ALLOC2
-- 
2.18.0

[Qemu-devel] [PATCH 04/11] qcow2: split out data processing threads state from BDRVQcow2State

[Vladimir Sementsov-Ogievskiy]

The state will grow in further commits, so it's good to give it its own
structure.

Make naming generic, as threads will be used for encryption too.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |  8 ++--
 block/qcow2-threads.c | 11 ++-
 block/qcow2.c |  2 +-
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index be84d7c96a..abe51a385c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -257,6 +257,11 @@ typedef struct Qcow2BitmapHeaderExt {
 uint64_t bitmap_directory_offset;
 } QEMU_PACKED Qcow2BitmapHeaderExt;
 
+typedef struct Qcow2ThreadsState {
+CoQueue task_queue;
+int count;
+} Qcow2ThreadsState;
+
 typedef struct BDRVQcow2State {
 int cluster_bits;
 int cluster_size;
@@ -336,8 +341,7 @@ typedef struct BDRVQcow2State {
 char *image_backing_file;
 char *image_backing_format;
 
-CoQueue compress_wait_queue;
-int nb_compress_threads;
+Qcow2ThreadsState threads;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 8c10191f2f..028cf3175e 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -145,6 +145,7 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
  const void *src, size_t src_size, Qcow2CompressFunc func)
 {
 BDRVQcow2State *s = bs->opaque;
+Qcow2ThreadsState *thr = >threads;
 ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
 Qcow2CompressData arg = {
 .dest = dest,
@@ -154,15 +155,15 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 .func = func,
 };
 
-while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
-qemu_co_queue_wait(>compress_wait_queue, NULL);
+while (thr->count >= MAX_COMPRESS_THREADS) {
+qemu_co_queue_wait(>task_queue, NULL);
 }
 
-s->nb_compress_threads++;
+thr->count++;
 thread_pool_submit_co(pool, qcow2_compress_pool_func, );
-s->nb_compress_threads--;
+thr->count--;
 
-qemu_co_queue_next(>compress_wait_queue);
+qemu_co_queue_next(>task_queue);
 
 return arg.ret;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 3fcb80d747..295ae926ee 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1600,7 +1600,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 }
 #endif
 
-qemu_co_queue_init(>compress_wait_queue);
+qemu_co_queue_init(>threads.task_queue);
 
 return ret;
 
-- 
2.18.0

[Qemu-devel] [PATCH 02/11] qcow2: add separate file for threaded data processing functions

[Vladimir Sementsov-Ogievskiy]

Move compression-on-threads to separate file. Encryption will be in it
too.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |   7 ++
 block/qcow2-threads.c | 197 ++
 block/qcow2.c | 169 
 block/Makefile.objs   |   2 +-
 4 files changed, 205 insertions(+), 170 deletions(-)
 create mode 100644 block/qcow2-threads.c

diff --git a/block/qcow2.h b/block/qcow2.h
index 5095f893a0..be84d7c96a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -695,4 +695,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
   const char *name,
   Error **errp);
 
+ssize_t coroutine_fn
+qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
+  const void *src, size_t src_size);
+ssize_t coroutine_fn
+qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
+const void *src, size_t src_size);
+
 #endif
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
new file mode 100644
index 00..b75fad6e07
--- /dev/null
+++ b/block/qcow2-threads.c
@@ -0,0 +1,197 @@
+/*
+ * Threaded data processing for Qcow2: compression, encryption
+ *
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#define ZLIB_CONST
+#include 
+
+#include "qcow2.h"
+#include "block/thread-pool.h"
+
+#define MAX_COMPRESS_THREADS 4
+
+typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
+ const void *src, size_t src_size);
+typedef struct Qcow2CompressData {
+void *dest;
+size_t dest_size;
+const void *src;
+size_t src_size;
+ssize_t ret;
+
+Qcow2CompressFunc func;
+} Qcow2CompressData;
+
+/*
+ * qcow2_compress()
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -1 destination buffer is not enough to store compressed data
+ *  -2 on any other error
+ */
+static ssize_t qcow2_compress(void *dest, size_t dest_size,
+  const void *src, size_t src_size)
+{
+ssize_t ret;
+z_stream strm;
+
+/* best compression, small window, no zlib header */
+memset(, 0, sizeof(strm));
+ret = deflateInit2(, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+   -12, 9, Z_DEFAULT_STRATEGY);
+if (ret != Z_OK) {
+return -2;
+}
+
+/* strm.next_in is not const in old zlib versions, such as those used on
+ * OpenBSD/NetBSD, so cast the const away */
+strm.avail_in = src_size;
+strm.next_in = (void *) src;
+strm.avail_out = dest_size;
+strm.next_out = dest;
+
+ret = deflate(, Z_FINISH);
+if (ret == Z_STREAM_END) {
+ret = dest_size - strm.avail_out;
+} else {
+ret = (ret == Z_OK ? -1 : -2);
+}
+
+deflateEnd();
+
+return ret;
+}
+
+/*
+ * qcow2_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes.
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  -1 on fail
+ */
+static ssize_t qcow2_decompress(void *dest, size_t dest_size,
+const void *src, size_t src_size)
+{
+int ret = 0;
+z_stream strm;
+
+memset(, 0, sizeof(strm));
+strm.avail_in = src_size;
+strm.next_in = (void *) src;
+strm.avail_out = dest_size;
+strm.next_out = dest;
+
+ret = inflateInit2(, -12);
+if (ret != Z_OK) {
+return -1;
+}
+
+ret = inflate(, Z_FINISH);
+if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) || strm.avail_out != 0) {
+/* 

[Qemu-devel] [PATCH 08/11] qcow2: bdrv_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

Background: decryption will be done in threads, to take benefit of it,
we should move it out of the lock first.

But let's go further: it turns out, that for locking around switch
cases we have only two variants: when we just do memset(0) not
releasing the lock (it is useless) and when we actually can handle the
whole case out of the lock. So, refactor the whole thing to reduce
locked code region and make it clean.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 1e28f17373..5467089cfe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1924,6 +1924,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 
 ret = qcow2_get_cluster_offset(bs, offset, _bytes, 
_offset);
 if (ret < 0) {
+qemu_co_mutex_unlock(>lock);
 goto fail;
 }
 
@@ -1932,39 +1933,38 @@ static coroutine_fn int 
qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 qemu_iovec_reset(_qiov);
 qemu_iovec_concat(_qiov, qiov, bytes_done, cur_bytes);
 
+if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
+ret == QCOW2_CLUSTER_ZERO_ALLOC ||
+(ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
+{
+/* No sense in releasing the lock */
+
+qemu_iovec_memset(_qiov, 0, 0, cur_bytes);
+
+bytes -= cur_bytes;
+offset += cur_bytes;
+bytes_done += cur_bytes;
+continue;
+}
+
+qemu_co_mutex_unlock(>lock);
+
 switch (ret) {
 case QCOW2_CLUSTER_UNALLOCATED:
-
-if (bs->backing) {
-BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
-qemu_co_mutex_unlock(>lock);
-ret = bdrv_co_preadv(bs->backing, offset, cur_bytes,
- _qiov, 0);
-qemu_co_mutex_lock(>lock);
-if (ret < 0) {
-goto fail;
-}
-} else {
-/* Note: in this case, no need to wait */
-qemu_iovec_memset(_qiov, 0, 0, cur_bytes);
+BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
+ret = bdrv_co_preadv(bs->backing, offset, cur_bytes, _qiov, 0);
+if (ret < 0) {
+goto fail;
 }
 break;
 
-case QCOW2_CLUSTER_ZERO_PLAIN:
-case QCOW2_CLUSTER_ZERO_ALLOC:
-qemu_iovec_memset(_qiov, 0, 0, cur_bytes);
-break;
-
 case QCOW2_CLUSTER_COMPRESSED:
-qemu_co_mutex_unlock(>lock);
 ret = qcow2_co_preadv_compressed(bs, cluster_offset,
  offset, cur_bytes,
  _qiov);
-qemu_co_mutex_lock(>lock);
 if (ret < 0) {
 goto fail;
 }
-
 break;
 
 case QCOW2_CLUSTER_NORMAL:
@@ -1997,11 +1997,9 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-qemu_co_mutex_unlock(>lock);
 ret = bdrv_co_preadv(bs->file,
  cluster_offset + offset_in_cluster,
  cur_bytes, _qiov, 0);
-qemu_co_mutex_lock(>lock);
 if (ret < 0) {
 goto fail;
 }
@@ -2032,12 +2030,14 @@ static coroutine_fn int 
qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 bytes -= cur_bytes;
 offset += cur_bytes;
 bytes_done += cur_bytes;
+
+qemu_co_mutex_lock(>lock);
 }
 ret = 0;
 
-fail:
 qemu_co_mutex_unlock(>lock);
 
+fail:
 qemu_iovec_destroy(_qiov);
 qemu_vfree(cluster_data);
 
-- 
2.18.0

[Qemu-devel] [PATCH 09/11] qcow2: qcow2_co_preadv: skip using hd_qiov when possible

[Vladimir Sementsov-Ogievskiy]

qemu_iovec_memset has @offset parameter, so using hd_qiov for it is not
needed.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5467089cfe..bbd6df3614 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1928,18 +1928,13 @@ static coroutine_fn int 
qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 goto fail;
 }
 
-offset_in_cluster = offset_into_cluster(s, offset);
-
-qemu_iovec_reset(_qiov);
-qemu_iovec_concat(_qiov, qiov, bytes_done, cur_bytes);
-
 if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
 ret == QCOW2_CLUSTER_ZERO_ALLOC ||
 (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
 {
 /* No sense in releasing the lock */
 
-qemu_iovec_memset(_qiov, 0, 0, cur_bytes);
+qemu_iovec_memset(qiov, bytes_done, 0, cur_bytes);
 
 bytes -= cur_bytes;
 offset += cur_bytes;
@@ -1947,6 +1942,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 continue;
 }
 
+offset_in_cluster = offset_into_cluster(s, offset);
+
+qemu_iovec_reset(_qiov);
+qemu_iovec_concat(_qiov, qiov, bytes_done, cur_bytes);
+
 qemu_co_mutex_unlock(>lock);
 
 switch (ret) {
-- 
2.18.0

[Qemu-devel] [PATCH 05/11] qcow2-threads: split out generic path

[Vladimir Sementsov-Ogievskiy]

Move generic part out of qcow2_co_do_compress, to reuse it for
encryption and rename things that would be shared with encryption path.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-threads.c | 41 -
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 028cf3175e..a09f07e2f9 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -31,7 +31,33 @@
 #include "qcow2.h"
 #include "block/thread-pool.h"
 
-#define MAX_COMPRESS_THREADS 4
+#define QCOW2_MAX_THREADS 4
+
+static int coroutine_fn
+qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func, void *arg)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+Qcow2ThreadsState *thr = >threads;
+ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+
+while (thr->count >= QCOW2_MAX_THREADS) {
+qemu_co_queue_wait(>task_queue, NULL);
+}
+
+thr->count++;
+ret = thread_pool_submit_co(pool, func, arg);
+thr->count--;
+
+qemu_co_queue_next(>task_queue);
+
+return ret;
+}
+
+
+/*
+ * Compression
+ */
 
 typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
  const void *src, size_t src_size);
@@ -144,9 +170,6 @@ static ssize_t coroutine_fn
 qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
  const void *src, size_t src_size, Qcow2CompressFunc func)
 {
-BDRVQcow2State *s = bs->opaque;
-Qcow2ThreadsState *thr = >threads;
-ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
 Qcow2CompressData arg = {
 .dest = dest,
 .dest_size = dest_size,
@@ -155,15 +178,7 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 .func = func,
 };
 
-while (thr->count >= MAX_COMPRESS_THREADS) {
-qemu_co_queue_wait(>task_queue, NULL);
-}
-
-thr->count++;
-thread_pool_submit_co(pool, qcow2_compress_pool_func, );
-thr->count--;
-
-qemu_co_queue_next(>task_queue);
+qcow2_co_process(bs, qcow2_compress_pool_func, );
 
 return arg.ret;
 }
-- 
2.18.0

[Qemu-devel] [PATCH 11/11] qcow2: do encryption in threads

[Vladimir Sementsov-Ogievskiy]

Do encryption/decryption in threads, like it is already done for
compression. This improves asynchronous encrypted io.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3fa7e3ea27..a7d77a2178 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2007,13 +2007,8 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 assert(s->crypto);
 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
-if (qcrypto_block_decrypt(s->crypto,
-  (s->crypt_physical_offset ?
-   cluster_offset + offset_in_cluster :
-   offset),
-  cluster_data,
-  cur_bytes,
-  NULL) < 0) {
+if (qcow2_co_decrypt(bs, cluster_offset, offset,
+ cluster_data, cur_bytes) < 0) {
 ret = -EIO;
 goto fail;
 }
@@ -2151,12 +2146,8 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
 qemu_iovec_to_buf(_qiov, 0, cluster_data, hd_qiov.size);
 
-if (qcrypto_block_encrypt(s->crypto,
-  (s->crypt_physical_offset ?
-   cluster_offset + offset_in_cluster :
-   offset),
-  cluster_data,
-  cur_bytes, NULL) < 0) {
+if (qcow2_co_encrypt(bs, cluster_offset, offset,
+ cluster_data, cur_bytes) < 0) {
 ret = -EIO;
 goto out_unlocked;
 }
-- 
2.18.0

[Qemu-devel] [PATCH 03/11] qcow2-threads: use thread_pool_submit_co

[Vladimir Sementsov-Ogievskiy]

Use thread_pool_submit_co, instead of reinventing it here. Note, that
thread_pool_submit_aio() never returns an error, so checking it was an
extra thing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-threads.c | 17 ++---
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index b75fad6e07..8c10191f2f 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -140,17 +140,11 @@ static int qcow2_compress_pool_func(void *opaque)
 return 0;
 }
 
-static void qcow2_compress_complete(void *opaque, int ret)
-{
-qemu_coroutine_enter(opaque);
-}
-
 static ssize_t coroutine_fn
 qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
  const void *src, size_t src_size, Qcow2CompressFunc func)
 {
 BDRVQcow2State *s = bs->opaque;
-BlockAIOCB *acb;
 ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
 Qcow2CompressData arg = {
 .dest = dest,
@@ -165,16 +159,9 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 }
 
 s->nb_compress_threads++;
-acb = thread_pool_submit_aio(pool, qcow2_compress_pool_func, ,
- qcow2_compress_complete,
- qemu_coroutine_self());
-
-if (!acb) {
-s->nb_compress_threads--;
-return -EINVAL;
-}
-qemu_coroutine_yield();
+thread_pool_submit_co(pool, qcow2_compress_pool_func, );
 s->nb_compress_threads--;
+
 qemu_co_queue_next(>compress_wait_queue);
 
 return arg.ret;
-- 
2.18.0

Re: [Qemu-devel] [RFC 16/48] tcg: add plugin_mask to TB hash

[Alex Bennée]

Emilio G. Cota  writes:

> Signed-off-by: Emilio G. Cota 

This commit message needs more description. What is this plugin mask
for? Are we extending the TB flags with a mask of loaded plugins so we
can correctly identify what code was generated under what plugins?

> ---
>  include/exec/exec-all.h   | 2 ++
>  include/exec/tb-hash.h| 6 --
>  include/exec/tb-lookup.h  | 1 +
>  accel/tcg/cpu-exec.c  | 6 +-
>  accel/tcg/translate-all.c | 6 --
>  5 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 232e2f8966..a1f60404b6 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -358,6 +358,8 @@ struct TranslationBlock {
>  /* Per-vCPU dynamic tracing state used to generate this TB */
>  uint32_t trace_vcpu_dstate;
>
> +uint32_t plugin_mask;
> +
>  struct tb_tc tc;
>
>  /* original tb when cflags has CF_NOCACHE */
> diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
> index 4f3a37d927..37292474b7 100644
> --- a/include/exec/tb-hash.h
> +++ b/include/exec/tb-hash.h
> @@ -59,9 +59,11 @@ static inline unsigned int 
> tb_jmp_cache_hash_func(target_ulong pc)
>
>  static inline
>  uint32_t tb_hash_func(tb_page_addr_t phys_pc, target_ulong pc, uint32_t 
> flags,
> -  uint32_t cf_mask, uint32_t trace_vcpu_dstate)
> +  uint32_t cf_mask, uint32_t trace_vcpu_dstate,
> +  uint32_t plugin_mask)
>  {
> -return qemu_xxhash7(phys_pc, pc, flags, cf_mask, trace_vcpu_dstate);
> +return qemu_xxhash8(phys_pc, pc, flags, cf_mask, trace_vcpu_dstate,
> +plugin_mask);
>  }
>
>  #endif
> diff --git a/include/exec/tb-lookup.h b/include/exec/tb-lookup.h
> index 492cb68289..dd1572f481 100644
> --- a/include/exec/tb-lookup.h
> +++ b/include/exec/tb-lookup.h
> @@ -33,6 +33,7 @@ tb_lookup__cpu_state(CPUState *cpu, target_ulong *pc, 
> target_ulong *cs_base,
> tb->cs_base == *cs_base &&
> tb->flags == *flags &&
> tb->trace_vcpu_dstate == *cpu->trace_dstate &&
> +   tb->plugin_mask == *cpu->plugin_mask &&
> (tb_cflags(tb) & (CF_HASH_MASK | CF_INVALID)) == cf_mask)) {
>  return tb;
>  }
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index d590f1f6c0..27aa3451da 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -287,6 +287,7 @@ struct tb_desc {
>  uint32_t flags;
>  uint32_t cf_mask;
>  uint32_t trace_vcpu_dstate;
> +uint32_t plugin_mask;
>  };
>
>  static bool tb_lookup_cmp(const void *p, const void *d)
> @@ -299,6 +300,7 @@ static bool tb_lookup_cmp(const void *p, const void *d)
>  tb->cs_base == desc->cs_base &&
>  tb->flags == desc->flags &&
>  tb->trace_vcpu_dstate == desc->trace_vcpu_dstate &&
> +tb->plugin_mask == desc->plugin_mask &&
>  (tb_cflags(tb) & (CF_HASH_MASK | CF_INVALID)) == desc->cf_mask) {
>  /* check next page if needed */
>  if (tb->page_addr[1] == -1) {
> @@ -330,13 +332,15 @@ TranslationBlock *tb_htable_lookup(CPUState *cpu, 
> target_ulong pc,
>  desc.flags = flags;
>  desc.cf_mask = cf_mask;
>  desc.trace_vcpu_dstate = *cpu->trace_dstate;
> +desc.plugin_mask = *cpu->plugin_mask;
>  desc.pc = pc;
>  phys_pc = get_page_addr_code(desc.env, pc);
>  if (phys_pc == -1) {
>  return NULL;
>  }
>  desc.phys_page1 = phys_pc & TARGET_PAGE_MASK;
> -h = tb_hash_func(phys_pc, pc, flags, cf_mask, *cpu->trace_dstate);
> +h = tb_hash_func(phys_pc, pc, flags, cf_mask, *cpu->trace_dstate,
> + *cpu->plugin_mask);
>  return qht_lookup_custom(_ctx.htable, , h, tb_lookup_cmp);
>  }
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index db2d28f8d3..3423cf74db 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1129,6 +1129,7 @@ static bool tb_cmp(const void *ap, const void *bp)
>  a->flags == b->flags &&
>  (tb_cflags(a) & CF_HASH_MASK) == (tb_cflags(b) & CF_HASH_MASK) &&
>  a->trace_vcpu_dstate == b->trace_vcpu_dstate &&
> +a->plugin_mask == b->plugin_mask &&
>  a->page_addr[0] == b->page_addr[0] &&
>  a->page_addr[1] == b->page_addr[1];
>  }
> @@ -1444,7 +1445,7 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, 
> bool rm_from_page_list)
>  /* remove the TB from the hash list */
>  phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
>  h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb_cflags(tb) & 
> CF_HASH_MASK,
> - tb->trace_vcpu_dstate);
> + tb->trace_vcpu_dstate, tb->plugin_mask);
>  if (!(tb->cflags & CF_NOCACHE) &&
>  !qht_remove(_ctx.htable, tb, h)) {
>  return;
> @@ -1640,7 +1641,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t 
> phys_pc,
>
>   

Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

[Philippe Mathieu-Daudé]

Hi Edgar,

On 23/11/18 14:54, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" 
> 
> Don't assert on RX descriptor settings when the receiver is
> disabled. This fixes an issue with incoming packets on an
> unused GEM.
> 
> Reported-by: mbilal 
> Signed-off-by: Edgar E. Iglesias 
> ---
>  hw/net/cadence_gem.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d95cc27f58..7f63411430 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
> uint8_t *buf, size_t size)
>  
>  /* Do nothing if receive is not enabled. */
>  if (!gem_can_receive(nc)) {
> -assert(!first_desc);

Maybe worth:

   trace_gem_receive_packet_drop(size);

>  return -1;

Shouldn't this be 'return 0'?

The "net/net.h" doc is scarce...

Regards,

Phil.

>  }
>  
> 

Re: [Qemu-devel] [Qemu-discuss] How to select specific qemu net 'nic' device

[Edgar E. Iglesias]

On Fri, Nov 23, 2018 at 02:20:17PM +0100, Edgar E. Iglesias wrote:
> On Fri, Nov 23, 2018 at 02:59:32PM +0500, mbilal wrote:
> > Hi,
> > 
> > Thanks for reply.
> > 
> > According to your suggestion I've tested with 3.1 rc2 release and problem is
> > still exist in this release also.
> > 
> > Here is my reproducible scenario.
> 
> 
> Thanks,
> 
> I've had a look and the assert looks bogus to me.
> We shouldn't be asserting on RX descriptor setups when the receiver is 
> disabled.
> 
> I'll send a patch in a moment.
> 
> Best regards,
> Edgar

+ Jason

Hi,

I've sent a patch to fix te GEM model.
The assert in the GEM model was wrong IMO but I also think the net layer
is behaving a bit suspicious in this case. The 
qemu_flush_or_purge_queued_packets()
path is delivering queued packets to a net model without checking can_receive().
Without digging too much into the details my gut feeling is that the net layer
shouldn't be doing this. I may be missing something though.

Here's the backtrace:
(gdb) bt
#0  0x739e0e97 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x739e2801 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x739d239a in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x739d2412 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x55b47de6 in gem_receive (nc=, 
buf=, size=346)
at /home/edgar/src/c/qemu/qemu/hw/net/cadence_gem.c:982
#5  0x55bf033d in nc_sendv_compat (flags=, iovcnt=1, 
iov=0x7fffd620, nc=0x56edd360)
at /home/edgar/src/c/qemu/qemu/net/net.c:701
#6  qemu_deliver_packet_iov (sender=, flags=, 
iov=0x7fffd620, iovcnt=1, opaque=0x56edd360)
at /home/edgar/src/c/qemu/qemu/net/net.c:733
#7  0x55bf2a85 in qemu_net_queue_deliver (size=, 
data=, flags=, sender=, 
queue=0x56edd4e0) at /home/edgar/src/c/qemu/qemu/net/queue.c:164
#8  qemu_net_queue_flush (queue=0x56edd4e0)
at /home/edgar/src/c/qemu/qemu/net/queue.c:261
#9  0x55bf0d3c in qemu_flush_or_purge_queued_packets (
nc=0x56edd360, purge=)
at /home/edgar/src/c/qemu/qemu/net/net.c:607
#10 0x55bf0dd9 in net_vm_change_state_handler (opaque=, 
running=0, state=)
at /home/edgar/src/c/qemu/qemu/net/net.c:1423
#11 0x55a641c7 in vm_state_notify (running=running@entry=0, 
state=state@entry=RUN_STATE_SHUTDOWN)
at /home/edgar/src/c/qemu/qemu/vl.c:1562
#12 0x55889b0a in do_vm_stop (state=RUN_STATE_SHUTDOWN, 
send_stop=) at /home/edgar/src/c/qemu/qemu/cpus.c:1074
#13 0x55844200 in main (argc=, argv=, 
envp=) at /home/edgar/src/c/qemu/qemu/vl.c:4648
(gdb) q


Cheers,
Edgar


> 
> 
> 
> > 
> > QEMU launching
> > --
> > 
> > ./qemu-system-aarch64 -M xlnx-zcu102 -m 1G -kernel /dev/null -S -s  -net nic
> > -net nic -net nic -net nic -net user,hostfwd=tcp::8080-:8080 -smp 1 -serial
> > stdio -serial /dev/null
> > 
> > gdb session
> > 
> > 
> > (gdb) file zcu102_net_uni.out
> > Reading symbols from zcu102_net_uni.out...done.
> > (gdb) tar rem :1234
> > Remote debugging using :1234
> > _ld_text_start () at os/arch/armv8/common/arch_asm.S:381
> > 381 MOV X14, X0
> > (gdb) load
> > Loading section .text, size 0x59c0c lma 0x0
> > Loading section .rodata, size 0x2bbc lma 0x59c0c
> > Loading section .rtl.data, size 0x770 lma 0x5c7c8
> > Loading section .data, size 0x4dc lma 0x5cf54
> > Start address 0x0, load size 381972
> > Transfer rate: 23313 KB/sec, 2021 bytes/write.
> > (gdb) c
> > Continuing.
> > ^C
> > 
> > 
> > So any breakpoint or Ctrl-C asserts the QEMU.
> > 
> > qemu-system-aarch64: qemu-3.1.0-rc2/hw/net/cadence_gem.c:982: gem_receive:
> > Assertion `!first_desc' failed.
> > 
> > 
> > Our networking demo application running fine unless you interrupt the GDB
> > (either with breakpoints or interrupt signal).
> > You can see following message on QEMU console which is the indication of
> > demo is fine (it also well tested on actual hardware)
> > 
> > Open the following Nucleus node address in your web browser:
> > http://127.0.0.1:8080/
> > 
> > 
> > I'm attaching demo application binary.
> > 
> > 
> > 
> > 
> > Thanks for care about this issue,
> > -Bilal
> > 
> > 
> > 
> > On 11/23/2018 01:59 PM, Peter Maydell wrote:
> > > On 23 November 2018 at 04:13, mbilal  wrote:
> > > > Hi,
> > > > 
> > > > I'm using qemu emulation for xilinx zcu102 platform, this board have 
> > > > four
> > > > networking GEM0, GEM1, GEM2 and GEM3 devices.
> > > > 
> > > > To run network demo on this board *only* require GEM3 device to be 
> > > > configure
> > > > while other GEM devices don't need to be configure, that's why u-boot 
> > > > and
> > > > other RTOS only configure GEM3 device.
> > > > 
> > > > 
> > > > QEMU is enabling these GEM devices  with networking '-net nic' option 
> > > > and
> > > > QEMU consider first '-net nic' option for GEM0 and second '-net nic' 
> > > > option
> > > > for GEM1 and so on. that's why if need to 

Re: [Qemu-devel] [PATCH v2 15/21] pci-bridge/dec: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

Hi Mao,

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> pci_dec_21154_device_class_init().
> 
> Cc: da...@gibson.dropbear.id.au
> Cc: m...@redhat.com
> Cc: marcel.apfelb...@gmail.com
> Cc: qemu-...@nongnu.org
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 
> Reviewed-by: David Gibson 
> Acked-by: David Gibson 
> ---
>  hw/pci-bridge/dec.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
> index 84492d5e5f..5b21c20e50 100644
> --- a/hw/pci-bridge/dec.c
> +++ b/hw/pci-bridge/dec.c
> @@ -98,7 +98,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
>  return pci_bridge_get_sec_bus(br);
>  }
>  
> -static int pci_dec_21154_device_init(SysBusDevice *dev)
> +static void pci_dec_21154_device_realize(DeviceState *dev, Error **errp)
>  {
>  PCIHostState *phb;

Why don't you use:

   SysBusDevice *sbd = SYS_BUS_DEVICE(dev);

like in all the other patches from this series...?

>  
> @@ -108,9 +108,8 @@ static int pci_dec_21154_device_init(SysBusDevice *dev)
>dev, "pci-conf-idx", 0x1000);
>  memory_region_init_io(>data_mem, OBJECT(dev), _host_data_le_ops,
>dev, "pci-data-idx", 0x1000);
> -sysbus_init_mmio(dev, >conf_mem);
> -sysbus_init_mmio(dev, >data_mem);
> -return 0;
> +sysbus_init_mmio(SYS_BUS_DEVICE(dev), >conf_mem);
> +sysbus_init_mmio(SYS_BUS_DEVICE(dev), >data_mem);

... this would save extra type checking here, as explain by Peter:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03644.html

>  }
>  
>  static void dec_21154_pci_host_realize(PCIDevice *d, Error **errp)
> @@ -150,9 +149,9 @@ static const TypeInfo dec_21154_pci_host_info = {
>  
>  static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -sdc->init = pci_dec_21154_device_init;
> +dc->realize = pci_dec_21154_device_realize;
>  }
>  
>  static const TypeInfo pci_dec_21154_device_info = {
> 

Re: [Qemu-devel] [PATCH v2 16/21] timer/etraxfs_timer: Convert sysbus init function to realize function

[Edgar E. Iglesias]

On Fri, Nov 23, 2018 at 11:30:35PM +0800, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> etraxfs_timer_class_init().
> 
> Cc: edgar.igles...@gmail.com
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 


Reviewed-by: Edgar E. Iglesias 


> ---
>  hw/timer/etraxfs_timer.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index d13bc30b2d..2280914b1d 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -315,9 +315,10 @@ static void etraxfs_timer_reset(void *opaque)
>  qemu_irq_lower(t->irq);
>  }
>  
> -static int etraxfs_timer_init(SysBusDevice *dev)
> +static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
>  {
>  ETRAXTimerState *t = ETRAX_TIMER(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  t->bh_t0 = qemu_bh_new(timer0_hit, t);
>  t->bh_t1 = qemu_bh_new(timer1_hit, t);
> @@ -326,21 +327,20 @@ static int etraxfs_timer_init(SysBusDevice *dev)
>  t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
>  t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
>  
> -sysbus_init_irq(dev, >irq);
> -sysbus_init_irq(dev, >nmi);
> +sysbus_init_irq(sbd, >irq);
> +sysbus_init_irq(sbd, >nmi);
>  
>  memory_region_init_io(>mmio, OBJECT(t), _ops, t,
>"etraxfs-timer", 0x5c);
> -sysbus_init_mmio(dev, >mmio);
> +sysbus_init_mmio(sbd, >mmio);
>  qemu_register_reset(etraxfs_timer_reset, t);
> -return 0;
>  }
>  
>  static void etraxfs_timer_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -sdc->init = etraxfs_timer_init;
> +dc->realize = etraxfs_timer_realize;
>  }
>  
>  static const TypeInfo etraxfs_timer_info = {
> -- 
> 2.17.1
> 
> 
> 

Re: [Qemu-devel] [PATCH] configure: fix elf2dmp check

[Paolo Bonzini]

On 23/11/18 10:01, Roman Kagan wrote:
> elf2dmp is keyed on "$posix" = "yes", but "$posix" doesn't seem to be
> set anywhere.
> 
> The original intent was presumably to skip building it on Windows, so
> check for "$mingw32" = "no" instead.
> 
> Signed-off-by: Roman Kagan 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 0a3c6a72c3..fc6ce0065d 100755
> --- a/configure
> +++ b/configure
> @@ -5722,7 +5722,7 @@ if test "$want_tools" = "yes" ; then
>if [ "$ivshmem" = "yes" ]; then
>  tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
>fi
> -  if [ "$posix" = "yes" ] && [ "$curl" = "yes" ]; then
> +  if [ "$mingw32" = "no" ] && [ "$curl" = "yes" ]; then
>  tools="elf2dmp $tools"
>fi
>  fi
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH v2 18/21] timer/puv3_ost: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> puv3_ost_class_init().
> 
> Cc: g...@mprc.pku.edu.cn
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/timer/puv3_ost.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
> index 0b3d717e60..3be58c7fdd 100644
> --- a/hw/timer/puv3_ost.c
> +++ b/hw/timer/puv3_ost.c
> @@ -113,16 +113,17 @@ static void puv3_ost_tick(void *opaque)
>  }
>  }
>  
> -static int puv3_ost_init(SysBusDevice *dev)
> +static void puv3_ost_realize(DeviceState *dev, Error **errp)
>  {
>  PUV3OSTState *s = PUV3_OST(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  s->reg_OIER = 0;
>  s->reg_OSSR = 0;
>  s->reg_OSMR0 = 0;
>  s->reg_OSCR = 0;
>  
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_irq(sbd, >irq);
>  
>  s->bh = qemu_bh_new(puv3_ost_tick, s);
>  s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
> @@ -130,16 +131,14 @@ static int puv3_ost_init(SysBusDevice *dev)
>  
>  memory_region_init_io(>iomem, OBJECT(s), _ost_ops, s, "puv3_ost",
>  PUV3_REGS_OFFSET);
> -sysbus_init_mmio(dev, >iomem);
> -
> -return 0;
> +sysbus_init_mmio(sbd, >iomem);
>  }
>  
>  static void puv3_ost_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -sdc->init = puv3_ost_init;
> +dc->realize = puv3_ost_realize;
>  }
>  
>  static const TypeInfo puv3_ost_info = {
> 

Re: [Qemu-devel] [PATCH v2 16/21] timer/etraxfs_timer: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> etraxfs_timer_class_init().
> 
> Cc: edgar.igles...@gmail.com
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/timer/etraxfs_timer.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index d13bc30b2d..2280914b1d 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -315,9 +315,10 @@ static void etraxfs_timer_reset(void *opaque)
>  qemu_irq_lower(t->irq);
>  }
>  
> -static int etraxfs_timer_init(SysBusDevice *dev)
> +static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
>  {
>  ETRAXTimerState *t = ETRAX_TIMER(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  t->bh_t0 = qemu_bh_new(timer0_hit, t);
>  t->bh_t1 = qemu_bh_new(timer1_hit, t);
> @@ -326,21 +327,20 @@ static int etraxfs_timer_init(SysBusDevice *dev)
>  t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
>  t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
>  
> -sysbus_init_irq(dev, >irq);
> -sysbus_init_irq(dev, >nmi);
> +sysbus_init_irq(sbd, >irq);
> +sysbus_init_irq(sbd, >nmi);
>  
>  memory_region_init_io(>mmio, OBJECT(t), _ops, t,
>"etraxfs-timer", 0x5c);
> -sysbus_init_mmio(dev, >mmio);
> +sysbus_init_mmio(sbd, >mmio);
>  qemu_register_reset(etraxfs_timer_reset, t);
> -return 0;
>  }
>  
>  static void etraxfs_timer_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -sdc->init = etraxfs_timer_init;
> +dc->realize = etraxfs_timer_realize;
>  }
>  
>  static const TypeInfo etraxfs_timer_info = {
> 

Re: [Qemu-devel] [PATCH v2 02/21] block/noenand: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> onenand_class_init().
> 
> Cc: kw...@redhat.com
> Cc: mre...@redhat.com
> Cc: qemu-bl...@nongnu.org
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/block/onenand.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index 2b48609776..f8a687 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -772,9 +772,9 @@ static const MemoryRegionOps onenand_ops = {
>  .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int onenand_initfn(SysBusDevice *sbd)
> +static void onenand_realize(DeviceState *dev, Error **errp)
>  {
> -DeviceState *dev = DEVICE(sbd);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  OneNANDState *s = ONE_NAND(dev);
>  uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
>  void *ram;
> @@ -794,14 +794,14 @@ static int onenand_initfn(SysBusDevice *sbd)
>0xff, size + (size >> 5));
>  } else {
>  if (blk_is_read_only(s->blk)) {
> -error_report("Can't use a read-only drive");
> -return -1;
> +error_setg(errp, "Can't use a read-only drive");
> +return;
>  }
>  blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>   BLK_PERM_ALL, _err);
>  if (local_err) {
> -error_report_err(local_err);
> -return -1;
> +error_propagate(errp, local_err);
> +return;
>  }
>  s->blk_cur = s->blk;
>  }
> @@ -826,7 +826,6 @@ static int onenand_initfn(SysBusDevice *sbd)
>   | ((s->id.dev & 0xff) << 8)
>   | (s->id.ver & 0xff),
>   _onenand, s);
> -return 0;
>  }
>  
>  static Property onenand_properties[] = {
> @@ -841,9 +840,8 @@ static Property onenand_properties[] = {
>  static void onenand_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = onenand_initfn;
> +dc->realize = onenand_realize;
>  dc->reset = onenand_system_reset;
>  dc->props = onenand_properties;
>  }
> 

Re: [Qemu-devel] SeaBIOS booting time optimization

[Stefano Garzarella]

On Fri, Nov 23, 2018 at 4:54 PM Kevin O'Connor  wrote:
>
> On Fri, Nov 23, 2018 at 12:18:13PM +0100, Stefano Garzarella wrote:
> > On Fri, Nov 23, 2018 at 7:21 AM Gerd Hoffmann  wrote:
> > > On Thu, Nov 22, 2018 at 04:13:38PM +0100, Stefano Garzarella wrote:
> > > > On Thu, Nov 22, 2018 at 12:51 PM Gerd Hoffmann  
> > > > wrote:
> > > > > On Thu, Nov 22, 2018 at 12:08:55PM +0100, Stefano Garzarella wrote:
> > > > > > Hi,
> > > > > > I continued to investigate how to reduce the boot time with SeaBIOS
> > > > > > and QEMU when it used with linuxboot_dma.bin (-kernel parameter).
> > > > > > I reached ~12ms with a SeaBIOS configuration (attached) where I
> > > > > > disabled debug output, all Hardware support (except SMM & MTRRs) 
> > > > > > and I
> > > > > > applied a small patch to disable VGA setup and console (attached).
> > > > >
> > > > > Is there any difference to "qemu -vga none" ?
> > > >
> > > > Using both (qemu -vga none, and my patch) we are around 10.8 ms.
> > > > Note: using only the patch, Linux is still able to initialize and use 
> > > > the VGA.
> > >
> > > But do you want linux use the vga console if you care about boot times?
> > > I'd expect virtio-console would be fastest in that case.
> >
> > I agree with you, but if we will implement a QEMU fastboot feature in
> > SeaBIOS, we are sure that it works with Linux kernel if someone uses
> > "qemu -kernel" and wants also use a VGA.
>
> It should be possible to use "-device VGA,romfile=" to enable VGA
> without an optionrom.
>
> > > > - QEMU -vga none + SeaBIOS config (CONFIG_DEBUG_LEVEL=0, disable all
> > > > HW support except
> > > > SMM & MTRRs) + Stephen's TPM patch
> > > >  qemu_init_end: 43.675803
> > > >  fw_start: 43.865178 (+0.189375)
> > > >  fw_do_boot: 58.093161 (+14.227983)
> > > >  linux_start_boot: 59.490308 (+1.397147)
> > > >  linux_start_user: 556.782354 (+497.292046)
> > > >
> > > > - QEMU -vga none + SeaBIOS config (CONFIG_DEBUG_LEVEL=0, disable all
> > > > HW support except
> > > > SMM & MTRRs, CONFIG_DISABLE_VGA=y) + Stephen's TPM patch
> > > >  qemu_init_end: 42.387412
> > > >  fw_start: 42.579257 (+0.191845)
> > > >  fw_do_boot: 53.381517 (+10.802260)
> > > >  linux_start_boot: 54.848643 (+1.467126)
> > > >  linux_start_user: 498.517050 (+443.668407)
> > >
> > > Interesting that CONFIG_DISABLE_VGA=y makes a noticable difference even
> > > without vga hardware being preset.  And not only in seabios but also for
> > > the linux kernel.
> > >
> > > Do you know why?
> >
> > In SeaBIOS I think the reasons are:
> > - vgarom_setup() scan all PCI devices to find a VGA
> > - enable_vga_console() invokes an int10 without check if there is a
> > VGA (maybe here we can add a check)
>
> Where is CONFIG_DISABLE_VGA set - it's not a standard SeaBIOS config
> option?

Yes, it is not standard.
I posted a small patch in this thread
(https://mail.coreboot.org/pipermail/seabios/2018-November/012592.html)
to explain my experiments.
CONFIG_DISABLE_VGA=y simply skips the vgarom_setup() and enable_vga_console().

>
> In my experience, the only meaningful delays are the result of
> accessing hardware.  Neither of the above two items would result in
> additional hardware accesses, so I don't think they would be the cause
> of an additional delay.

Thanks, I'll investigate better the delays.

Cheers,
Stefano

>
> -Kevin



-- 
Stefano Garzarella
Red Hat

Re: [Qemu-devel] [RFC v2 00/28] vSMMUv3/pSMMUv3 2 stage VFIO integration

[Shameerali Kolothum Thodi]

> -Original Message-
> From: Qemu-devel [mailto:qemu-devel-
> bounces+shameerali.kolothum.thodi=huawei@nongnu.org] On Behalf Of
> Eric Auger
> Sent: 21 September 2018 09:18
> To: eric.auger@gmail.com; eric.au...@redhat.com; qemu-
> de...@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org
> Cc: yi.l@intel.com; cd...@kernel.org; m...@redhat.com; jean-
> philippe.bruc...@arm.com; pet...@redhat.com; alex.william...@redhat.com
> Subject: [Qemu-devel] [RFC v2 00/28] vSMMUv3/pSMMUv3 2 stage VFIO
> integration
> 

[...]

> This series can be found at:
> https://github.com/eauger/qemu/tree/v3.0.0_2stage-rfc-v2
> 
> Testing:
> - For testing use my kernel branch
>   https://github.com/eauger/linux/tree/v4.19-rc4-2stage-rfc-v2 [1]
> - Tested on Qualcomm HW with 1/2 assigned e1000e NICs. Hotplug
>   was also tested OK.
> - Known limitation:
>   - currently sending an NH_ASID command instead of NH_VA
> upon guest NH_VA. This may cause important perf downgrade.
> Propagating NH_VA does not work at the moment.

Hi Eric,

I had a  go with this RFC on one of our platform and noted that when the Guest 
boots with ACPI , the ping doesn't work with the assigned ixgbevf NIC though 
the network device gets detected and added to the iommu group. 

It's all fine when I boot the Guest with DT. Have you tested this with ACPI yet 
or
am I missing anything obvious here?

Please let me know.

Thanks,
Shameer
 
> References:
> - [1] [RFC v2 00/20] SMMUv3 Nested Stage Setup
>   (https://lkml.org/lkml/2018/9/18/1087)
>   The User API still is unstable and under discussion.
> 
> History:
> v1 -> v2:
> - Fixed dual assignment (asid now correctly propagated on TLB invalidations)
> - Integrated fault reporting
> 
> Next Steps:
> - Mature the user API with people involved in SVA work (KVM forum may be a
>   good opportunity to meet)
> - Submit the IOMMU cfg notifier changes and some VFIO changes separately,
> to
>   progress independently on the kernel dependency
> 
> Eric Auger (28):
>   hw/arm/smmu-common: Fix the name of the iommu memory regions
>   hw/arm/smmuv3: fix eventq recording and IRQ triggerring
>   update-linux-headers: Import iommu.h
>   linux-headers: Partial header update
>   memory: add IOMMU_ATTR_VFIO_NESTED IOMMU memory region attribute
>   hw/arm/smmuv3: Implement get_attr API to report
> IOMMU_ATTR_VFIO_NESTED
>   hw/vfio/common: Refactor container initialization
>   hw/vfio/common: Force nested if iommu requires it
>   memory: Introduce IOMMUIOLTBNotifier
>   memory: rename memory_region notify_iommu, notify_one
>   memory: Add IOMMUConfigNotifier
>   memory: Add arch_id in IOTLBEntry
>   hw/arm/smmuv3: Store s1ctrptr in translation config data
>   hw/arm/smmuv3: Implement dummy replay
>   hw/arm/smmuv3: Notify on config changes
>   hw/arm/smmuv3: Fill the IOTLBEntry arch_id on NH_VA invalidation
>   hw/vfio/common: Introduce vfio_alloc_guest_iommu helper
>   hw/vfio/common: Introduce vfio_dma_(un)map_ram_section helpers
>   hw/vfio/common: Register specific nested mode notifiers and
> memory_listener
>   hw/vfio/common: Register MAP notifier for MSI binding
>   target/arm/kvm: Notifies IOMMU on MSI stage 1 binding
>   vfio/pci: Always set up MSI route before enabling vectors
>   hw/arm/smmuv3: Remove warning about unsupported MAP notifiers
>   memory: Introduce IOMMU_NOTIFIER_INIT_CFG IOMMU Config Notifier
>   memory: Introduce IOMMU Memory Region inject_faults API
>   hw/vfio/common: Handle fault_handler
>   hw/arm/smmuv3: Init fault handling
>   hw/arm/smmuv3: Implement fault injection
> 
>  exec.c  |  12 +-
>  hw/arm/smmu-common.c|  12 +-
>  hw/arm/smmuv3-internal.h|  26 +-
>  hw/arm/smmuv3.c | 189 +++--
>  hw/i386/intel_iommu.c   |  16 +-
>  hw/misc/tz-mpc.c|   8 +-
>  hw/ppc/spapr_iommu.c|   2 +-
>  hw/s390x/s390-pci-inst.c|   4 +-
>  hw/vfio/common.c| 666 
>  hw/vfio/pci.c   |   1 +
>  hw/vfio/trace-events|   4 +-
>  hw/virtio/vhost.c   |  12 +-
>  include/exec/memory.h   | 140 +--
>  include/hw/arm/smmu-common.h|   1 +
>  include/hw/vfio/vfio-common.h   |   1 +
>  linux-headers/linux/iommu.h | 237 
>  linux-headers/linux/vfio.h  |  48 +++
>  memory.c|  66 +++-
>  scripts/update-linux-headers.sh |   2 +-
>  target/arm/kvm.c|  46 +--
>  20 files changed, 1206 insertions(+), 287 deletions(-)
>  create mode 100644 linux-headers/linux/iommu.h
> 
> --
> 2.17.1
> 

Re: [Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Michael S. Tsirkin]

On Thu, Nov 22, 2018 at 11:29:32AM +0100, Igor Mammedov wrote:
>  * arm/virt is broken but it looks like linux doesn't care, lets fix it 
> anyways
>  * x86, got lucky since we didn't use extended fields,
>fix it so that it will calculate checksum using correct length
>so that it would be easier to unify arm/x86 into one impl.
> 

I'd say patch 2 should go into this release.
What about patch 1? It's cosmetic and it isn't the best we can do.

Let me know whether you agree.

> Igor Mammedov (2):
>   pc: acpi: use correct RSDT length for checksum
>   arm/virt: acpi: fix incorrect checksums in RSDP
> 
>  hw/arm/virt-acpi-build.c | 7 ++-
>  hw/i386/acpi-build.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 1/2] pc: acpi: use correct RSDT length for checksum

[Michael S. Tsirkin]

On Thu, Nov 22, 2018 at 11:29:33AM +0100, Igor Mammedov wrote:
> AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
> for checksum calculation isn't correct since we are adding extra 16 bytes.
> But acpi_data_push() zeroes out table, so just by luck we are summing up
> exta zeros which still yelds correct checksum.
> 
> Fix it up by explicitly stating table size instead of using
> pointer arithmetics on stucture.
> 
> PS:
> Extra 16 bytes are still wasted, but droping them will break migration
> for machines older than 2.3 due to size mismatch, for 2.3 and older it's
> not an issue since they are using resizable memory regions (a1666142d)
> for ACPI blobs. So keep wasting memory to avoid breaking old machines.

I'd like this explanation in code comments please.

> Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
> Signed-off-by: Igor Mammedov 
> ---
> there is no changes to the current RSDP content caused by this patch
> ---
>  hw/i386/acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 236a20e..131c565 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2567,7 +2567,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned rsdt_tbl_offset)
>  
>  /* Checksum to be filled by Guest linker */
>  bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -(char *)rsdp - rsdp_table->data, sizeof *rsdp,
> +(char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
>  (char *)>checksum - rsdp_table->data);
>  
>  return rsdp_table;

I dislike hard-coded size math like this.
How about a sub-structure for RSDPv1?


> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v5 3/9] block: add empty account cookie type

[Vladimir Sementsov-Ogievskiy]

31.10.2018 14:34, Anton Nefedov wrote:
> This adds some protection from accounting unitialized cookie.

uninitialized

> That is, block_acct_failed/done without previous block_acct_start;
> in that case, cookie probably holds values from previous operation.
> 
> (Note: it might also be unitialized holding garbage value and there is

and here.

>   still "< BLOCK_MAX_IOTYPE" assertion for that.
>   So block_acct_failed/done without previous block_acct_start should be used
>   with caution.)
> 
> Currently this is particularly useful in ide code where it's hard to
> keep track whether the request started accounting or not. For example,
> trim requests do the accounting separately.

so, is it an existing bug? Could you please give an example in the code of such 
wrong (before the patch) accounting?

> 
> Signed-off-by: Anton Nefedov 
> ---
>   include/block/accounting.h | 1 +
>   block/accounting.c | 6 ++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index ba8b04d572..878b4c3581 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>   typedef struct BlockAcctStats BlockAcctStats;
>   
>   enum BlockAcctType {
> +BLOCK_ACCT_NONE = 0,
>   BLOCK_ACCT_READ,
>   BLOCK_ACCT_WRITE,
>   BLOCK_ACCT_FLUSH,
> diff --git a/block/accounting.c b/block/accounting.c
> index 70a3d9a426..8d41c8a83a 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, 
> BlockAcctCookie *cookie,
>   
>   assert(cookie->type < BLOCK_MAX_IOTYPE);
>   
> +if (cookie->type == BLOCK_ACCT_NONE) {
> +return;
> +}
> +
>   qemu_mutex_lock(>lock);
>   
>   if (failed) {
> @@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, 
> BlockAcctCookie *cookie,
>   }
>   
>   qemu_mutex_unlock(>lock);
> +
> +cookie->type = BLOCK_ACCT_NONE;
>   }
>   
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Michael S. Tsirkin]

On Thu, Nov 22, 2018 at 01:35:05PM +, Peter Maydell wrote:
> The API of cpu_physical_memory_write_rom() is odd, because it
> takes an AddressSpace, unlike all the other cpu_physical_memory_*
> access functions. We note this oddity as a TODO in the
> docs/devel/loads-stores.rst documentation.
> 
> Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
> and give it an API that matches address_space_write().
> We also adjest the cpu_physical_memory_write_rom_internal()
> function which is local to exec.c similarly.
> 
> thanks
> -- PMM


Acked-by: Michael S. Tsirkin 

> Peter Maydell (2):
>   exec.c: Rename cpu_physical_memory_write_rom_internal()
>   Rename cpu_physical_memory_write_rom() to address_space_write_rom()
> 
>  include/exec/cpu-common.h   |  2 --
>  include/exec/memory.h   | 26 ++
>  exec.c  | 30 +++---
>  hw/core/loader.c|  4 ++--
>  hw/intc/apic.c  |  7 ---
>  hw/misc/tz-mpc.c|  2 +-
>  hw/sparc/sun4m.c|  5 +++--
>  docs/devel/loads-stores.rst | 35 ---
>  8 files changed, 71 insertions(+), 40 deletions(-)
> 
> -- 
> 2.19.1

Re: [Qemu-devel] SeaBIOS booting time optimization

[Kevin O'Connor]

On Fri, Nov 23, 2018 at 12:18:13PM +0100, Stefano Garzarella wrote:
> On Fri, Nov 23, 2018 at 7:21 AM Gerd Hoffmann  wrote:
> > On Thu, Nov 22, 2018 at 04:13:38PM +0100, Stefano Garzarella wrote:
> > > On Thu, Nov 22, 2018 at 12:51 PM Gerd Hoffmann  wrote:
> > > > On Thu, Nov 22, 2018 at 12:08:55PM +0100, Stefano Garzarella wrote:
> > > > > Hi,
> > > > > I continued to investigate how to reduce the boot time with SeaBIOS
> > > > > and QEMU when it used with linuxboot_dma.bin (-kernel parameter).
> > > > > I reached ~12ms with a SeaBIOS configuration (attached) where I
> > > > > disabled debug output, all Hardware support (except SMM & MTRRs) and I
> > > > > applied a small patch to disable VGA setup and console (attached).
> > > >
> > > > Is there any difference to "qemu -vga none" ?
> > >
> > > Using both (qemu -vga none, and my patch) we are around 10.8 ms.
> > > Note: using only the patch, Linux is still able to initialize and use the 
> > > VGA.
> >
> > But do you want linux use the vga console if you care about boot times?
> > I'd expect virtio-console would be fastest in that case.
> 
> I agree with you, but if we will implement a QEMU fastboot feature in
> SeaBIOS, we are sure that it works with Linux kernel if someone uses
> "qemu -kernel" and wants also use a VGA.

It should be possible to use "-device VGA,romfile=" to enable VGA
without an optionrom.

> > > - QEMU -vga none + SeaBIOS config (CONFIG_DEBUG_LEVEL=0, disable all
> > > HW support except
> > > SMM & MTRRs) + Stephen's TPM patch
> > >  qemu_init_end: 43.675803
> > >  fw_start: 43.865178 (+0.189375)
> > >  fw_do_boot: 58.093161 (+14.227983)
> > >  linux_start_boot: 59.490308 (+1.397147)
> > >  linux_start_user: 556.782354 (+497.292046)
> > >
> > > - QEMU -vga none + SeaBIOS config (CONFIG_DEBUG_LEVEL=0, disable all
> > > HW support except
> > > SMM & MTRRs, CONFIG_DISABLE_VGA=y) + Stephen's TPM patch
> > >  qemu_init_end: 42.387412
> > >  fw_start: 42.579257 (+0.191845)
> > >  fw_do_boot: 53.381517 (+10.802260)
> > >  linux_start_boot: 54.848643 (+1.467126)
> > >  linux_start_user: 498.517050 (+443.668407)
> >
> > Interesting that CONFIG_DISABLE_VGA=y makes a noticable difference even
> > without vga hardware being preset.  And not only in seabios but also for
> > the linux kernel.
> >
> > Do you know why?
> 
> In SeaBIOS I think the reasons are:
> - vgarom_setup() scan all PCI devices to find a VGA
> - enable_vga_console() invokes an int10 without check if there is a
> VGA (maybe here we can add a check)

Where is CONFIG_DISABLE_VGA set - it's not a standard SeaBIOS config
option?

In my experience, the only meaningful delays are the result of
accessing hardware.  Neither of the above two items would result in
additional hardware accesses, so I don't think they would be the cause
of an additional delay.

-Kevin

Re: [Qemu-devel] [PATCH v2 17/21] timer/grlib_gptimer: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> grlib_gptimer_class_init().
> 
> Cc: chout...@adacore.com
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/timer/grlib_gptimer.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
> index 4ed96e970a..183eddc073 100644
> --- a/hw/timer/grlib_gptimer.c
> +++ b/hw/timer/grlib_gptimer.c
> @@ -347,10 +347,11 @@ static void grlib_gptimer_reset(DeviceState *d)
>  }
>  }
>  
> -static int grlib_gptimer_init(SysBusDevice *dev)
> +static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
>  {
>  GPTimerUnit  *unit = GRLIB_GPTIMER(dev);
>  unsigned int  i;
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  assert(unit->nr_timers > 0);
>  assert(unit->nr_timers <= GPTIMER_MAX_TIMERS);
> @@ -366,7 +367,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
>  timer->id = i;
>  
>  /* One IRQ line for each timer */
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_irq(sbd, >irq);
>  
>  ptimer_set_freq(timer->ptimer, unit->freq_hz);
>  }
> @@ -375,8 +376,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
>unit, "gptimer",
>UNIT_REG_SIZE + GPTIMER_REG_SIZE * 
> unit->nr_timers);
>  
> -sysbus_init_mmio(dev, >iomem);
> -return 0;
> +sysbus_init_mmio(sbd, >iomem);
>  }
>  
>  static Property grlib_gptimer_properties[] = {
> @@ -389,9 +389,8 @@ static Property grlib_gptimer_properties[] = {
>  static void grlib_gptimer_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = grlib_gptimer_init;
> +dc->realize = grlib_gptimer_realize;
>  dc->reset = grlib_gptimer_reset;
>  dc->props = grlib_gptimer_properties;
>  }
> 

Re: [Qemu-devel] [PATCH v2 05/21] display/g364fb: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> g364fb_sysbus_class_init().
> 
> Cc: pbonz...@redhat.com
> Cc: kra...@redhat.com
> Cc: f4...@amsat.org
> Cc: alistair.fran...@wdc.com
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 
> Reviewed-by: Alistair Francis 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/display/g364fb.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 8ad7e5d824..3407adf98d 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -489,18 +489,16 @@ typedef struct {
>  G364State g364;
>  } G364SysBusState;
>  
> -static int g364fb_sysbus_init(SysBusDevice *sbd)
> +static void g364fb_sysbus_realize(DeviceState *dev, Error **errp)
>  {
> -DeviceState *dev = DEVICE(sbd);
>  G364SysBusState *sbs = G364(dev);
>  G364State *s = >g364;
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  g364fb_init(dev, s);
>  sysbus_init_irq(sbd, >irq);
>  sysbus_init_mmio(sbd, >mem_ctrl);
>  sysbus_init_mmio(sbd, >mem_vram);
> -
> -return 0;
>  }
>  
>  static void g364fb_sysbus_reset(DeviceState *d)
> @@ -518,9 +516,8 @@ static Property g364fb_sysbus_properties[] = {
>  static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = g364fb_sysbus_init;
> +dc->realize = g364fb_sysbus_realize;
>  set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>  dc->desc = "G364 framebuffer";
>  dc->reset = g364fb_sysbus_reset;
> 

Re: [Qemu-devel] [PATCH v2 12/21] milkymist-pfpu: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> milkymist_pfpu_class_init().
> 
> Cc: mich...@walle.cc
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/misc/milkymist-pfpu.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
> index 86f5e383b0..4a03c7ee63 100644
> --- a/hw/misc/milkymist-pfpu.c
> +++ b/hw/misc/milkymist-pfpu.c
> @@ -497,17 +497,16 @@ static void milkymist_pfpu_reset(DeviceState *d)
>  }
>  }
>  
> -static int milkymist_pfpu_init(SysBusDevice *dev)
> +static void milkymist_pfpu_realize(DeviceState *dev, Error **errp)
>  {
>  MilkymistPFPUState *s = MILKYMIST_PFPU(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_irq(sbd, >irq);
>  
>  memory_region_init_io(>regs_region, OBJECT(dev), _mmio_ops, s,
>  "milkymist-pfpu", MICROCODE_END * 4);
> -sysbus_init_mmio(dev, >regs_region);
> -
> -return 0;
> +sysbus_init_mmio(sbd, >regs_region);
>  }
>  
>  static const VMStateDescription vmstate_milkymist_pfpu = {
> @@ -527,9 +526,8 @@ static const VMStateDescription vmstate_milkymist_pfpu = {
>  static void milkymist_pfpu_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = milkymist_pfpu_init;
> +dc->realize = milkymist_pfpu_realize;
>  dc->reset = milkymist_pfpu_reset;
>  dc->vmsd = _milkymist_pfpu;
>  }
> 

Re: [Qemu-devel] [PATCH v2 08/21] milkymist-softusb: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> milkymist_softusb_class_init().
> 
> Cc: mich...@walle.cc
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/input/milkymist-softusb.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/input/milkymist-softusb.c b/hw/input/milkymist-softusb.c
> index ef8f47cd83..8766a17d9e 100644
> --- a/hw/input/milkymist-softusb.c
> +++ b/hw/input/milkymist-softusb.c
> @@ -245,32 +245,31 @@ static void milkymist_softusb_reset(DeviceState *d)
>  s->regs[R_CTRL] = CTRL_RESET;
>  }
>  
> -static int milkymist_softusb_init(SysBusDevice *dev)
> +static void milkymist_softusb_realize(DeviceState *dev, Error **errp)
>  {
>  MilkymistSoftUsbState *s = MILKYMIST_SOFTUSB(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_irq(sbd, >irq);
>  
>  memory_region_init_io(>regs_region, OBJECT(s), _mmio_ops, s,
>"milkymist-softusb", R_MAX * 4);
> -sysbus_init_mmio(dev, >regs_region);
> +sysbus_init_mmio(sbd, >regs_region);
>  
>  /* register pmem and dmem */
>  memory_region_init_ram_nomigrate(>pmem, OBJECT(s), 
> "milkymist-softusb.pmem",
> s->pmem_size, _fatal);
>  vmstate_register_ram_global(>pmem);
>  s->pmem_ptr = memory_region_get_ram_ptr(>pmem);
> -sysbus_init_mmio(dev, >pmem);
> +sysbus_init_mmio(sbd, >pmem);
>  memory_region_init_ram_nomigrate(>dmem, OBJECT(s), 
> "milkymist-softusb.dmem",
> s->dmem_size, _fatal);
>  vmstate_register_ram_global(>dmem);
>  s->dmem_ptr = memory_region_get_ram_ptr(>dmem);
> -sysbus_init_mmio(dev, >dmem);
> +sysbus_init_mmio(sbd, >dmem);
>  
>  hid_init(>hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
>  hid_init(>hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
> -
> -return 0;
>  }
>  
>  static const VMStateDescription vmstate_milkymist_softusb = {
> @@ -296,9 +295,8 @@ static Property milkymist_softusb_properties[] = {
>  static void milkymist_softusb_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = milkymist_softusb_init;
> +dc->realize = milkymist_softusb_realize;
>  dc->reset = milkymist_softusb_reset;
>  dc->vmsd = _milkymist_softusb;
>  dc->props = milkymist_softusb_properties;
> 

Re: [Qemu-devel] [PATCH v2 09/21] input/pl050: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> pl050_class_init().
> 
> Cc: peter.mayd...@linaro.org
> Cc: qemu-...@nongnu.org
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/input/pl050.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/input/pl050.c b/hw/input/pl050.c
> index be9cd57b17..15bffbfcad 100644
> --- a/hw/input/pl050.c
> +++ b/hw/input/pl050.c
> @@ -139,19 +139,19 @@ static const MemoryRegionOps pl050_ops = {
>  .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int pl050_initfn(SysBusDevice *dev)
> +static void pl050_realize(DeviceState *dev, Error **errp)
>  {
>  PL050State *s = PL050(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  memory_region_init_io(>iomem, OBJECT(s), _ops, s, "pl050", 
> 0x1000);
> -sysbus_init_mmio(dev, >iomem);
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_mmio(sbd, >iomem);
> +sysbus_init_irq(sbd, >irq);
>  if (s->is_mouse) {
>  s->dev = ps2_mouse_init(pl050_update, s);
>  } else {
>  s->dev = ps2_kbd_init(pl050_update, s);
>  }
> -return 0;
>  }
>  
>  static void pl050_keyboard_init(Object *obj)
> @@ -183,9 +183,8 @@ static const TypeInfo pl050_mouse_info = {
>  static void pl050_class_init(ObjectClass *oc, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(oc);
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(oc);
>  
> -sdc->init = pl050_initfn;
> +dc->realize = pl050_realize;
>  dc->vmsd = _pl050;
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 07/21] gpio/puv3_gpio: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> puv3_gpio_class_init().
> 
> Cc: g...@mprc.pku.edu.cn
> Cc: peter.mayd...@linaro.org
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/gpio/puv3_gpio.c | 29 ++---
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/gpio/puv3_gpio.c b/hw/gpio/puv3_gpio.c
> index 445afccf9f..33241b8564 100644
> --- a/hw/gpio/puv3_gpio.c
> +++ b/hw/gpio/puv3_gpio.c
> @@ -99,36 +99,35 @@ static const MemoryRegionOps puv3_gpio_ops = {
>  .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int puv3_gpio_init(SysBusDevice *dev)
> +static void puv3_gpio_realize(DeviceState *dev, Error **errp)
>  {
>  PUV3GPIOState *s = PUV3_GPIO(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  s->reg_GPLR = 0;
>  s->reg_GPDR = 0;
>  
>  /* FIXME: these irqs not handled yet */
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW0]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW1]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW2]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW3]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW4]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW5]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW6]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOLOW7]);
> -sysbus_init_irq(dev, >irq[PUV3_IRQS_GPIOHIGH]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW0]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW1]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW2]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW3]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW4]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW5]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW6]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOLOW7]);
> +sysbus_init_irq(sbd, >irq[PUV3_IRQS_GPIOHIGH]);
>  
>  memory_region_init_io(>iomem, OBJECT(s), _gpio_ops, s, 
> "puv3_gpio",
>  PUV3_REGS_OFFSET);
> -sysbus_init_mmio(dev, >iomem);
> -
> -return 0;
> +sysbus_init_mmio(sbd, >iomem);
>  }
>  
>  static void puv3_gpio_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -sdc->init = puv3_gpio_init;
> +dc->realize = puv3_gpio_realize;
>  }
>  
>  static const TypeInfo puv3_gpio_info = {
> 

[Qemu-devel] [PATCH v2 21/21] core/sysbus: remove the SysBusDeviceClass::init path

[Mao Zhongyi]

Currently, all sysbus devices have been converted to realize(),
so remove this path.

Cc: ehabk...@redhat.com
Cc: th...@redhat.com
Cc: pbonz...@redhat.com
Cc: arm...@redhat.com
Cc: peter.mayd...@linaro.org
Cc: richard.hender...@linaro.org
Cc: alistair.fran...@wdc.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/core/sysbus.c| 15 +--
 include/hw/sysbus.h |  3 ---
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 7ac36ad3e7..9f9edbcab9 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -201,18 +201,13 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
ioport, uint32_t size)
 }
 }
 
-/* TODO remove once all sysbus devices have been converted to realize */
+/* The purpose of preserving this empty realize function
+ * is to prevent the parent_realize field of some subclasses
+ * from being set to NULL to break the normal init/realize
+ * of some devices.
+ */
 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;
-}
-if (sbc->init(sd) < 0) {
-error_setg(errp, "Device initialization failed");
-}
 }
 
 DeviceState *sysbus_create_varargs(const char *name,
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 0b59a3b8d6..1aedcf05c9 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice;
 typedef struct SysBusDeviceClass {
 /*< private >*/
 DeviceClass parent_class;
-/*< public >*/
-
-int (*init)(SysBusDevice *dev);
 
 /*
  * Let the sysbus device format its own non-PIO, non-MMIO unit address.
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 10/21] intc/puv3_intc: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> puv3_intc_class_init().
> 
> Cc: g...@mprc.pku.edu.cn
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/intc/puv3_intc.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/puv3_intc.c b/hw/intc/puv3_intc.c
> index ef8488aacc..69ddc8c19a 100644
> --- a/hw/intc/puv3_intc.c
> +++ b/hw/intc/puv3_intc.c
> @@ -101,10 +101,10 @@ static const MemoryRegionOps puv3_intc_ops = {
>  .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int puv3_intc_init(SysBusDevice *sbd)
> +static void puv3_intc_realize(DeviceState *dev, Error **errp)
>  {
> -DeviceState *dev = DEVICE(sbd);
>  PUV3INTCState *s = PUV3_INTC(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  qdev_init_gpio_in(dev, puv3_intc_handler, PUV3_IRQS_NR);
>  sysbus_init_irq(sbd, >parent_irq);
> @@ -115,15 +115,12 @@ static int puv3_intc_init(SysBusDevice *sbd)
>  memory_region_init_io(>iomem, OBJECT(s), _intc_ops, s, 
> "puv3_intc",
>PUV3_REGS_OFFSET);
>  sysbus_init_mmio(sbd, >iomem);
> -
> -return 0;
>  }
>  
>  static void puv3_intc_class_init(ObjectClass *klass, void *data)
>  {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> -
> -sdc->init = puv3_intc_init;
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +dc->realize = puv3_intc_realize;
>  }
>  
>  static const TypeInfo puv3_intc_info = {
> 

[Qemu-devel] [PATCH v2 18/21] timer/puv3_ost: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
puv3_ost_class_init().

Cc: g...@mprc.pku.edu.cn

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/timer/puv3_ost.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/timer/puv3_ost.c b/hw/timer/puv3_ost.c
index 0b3d717e60..3be58c7fdd 100644
--- a/hw/timer/puv3_ost.c
+++ b/hw/timer/puv3_ost.c
@@ -113,16 +113,17 @@ static void puv3_ost_tick(void *opaque)
 }
 }
 
-static int puv3_ost_init(SysBusDevice *dev)
+static void puv3_ost_realize(DeviceState *dev, Error **errp)
 {
 PUV3OSTState *s = PUV3_OST(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 s->reg_OIER = 0;
 s->reg_OSSR = 0;
 s->reg_OSMR0 = 0;
 s->reg_OSCR = 0;
 
-sysbus_init_irq(dev, >irq);
+sysbus_init_irq(sbd, >irq);
 
 s->bh = qemu_bh_new(puv3_ost_tick, s);
 s->ptimer = ptimer_init(s->bh, PTIMER_POLICY_DEFAULT);
@@ -130,16 +131,14 @@ static int puv3_ost_init(SysBusDevice *dev)
 
 memory_region_init_io(>iomem, OBJECT(s), _ost_ops, s, "puv3_ost",
 PUV3_REGS_OFFSET);
-sysbus_init_mmio(dev, >iomem);
-
-return 0;
+sysbus_init_mmio(sbd, >iomem);
 }
 
 static void puv3_ost_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = puv3_ost_init;
+dc->realize = puv3_ost_realize;
 }
 
 static const TypeInfo puv3_ost_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 20/21] xen_backend: remove xen_sysdev_init() function

[Mao Zhongyi]

The init function doesn't do anything at all, so we
just omit it.

Cc: sstabell...@kernel.org
Cc: anthony.per...@citrix.com
Cc: xen-de...@lists.xenproject.org
Cc: peter.mayd...@linaro.org

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/xen/xen_backend.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 9a8e8771ec..0bc6b1de60 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -809,11 +809,6 @@ static const TypeInfo xensysbus_info = {
 }
 };
 
-static int xen_sysdev_init(SysBusDevice *dev)
-{
-return 0;
-}
-
 static Property xen_sysdev_properties[] = {
 {/* end of property list */},
 };
@@ -821,9 +816,7 @@ static Property xen_sysdev_properties[] = {
 static void xen_sysdev_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = xen_sysdev_init;
 dc->props = xen_sysdev_properties;
 dc->bus_type = TYPE_XENSYSBUS;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 15/21] pci-bridge/dec: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
pci_dec_21154_device_class_init().

Cc: da...@gibson.dropbear.id.au
Cc: m...@redhat.com
Cc: marcel.apfelb...@gmail.com
Cc: qemu-...@nongnu.org

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: David Gibson 
Acked-by: David Gibson 
---
 hw/pci-bridge/dec.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index 84492d5e5f..5b21c20e50 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -98,7 +98,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
 return pci_bridge_get_sec_bus(br);
 }
 
-static int pci_dec_21154_device_init(SysBusDevice *dev)
+static void pci_dec_21154_device_realize(DeviceState *dev, Error **errp)
 {
 PCIHostState *phb;
 
@@ -108,9 +108,8 @@ static int pci_dec_21154_device_init(SysBusDevice *dev)
   dev, "pci-conf-idx", 0x1000);
 memory_region_init_io(>data_mem, OBJECT(dev), _host_data_le_ops,
   dev, "pci-data-idx", 0x1000);
-sysbus_init_mmio(dev, >conf_mem);
-sysbus_init_mmio(dev, >data_mem);
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >conf_mem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >data_mem);
 }
 
 static void dec_21154_pci_host_realize(PCIDevice *d, Error **errp)
@@ -150,9 +149,9 @@ static const TypeInfo dec_21154_pci_host_info = {
 
 static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = pci_dec_21154_device_init;
+dc->realize = pci_dec_21154_device_realize;
 }
 
 static const TypeInfo pci_dec_21154_device_info = {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 03/21] char/grlib_apbuart: Convert sysbus init function to realize function

[Philippe Mathieu-Daudé]

On 23/11/18 16:30, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> grlib_apbuart_class_init().
> 
> Cc: chout...@adacore.com
> Cc: marcandre.lur...@redhat.com
> Cc: pbonz...@redhat.com
> 
> Signed-off-by: Mao Zhongyi 
> Signed-off-by: Zhang Shengju 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/char/grlib_apbuart.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
> index bac11bec58..e1d258b611 100644
> --- a/hw/char/grlib_apbuart.c
> +++ b/hw/char/grlib_apbuart.c
> @@ -239,9 +239,10 @@ static const MemoryRegionOps grlib_apbuart_ops = {
>  .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int grlib_apbuart_init(SysBusDevice *dev)
> +static void grlib_apbuart_realize(DeviceState *dev, Error **errp)
>  {
>  UART *uart = GRLIB_APB_UART(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>  qemu_chr_fe_set_handlers(>chr,
>   grlib_apbuart_can_receive,
> @@ -249,14 +250,12 @@ static int grlib_apbuart_init(SysBusDevice *dev)
>   grlib_apbuart_event,
>   NULL, uart, NULL, true);
>  
> -sysbus_init_irq(dev, >irq);
> +sysbus_init_irq(sbd, >irq);
>  
>  memory_region_init_io(>iomem, OBJECT(uart), _apbuart_ops, 
> uart,
>"uart", UART_REG_SIZE);
>  
> -sysbus_init_mmio(dev, >iomem);
> -
> -return 0;
> +sysbus_init_mmio(sbd, >iomem);
>  }
>  
>  static void grlib_apbuart_reset(DeviceState *d)
> @@ -280,9 +279,8 @@ static Property grlib_apbuart_properties[] = {
>  static void grlib_apbuart_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -k->init = grlib_apbuart_init;
> +dc->realize = grlib_apbuart_realize;
>  dc->reset = grlib_apbuart_reset;
>  dc->props = grlib_apbuart_properties;
>  }
> 

[Qemu-devel] [PATCH v2 16/21] timer/etraxfs_timer: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
etraxfs_timer_class_init().

Cc: edgar.igles...@gmail.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/timer/etraxfs_timer.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index d13bc30b2d..2280914b1d 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -315,9 +315,10 @@ static void etraxfs_timer_reset(void *opaque)
 qemu_irq_lower(t->irq);
 }
 
-static int etraxfs_timer_init(SysBusDevice *dev)
+static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
 {
 ETRAXTimerState *t = ETRAX_TIMER(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 t->bh_t0 = qemu_bh_new(timer0_hit, t);
 t->bh_t1 = qemu_bh_new(timer1_hit, t);
@@ -326,21 +327,20 @@ static int etraxfs_timer_init(SysBusDevice *dev)
 t->ptimer_t1 = ptimer_init(t->bh_t1, PTIMER_POLICY_DEFAULT);
 t->ptimer_wd = ptimer_init(t->bh_wd, PTIMER_POLICY_DEFAULT);
 
-sysbus_init_irq(dev, >irq);
-sysbus_init_irq(dev, >nmi);
+sysbus_init_irq(sbd, >irq);
+sysbus_init_irq(sbd, >nmi);
 
 memory_region_init_io(>mmio, OBJECT(t), _ops, t,
   "etraxfs-timer", 0x5c);
-sysbus_init_mmio(dev, >mmio);
+sysbus_init_mmio(sbd, >mmio);
 qemu_register_reset(etraxfs_timer_reset, t);
-return 0;
 }
 
 static void etraxfs_timer_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = etraxfs_timer_init;
+dc->realize = etraxfs_timer_realize;
 }
 
 static const TypeInfo etraxfs_timer_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 02/21] block/noenand: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
onenand_class_init().

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: qemu-bl...@nongnu.org

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/block/onenand.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 2b48609776..f8a687 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -772,9 +772,9 @@ static const MemoryRegionOps onenand_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int onenand_initfn(SysBusDevice *sbd)
+static void onenand_realize(DeviceState *dev, Error **errp)
 {
-DeviceState *dev = DEVICE(sbd);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 OneNANDState *s = ONE_NAND(dev);
 uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
 void *ram;
@@ -794,14 +794,14 @@ static int onenand_initfn(SysBusDevice *sbd)
   0xff, size + (size >> 5));
 } else {
 if (blk_is_read_only(s->blk)) {
-error_report("Can't use a read-only drive");
-return -1;
+error_setg(errp, "Can't use a read-only drive");
+return;
 }
 blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, _err);
 if (local_err) {
-error_report_err(local_err);
-return -1;
+error_propagate(errp, local_err);
+return;
 }
 s->blk_cur = s->blk;
 }
@@ -826,7 +826,6 @@ static int onenand_initfn(SysBusDevice *sbd)
  | ((s->id.dev & 0xff) << 8)
  | (s->id.ver & 0xff),
  _onenand, s);
-return 0;
 }
 
 static Property onenand_properties[] = {
@@ -841,9 +840,8 @@ static Property onenand_properties[] = {
 static void onenand_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = onenand_initfn;
+dc->realize = onenand_realize;
 dc->reset = onenand_system_reset;
 dc->props = onenand_properties;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 10/21] intc/puv3_intc: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
puv3_intc_class_init().

Cc: g...@mprc.pku.edu.cn

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/intc/puv3_intc.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/intc/puv3_intc.c b/hw/intc/puv3_intc.c
index ef8488aacc..69ddc8c19a 100644
--- a/hw/intc/puv3_intc.c
+++ b/hw/intc/puv3_intc.c
@@ -101,10 +101,10 @@ static const MemoryRegionOps puv3_intc_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int puv3_intc_init(SysBusDevice *sbd)
+static void puv3_intc_realize(DeviceState *dev, Error **errp)
 {
-DeviceState *dev = DEVICE(sbd);
 PUV3INTCState *s = PUV3_INTC(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 qdev_init_gpio_in(dev, puv3_intc_handler, PUV3_IRQS_NR);
 sysbus_init_irq(sbd, >parent_irq);
@@ -115,15 +115,12 @@ static int puv3_intc_init(SysBusDevice *sbd)
 memory_region_init_io(>iomem, OBJECT(s), _intc_ops, s, "puv3_intc",
   PUV3_REGS_OFFSET);
 sysbus_init_mmio(sbd, >iomem);
-
-return 0;
 }
 
 static void puv3_intc_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
-
-sdc->init = puv3_intc_init;
+DeviceClass *dc = DEVICE_CLASS(klass);
+dc->realize = puv3_intc_realize;
 }
 
 static const TypeInfo puv3_intc_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 05/21] display/g364fb: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
g364fb_sysbus_class_init().

Cc: pbonz...@redhat.com
Cc: kra...@redhat.com
Cc: f4...@amsat.org
Cc: alistair.fran...@wdc.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Alistair Francis 
---
 hw/display/g364fb.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 8ad7e5d824..3407adf98d 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -489,18 +489,16 @@ typedef struct {
 G364State g364;
 } G364SysBusState;
 
-static int g364fb_sysbus_init(SysBusDevice *sbd)
+static void g364fb_sysbus_realize(DeviceState *dev, Error **errp)
 {
-DeviceState *dev = DEVICE(sbd);
 G364SysBusState *sbs = G364(dev);
 G364State *s = >g364;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 g364fb_init(dev, s);
 sysbus_init_irq(sbd, >irq);
 sysbus_init_mmio(sbd, >mem_ctrl);
 sysbus_init_mmio(sbd, >mem_vram);
-
-return 0;
 }
 
 static void g364fb_sysbus_reset(DeviceState *d)
@@ -518,9 +516,8 @@ static Property g364fb_sysbus_properties[] = {
 static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = g364fb_sysbus_init;
+dc->realize = g364fb_sysbus_realize;
 set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 dc->desc = "G364 framebuffer";
 dc->reset = g364fb_sysbus_reset;
-- 
2.17.1

[Qemu-devel] [PATCH v2 00/21] QOM'ify SysBusDeviceClass->init

[Mao Zhongyi]

The SysBusDeviceClass::init() interface is considered
as a legacy interface and there are currently some
efforts going on to get rid of it. Thus convert 
SysBusDeviceClass::init to DeviceClass::realize.

v2 -> v1:

- SYS_BUS_DEVICE(dev) was used in a function several
  times, so use a variable 'sbd' to replace it, like:
  SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
- remove the xen_sysdev_init() function
- drop the patch21 in v1
- fix the broken in sysbus_realize of patch22


Cc: alistair.fran...@wdc.com
Cc: anthony.per...@citrix.com
Cc: arm...@redhat.com
Cc: borntrae...@de.ibm.com
Cc: chout...@adacore.com
Cc: coh...@redhat.com
Cc: da...@gibson.dropbear.id.au
Cc: da...@redhat.com
Cc: edgar.igles...@gmail.com
Cc: ehabk...@redhat.com
Cc: f4...@amsat.org
Cc: g...@mprc.pku.edu.cn
Cc: jan.kis...@web.de
Cc: kra...@redhat.com
Cc: kw...@redhat.com
Cc: marcandre.lur...@redhat.com
Cc: marcel.apfelb...@gmail.com
Cc: mich...@walle.cc
Cc: mre...@redhat.com
Cc: m...@redhat.com
Cc: pbonz...@redhat.com
Cc: peter.mayd...@linaro.org
Cc: peter.mayd...@linaro.org

  
Cc: qemu-...@nongnu.org
Cc: qemu-bl...@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-s3...@nongnu.org
Cc: richard.hender...@linaro.org
Cc: r...@twiddle.net
Cc: sstabell...@kernel.org
Cc: th...@redhat.com
Cc: xen-de...@lists.xenproject.org

Mao Zhongyi (21):
  musicpal: Convert sysbus init function to realize function
  block/noenand: Convert sysbus init function to realize function
  char/grlib_apbuart: Convert sysbus init function to realize function
  core/empty_slot: Convert sysbus init function to realize function
  display/g364fb: Convert sysbus init function to realize function
  dma/puv3_dma: Convert sysbus init function to realize function
  gpio/puv3_gpio: Convert sysbus init function to realize function
  milkymist-softusb: Convert sysbus init function to realize function
  input/pl050: Convert sysbus init function to realize function
  intc/puv3_intc: Convert sysbus init function to realize function
  milkymist-hpdmc: Convert sysbus init function to realize function
  milkymist-pfpu: Convert sysbus init function to realize function
  puv3_pm.c: Convert sysbus init function to realize function
  nvram/ds1225y: Convert sysbus init function to realize function
  pci-bridge/dec: Convert sysbus init function to realize function
  timer/etraxfs_timer: Convert sysbus init function to realize function
  timer/grlib_gptimer: Convert sysbus init function to realize function
  timer/puv3_ost: Convert sysbus init function to realize function
  usb/tusb6010: Convert sysbus init function to realize function
  xen_backend: remove xen_sysdev_init() function
  core/sysbus: remove the SysBusDeviceClass::init path

 hw/arm/musicpal.c|  9 -
 hw/block/onenand.c   | 16 +++-
 hw/char/grlib_apbuart.c  | 12 +---
 hw/core/empty_slot.c |  9 -
 hw/core/sysbus.c | 15 +--
 hw/display/g364fb.c  |  9 +++--
 hw/dma/puv3_dma.c| 10 --
 hw/gpio/puv3_gpio.c  | 29 ++---
 hw/input/milkymist-softusb.c | 16 +++-
 hw/input/pl050.c | 11 +--
 hw/intc/puv3_intc.c  | 11 ---
 hw/misc/milkymist-hpdmc.c|  9 +++--
 hw/misc/milkymist-pfpu.c | 12 +---
 hw/misc/puv3_pm.c| 10 --
 hw/nvram/ds1225y.c   | 12 +---
 hw/pci-bridge/dec.c  | 11 +--
 hw/timer/etraxfs_timer.c | 14 +++---
 hw/timer/grlib_gptimer.c | 11 +--
 hw/timer/puv3_ost.c  | 13 ++---
 hw/usb/tusb6010.c|  8 +++-
 hw/xen/xen_backend.c |  7 ---
 include/hw/sysbus.h  |  3 ---
 22 files changed, 105 insertions(+), 152 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 13/21] puv3_pm.c: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
puv3_pm_class_init().

Cc: g...@mprc.pku.edu.cn

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/misc/puv3_pm.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/misc/puv3_pm.c b/hw/misc/puv3_pm.c
index 577cebaac7..afe191fbe1 100644
--- a/hw/misc/puv3_pm.c
+++ b/hw/misc/puv3_pm.c
@@ -119,7 +119,7 @@ static const MemoryRegionOps puv3_pm_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int puv3_pm_init(SysBusDevice *dev)
+static void puv3_pm_realize(DeviceState *dev, Error **errp)
 {
 PUV3PMState *s = PUV3_PM(dev);
 
@@ -127,16 +127,14 @@ static int puv3_pm_init(SysBusDevice *dev)
 
 memory_region_init_io(>iomem, OBJECT(s), _pm_ops, s, "puv3_pm",
 PUV3_REGS_OFFSET);
-sysbus_init_mmio(dev, >iomem);
-
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
 }
 
 static void puv3_pm_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = puv3_pm_init;
+dc->realize = puv3_pm_realize;
 }
 
 static const TypeInfo puv3_pm_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 14/21] nvram/ds1225y: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
nvram_sysbus_class_init().

Cc: pbonz...@redhat.com
Cc: marcandre.lur...@redhat.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nvram/ds1225y.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/ds1225y.c b/hw/nvram/ds1225y.c
index ad7345f288..b6ef463db0 100644
--- a/hw/nvram/ds1225y.c
+++ b/hw/nvram/ds1225y.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "trace.h"
+#include "qemu/error-report.h"
 
 typedef struct {
 MemoryRegion iomem;
@@ -113,7 +114,7 @@ typedef struct {
 NvRamState nvram;
 } SysBusNvRamState;
 
-static int nvram_sysbus_initfn(SysBusDevice *dev)
+static void nvram_sysbus_realize(DeviceState *dev, Error **errp)
 {
 SysBusNvRamState *sys = DS1225Y(dev);
 NvRamState *s = >nvram;
@@ -123,20 +124,18 @@ static int nvram_sysbus_initfn(SysBusDevice *dev)
 
 memory_region_init_io(>iomem, OBJECT(s), _ops, s,
   "nvram", s->chip_size);
-sysbus_init_mmio(dev, >iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
 
 /* Read current file */
 file = s->filename ? fopen(s->filename, "rb") : NULL;
 if (file) {
 /* Read nvram contents */
 if (fread(s->contents, s->chip_size, 1, file) != 1) {
-printf("nvram_sysbus_initfn: short read\n");
+error_report("nvram_sysbus_realize: short read");
 }
 fclose(file);
 }
 nvram_post_load(s, 0);
-
-return 0;
 }
 
 static Property nvram_sysbus_properties[] = {
@@ -148,9 +147,8 @@ static Property nvram_sysbus_properties[] = {
 static void nvram_sysbus_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = nvram_sysbus_initfn;
+dc->realize = nvram_sysbus_realize;
 dc->vmsd = _nvram;
 dc->props = nvram_sysbus_properties;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 17/21] timer/grlib_gptimer: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
grlib_gptimer_class_init().

Cc: chout...@adacore.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/timer/grlib_gptimer.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index 4ed96e970a..183eddc073 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -347,10 +347,11 @@ static void grlib_gptimer_reset(DeviceState *d)
 }
 }
 
-static int grlib_gptimer_init(SysBusDevice *dev)
+static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
 {
 GPTimerUnit  *unit = GRLIB_GPTIMER(dev);
 unsigned int  i;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 assert(unit->nr_timers > 0);
 assert(unit->nr_timers <= GPTIMER_MAX_TIMERS);
@@ -366,7 +367,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
 timer->id = i;
 
 /* One IRQ line for each timer */
-sysbus_init_irq(dev, >irq);
+sysbus_init_irq(sbd, >irq);
 
 ptimer_set_freq(timer->ptimer, unit->freq_hz);
 }
@@ -375,8 +376,7 @@ static int grlib_gptimer_init(SysBusDevice *dev)
   unit, "gptimer",
   UNIT_REG_SIZE + GPTIMER_REG_SIZE * unit->nr_timers);
 
-sysbus_init_mmio(dev, >iomem);
-return 0;
+sysbus_init_mmio(sbd, >iomem);
 }
 
 static Property grlib_gptimer_properties[] = {
@@ -389,9 +389,8 @@ static Property grlib_gptimer_properties[] = {
 static void grlib_gptimer_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = grlib_gptimer_init;
+dc->realize = grlib_gptimer_realize;
 dc->reset = grlib_gptimer_reset;
 dc->props = grlib_gptimer_properties;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 11/21] milkymist-hpdmc: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
milkymist_hpdmc_class_init().

Cc: g...@mprc.pku.edu.cn
Cc: mich...@walle.cc

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/misc/milkymist-hpdmc.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/misc/milkymist-hpdmc.c b/hw/misc/milkymist-hpdmc.c
index e6140eec6b..44dc0698ec 100644
--- a/hw/misc/milkymist-hpdmc.c
+++ b/hw/misc/milkymist-hpdmc.c
@@ -129,15 +129,13 @@ static void milkymist_hpdmc_reset(DeviceState *d)
  | IODELAY_PLL2_LOCKED;
 }
 
-static int milkymist_hpdmc_init(SysBusDevice *dev)
+static void milkymist_hpdmc_realize(DeviceState *dev, Error **errp)
 {
 MilkymistHpdmcState *s = MILKYMIST_HPDMC(dev);
 
 memory_region_init_io(>regs_region, OBJECT(dev), _mmio_ops, s,
 "milkymist-hpdmc", R_MAX * 4);
-sysbus_init_mmio(dev, >regs_region);
-
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >regs_region);
 }
 
 static const VMStateDescription vmstate_milkymist_hpdmc = {
@@ -153,9 +151,8 @@ static const VMStateDescription vmstate_milkymist_hpdmc = {
 static void milkymist_hpdmc_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = milkymist_hpdmc_init;
+dc->realize = milkymist_hpdmc_realize;
 dc->reset = milkymist_hpdmc_reset;
 dc->vmsd = _milkymist_hpdmc;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 19/21] usb/tusb6010: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
tusb6010_class_init().

Cc: kra...@redhat.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/usb/tusb6010.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/usb/tusb6010.c b/hw/usb/tusb6010.c
index a2128024c1..501706e2b2 100644
--- a/hw/usb/tusb6010.c
+++ b/hw/usb/tusb6010.c
@@ -808,10 +808,10 @@ static void tusb6010_reset(DeviceState *dev)
 musb_reset(s->musb);
 }
 
-static int tusb6010_init(SysBusDevice *sbd)
+static void tusb6010_realize(DeviceState *dev, Error **errp)
 {
-DeviceState *dev = DEVICE(sbd);
 TUSBState *s = TUSB(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
 s->otg_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tusb_otg_tick, s);
 s->pwr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tusb_power_tick, s);
@@ -822,15 +822,13 @@ static int tusb6010_init(SysBusDevice *sbd)
 sysbus_init_irq(sbd, >irq);
 qdev_init_gpio_in(dev, tusb6010_irq, musb_irq_max + 1);
 s->musb = musb_init(dev, 1);
-return 0;
 }
 
 static void tusb6010_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = tusb6010_init;
+dc->realize = tusb6010_realize;
 dc->reset = tusb6010_reset;
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 06/21] dma/puv3_dma: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
puv3_dma_class_init().

Cc: g...@mprc.pku.edu.cn

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/dma/puv3_dma.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/dma/puv3_dma.c b/hw/dma/puv3_dma.c
index b97a6c1767..c89eade029 100644
--- a/hw/dma/puv3_dma.c
+++ b/hw/dma/puv3_dma.c
@@ -76,7 +76,7 @@ static const MemoryRegionOps puv3_dma_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int puv3_dma_init(SysBusDevice *dev)
+static void puv3_dma_realize(DeviceState *dev, Error **errp)
 {
 PUV3DMAState *s = PUV3_DMA(dev);
 int i;
@@ -87,16 +87,14 @@ static int puv3_dma_init(SysBusDevice *dev)
 
 memory_region_init_io(>iomem, OBJECT(s), _dma_ops, s, "puv3_dma",
 PUV3_REGS_OFFSET);
-sysbus_init_mmio(dev, >iomem);
-
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
 }
 
 static void puv3_dma_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = puv3_dma_init;
+dc->realize = puv3_dma_realize;
 }
 
 static const TypeInfo puv3_dma_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 12/21] milkymist-pfpu: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
milkymist_pfpu_class_init().

Cc: mich...@walle.cc

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 hw/misc/milkymist-pfpu.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
index 86f5e383b0..4a03c7ee63 100644
--- a/hw/misc/milkymist-pfpu.c
+++ b/hw/misc/milkymist-pfpu.c
@@ -497,17 +497,16 @@ static void milkymist_pfpu_reset(DeviceState *d)
 }
 }
 
-static int milkymist_pfpu_init(SysBusDevice *dev)
+static void milkymist_pfpu_realize(DeviceState *dev, Error **errp)
 {
 MilkymistPFPUState *s = MILKYMIST_PFPU(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-sysbus_init_irq(dev, >irq);
+sysbus_init_irq(sbd, >irq);
 
 memory_region_init_io(>regs_region, OBJECT(dev), _mmio_ops, s,
 "milkymist-pfpu", MICROCODE_END * 4);
-sysbus_init_mmio(dev, >regs_region);
-
-return 0;
+sysbus_init_mmio(sbd, >regs_region);
 }
 
 static const VMStateDescription vmstate_milkymist_pfpu = {
@@ -527,9 +526,8 @@ static const VMStateDescription vmstate_milkymist_pfpu = {
 static void milkymist_pfpu_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-k->init = milkymist_pfpu_init;
+dc->realize = milkymist_pfpu_realize;
 dc->reset = milkymist_pfpu_reset;
 dc->vmsd = _milkymist_pfpu;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 01/21] musicpal: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
mv88w8618_wlan_class_init().

Cc: jan.kis...@web.de
Cc: peter.mayd...@linaro.org
Cc: qemu-...@nongnu.org

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/arm/musicpal.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 9648b3af44..7ffcdbb097 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1147,14 +1147,13 @@ static const MemoryRegionOps mv88w8618_wlan_ops = {
 .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int mv88w8618_wlan_init(SysBusDevice *dev)
+static void mv88w8618_wlan_realize(DeviceState *dev, Error **errp)
 {
 MemoryRegion *iomem = g_new(MemoryRegion, 1);
 
 memory_region_init_io(iomem, OBJECT(dev), _wlan_ops, NULL,
   "musicpal-wlan", MP_WLAN_SIZE);
-sysbus_init_mmio(dev, iomem);
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), iomem);
 }
 
 /* GPIO register offsets */
@@ -1720,9 +1719,9 @@ DEFINE_MACHINE("musicpal", musicpal_machine_init)
 
 static void mv88w8618_wlan_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-sdc->init = mv88w8618_wlan_init;
+dc->realize = mv88w8618_wlan_realize;
 }
 
 static const TypeInfo mv88w8618_wlan_info = {
-- 
2.17.1

[Qemu-devel] [PATCH v2 04/21] core/empty_slot: Convert sysbus init function to realize function

[Mao Zhongyi]

Use DeviceClass rather than SysBusDeviceClass in
empty_slot_class_init().

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/core/empty_slot.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/core/empty_slot.c b/hw/core/empty_slot.c
index c1b9c2b104..239f78e2a7 100644
--- a/hw/core/empty_slot.c
+++ b/hw/core/empty_slot.c
@@ -71,21 +71,20 @@ void empty_slot_init(hwaddr addr, uint64_t slot_size)
 }
 }
 
-static int empty_slot_init1(SysBusDevice *dev)
+static void empty_slot_realize(DeviceState *dev, Error **errp)
 {
 EmptySlot *s = EMPTY_SLOT(dev);
 
 memory_region_init_io(>iomem, OBJECT(s), _slot_ops, s,
   "empty-slot", s->size);
-sysbus_init_mmio(dev, >iomem);
-return 0;
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
 }
 
 static void empty_slot_class_init(ObjectClass *klass, void *data)
 {
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
 
-k->init = empty_slot_init1;
+dc->realize = empty_slot_realize;
 }
 
 static const TypeInfo empty_slot_info = {
-- 
2.17.1