Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Peter Xu]

On Tue, Jan 09, 2018 at 04:53:40PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > We can simplify object_property_get_str() using the new
> > qobject_get_try_str().
> > 
> > Reviewed-by: Fam Zheng 
> > Reviewed-by: Stefan Hajnoczi 
> > Signed-off-by: Peter Xu 
> > ---
> >  qom/object.c | 9 +++--
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Eric Blake 
> 
> I'm not opposed to your patch split (particularly since it makes
> backports easier if it just needs the new function and then your later
> uses of the new function, without touching existing uses); but I might
> have merged this with the previous patch so that the new API has a
> client right away, proving why the new API is worthwhile as part of its
> introduction.

Sure, I'll follow the rule next time.

(I can simply squash it but I got a few r-bs for separate patches
 already, so I'll keep them separated for now)

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH 0/2] tcg/ppc updates

[Richard Henderson]

I found two problems testing my ARM SVE branch on PPC64.


r~


Richard Henderson (2):
  tcg/ppc: Support tlb offsets larger than 64k
  tcg/ppc: Allow a 32-bit offset to the constant pool

 tcg/ppc/tcg-target.inc.c | 84 +++-
 1 file changed, 47 insertions(+), 37 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 2/2] tcg/ppc: Allow a 32-bit offset to the constant pool

[Richard Henderson]

We recently relaxed the limit of the number of opcodes that can
appear in a TranslationBlock.  In certain cases this has resulted
in relocation overflow.

Signed-off-by: Richard Henderson 
---
 tcg/ppc/tcg-target.inc.c | 67 
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 74f9b4aa34..86f7de5f7e 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -222,33 +222,6 @@ static inline void tcg_out_bc_noaddr(TCGContext *s, int 
insn)
 tcg_out32(s, insn | retrans);
 }
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
-intptr_t value, intptr_t addend)
-{
-tcg_insn_unit *target;
-tcg_insn_unit old;
-
-value += addend;
-target = (tcg_insn_unit *)value;
-
-switch (type) {
-case R_PPC_REL14:
-reloc_pc14(code_ptr, target);
-break;
-case R_PPC_REL24:
-reloc_pc24(code_ptr, target);
-break;
-case R_PPC_ADDR16:
-assert(value == (int16_t)value);
-old = *code_ptr;
-old = deposit32(old, 0, 16, value);
-*code_ptr = old;
-break;
-default:
-tcg_abort();
-}
-}
-
 /* parse target specific constraints */
 static const char *target_parse_constraint(TCGArgConstraint *ct,
const char *ct_str, TCGType type)
@@ -552,6 +525,43 @@ static const uint32_t tcg_to_isel[] = {
 [TCG_COND_GTU] = ISEL | BC_(7, CR_GT),
 };
 
+static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+intptr_t value, intptr_t addend)
+{
+tcg_insn_unit *target;
+tcg_insn_unit old;
+
+value += addend;
+target = (tcg_insn_unit *)value;
+
+switch (type) {
+case R_PPC_REL14:
+reloc_pc14(code_ptr, target);
+break;
+case R_PPC_REL24:
+reloc_pc24(code_ptr, target);
+break;
+case R_PPC_ADDR16:
+/* We are abusing this relocation type.  This points to a pair
+   of insns, addis + load.  If the displacement is small, we
+   can nop out the addis.  */
+if (value == (int16_t)value) {
+code_ptr[0] = NOP;
+old = deposit32(code_ptr[1], 0, 16, value);
+code_ptr[1] = deposit32(old, 16, 5, TCG_REG_TB);
+} else {
+int16_t lo = value;
+int hi = value - lo;
+assert(hi + lo == value);
+code_ptr[0] = deposit32(code_ptr[0], 0, 16, hi >> 16);
+code_ptr[1] = deposit32(code_ptr[1], 0, 16, lo);
+}
+break;
+default:
+g_assert_not_reached();
+}
+}
+
 static void tcg_out_mem_long(TCGContext *s, int opi, int opx, TCGReg rt,
  TCGReg base, tcg_target_long offset);
 
@@ -690,7 +700,8 @@ static void tcg_out_movi_int(TCGContext *s, TCGType type, 
TCGReg ret,
 if (!in_prologue && USE_REG_TB) {
 new_pool_label(s, arg, R_PPC_ADDR16, s->code_ptr,
-(intptr_t)s->code_gen_ptr);
-tcg_out32(s, LD | TAI(ret, TCG_REG_TB, 0));
+tcg_out32(s, ADDIS | TAI(ret, TCG_REG_TB, 0));
+tcg_out32(s, LD | TAI(ret, ret, 0));
 return;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH 1/2] tcg/ppc: Support tlb offsets larger than 64k

[Richard Henderson]

AArch64 with SVE has an offset of 80k to the 8th TLB.

Signed-off-by: Richard Henderson 
---
 tcg/ppc/tcg-target.inc.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 879885b68b..74f9b4aa34 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -1524,16 +1524,15 @@ static TCGReg tcg_out_tlb_read(TCGContext *s, TCGMemOp 
opc,
 
 /* Compensate for very large offsets.  */
 if (add_off >= 0x8000) {
-/* Most target env are smaller than 32k; none are larger than 64k.
-   Simplify the logic here merely to offset by 0x7ff0, giving us a
-   range just shy of 64k.  Check this assumption.  */
-QEMU_BUILD_BUG_ON(offsetof(CPUArchState,
-   tlb_table[NB_MMU_MODES - 1][1])
-  > 0x7ff0 + 0x7fff);
-tcg_out32(s, ADDI | TAI(TCG_REG_TMP1, base, 0x7ff0));
+int low = (int16_t)cmp_off;
+int high = cmp_off - low;
+assert((high & 0x) == 0);
+assert(cmp_off - high == (int16_t)(cmp_off - high));
+assert(add_off - high == (int16_t)(add_off - high));
+tcg_out32(s, ADDIS | TAI(TCG_REG_TMP1, base, high >> 16));
 base = TCG_REG_TMP1;
-cmp_off -= 0x7ff0;
-add_off -= 0x7ff0;
+cmp_off -= high;
+add_off -= high;
 }
 
 /* Extraction and shifting, part 2.  */
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 03/27] qobject: introduce qobject_get_try_str()

[Peter Xu]

On Tue, Jan 09, 2018 at 04:50:39PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > A quick way to fetch string from qobject when it's a QString.
> > 
> > Reviewed-by: Fam Zheng 
> > Reviewed-by: Stefan Hajnoczi 
> > Signed-off-by: Peter Xu 
> > ---
> >  include/qapi/qmp/qstring.h |  1 +
> >  qobject/qstring.c  | 11 +++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> > index a145c8ca00..6517d8e377 100644
> > --- a/include/qapi/qmp/qstring.h
> > +++ b/include/qapi/qmp/qstring.h
> > @@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, 
> > int end);
> >  size_t qstring_get_length(const QString *qstring);
> >  const char *qstring_get_str(const QString *qstring);
> >  const char *qstring_get_try_str(const QString *qstring);
> > +const char *qobject_get_try_str(const QObject *qstring);
> 
> The naming is consistent, so I won't reject the patch, but 'try_get_str'
> reads better than 'get_try_str'.  Of course, fixing the code base to
> read well AND be consistent is a much bigger task, and I'm not asking
> you to tackle it.

I agree.

> 
> 
> >  
> > +/**
> > + * qobject_get_try_str(): Return a pointer of the backstore string
> 
> The word "backstore" doesn't appear anywhere in qemu.git, and flags as a
> typo.  I'd prefer:
> 
> Return a pointer to the corresponding string

I'll take this one.  Thanks,

> 
> or maybe "backing string"

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Zhoujian (jay)]

> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Wednesday, January 10, 2018 2:02 PM
> To: Zhoujian (jay) ; qemu-devel@nongnu.org
> Cc: Huangweidong (C) ; m...@redhat.com; wangxin (U)
> ; Gonglei (Arei) ;
> imamm...@redhat.com; Liuzhe (Ahriy, Euler) 
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
> 
> 
> 
> On 2018年01月10日 12:18, Zhoujian (jay) wrote:
> > Sorry about missing to cc Jason.
> >
> >
> > Close the fd of the tap unconditionally when netdev_add
> > tap,id=net0,vhost=on failed in net_init_tap_one() will make the
> > followed up device_add virtio-net-pci,netdev=net0 failed too, which prints:
> >
> >  TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
> >  ioctl() failed: Bad file descriptor
> >
> > This patch makes the followed up device_add be able to fall back to
> > userspace virtio when netdev_add failed with vhost turning on but vhost
> force flag does not set.
> >
> > Here is the original discussion:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html
> >
> >
> > This is a RFC version, if I'm wrong, please let me know, thanks!
> >
> > PS: there're two places updated, see below.
> >
> >
> >> -Original Message-
> >> From: Zhoujian (jay)
> >> Sent: Wednesday, January 10, 2018 12:40 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: m...@redhat.com; imamm...@redhat.com; Huangweidong (C)
> >> ; wangxin (U) ;
> >> Gonglei
> >> (Arei) ; Liuzhe (Ahriy, Euler)
> >> ; Zhoujian (jay) 
> >> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost
> >> failed to initialize
> >>
> >> Making the followed up device_add to fall back to userspace virtio
> >> when netdev_add fails if vhost force flag does not set.
> >>
> >> Suggested-by: Michael S. Tsirkin 
> >> Suggested-by: Igor Mammedov 
> >> Signed-off-by: Jay Zhou 
> >> ---
> >>   net/tap.c | 25 ++---
> >>   1 file changed, 18 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/tap.c b/net/tap.c
> >> index 979e622..03f226f 100644
> >> --- a/net/tap.c
> >> +++ b/net/tap.c
> >> @@ -642,7 +642,8 @@ static void net_init_tap_one(const
> >> NetdevTapOptions *tap, NetClientState *peer,
> >>const char *model, const char *name,
> >>const char *ifname, const char *script,
> >>const char *downscript, const char
> *vhostfdname,
> >> - int vnet_hdr, int fd, Error **errp)
> >> + int vnet_hdr, int fd, Error **errp,
> >> + bool *vhost_init_failed)
> >>   {
> >>   Error *err = NULL;
> >>   TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> >> @@ -
> >> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions
> >> *tap, NetClientState *peer,
> >>   vhostfd = monitor_fd_param(cur_mon, vhostfdname, );
> >>   if (vhostfd == -1) {
> >>   error_propagate(errp, err);
> >> +*vhost_init_failed = true;
> >>   return;
> >>   }
> >>   } else {
> >> @@ -694,6 +696,7 @@ static void net_init_tap_one(const
> >> NetdevTapOptions *tap, NetClientState *peer,
> >>   if (vhostfd < 0) {
> >>   error_setg_errno(errp, errno,
> >>"tap: open vhost char device
> >> failed");
> >> +*vhost_init_failed = true;
> >>   return;
> >>   }
> >>   fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7
> >> @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
> >>   if (!s->vhost_net) {
> >>   error_setg(errp,
> >>  "vhost-net requested but could not be
> >> initialized");
> >> +*vhost_init_failed = true;
> 
> Why not simply check s->vhost_net after call net_init_tap_one()?

s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
failure reasons.

fd should be closed in these two cases:
(1) tap_set_sndbuf() failed, regardless of whether having vhost or vhostforce 
flag
(2) with vhostforce flag

fd should not be closed if tap_set_sndbuf() succeeded and vhost init failed 
without
vhostforce flag.

Regards,
Jay

> 
> Thanks
> 
> >>   return;
> >>   }
> >>   } else if (vhostfdname) {
> >> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char
> *name,
> >>   Error *err = NULL;
> >>   const char *vhostfdname;
> >>   char ifname[128];
> >> +bool vhost_init_failed = 

Re: [Qemu-devel] [PULL 07/11] cpu-exec: don't overwrite exception_index

[Pavel Dovgalyuk]

> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> On 9 January 2018 at 13:21, Pavel Dovgalyuk  wrote:
> > I tried to get some logs with the following code.
> > It prints that there was an exception 5 and it was overwritten by the 
> > standard code.
> > Fixed code prevents this overwrite.
> >
> > I guess that one of the following is true:
> >  - unfixed version misses some exceptions
> >  - fixed version processes some exceptions twice (e.g., when there is no 
> > clear exception)
> >
> > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > index 280200f..fa810f7 100644
> > --- a/accel/tcg/cpu-exec.c
> > +++ b/accel/tcg/cpu-exec.c
> > @@ -605,6 +605,8 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
> >  /* Finally, check if we need to exit to the main loop.  */
> >  if (unlikely(atomic_read(>exit_request)
> >  || (use_icount && cpu->icount_decr.u16.low + cpu->icount_extra == 
> > 0)))
> > +if (cpu->exception_index != -1 && cpu->exception_index != 
> > EXCP_INTERRUP
> > +qemu_log("overwriting excp_index %x\n", cpu->exception_index);
> >  atomic_set(>exit_request, 0);
> >  cpu->exception_index = EXCP_INTERRUPT;
> >  return true;
> 
> This looks like it's just working around whatever is going on
> (why should EXCP_INTERRUPT be special?). What we need to do is
> find out what's actually happening here...

The failure cause is in incorrect interrupt processing.
When ARM processes hardware interrupt in arm_cpu_exec_interrupt(),
it executes cs->exception_index = excp_idx;

This assumes, that the exception will be processed later.
But it is processed immediately by calling cc->do_interrupt(cs);
instead of leaving this job to cpu_exec.

I guess these calls should be removed to match the cpu_exec execution pattern.

Pavel Dovgalyuk

[Qemu-devel] [PATCH] target/i386: hax: Move hax_setup_qemu_emulator.

[Tao Wu via Qemu-devel]

hax_setup_qemu_emulator reference env->efer which is updated in
hax_get_msrs, so it has to be called after hax_get_msrs. This fix
the bug that sometimes dump_state show 32 bits regs even in 64 bits
mode.

Signed-off-by: Tao Wu 
---
 target/i386/hax-all.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index 3ce6950296..a933bd462d 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -887,9 +887,6 @@ static int hax_sync_vcpu_register(CPUArchState *env, int 
set)
 return -1;
 }
 }
-if (!set) {
-hax_setup_qemu_emulator(env);
-}
 return 0;
 }
 
@@ -1070,6 +1067,7 @@ static int hax_arch_get_registers(CPUArchState *env)
 return ret;
 }
 
+hax_setup_qemu_emulator(env);
 return 0;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [Qemu-devel] [PATCH v4 00/11] Add make check tests for Migration

[Peter Xu]

On Fri, Jan 05, 2018 at 10:52:35PM +0100, Juan Quintela wrote:
> 
> Hi
> 
> In v4:
> 
> Based-on: 20180105205109.683-1-quint...@redhat.com
> 
> Changes:
> - rebase on top on v4 info_migrate patches
> - Tune sleeps to make patches fast
> - Create a deprecated test for deprecated commands (i.e. make peterxu happy)
> - create migrate_start_postcopy function
> - fix naming/sizes between power and x86
> - cleanup comments to match code
> 
> Please, review.

I left some comments on specific patches, for the rest I think the
series looks good to me.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 05/11] tests: Add deprecated commands migration test

[Peter Xu]

On Fri, Jan 05, 2018 at 10:52:40PM +0100, Juan Quintela wrote:
> We add deprecated commands on a new test, so we don't have to add it
> on normal tests.
> 
> Signed-off-by: Juan Quintela 
> ---
>  tests/migration-test.c | 59 
> ++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index f469235d0b..bcb0a82d42 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -513,6 +513,64 @@ static void test_migrate_end(QTestState *from, 
> QTestState *to)
>  cleanup("dest_serial");
>  }
>  
> +static void deprecated_set_downtime(QTestState *who, const double value)
> +{
> +QDict *rsp;
> +gchar *cmd;
> +char *expected;
> +int64_t result_int;
> +
> +cmd = g_strdup_printf("{ 'execute': 'migrate_set_downtime',"
> +  "'arguments': { 'value': %g } }", value);
> +rsp = qtest_qmp(who, cmd);
> +g_free(cmd);
> +g_assert(qdict_haskey(rsp, "return"));
> +QDECREF(rsp);
> +result_int = value * 1000L;
> +expected = g_strdup_printf("%" PRId64, result_int);
> +migrate_check_parameter(who, "downtime-limit", expected);
> +g_free(expected);
> +}
> +
> +static void deprecated_set_speed(QTestState *who, const char *value)
> +{
> +QDict *rsp;
> +gchar *cmd;
> +
> +cmd = g_strdup_printf("{ 'execute': 'migrate_set_speed',"
> +  "'arguments': { 'value': %s } }", value);
> +rsp = qtest_qmp(who, cmd);
> +g_free(cmd);
> +g_assert(qdict_haskey(rsp, "return"));
> +QDECREF(rsp);
> +migrate_check_parameter(who, "max-bandwidth", value);
> +}
> +
> +static void test_deprecated(void)
> +{
> +char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +QTestState *from, *to;
> +
> +/* Create source and destination guests.  This way we can reuse
> + * test_migrate_start */
> +test_migrate_start(, , uri);
> +
> +deprecated_set_downtime(from, 0.12345);
> +deprecated_set_downtime(to, 0.12345);
> +deprecated_set_speed(from, "12345");
> +deprecated_set_speed(to, "12345");
> +
> +g_free(uri);
> +
> +qtest_quit(from);
> +qtest_quit(to);
> +
> +cleanup("bootsect");
> +cleanup("migsocket");
> +cleanup("src_serial");
> +cleanup("dest_serial");

I thought calling qtest_start() with a single VM would be even
simpler, but this is good enough for me... Thanks for that.  :-)

Reviewed-by: Peter Xu 

> +}
> +
>  static void test_migrate(void)
>  {
>  char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -580,6 +638,7 @@ int main(int argc, char **argv)
>  module_call_init(MODULE_INIT_QOM);
>  
>  qtest_add_func("/migration/postcopy/unix", test_migrate);
> +qtest_add_func("/migration/deprecated", test_deprecated);
>  
>  ret = g_test_run();
>  
> -- 
> 2.14.3
> 

-- 
Peter Xu

[Qemu-devel] [PATCH 1/2] target/arm: Split out vfp_expand_imm

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 44 
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index ecb72e4d9c..e03cd3801a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -5014,6 +5014,33 @@ static void disas_fp_3src(DisasContext *s, uint32_t insn)
 }
 }
 
+/* The imm8 encodes the sign bit, enough bits to represent an exponent in
+ * the range 011xx to 100xx, and the most significant 4 bits of
+ * the mantissa; see VFPExpandImm() in the v8 ARM ARM.
+ */
+static uint64_t vfp_expand_imm(int size, uint8_t imm8)
+{
+uint64_t imm;
+
+switch (size) {
+case MO_64:
+imm = (extract32(imm8, 7, 1) ? 0x8000 : 0) |
+(extract32(imm8, 6, 1) ? 0x3fc0 : 0x4000) |
+extract32(imm8, 0, 6);
+imm <<= 48;
+break;
+case MO_32:
+imm = (extract32(imm8, 7, 1) ? 0x8000 : 0) |
+(extract32(imm8, 6, 1) ? 0x3e00 : 0x4000) |
+(extract32(imm8, 0, 6) << 3);
+imm <<= 16;
+break;
+default:
+g_assert_not_reached();
+}
+return imm;
+}
+
 /* Floating point immediate
  *   31  30  29 28   24 23  22  21 2013 12   10 95 40
  * +---+---+---+---+--+---++---+--+--+
@@ -5037,22 +5064,7 @@ static void disas_fp_imm(DisasContext *s, uint32_t insn)
 return;
 }
 
-/* The imm8 encodes the sign bit, enough bits to represent
- * an exponent in the range 011xx to 100xx,
- * and the most significant 4 bits of the mantissa; see
- * VFPExpandImm() in the v8 ARM ARM.
- */
-if (is_double) {
-imm = (extract32(imm8, 7, 1) ? 0x8000 : 0) |
-(extract32(imm8, 6, 1) ? 0x3fc0 : 0x4000) |
-extract32(imm8, 0, 6);
-imm <<= 48;
-} else {
-imm = (extract32(imm8, 7, 1) ? 0x8000 : 0) |
-(extract32(imm8, 6, 1) ? 0x3e00 : 0x4000) |
-(extract32(imm8, 0, 6) << 3);
-imm <<= 16;
-}
+imm = vfp_expand_imm(MO_32 + is_double, imm8);
 
 tcg_res = tcg_const_i64(imm);
 write_fp_dreg(s, rd, tcg_res);
-- 
2.13.6

[Qemu-devel] [PATCH 2/2] target/arm: Add fp16 support to vfp_expand_imm

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e03cd3801a..4fe9d82a55 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -5035,6 +5035,11 @@ static uint64_t vfp_expand_imm(int size, uint8_t imm8)
 (extract32(imm8, 0, 6) << 3);
 imm <<= 16;
 break;
+case MO_16:
+imm = (extract32(imm8, 7, 1) ? 0x8000 : 0) |
+(extract32(imm8, 6, 1) ? 0x3000 : 0x4000) |
+(extract32(imm8, 0, 6) << 6);
+break;
 default:
 g_assert_not_reached();
 }
-- 
2.13.6

[Qemu-devel] [PATCH 0/2] target/arm: split out vfp_expand_imm

[Richard Henderson]

One more piece of target/arm prep work from my SVE branch.

I saw that Alex was touching the same bit of code in his ARMv8.2
fp16 patch set and thought we should coordinate on this.


r~


Richard Henderson (2):
  target/arm: Split out vfp_expand_imm
  target/arm: Add fp16 support to vfp_expand_imm

 target/arm/translate-a64.c | 49 +++---
 1 file changed, 33 insertions(+), 16 deletions(-)

-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 04/11] tests: Use consistent names and sizes for migration

[Peter Xu]

On Fri, Jan 05, 2018 at 10:52:39PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela 
> ---
>  tests/migration-test.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index d81f22118b..f469235d0b 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -440,13 +440,13 @@ static void test_migrate_start(QTestState **from, 
> QTestState **to,
>  
>  if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>  init_bootfile_x86(bootpath);
> -cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
> -  " -name pcsource,debug-threads=on"
> +cmd_src = g_strdup_printf("-machine accel=%s -m 256M"
> +  " -name source,debug-threads=on"

A pure question: when will the name matter?

>" -serial file:%s/src_serial"
>" -drive file=%s,format=raw",
>accel, tmpfs, bootpath);
> -cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
> -  " -name pcdest,debug-threads=on"
> +cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
> +  " -name target,debug-threads=on"
>" -serial file:%s/dest_serial"
>" -drive file=%s,format=raw"
>" -incoming %s",
> @@ -459,12 +459,12 @@ static void test_migrate_start(QTestState **from, 
> QTestState **to,
>  }
>  init_bootfile_ppc(bootpath);
>  cmd_src = g_strdup_printf("-machine accel=%s -m 256M"
> -  " -name pcsource,debug-threads=on"
> +  " -name source,debug-threads=on"
>" -serial file:%s/src_serial"
>" -drive file=%s,if=pflash,format=raw",
>accel, tmpfs, bootpath);
>  cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
> -  " -name pcdest,debug-threads=on"
> +  " -name target,debug-threads=on"
>" -serial file:%s/dest_serial"
>" -drive file=%s,if=pflash,format=raw"
>" -incoming %s",
> -- 
> 2.14.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 02/11] tests: Migration ppc test was missing arguments

[Peter Xu]

On Fri, Jan 05, 2018 at 10:52:37PM +0100, Juan Quintela wrote:
> Argument file is also needed there.
> 
> Signed-off-by: Juan Quintela 

Reviewed-by: Peter Xu 

Does it also mean this?

Fixes: aaf89c8a49a8c ("test: port postcopy test to ppc64")

> ---
>  tests/migration-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 0448bc77dc..32f3bb86a8 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -464,8 +464,9 @@ static void test_migrate_start(QTestState **from, 
> QTestState **to,
>  cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>" -name pcdest,debug-threads=on"
>" -serial file:%s/dest_serial"
> +  " -drive file=%s,if=pflash,format=raw"
>" -incoming %s",
> -  accel, tmpfs, uri);
> +  accel, tmpfs, bootpath, uri);
>  } else {
>  g_assert_not_reached();
>  }
> -- 
> 2.14.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/2] Add save-snapshot, load-snapshot and delete-snapshot to QAPI

[Peter Xu]

On Sun, Jan 07, 2018 at 01:23:35PM +0100, Richard Palethorpe wrote:
> Add QAPI wrapper functions for the existing snapshot functionality. These
> functions behave the same way as the HMP savevm, loadvm and delvm
> commands. This will allow applications, such as OpenQA, to programmatically
> revert the VM to a previous state with no dependence on HMP or qemu-img.
> 
> I used the term snapshot instead of VM because I think it is less ambiguous
> and matches the internal function names.
> 
> Signed-off-by: Richard Palethorpe 
> ---
>  migration/savevm.c | 27 ++
>  qapi-schema.json   | 66 
> ++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b7908f62be..d7bc0f0d41 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2242,6 +2242,11 @@ int save_snapshot(const char *name, Error **errp)
>  return ret;
>  }
>  
> +void qmp_save_snapshot(const char *name, Error **errp)
> +{
> +save_snapshot(name, errp);
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, bool has_live, bool 
> live,
>  Error **errp)
>  {
> @@ -2404,6 +2409,28 @@ err_drain:
>  return ret;
>  }
>  
> +void qmp_load_snapshot(const char *name, Error **errp)
> +{
> +int saved_vm_running = runstate_is_running();
> +
> +vm_stop(RUN_STATE_RESTORE_VM);
> +
> +if (!load_snapshot(name, errp) && saved_vm_running) {
> +vm_start();
> +}
> +}

Hi, Richard,

I think it's good to have, but from interface POV for sure I'll leave
it for maintainers.

For the code, I would prefer at least unify the code instead of
duplicating it.  I think calling qmp_*() functions in hmp_*() would be
good since that's what we do now mostly AFAICT.

Also, comparing to exposing snapshot operations, I am curious about
what would be the difference if we just migrate (which can use QMP) to
a file and then load that file using incoming migration.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Jason Wang]

On 2018年01月10日 12:18, Zhoujian (jay) wrote:

Sorry about missing to cc Jason.


Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on 
failed
in net_init_tap_one() will make the followed up device_add 
virtio-net-pci,netdev=net0
failed too, which prints:

 TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
 ioctl() failed: Bad file descriptor

This patch makes the followed up device_add be able to fall back to userspace 
virtio
when netdev_add failed with vhost turning on but vhost force flag does not set.

Here is the original discussion:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html


This is a RFC version, if I'm wrong, please let me know, thanks!

PS: there're two places updated, see below.



-Original Message-
From: Zhoujian (jay)
Sent: Wednesday, January 10, 2018 12:40 AM
To: qemu-devel@nongnu.org
Cc: m...@redhat.com; imamm...@redhat.com; Huangweidong (C)
; wangxin (U) ; Gonglei
(Arei) ; Liuzhe (Ahriy, Euler) ;
Zhoujian (jay) 
Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
initialize

Making the followed up device_add to fall back to userspace virtio when
netdev_add fails if vhost force flag does not set.

Suggested-by: Michael S. Tsirkin 
Suggested-by: Igor Mammedov 
Signed-off-by: Jay Zhou 
---
  net/tap.c | 25 ++---
  1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 979e622..03f226f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
NetClientState *peer,
   const char *model, const char *name,
   const char *ifname, const char *script,
   const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp)
+ int vnet_hdr, int fd, Error **errp,
+ bool *vhost_init_failed)
  {
  Error *err = NULL;
  TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
NetClientState *peer,
  vhostfd = monitor_fd_param(cur_mon, vhostfdname, );
  if (vhostfd == -1) {
  error_propagate(errp, err);
+*vhost_init_failed = true;
  return;
  }
  } else {
@@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
NetClientState *peer,
  if (vhostfd < 0) {
  error_setg_errno(errp, errno,
   "tap: open vhost char device failed");
+*vhost_init_failed = true;
  return;
  }
  fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
  if (!s->vhost_net) {
  error_setg(errp,
 "vhost-net requested but could not be initialized");
+*vhost_init_failed = true;


Why not simply check s->vhost_net after call net_init_tap_one()?

Thanks


  return;
  }
  } else if (vhostfdname) {
@@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
  Error *err = NULL;
  const char *vhostfdname;
  char ifname[128];
+bool vhost_init_failed = false;

  assert(netdev->type == NET_CLIENT_DRIVER_TAP);
  tap = >u.tap;
@@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,

  net_init_tap_one(tap, peer, "tap", name, NULL,
   script, downscript,
- vhostfdname, vnet_hdr, fd, );
+ vhostfdname, vnet_hdr, fd, ,
+ _init_failed);
  if (err) {
  error_propagate(errp, err);
  return -1;
@@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
  net_init_tap_one(tap, peer, "tap", name, ifname,
   script, downscript,
   tap->has_vhostfds ? vhost_fds[i] : NULL,
- vnet_hdr, fd, );
+ vnet_hdr, fd, , _init_failed);
  if (err) {
  error_propagate(errp, err);
  goto free_fail;
@@ -874,10 +879,12 @@ free_fail:

  net_init_tap_one(tap, peer, "bridge", name, ifname,
   script, downscript, vhostfdname,
- vnet_hdr, fd, );
+ vnet_hdr, fd, , _init_failed);
  if (err) {
  error_propagate(errp, err);
-close(fd);
+if 

Re: [Qemu-devel] [v23 1/2] virtio-crypto: Add virtio crypto device specification

[Longpeng (Mike)]

Hi Halil,

We are fixing the Intel BUG these days, so I will go through your comments after
we're done. Thanks.

-- 
Regards,
Longpeng(Mike)

On 2018/1/10 1:05, Halil Pasic wrote:

> 
> 
> On 12/30/2017 10:35 AM, Longpeng(Mike) wrote:
>> From: Gonglei 
>>
>> The virtio crypto device is a virtual crypto device (ie. hardware
>> crypto accelerator card). Currently, the virtio crypto device provides
>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>
>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>
>> VIRTIO-153
>>
>> Signed-off-by: Gonglei 
>> Signed-off-by: Zhoujian 
>> Signed-off-by: Longpeng(Mike) 
>> ---
>>  acknowledgements.tex |4 +
>>  content.tex  |2 +
>>  virtio-crypto.tex| 1525 
>> ++
>>  3 files changed, 1531 insertions(+)
>>  create mode 100644 virtio-crypto.tex
>>
>> diff --git a/acknowledgements.tex b/acknowledgements.tex
>> index 2d893d9..cdde247 100644
>> --- a/acknowledgements.tex
>> +++ b/acknowledgements.tex
>> @@ -16,10 +16,13 @@ Daniel Kiper,Oracle  \newline
>>  Geoff Brown,Machine-to-Machine Intelligence (M2MI) Corporation  
>> \newline
>>  Gershon Janssen,Individual Member   \newline
>>  James Bottomley,Parallels IP Holdings GmbH  \newline
>> +Jian Zhou,  Huawei  \newline
>> +Lei Gong,   Huawei  \newline
>>  Luiz Capitulino,Red Hat \newline
>>  Michael S. Tsirkin, Red Hat \newline
>>  Paolo Bonzini,  Red Hat \newline
>>  Pawel Moll, ARM \newline
>> +Peng Long,  Huawei  \newline
>>  Richard Sohn,   Alcatel-Lucent \newline
>>  Rusty Russell,  IBM \newline
>>  Sasha Levin,Oracle  \newline
>> @@ -38,6 +41,7 @@ Brian Foley,  ARM \newline
>>  David Alan Gilbert, Red Hat \newline
>>  Fam Zheng, Red Hat  \newline
>>  Gerd Hoffmann, Red Hat  \newline
>> +Halil Pasic,IBM \newline
>>  Jason Wang, Red Hat \newline
>>  Laura Novich, Red Hat   \newline
>>  Patrick Durusau,Technical Advisory Board, OASIS \newline
>> diff --git a/content.tex b/content.tex
>> index c840588..d1d3b09 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -5819,6 +5819,8 @@ descriptor for the \field{sense_len}, \field{residual},
>>  \field{status_qualifier}, \field{status}, \field{response} and
>>  \field{sense} fields.
>>
>> +\input{virtio-crypto.tex}
>> +
>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>
>>  Currently these device-independent feature bits defined:
>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> new file mode 100644
>> index 000..4bd5b51
>> --- /dev/null
>> +++ b/virtio-crypto.tex
>> @@ -0,0 +1,1525 @@
>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>> +
>> +The virtio crypto device is a virtual cryptography device as well as a
>> +virtual cryptographic accelerator. The virtio crypto device provides the
>> +following crypto services: CIPHER, MAC, HASH, and AEAD. Virtio crypto
>> +devices have a single control queue and at least one data queue. Crypto
>> +operation requests are placed into a data queue, and serviced by the
>> +device. Some crypto operation requests are only valid in the context of a
>> +session. The role of the control queue is facilitating control operation
>> +requests. Sessions management is realized with control operation
>> +requests.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
>> +
>> +20
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] dataq1
>> +\item[\ldots]
>> +\item[N-1] dataqN
>> +\item[N] controlq
>> +\end{description}
>> +
>> +N is set by \field{max_dataqueues}.
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature 
>> bits}
>> +
>> +\begin{description}
>> +\item VIRTIO_CRYPTO_F_REVISION_1 (0) revision 1. Revision 1 has a specific
>> +request format and other enhancements (which result in some additional
>> +requirements).
>> +\item VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE (1) stateless mode requests are
>> +supported by the CIPHER service.
>> +\item VIRTIO_CRYPTO_F_HASH_STATELESS_MODE (2) stateless mode requests are
>> +supported by the HASH service.
>> +\item VIRTIO_CRYPTO_F_MAC_STATELESS_MODE (3) stateless mode requests are
>> +supported by the MAC service.
>> +\item VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE (4) stateless mode requests are
>> +supported by the AEAD service.
>> +\end{description}
>> +
>> +
>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto 
>> Device / Feature bits}
>> +
>> +Some crypto feature bits require other crypto feature bits
>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature 
>> Bits}):
>> +
>> +\begin{description}
>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires 
>> VIRTIO_CRYPTO_F_REVISION_1.
>> 

[Qemu-devel] [PATCH] tcg/arm: Fix double-word comparisons

[Richard Henderson]

The code sequence we were generating was only good for unsigned
comparisons.  For signed comparisions, use the sequence from gcc.

Fixes booting of ppc64 firmware, with a patch changing the code
sequence for ppc comparisons.

Signed-off-by: Richard Henderson 
---
 tcg/arm/tcg-target.inc.c | 112 +--
 1 file changed, 80 insertions(+), 32 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index 98a1253..b9890c8 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -239,10 +239,11 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
 }
 }
 
-#define TCG_CT_CONST_ARM  0x100
-#define TCG_CT_CONST_INV  0x200
-#define TCG_CT_CONST_NEG  0x400
-#define TCG_CT_CONST_ZERO 0x800
+#define TCG_CT_CONST_ARM 0x0100
+#define TCG_CT_CONST_INV 0x0200
+#define TCG_CT_CONST_NEG 0x0400
+#define TCG_CT_CONST_INVNEG  0x0800
+#define TCG_CT_CONST_ZERO0x1000
 
 /* parse target specific constraints */
 static const char *target_parse_constraint(TCGArgConstraint *ct,
@@ -258,6 +259,9 @@ static const char *target_parse_constraint(TCGArgConstraint 
*ct,
 case 'N': /* The gcc constraint letter is L, already used here.  */
 ct->ct |= TCG_CT_CONST_NEG;
 break;
+case 'M':
+ct->ct |= TCG_CT_CONST_INVNEG;
+break;
 case 'Z':
 ct->ct |= TCG_CT_CONST_ZERO;
 break;
@@ -351,8 +355,7 @@ static inline int check_fit_imm(uint32_t imm)
 static inline int tcg_target_const_match(tcg_target_long val, TCGType type,
  const TCGArgConstraint *arg_ct)
 {
-int ct;
-ct = arg_ct->ct;
+int ct = arg_ct->ct;
 if (ct & TCG_CT_CONST) {
 return 1;
 } else if ((ct & TCG_CT_CONST_ARM) && check_fit_imm(val)) {
@@ -361,6 +364,9 @@ static inline int tcg_target_const_match(tcg_target_long 
val, TCGType type,
 return 1;
 } else if ((ct & TCG_CT_CONST_NEG) && check_fit_imm(-val)) {
 return 1;
+} else if ((ct & TCG_CT_CONST_INVNEG)
+   && check_fit_imm(~val) && check_fit_imm(-val)) {
+return 1;
 } else if ((ct & TCG_CT_CONST_ZERO) && val == 0) {
 return 1;
 } else {
@@ -1103,6 +1109,64 @@ static inline void tcg_out_mb(TCGContext *s, TCGArg a0)
 }
 }
 
+static TCGCond tcg_out_cmp2(TCGContext *s, const TCGArg *args,
+const int *const_args)
+{
+TCGReg al = args[0];
+TCGReg ah = args[1];
+TCGArg bl = args[2];
+TCGArg bh = args[3];
+TCGCond cond = args[4];
+int const_bl = const_args[2];
+int const_bh = const_args[3];
+
+switch (cond) {
+case TCG_COND_EQ:
+case TCG_COND_NE:
+case TCG_COND_LTU:
+case TCG_COND_LEU:
+case TCG_COND_GTU:
+case TCG_COND_GEU:
+/* We perform a conditional comparision.  If the high half is
+   equal, then overwrite the flags with the comparison of the
+   low half.  The resulting flags cover the whole.  */
+tcg_out_dat_rIN(s, COND_AL, ARITH_CMP, ARITH_CMN, 0, ah, bh, const_bh);
+tcg_out_dat_rIN(s, COND_EQ, ARITH_CMP, ARITH_CMN, 0, al, bl, const_bl);
+return cond;
+
+case TCG_COND_LT:
+case TCG_COND_GE:
+/* We perform a double-word subtraction and examine the result.
+   We do not actually need the result of the subtract, so the
+   low part "subtract" is a compare.  For the high half we have
+   no choice but to compute into a temporary.  */
+tcg_out_dat_rIN(s, COND_AL, ARITH_CMP, ARITH_CMN, 0, al, bl, const_bl);
+tcg_out_dat_rIK(s, COND_AL, ARITH_SBC | TO_CPSR, ARITH_ADC | TO_CPSR,
+TCG_REG_TMP, ah, bh, const_bh);
+return cond;
+
+case TCG_COND_LE:
+case TCG_COND_GT:
+/* Similar, but with swapped arguments.  And of course we must
+   force the immediates into a register.  */
+if (const_bl) {
+tcg_out_movi(s, TCG_TYPE_REG, TCG_REG_TMP, bl);
+bl = TCG_REG_TMP;
+}
+tcg_out_dat_rIN(s, COND_AL, ARITH_CMP, ARITH_CMN, 0, bl, al, 0);
+if (const_bh) {
+tcg_out_movi(s, TCG_TYPE_REG, TCG_REG_TMP, bh);
+bh = TCG_REG_TMP;
+}
+tcg_out_dat_rIK(s, COND_AL, ARITH_SBC | TO_CPSR, ARITH_ADC | TO_CPSR,
+TCG_REG_TMP, bh, ah, 0);
+return tcg_swap_cond(cond);
+
+default:
+g_assert_not_reached();
+}
+}
+
 #ifdef CONFIG_SOFTMMU
 #include "tcg-ldst.inc.c"
 
@@ -1964,22 +2028,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode 
opc,
 tcg_out_goto_label(s, tcg_cond_to_arm_cond[args[2]],
arg_label(args[3]));
 break;
-case INDEX_op_brcond2_i32:
-/* The resulting conditions are:
- * TCG_COND_EQ-->  a0 == a2 && a1 == a3,
- * TCG_COND_NE--> (a0 != a2 && a1 == a3) ||  a1 != a3,
- * 

Re: [Qemu-devel] [PATCH v14 2/9] ACPI: Add APEI GHES table generation and CPER record support

[gengdongjiu]

Peter,
  Thank you very much for your time to review it and give some comments.

On 2018/1/10 0:51, Peter Maydell wrote:
>> the error type to Multi-bit ECC.
>>
>> [1]:
>> UEFI Spec 2.6 Errata A:
>>
>> "N.2.5 Memory Error Section"
>> -+---+--+---+
>> Mnemonic |   Byte Offset |  Byte Length |Description 
>>|
>> -+---+--+---+
>>  |   |  .   |... 
>>|
>> -+---+--+---+
>> Memory Error Type| 72|   1  |Identifies the type of 
>> error that occurred:|
>>  |   |  | 0 – Unknown
>>   |
>>  |   |  | 1 – No error   
>>   |
>>  |   |  | 2 – Single-bit ECC 
>>   |
>>  |   |  | 3 – Multi-bit ECC  
>>   |
>>  |   |  | 4 – Single-symbol ChipKill 
>> ECC   |
>>  |   |  | 5 – Multi-symbol ChipKill 
>> ECC|
>>  |   |  | 6 – Master abort   
>>|
>>  |   |  | 7 – Target abort   
>>|
>>  |   |  | 8 – Parity Error   
>>|
>>  |   |  | 9 – Watchdog timeout   
>>|
>>  |   |  | 10 – Invalid address   
>>|
>>  |   |  | 11 – Mirror Broken 
>>|
>>  |   |  | 12 – Memory Sparing
>>|
>>  |   |  | 13 - Scrub corrected error 
>>|
>>  |   |  | 14 - Scrub uncorrected 
>> error  |
>>  |   |  | 15 - Physical Memory 
>> Map-out event|
>>  |   |  | All other values reserved. 
>>|
>> -+---+--+---+
>>  |   |  .   |... 
>>|
>> -+---+--+---+
> There's a value specified for "we don't know what the error type is",
> which is "0 - Unknown". I think we should use that rather than claiming
> that we have a particular type of error when we don't actually know that.
Agree peter's suggestion. It seems "0 - Unknown" makes sense.

> 
> I agree with James that we don't want to report a particular type of
> error to the guest anyway -- the VM is a virtual environment, and
> the exact reason why the hardware/firmware/host kernel have decided
> that a bit of RAM isn't usable any more doesn't matter to the guest.
> We just want to report "this RAM has gone away, sorry" to it.
Agree with peter, appreciate for the comments.

> 
> thanks
> -- PMM

Re: [Qemu-devel] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[David Gibson]

On Tue, Jan 09, 2018 at 08:21:03PM +1100, Suraj Jitindar Singh wrote:
> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query
> behaviours and available characteristics of the cpu.
> 
> Implement the handler for this new H-Call which formulates its response
> based on the setting of the new capabilities added in the previous
> patch.
> 
> Note: Currently we return H_FUNCTION under TCG which will direct the
>   guest to fall back to doing a displacement flush
> 
> Discussion:
> Is TCG affected?

Very likely :(.

> Is there any point in telling the guest to do these workarounds on TCG
> given they're unlikely to translate to host instructions which have the
> desired effect?

Probably not.  We might have to just advertise broken on TCG, at least
until someone has time to figure out the details.

> ---
>  hw/ppc/spapr_hcall.c   | 81 
> ++
>  include/hw/ppc/spapr.h |  1 +
>  2 files changed, 82 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 51eba52e86..b62b47c8d9 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1654,6 +1654,84 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  return H_SUCCESS;
>  }
>  
> +#define CPU_CHARACTERISTIC_SPEC_BARRIER (1ULL << (63 - 0))
> +#define CPU_CHARACTERISTIC_BCCTR_SERIAL (1ULL << (63 - 1))
> +#define CPU_CHARACTERISTIC_ORI_L1_CACHE (1ULL << (63 - 2))
> +#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE  (1ULL << (63 - 3))
> +#define CPU_CHARACTERISTIC_L1_CACHE_PRIV(1ULL << (63 - 4))
> +#define CPU_CHARACTERISTIC_BRANCH_HINTS (1ULL << (63 - 5))
> +#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF(1ULL << (63 - 6))
> +#define CPU_BEHAVIOUR_FAVOUR_SECURITY   (1ULL << (63 - 0))
> +#define CPU_BEHAVIOUR_L1_CACHE_FLUSH(1ULL << (63 - 1))
> +#define CPU_BEHAVIOUR_SPEC_BARRIER  (1ULL << (63 - 2))
> +
> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> +  sPAPRMachineState *spapr,
> +  target_ulong opcode,
> +  target_ulong *args)
> +{
> +uint64_t characteristics = CPU_CHARACTERISTIC_BRANCH_HINTS;
> +uint64_t behaviour = CPU_BEHAVIOUR_FAVOUR_SECURITY;

I guess we're going to want another knob for the favour security vs
favour performance bit here.

> +uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC);
> +uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC);
> +uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS);
> +
> +/* TODO: Is TCG vulnerable? */

Good question, but in any case..

> +if (!kvm_enabled()) {
> +return H_FUNCTION;
> +}

..this should still advertise things based on the caps.  The point we
apply the caps to the virtual hardware is where we need to consider
TCG's vulnerability.

> +
> +switch (safe_cache) {
> +case SPAPR_CAP_WORKAROUND:
> +characteristics |= CPU_CHARACTERISTIC_ORI_L1_CACHE;
> +characteristics |= CPU_CHARACTERISTIC_MTTRIG_L1_CACHE;
> +characteristics |= CPU_CHARACTERISTIC_L1_CACHE_PRIV;
> +behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
> +break;
> +case SPAPR_CAP_FIXED:
> +break;
> +default: /* broken */
> +if (safe_cache != SPAPR_CAP_BROKEN) {
> +error_report("Invalid value for KVM_CAP_PPC_SAFE_CACHE (%d), 
> assuming broken",
> + safe_cache);
> +}
> +behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
> +break;
> +}
> +
> +switch (safe_bounds_check) {
> +case SPAPR_CAP_WORKAROUND:
> +characteristics |= CPU_CHARACTERISTIC_SPEC_BARRIER;
> +behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
> +break;
> +case SPAPR_CAP_FIXED:
> +break;
> +default: /* broken */
> +if (safe_bounds_check != SPAPR_CAP_BROKEN) {
> +error_report("Invalid value for KVM_CAP_PPC_SAFE_BOUNDS_CHECK 
> (%d), assuming broken",
> + safe_bounds_check);
> +}
> +behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
> +break;
> +}
> +
> +switch (safe_indirect_branch) {
> +case SPAPR_CAP_FIXED:
> +characteristics |= CPU_CHARACTERISTIC_BCCTR_SERIAL;
> +default: /* broken */
> +if (safe_indirect_branch != SPAPR_CAP_BROKEN) {
> +error_report("Invalid value for KVM_CAP_PPC_SAFE_INDIRECT_BRANCH 
> (%d), assuming broken",
> + safe_indirect_branch);
> +}
> +break;
> +}
> +
> +args[0] = characteristics;
> +args[1] = behaviour;
> +
> +return H_SUCCESS;
> +}
> +
>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - 
> KVMPPC_HCALL_BASE + 1];
>  
> @@ -1733,6 +1811,9 

Re: [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[David Gibson]

On Tue, Jan 09, 2018 at 08:21:02PM +1100, Suraj Jitindar Singh wrote:
> This patch adds three new capabilities:
> cap-cfpc -> safe_cache
> cap-sbbc -> safe_bounds_check
> cap-ibs  -> safe_indirect_branch
> 
> Each capability is tristate with the possible values "broken",
> "workaround" or "fixed". Add generic getter and setter functions for
> this new capability type. Add these new capabilities to the capabilities
> list. The maximum value for the capabilities is queried from kvm through
> new kvm capabilities. The requested values are considered to be
> compatible if kvm can support an equal or higher value for each
> capability.
> 
> Discussion:
> Currently these new capabilities default to broken to allow for
> backwards compatibility, is this the best option?
> ---
>  hw/ppc/spapr.c|   6 ++
>  hw/ppc/spapr_caps.c   | 224 
> ++
>  include/hw/ppc/spapr.h|  15 +++-
>  linux-headers/linux/kvm.h |   3 +
>  target/ppc/kvm.c  |  28 ++
>  target/ppc/kvm_ppc.h  |  18 
>  6 files changed, 293 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7fa45729ba..d9700b0254 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr = {
>  _spapr_cap_htm,
>  _spapr_cap_vsx,
>  _spapr_cap_dfp,
> +_spapr_cap_cfpc,
> +_spapr_cap_sbbc,
> +_spapr_cap_ibs,
>  NULL
>  }
>  };
> @@ -3837,6 +3840,9 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>  smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>  smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
>  smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> +smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>  spapr_caps_add_properties(smc, _abort);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index af40f2e469..e6910aa191 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj, Visitor *v, 
> const char *name,
>   SPAPR_CAP_CMD_LINE;
>  }
>  
> +static void spapr_cap_get_tristate(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +sPAPRCapabilityInfo *cap = opaque;
> +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +char *val = NULL;
> +uint8_t value = spapr_get_cap(spapr, cap->index);
> +
> +switch (value) {
> +case SPAPR_CAP_BROKEN:
> +val = g_strdup("broken");
> +break;
> +case SPAPR_CAP_WORKAROUND:
> +val = g_strdup("workaround");
> +break;
> +case SPAPR_CAP_FIXED:
> +val = g_strdup("fixed");
> +break;
> +default:
> +break;
> +}
> +
> +visit_type_str(v, name, , errp);
> +g_free(val);
> +}
> +
> +static void spapr_cap_set_tristate(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +sPAPRCapabilityInfo *cap = opaque;
> +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +char *val;
> +Error *local_err = NULL;
> +uint8_t value;
> +
> +visit_type_str(v, name, , _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +if (!strcasecmp(val, "broken")) {
> +value = SPAPR_CAP_BROKEN;
> +} else if (!strcasecmp(val, "workaround")) {
> +value = SPAPR_CAP_WORKAROUND;
> +} else if (!strcasecmp(val, "fixed")) {
> +value = SPAPR_CAP_FIXED;
> +} else {
> +error_setg(errp, "Invalid capability mode \"%s\" for cap-%s", val,
> +   cap->name);
> +goto out;
> +}
> +
> +spapr->cmd_line_caps.caps[cap->index] = value | SPAPR_CAP_CMD_LINE;
> +out:
> +g_free(val);
> +}
> +
>  static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error 
> **errp)
>  {
>  if (!val) {
> @@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState *spapr, 
> uint8_t val, Error **errp)
>  }
>  }
>  
> +static void cap_safe_cache_allow(sPAPRMachineState *spapr, uint8_t val,
> + Error **errp)
> +{
> +if (kvm_enabled() && (val > kvmppc_get_cap_safe_cache())) {
> +error_setg(errp, "Requested safe cache capability level not 
> supported by kvm, try a different value for cap-cfpc");
> +}

Maybe throw in a FIXME comment for TCG.  Given the bugs can be
triggered via Javascript, there's a good chance they can be triggered
via TCG as well - but obviously we don't want to delay the KVM fixes
to work out what the TCG situation is.

> +}
> +
> +static void cap_safe_bounds_check_allow(sPAPRMachineState *spapr, uint8_t 

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Alexey Perevalov]

On 01/05/2018 12:59 PM, Juan Quintela wrote:

Eric Blake  wrote:

On 01/03/2018 03:38 AM, Juan Quintela wrote:

Hi

This are the changes for migration that are already reviewed.

Please, apply.

Alexey Perevalov (6):
   migration: introduce postcopy-blocktime capability
   migration: add postcopy blocktime ctx into MigrationIncomingState
   migration: calculate vCPU blocktime on dst side
   migration: postcopy_blocktime documentation
   migration: add blocktime calculation into migration-test
   migration: add postcopy total blocktime into query-migrate

I had unanswered questions about these patches in the v12 series, where
I'm not sure if the interface is still quite right.

To be fair, I had alroady integrated the patches before I saw your questions.


We're still early
enough that we could adjust the interface after the fact depending on
how the questions are answered;

I think this is the best approach, so far I can see two questions:

- do we want to make it conditional?  it requires some locking, but I
   haven't meassured it to see how slow/fast is it.

- the other was documentation.

I will like Alexey to answer.  Depending of how slow it is, I can agree
to make it non-optional.

Ok, I'll give a logs with traces, maybe gprof result, today
or tomorrow.



but we're also early enough that it may
be smarter to get the interface right before including it in a pull
request.  I'll leave it to Peter and Juan to sort out whether this means
an updated pull request is needed, or to take this as-is.

Thanks, Juan.





--
Best regards,
Alexey Perevalov

Re: [Qemu-devel] [RFC v6 00/27] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Tue, Jan 09, 2018 at 02:52:49PM +, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:30PM +0800, Peter Xu wrote:
> > This v6 kept most of the high level design untouched, however there
> > are quite a few of changes that try to make the series more robust and
> > safe.  My sincere thank to the reviewers (especially Stefan!) on all
> > review comments to previous one.  Let's see how this version goes.
> > 
> > v6:
> 
> I've finished my review of this revision and posted comments.

Thanks Stefan (and all the reviewers)!  I'll revisit the series in one
or two days.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Zhoujian (jay)]

Sorry about missing to cc Jason.


Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on 
failed
in net_init_tap_one() will make the followed up device_add 
virtio-net-pci,netdev=net0
failed too, which prints:

TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD 
ioctl() failed: Bad file descriptor

This patch makes the followed up device_add be able to fall back to userspace 
virtio
when netdev_add failed with vhost turning on but vhost force flag does not set.

Here is the original discussion:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html


This is a RFC version, if I'm wrong, please let me know, thanks!

PS: there're two places updated, see below.


> -Original Message-
> From: Zhoujian (jay)
> Sent: Wednesday, January 10, 2018 12:40 AM
> To: qemu-devel@nongnu.org
> Cc: m...@redhat.com; imamm...@redhat.com; Huangweidong (C)
> ; wangxin (U) ; Gonglei
> (Arei) ; Liuzhe (Ahriy, Euler) ;
> Zhoujian (jay) 
> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
> initialize
> 
> Making the followed up device_add to fall back to userspace virtio when
> netdev_add fails if vhost force flag does not set.
> 
> Suggested-by: Michael S. Tsirkin 
> Suggested-by: Igor Mammedov 
> Signed-off-by: Jay Zhou 
> ---
>  net/tap.c | 25 ++---
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..03f226f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>   const char *model, const char *name,
>   const char *ifname, const char *script,
>   const char *downscript, const char *vhostfdname,
> - int vnet_hdr, int fd, Error **errp)
> + int vnet_hdr, int fd, Error **errp,
> + bool *vhost_init_failed)
>  {
>  Error *err = NULL;
>  TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>  vhostfd = monitor_fd_param(cur_mon, vhostfdname, );
>  if (vhostfd == -1) {
>  error_propagate(errp, err);
> +*vhost_init_failed = true;
>  return;
>  }
>  } else {
> @@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>  if (vhostfd < 0) {
>  error_setg_errno(errp, errno,
>   "tap: open vhost char device failed");
> +*vhost_init_failed = true;
>  return;
>  }
>  fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
> void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>  if (!s->vhost_net) {
>  error_setg(errp,
> "vhost-net requested but could not be initialized");
> +*vhost_init_failed = true;
>  return;
>  }
>  } else if (vhostfdname) {
> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>  Error *err = NULL;
>  const char *vhostfdname;
>  char ifname[128];
> +bool vhost_init_failed = false;
> 
>  assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>  tap = >u.tap;
> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> 
>  net_init_tap_one(tap, peer, "tap", name, NULL,
>   script, downscript,
> - vhostfdname, vnet_hdr, fd, );
> + vhostfdname, vnet_hdr, fd, ,
> + _init_failed);
>  if (err) {
>  error_propagate(errp, err);
>  return -1;
> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>  net_init_tap_one(tap, peer, "tap", name, ifname,
>   script, downscript,
>   tap->has_vhostfds ? vhost_fds[i] : NULL,
> - vnet_hdr, fd, );
> + vnet_hdr, fd, , _init_failed);
>  if (err) {
>  error_propagate(errp, err);
>  goto free_fail;
> @@ -874,10 +879,12 @@ free_fail:
> 
>  net_init_tap_one(tap, peer, "bridge", name, ifname,
>   script, downscript, vhostfdname,
> - vnet_hdr, fd, );
> + vnet_hdr, fd, , _init_failed);
>  if (err) {
>  error_propagate(errp, err);
> -close(fd);
> +

Re: [Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[David Gibson]

On Tue, Jan 09, 2018 at 08:21:01PM +1100, Suraj Jitindar Singh wrote:
> Currently spapr_caps are tied to boolean values (on or off). This patch
> reworks the caps so that they can have any value between 0 and 127,
> inclusive. This allows more capabilities with various values to be
> represented in the same way internally. Capabilities are numbered in
> ascending order. The internal representation of capability values is an
> array of uint8s in the sPAPRMachineState, indexed by capability number.
> Note: The MSB (0x80) of a capability is reserved to track whether the
>   capability was set from the command line.
> 
> Capabilities can have their own name, description, options, getter and
> setter functions, type and allow functions. They also each have their own
> section in the migration stream. Capabilities are only migrated if they
> were explictly set on the command line, with the assumption that
> otherwise the default will match.
> 
> On migration we ensure that the capability value on the destination
> is greater than or equal to the capability value from the source. So
> long at this remains the case then the migration is considered
> compatible and allowed to continue.
> 
> This patch implements generic getter and setter functions for boolean
> capabilities. It also converts the existings cap-htm, cap-vsx and
> cap-dfp capabilities to this new format.
> ---
>  hw/ppc/spapr.c |  19 +--
>  hw/ppc/spapr_caps.c| 335 
> -
>  include/hw/ppc/spapr.h |  41 +++---
>  3 files changed, 222 insertions(+), 173 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d1acfe8858..7fa45729ba 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -320,7 +320,7 @@ static void spapr_populate_pa_features(sPAPRMachineState 
> *spapr,
>   */
>  pa_features[3] |= 0x20;
>  }
> -if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {

I'd prefer to see explicit >0 or !=0 to emphasise that the returned
value is now an integer not a bool.

>  pa_features[24] |= 0x80;/* Transactional memory support */
>  }
>  if (legacy_guest && pa_size > 40) {
> @@ -563,7 +563,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>   *
>   * Only CPUs for which we create core types in spapr_cpu_core.c
>   * are possible, and all of those have VMX */
> -if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_VSX)) {
>  _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
>  } else {
>  _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> @@ -572,7 +572,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>  /* Advertise DFP (Decimal Floating Point) if available
>   *   0 / no property == no DFP
>   *   1   == DFP available */
> -if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_DFP)) {
>  _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
>  }
>  
> @@ -1762,7 +1762,9 @@ static const VMStateDescription vmstate_spapr = {
>  _spapr_ov5_cas,
>  _spapr_patb_entry,
>  _spapr_pending_events,
> -_spapr_caps,
> +_spapr_cap_htm,
> +_spapr_cap_vsx,
> +_spapr_cap_dfp,
>  NULL
>  }
>  };
> @@ -2323,8 +2325,6 @@ static void spapr_machine_init(MachineState *machine)
>  char *filename;
>  Error *resize_hpt_err = NULL;
>  
> -spapr_caps_validate(spapr, _fatal);
> -
>  msi_nonbroken = true;
>  
>  QLIST_INIT(>phbs);
> @@ -3834,7 +3834,9 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>   */
>  mc->numa_mem_align_shift = 28;
>  
> -smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
> +smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> +smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> +smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
>  spapr_caps_add_properties(smc, _abort);
>  }
>  
> @@ -3916,8 +3918,7 @@ static void 
> spapr_machine_2_11_class_options(MachineClass *mc)
>  sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>  spapr_machine_2_12_class_options(mc);
> -smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
> -   | SPAPR_CAP_DFP);
> +smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
>  SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9d070a306c..af40f2e469 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -35,33 +35,71 @@
>  typedef struct sPAPRCapabilityInfo {
>  const char *name;
>  const char *description;
> -uint64_t flag;
> +const char *options;
> +int index;
>  
> +/* Getter and Setter Function Pointers */
> +

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[David Gibson]

On Wed, Jan 10, 2018 at 11:19:33AM +1100, Suraj Jitindar Singh wrote:
> On Tue, 2018-01-09 at 13:07 +0100, Andrea Bolognani wrote:
> > On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
> > [...]
> > > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > > Error **errp)
> > > +{
> > > +if (!val) {
> > > +/* TODO: We don't support disabling htm yet */
> > > +return;
> > > +}
> > >  if (tcg_enabled()) {
> > >  error_setg(errp,
> > > -   "No Transactional Memory support in TCG, try
> > > cap-htm=off");
> > > +   "No Transactional Memory support in TCG, try
> > > cap-htm=0");
> > >  } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> > >  error_setg(errp,
> > > -"KVM implementation does not support Transactional Memory, try
> > > cap-htm=off"
> > > +"KVM implementation does not support Transactional Memory, try
> > > cap-htm=0"
> > >  );
> > >  }
> > >  }
> > 
> > Changing the command-line interface from off/on to 0/1 seems
> > unnecessary, given that broken/workaround/fixed are used for the
> > capabilities you introduce later in the series. off/on look much
> > better IMHO.
> 
> These are booleans so they have to be "on"/"off" anyway... 0/1 doesn't
> work. My bad :/ I'll fix the message.
> 
> > 
> > [...]
> > > -static bool spapr_caps_needed(void *opaque)
> > > -{
> > > -sPAPRMachineState *spapr = opaque;
> > > -
> > > -return (spapr->forced_caps.mask != 0) || (spapr-
> > > >forbidden_caps.mask != 0);
> > > -}
> > > -
> > >  /* This has to be called from the top-level spapr post_load, not
> > > the
> > >   * caps specific one.  Otherwise it wouldn't be called when the
> > > source
> > >   * caps are all defaults, which could still conflict with
> > > overridden
> > >   * caps on the destination */
> > >  int spapr_caps_post_migration(sPAPRMachineState *spapr)
> > >  {
> > > -uint64_t allcaps = 0;
> > >  int i;
> > >  bool ok = true;
> > >  sPAPRCapabilities dstcaps = spapr->effective_caps;
> > >  sPAPRCapabilities srccaps;
> > >  
> > >  srccaps = default_caps_with_cpu(spapr, first_cpu);
> > > -srccaps.mask |= spapr->mig_forced_caps.mask;
> > > -srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > > +for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > > +if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > > +srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > > ~SPAPR_CAP_CMD_LINE;
> > > +}
> > > +}
> > >  
> > > -for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > > +for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >  sPAPRCapabilityInfo *info = _table[i];
> > >  
> > > -allcaps |= info->flag;
> > > -
> > > -if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > > >flag)) {
> > > -error_report("cap-%s=on in incoming stream, but off in
> > > destination",
> > > - info->name);
> > > +if (srccaps.caps[i] > dstcaps.caps[i]) {
> > > +error_report("cap-%s higher level (%d) in incoming
> > > stream than on destination (%d)",
> > > + info->name, srccaps.caps[i],
> > > dstcaps.caps[i]);
> > >  ok = false;
> > >  }
> > >  
> > > -if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > > >flag)) {
> > > -warn_report("cap-%s=off in incoming stream, but on in
> > > destination",
> > > - info->name);
> > > +if (srccaps.caps[i] < dstcaps.caps[i]) {
> > > +warn_report("cap-%s lower level (%d) in incoming
> > > stream than on destination (%d)",
> > > + info->name, srccaps.caps[i],
> > > dstcaps.caps[i]);
> > >  }
> > >  }
> > 
> > These numeric comparisons make me feel very uneasy :)
> > 
> > What if we need to add more possible values down the line? Should
> > there be at least some room between existing values to avoid painting
> > ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...
> > 
> > You clearly know more about the problem than I do, so feel free to
> > dismiss all of the above... I thought I would bring up my worries
> > just in case :)
> 
> For these capabilities I think we're ok to keep it as 0/1/2. In the
> event we need a bigger range another capability can be added with other
> possible values which was the whole point of introducing this generic
> framework. The basic idea is the receiving side must always support a
> higher "level" than the source.
> 
> With these new capabilities it's more likely we'll have to add an
> entirly new one than require more possible values. :)
> 
> It could even be possible to have a per capability comparison function
> to confirm compatibility in future. But again thats an exercise for
> when/if more complex capabilities are added.

I concur.  New capabilities which require a more detailed set of
values are reasonably likely.  New 

Re: [Qemu-devel] Call for GSoC & Outreachy 2018 mentors & project ideas

[Fam Zheng]

On Tue, 01/09 13:45, Alistair Francis wrote:
> On Tue, Jan 2, 2018 at 12:23 PM, Stefan Hajnoczi  wrote:
> > QEMU will apply to the Google Summer of Code
> > (https://summerofcode.withgoogle.com/) and Outreachy
> > (https://www.outreachy.org/) open source internship programs again
> > this year.
> >
> > Do you want to mentor newcomers beginning open source development in
> > our community?
> >
> > Please post your project ideas on the wiki by January 23rd:
> > https://wiki.qemu.org/Google_Summer_of_Code_2018
> >
> > Project ideas must:
> >  * Be doable in 12 weeks by someone fluent in C programming but not
> > familiar with the codebase.
> >  * Have a clear scope and few dependencies.
> >  * Have a high chance of being merged.
> >  * Consist of smaller steps that can be merged incrementally.
> >
> > Active QEMU, KVM, and Jailhouse contributors are invited to become
> > mentors.  Mentoring is a volunteer activity that requires around 5
> > hours per week to interact with your intern, review code, etc.  GSoC
> > and Outreachy internships run from May through August.
> >
> > For background on why QEMU participates in open source internship
> > programs and how it works, check out my KVM Forum presentation:
> > https://www.youtube.com/watch?v=xNVCX7YMUL8
> > https://vmsplice.net/~stefan/stefanha-kvm-forum-2016.pdf
> >
> > KVM and Jailhouse are invited to participate under the QEMU GSoC
> > umbrella organization.
> >
> > Please let me know if you have any questions!
> 
> Hey,
> 
> Can anyone who has done this before chime in.
> 
> What do you think about getting someone to cleanup and improve the GDB
> support in QEMU? Would that be the right difficulty of task for a GSoC
> project?

Yeah. I don't know much about GDB stub in QEMU but it sounds like a good idea if
the requirements (what exactly to improve) are concrete.  In general, a few
factors to consider are:

* Is it managable for a student who is not familiar with QEMU code but has fair
  proficiency in C and Linux programming?
* Is it roughly 3 months' (full time) amount of work for such a student?
* Is there a mentor that can guide and help (like said above, devote a few hours
  every week in the process, meeting with the student, answering questions and
  reviewing patches)?

Fam

Re: [Qemu-devel] [PULL 00/12] ppc-for-2.12 queue 20180108

[David Gibson]

On Tue, Jan 09, 2018 at 03:15:25PM +, Peter Maydell wrote:
> On 9 January 2018 at 12:16, David Gibson  wrote:
> > Thanks.  Even if you can identify which patch it is and we can
> > postpone that one would be a bug help.
> 
> Bisection blames this one:
> 
> pbonz...@redhat.com (1):
>   target-ppc: optimize cmp translation

Aha!  Ok, I'll pull that one out until we find a fix.

[snip]
> (This is a build for arm32, running in a chroot on an aarch64
> box, non-debug build.)

Ah.. but since it's a chroot, still an aarch64 cpu behind it all.  I
think the machines I managed to borrow were all much older slower
really truly arm32 machines, which I guess didn't show the problem.

Oh.. and probably a debug (default) build as well.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] maint: Mention web site maintenance in README

[Fam Zheng]

On Wed, 11/29 09:25, Eric Blake wrote:
> Now that we have a website that accepts patches on the list, the
> main project should make it easier to find information about that
> process.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PULL 00/12] ppc-for-2.12 queue 20180108

[Richard Henderson]

On 01/09/2018 07:15 AM, Peter Maydell wrote:
> (This is a build for arm32, running in a chroot on an aarch64
> box, non-debug build.)

It's an arm backend problem.

For 64-bit setcond2, we do

0x6e5c3fa4:  e1570008  cmp  r7, r8
0x6e5c3fa8:  01550004  cmpeqr5, r4
0x6e5c3fac:  b3a09001  movltsb, #1
0x6e5c3fb0:  a3a09000  movgesb, #0

which looks good, but only works for LTU not LT.

I'll work on a fix.


r~

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:19 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to
> > query
> > behaviours and available characteristics of the cpu.
> > 
> > Implement the handler for this new H-Call which formulates its
> > response
> > based on the setting of the new capabilities added in the previous
> > patch.
> > 
> > Note: Currently we return H_FUNCTION under TCG which will direct
> > the
> >   guest to fall back to doing a displacement flush
> > 
> > Discussion:
> > Is TCG affected?
> > Is there any point in telling the guest to do these workarounds on
> > TCG
> > given they're unlikely to translate to host instructions which have
> > the
> > desired effect?
> 
> Hi, Suraj.
> 
> What if this is left to the cover letter?

Again, only because this is RFC.

> 
> > ---
> >  hw/ppc/spapr_hcall.c   | 81
> > ++
> >  include/hw/ppc/spapr.h |  1 +
> >  2 files changed, 82 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 51eba52e86..b62b47c8d9 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1654,6 +1654,84 @@ static target_ulong
> > h_client_architecture_support(PowerPCCPU *cpu,
> >  return H_SUCCESS;
> >  }
> > 
> > +#define CPU_CHARACTERISTIC_SPEC_BARRIER (1ULL << (63 - 0))
> > +#define CPU_CHARACTERISTIC_BCCTR_SERIAL (1ULL << (63 - 1))
> > +#define CPU_CHARACTERISTIC_ORI_L1_CACHE (1ULL << (63 - 2))
> > +#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE  (1ULL << (63 - 3))
> > +#define CPU_CHARACTERISTIC_L1_CACHE_PRIV(1ULL << (63 - 4))
> > +#define CPU_CHARACTERISTIC_BRANCH_HINTS (1ULL << (63 - 5))
> > +#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF(1ULL << (63 - 6))
> > +#define CPU_BEHAVIOUR_FAVOUR_SECURITY   (1ULL << (63 - 0))
> > +#define CPU_BEHAVIOUR_L1_CACHE_FLUSH(1ULL << (63 - 1))
> > +#define CPU_BEHAVIOUR_SPEC_BARRIER  (1ULL << (63 - 2))
> 
> Can PPC_BIT be used here?

Yep, will do

> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:15 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > This patch adds three new capabilities:
> > cap-cfpc -> safe_cache
> > cap-sbbc -> safe_bounds_check
> > cap-ibs  -> safe_indirect_branch
> 
> Hi, Suraj.
> 
> What about splitting this into smaller patches, one per capability?

Yep

> 
> > Each capability is tristate with the possible values "broken",
> > "workaround" or "fixed". Add generic getter and setter functions
> > for
> > this new capability type. Add these new capabilities to the
> > capabilities
> > list. The maximum value for the capabilities is queried from kvm
> > through
> > new kvm capabilities. The requested values are considered to be
> > compatible if kvm can support an equal or higher value for each
> > capability.
> > 
> > Discussion:
> > Currently these new capabilities default to broken to allow for
> > backwards compatibility, is this the best option?
> 
> This could be placed in the cover letter, not in the commit 

Only here because this is an RFC

> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 10:02 -0200, jos...@linux.vnet.ibm.com wrote:
> On Tue, Jan 09, 2018 at 08:21:02PM +1100, Suraj Jitindar Singh wrote:
> > This patch adds three new capabilities:
> > cap-cfpc -> safe_cache
> > cap-sbbc -> safe_bounds_check
> > cap-ibs  -> safe_indirect_branch
> > 
> > Each capability is tristate with the possible values "broken",
> > "workaround" or "fixed". Add generic getter and setter functions
> > for
> > this new capability type. Add these new capabilities to the
> > capabilities
> > list. The maximum value for the capabilities is queried from kvm
> > through
> > new kvm capabilities. The requested values are considered to be
> > compatible if kvm can support an equal or higher value for each
> > capability.
> > 
> > Discussion:
> > Currently these new capabilities default to broken to allow for
> > backwards compatibility, is this the best option?
> > ---
> >  hw/ppc/spapr.c|   6 ++
> >  hw/ppc/spapr_caps.c   | 224
> > ++
> >  include/hw/ppc/spapr.h|  15 +++-
> >  linux-headers/linux/kvm.h |   3 +
> >  target/ppc/kvm.c  |  28 ++
> >  target/ppc/kvm_ppc.h  |  18 
> >  6 files changed, 293 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7fa45729ba..d9700b0254 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr
> > = {
> >  _spapr_cap_htm,
> >  _spapr_cap_vsx,
> >  _spapr_cap_dfp,
> > +_spapr_cap_cfpc,
> > +_spapr_cap_sbbc,
> > +_spapr_cap_ibs,
> >  NULL
> >  }
> >  };
> > @@ -3837,6 +3840,9 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> >  smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> >  smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> >  smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> > +smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> > +smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> > +smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> >  spapr_caps_add_properties(smc, _abort);
> >  }
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index af40f2e469..e6910aa191 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj,
> > Visitor *v, const char *name,
> >   SPAPR_CAP_CMD_LINE;
> >  }
> > 
> > +static void spapr_cap_get_tristate(Object *obj, Visitor *v, const
> > char *name,
> > +   void *opaque, Error **errp)
> > +{
> > +sPAPRCapabilityInfo *cap = opaque;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +char *val = NULL;
> > +uint8_t value = spapr_get_cap(spapr, cap->index);
> > +
> > +switch (value) {
> > +case SPAPR_CAP_BROKEN:
> > +val = g_strdup("broken");
> > +break;
> > +case SPAPR_CAP_WORKAROUND:
> > +val = g_strdup("workaround");
> > +break;
> > +case SPAPR_CAP_FIXED:
> > +val = g_strdup("fixed");
> > +break;
> > +default:
> > +break;
> 
> Hello Suraj
> 
> Is default a possible case? If so, will val be handled correctly by
> visit_type_str and g_free?

The default case would represent an internal error somehow and should
throw an error. Will change that.

Thanks :)

> 
> if not, maybe a throwing an error like you did in
> spapr_cap_set_tristate could be better.
> 
> > +}
> > +
> > +visit_type_str(v, name, , errp);
> > +g_free(val);
> > +}
> > +
> > +static void spapr_cap_set_tristate(Object *obj, Visitor *v, const
> > char *name,
> > +   void *opaque, Error **errp)
> > +{
> > +sPAPRCapabilityInfo *cap = opaque;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +char *val;
> > +Error *local_err = NULL;
> > +uint8_t value;
> > +
> > +visit_type_str(v, name, , _err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +return;
> > +}
> > +
> > +if (!strcasecmp(val, "broken")) {
> > +value = SPAPR_CAP_BROKEN;
> > +} else if (!strcasecmp(val, "workaround")) {
> > +value = SPAPR_CAP_WORKAROUND;
> > +} else if (!strcasecmp(val, "fixed")) {
> > +value = SPAPR_CAP_FIXED;
> > +} else {
> > +error_setg(errp, "Invalid capability mode \"%s\" for cap-
> > %s", val,
> > +   cap->name);
> > +goto out;
> > +}
> > +
> > +spapr->cmd_line_caps.caps[cap->index] = value |
> > SPAPR_CAP_CMD_LINE;
> > +out:
> > +g_free(val);
> > +}
> > +
> >  static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> >  {
> >  if (!val) {
> > @@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState
> > *spapr, uint8_t val, Error **errp)
> 

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:13 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > Currently spapr_caps are tied to boolean values (on or off). This
> > patch
> > reworks the caps so that they can have any value between 0 and 127,
> > inclusive. This allows more capabilities with various values to be
> > represented in the same way internally. Capabilities are numbered
> > in
> > ascending order. The internal representation of capability values
> > is an
> > array of uint8s in the sPAPRMachineState, indexed by capability
> > number.
> > Note: The MSB (0x80) of a capability is reserved to track whether
> > the
> >   capability was set from the command line.
> > 
> > Capabilities can have their own name, description, options, getter
> > and
> > setter functions, type and allow functions. They also each have
> > their own
> > section in the migration stream. Capabilities are only migrated if
> > they
> > were explictly set on the command line, with the assumption that
> > otherwise the default will match.
> > 
> > On migration we ensure that the capability value on the destination
> > is greater than or equal to the capability value from the source.
> > So
> > long at this remains the case then the migration is considered
> > compatible and allowed to continue.
> > 
> > This patch implements generic getter and setter functions for
> > boolean
> > capabilities. It also converts the existings cap-htm, cap-vsx and
> > cap-dfp capabilities to this new format.
> 
> Hi, Suraj.
> 
> I've got the impression that this patch does a lot of things. What
> about
> splitting this patch into the following?
> 
> - rename spapr_has_cap() -> spapr_get_cap()
> - introduce each spapr_cap_[gs]et_bool() in separate patches
> - make use of spapr_cap[gs]et_bool()
> - convert capabilities internal representation to uint8
> - add each new capability separately

Absolutely. This was an RFC to get the code out there for comment.
I'll try to split it up further as suggested for the actual patch
series :)

> 
> Perhaps it can be broken into even smaller changes.
> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 13:07 +0100, Andrea Bolognani wrote:
> On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
> [...]
> > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > +{
> > +if (!val) {
> > +/* TODO: We don't support disabling htm yet */
> > +return;
> > +}
> >  if (tcg_enabled()) {
> >  error_setg(errp,
> > -   "No Transactional Memory support in TCG, try
> > cap-htm=off");
> > +   "No Transactional Memory support in TCG, try
> > cap-htm=0");
> >  } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> >  error_setg(errp,
> > -"KVM implementation does not support Transactional Memory, try
> > cap-htm=off"
> > +"KVM implementation does not support Transactional Memory, try
> > cap-htm=0"
> >  );
> >  }
> >  }
> 
> Changing the command-line interface from off/on to 0/1 seems
> unnecessary, given that broken/workaround/fixed are used for the
> capabilities you introduce later in the series. off/on look much
> better IMHO.

These are booleans so they have to be "on"/"off" anyway... 0/1 doesn't
work. My bad :/ I'll fix the message.

> 
> [...]
> > -static bool spapr_caps_needed(void *opaque)
> > -{
> > -sPAPRMachineState *spapr = opaque;
> > -
> > -return (spapr->forced_caps.mask != 0) || (spapr-
> > >forbidden_caps.mask != 0);
> > -}
> > -
> >  /* This has to be called from the top-level spapr post_load, not
> > the
> >   * caps specific one.  Otherwise it wouldn't be called when the
> > source
> >   * caps are all defaults, which could still conflict with
> > overridden
> >   * caps on the destination */
> >  int spapr_caps_post_migration(sPAPRMachineState *spapr)
> >  {
> > -uint64_t allcaps = 0;
> >  int i;
> >  bool ok = true;
> >  sPAPRCapabilities dstcaps = spapr->effective_caps;
> >  sPAPRCapabilities srccaps;
> >  
> >  srccaps = default_caps_with_cpu(spapr, first_cpu);
> > -srccaps.mask |= spapr->mig_forced_caps.mask;
> > -srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > +for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > +if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > +srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > +}
> > +}
> >  
> > -for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +for (i = 0; i < SPAPR_CAP_NUM; i++) {
> >  sPAPRCapabilityInfo *info = _table[i];
> >  
> > -allcaps |= info->flag;
> > -
> > -if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > >flag)) {
> > -error_report("cap-%s=on in incoming stream, but off in
> > destination",
> > - info->name);
> > +if (srccaps.caps[i] > dstcaps.caps[i]) {
> > +error_report("cap-%s higher level (%d) in incoming
> > stream than on destination (%d)",
> > + info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >  ok = false;
> >  }
> >  
> > -if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > >flag)) {
> > -warn_report("cap-%s=off in incoming stream, but on in
> > destination",
> > - info->name);
> > +if (srccaps.caps[i] < dstcaps.caps[i]) {
> > +warn_report("cap-%s lower level (%d) in incoming
> > stream than on destination (%d)",
> > + info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >  }
> >  }
> 
> These numeric comparisons make me feel very uneasy :)
> 
> What if we need to add more possible values down the line? Should
> there be at least some room between existing values to avoid painting
> ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...
> 
> You clearly know more about the problem than I do, so feel free to
> dismiss all of the above... I thought I would bring up my worries
> just in case :)

For these capabilities I think we're ok to keep it as 0/1/2. In the
event we need a bigger range another capability can be added with other
possible values which was the whole point of introducing this generic
framework. The basic idea is the receiving side must always support a
higher "level" than the source.

With these new capabilities it's more likely we'll have to add an
entirly new one than require more possible values. :)

It could even be possible to have a per capability comparison function
to confirm compatibility in future. But again thats an exercise for
when/if more complex capabilities are added.

> 

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:

Grammar in the subject:

"allow to $VERB" is not idiomatic English; correct is either "allow
${VERB}ing" or "allow $SUBJECT to $VERB".  Concretely, s/to use/using/

> For each Monitor, add one field "use_io_thr" to show whether it will be
> using the dedicated monitor IO thread to handle input/output.  When set,
> monitor IO parsing work will be offloaded to dedicated monitor IO

s/to/to the/

> thread, rather than the original main loop thread.
> 
> This only works for QMP.  HMP will always be run on main loop thread.

s/on/on the/

> 
> Currently we're still keeping use_io_thr to off always.  Will turn it on

s/to off/off/

> later at some point.
> 
> One thing to mention is that we cannot set use_io_thr for every QMP
> monitors.  The problem is that MUXed typed chardevs may not work well

s/monitors/monitor/

> with it now. When MUX is used, frontend of chardev can be the monitor
> plus something else.  The only thing we know would be safe to be run
> outside main thread so far is the monitor frontend. All the rest of the
> frontends should still be run in main thread only.
> 
> Signed-off-by: Peter Xu 
> ---
>  monitor.c | 41 -
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
I have nothing to add on the code review, but it looks like Stefan had
valid comments that need addressing.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:

Grammar in the subject:

s/monitor dedicate/a dedicated monitor/

> Create one IOThread for the monitors, prepared to handle all the
> input/output IOs using existing iothread framework.
> 
> Signed-off-by: Peter Xu 
> ---
>  monitor.c | 29 +
>  1 file changed, 29 insertions(+)
> 

> @@ -207,6 +208,11 @@ struct Monitor {
>  QTAILQ_ENTRY(Monitor) entry;
>  };
>  
> +/* Let's add monitor global variables to this struct. */

Is this comment a placeholder for future additions in later patches of
the series?  If so, a temporary TODO or FIXME might make it easier to
track that this patch is part of a series; if not, you could delete the
comment altogether.

> +static struct {
> +IOThread *mon_iothread;
> +} mon_global;
> +

Up to you what to do about the comment; tweaking it (or leaving it
unchanged) is not a semantic change, so I'm fine with:

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 11/31] hw/arm/exynos4210: use the "samsung, exynos4210-dw-mshc" device

[Alistair Francis]

On Mon, Jan 8, 2018 at 7:42 AM, Philippe Mathieu-Daudé  wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/exynos4210.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index e8e1d81e62..eb95131221 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -75,7 +75,6 @@
>  #define EXYNOS4210_INT_COMBINER_BASE_ADDR   0x10448000
>
>  /* SD/MMC host controllers */
> -#define EXYNOS4210_SDHCI_CAPABILITIES   0x05E80080
>  #define EXYNOS4210_SDHCI_BASE_ADDR  0x1251
>  #define EXYNOS4210_SDHCI_ADDR(n)(EXYNOS4210_SDHCI_BASE_ADDR + \
>  0x0001 * (n))
> @@ -377,13 +376,10 @@ Exynos4210State *exynos4210_init(MemoryRegion 
> *system_mem)
>  BlockBackend *blk;
>  DriveInfo *di;
>
> -dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
> -qdev_prop_set_uint32(dev, "capareg", EXYNOS4210_SDHCI_CAPABILITIES);
> -qdev_init_nofail(dev);
> -
> -busdev = SYS_BUS_DEVICE(dev);
> -sysbus_mmio_map(busdev, 0, EXYNOS4210_SDHCI_ADDR(n));
> -sysbus_connect_irq(busdev, 0, s->irq_table[exynos4210_get_irq(29, 
> n)]);
> +dev = sysbus_create_varargs("samsung,exynos4210-dw-mshc",

Isn't this a legacy function, shouldn't we be moving away from it?

Alistair

> +EXYNOS4210_SDHCI_ADDR(n),
> +s->irq_table[exynos4210_get_irq(29, n)],
> +NULL);
>
>  di = drive_get(IF_SD, 0, n);
>  blk = di ? blk_by_legacy_dinfo(di) : NULL;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v5 10/31] sdhci: add a Designware/Samsung host controller

[Alistair Francis]

On Mon, Jan 8, 2018 at 7:42 AM, Philippe Mathieu-Daudé  wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/dw-mshc.c | 64 
> +
>  hw/sd/Makefile.objs |  1 +
>  2 files changed, 65 insertions(+)
>  create mode 100644 hw/sd/dw-mshc.c
>
> diff --git a/hw/sd/dw-mshc.c b/hw/sd/dw-mshc.c
> new file mode 100644
> index 00..c2869cd569
> --- /dev/null
> +++ b/hw/sd/dw-mshc.c
> @@ -0,0 +1,64 @@
> +/*
> + * Synopsys Designware Mobile Storage Host Controller emulation
> + * (and Samsung Exynos specific extensions)
> + *
> + * Copyright (C) 2018 Philippe Mathieu-Daudé 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "hw/sd/sdhci.h"
> +#include "qapi/error.h"
> +
> +/* Compatible with:
> + * - SD Host Controller Specification Version 2.0
> + * - SDIO Specification Version 2.0
> + * - MMC Specification Version 4.3
> + *
> + * - SDMA
> + * - ADMA
> + */
> +static void exynos4210_dw_mshc_realize(DeviceState *dev, Error **errp)
> +{
> +SDHCICommonClass *cc = SYSBUS_SDHCI_COMMON_GET_CLASS(dev);
> +Object *obj = OBJECT(dev);
> +Error *local_err = NULL;
> +
> +object_property_set_uint(obj, 2, "sd-spec-version", _err);
> +object_property_set_bool(obj, true, "suspend", _err);
> +object_property_set_bool(obj, true, "1v8", _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +cc->parent_realize(dev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}
> +
> +static void exynos4210_dw_mshc_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +SDHCICommonClass *cc = SYSBUS_SDHCI_COMMON_CLASS(klass);
> +
> +cc->parent_realize = dc->realize;

This need an explanation, why are we doing this weird realise logic?

Alistair

> +dc->realize = exynos4210_dw_mshc_realize;
> +}
> +
> +static const TypeInfo exynos4210_dw_mshc_info = {
> +.name = "samsung,exynos4210-dw-mshc",
> +.parent = TYPE_SYSBUS_SDHCI,
> +.class_init = exynos4210_dw_mshc_class_init,
> +};
> +
> +static void dw_mshc_sdhc_register_types(void)
> +{
> +type_register_static(_dw_mshc_info);
> +}
> +
> +type_init(dw_mshc_sdhc_register_types)
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index 0fe2501017..fd866d7f94 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -3,6 +3,7 @@ common-obj-$(CONFIG_SD) += core.o
>
>  # SD/MMC host adapters
>  common-obj-$(CONFIG_PL181) += pl181.o
> +common-obj-$(CONFIG_EXYNOS4) += dw-mshc.o
>  common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> There are many places for monitor init its globals, at least:

Reads awkwardly; maybe:

...many places where the monitor initializes its globals,...

> 
> - monitor_init_qmp_commands() at the very beginning
> - single function to init monitor_lock
> - in the first entry of monitor_init() using "is_first_init"
> 
> Unify them a bit.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Peter Xu 
> ---

> +void monitor_init_globals(void)
> +{
> +monitor_init_qmp_commands();
> +monitor_qapi_event_init();
> +sortcmdlist();
> +qemu_mutex_init(_lock);
> +}
> +
>  /* These functions just adapt the readline interface in a typesafe way.  We
>   * could cast function pointers but that discards compiler checks.
>   */
> @@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt, 
> va_list ap)
>  }
>  }
>  
> -static void __attribute__((constructor)) monitor_lock_init(void)
> -{
> -qemu_mutex_init(_lock);
> -}

The later initialization of the monitor_lock mutex is a potential
semantic change.  Please beef up the commit message to document why it
is safe.  In fact, I requested this back on my review of v1, but it
still hasn't been done. :(

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html

If my read of history is correct, I think it is sufficient to point to
commit 05875687 as a place where we no longer care about constructor
semantics because we are no longer dealing with module_call_init().  But
you may find a better place to point to.  You already found that
d622cb587 was what introduced the constructor in the first place, but I
didn't spend time today auditing the state of qemu back at that time to
see if the constructor was really necessary back then or just a
convenience for lack of a better initialization point.

Alternatively, if you can't find a good commit message to point to, at
least document how you (and I) tested things, using gdb watchpoints, to
prove it is a safe delay.

Only if you improve the commit message, you may add:
Reviewed-by: Eric Blake 

> +++ b/vl.c
> @@ -3099,7 +3099,6 @@ int main(int argc, char **argv, char **envp)
>  qemu_init_exec_dir(argv[0]);
>  
>  module_call_init(MODULE_INIT_QOM);
> -monitor_init_qmp_commands();
>  
>  qemu_add_opts(_drive_opts);
>  qemu_add_drive_opts(_legacy_drive_opts);
> @@ -4645,6 +4644,12 @@ int main(int argc, char **argv, char **envp)
>  default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>  default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>  
> +/*
> + * Note: qtest_enabled() (which used in monitor_qapi_event_init())

s/which/which is/

> + * depend on configure_accelerator() above.

s/depend/depends/

> + */
> +monitor_init_globals();
> +
>  if (qemu_opts_foreach(qemu_find_opts("mon"),
>mon_init_func, NULL, NULL)) {
>  exit(1);
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v6 06/27] monitor: move the cur_mon hack deeper for QMP

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> In monitor_qmp_read(), we have the hack to temporarily replace the
> cur_mon pointer.  Now we move this hack deeper inside the QMP dispatcher
> routine since the Monitor pointer can be actually obtained using
> container_of() upon the parser object, just like most of the other JSON
> parser users do.
> 
> This does not make much sense as a single patch.  However, this will be
> a big step for the next patch, when the QMP dispatcher routine will be
> split from the QMP parser.
> 
> Signed-off-by: Peter Xu 
> ---
>  monitor.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> We can simplify object_property_get_str() using the new
> qobject_get_try_str().
> 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Peter Xu 
> ---
>  qom/object.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake 

I'm not opposed to your patch split (particularly since it makes
backports easier if it just needs the new function and then your later
uses of the new function, without touching existing uses); but I might
have merged this with the previous patch so that the new API has a
client right away, proving why the new API is worthwhile as part of its
introduction.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v6 03/27] qobject: introduce qobject_get_try_str()

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> A quick way to fetch string from qobject when it's a QString.
> 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Peter Xu 
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qstring.c  | 11 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index a145c8ca00..6517d8e377 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, 
> int end);
>  size_t qstring_get_length(const QString *qstring);
>  const char *qstring_get_str(const QString *qstring);
>  const char *qstring_get_try_str(const QString *qstring);
> +const char *qobject_get_try_str(const QObject *qstring);

The naming is consistent, so I won't reject the patch, but 'try_get_str'
reads better than 'get_try_str'.  Of course, fixing the code base to
read well AND be consistent is a much bigger task, and I'm not asking
you to tackle it.


>  
> +/**
> + * qobject_get_try_str(): Return a pointer of the backstore string

The word "backstore" doesn't appear anywhere in qemu.git, and flags as a
typo.  I'd prefer:

Return a pointer to the corresponding string

or maybe "backing string"

> + *
> + * NOTE: the string will only be returned if the object is valid, and
> + * its type is QString, otherwise NULL is returned.
> + */
> +const char *qobject_get_try_str(const QObject *qstring)
> +{
> +return qstring_get_try_str(qobject_to_qstring(qstring));
> +}
> +
>  /**
>   * qstring_is_equal(): Test whether the two QStrings are equal
>   */
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC v6 02/27] qobject: introduce qstring_get_try_str()

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> The only difference from qstring_get_str() is that it allows the qstring
> to be NULL.  If so, NULL is returned.
> 
> CC: Eric Blake 
> CC: Markus Armbruster 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Peter Xu 
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qstring.c  | 10 ++
>  2 files changed, 11 insertions(+)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] maint: Mention web site maintenance in README

[Eric Blake]

On 11/29/2017 09:25 AM, Eric Blake wrote:
> Now that we have a website that accepts patches on the list, the
> main project should make it easier to find information about that
> process.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> Doc only, so it could go in -rc3 if we have a reason to slip it
> in this late; but I'm also fine if it waits for 2.12.

It's now 2.12 material. Ping!
CC: qemu-trivial

> 
>  README | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/README b/README
> index b92a07a61a..2c8e1c8cc4 100644
> --- a/README
> +++ b/README
> @@ -68,6 +68,10 @@ the QEMU website
>https://qemu.org/Contribute/SubmitAPatch
>https://qemu.org/Contribute/TrivialPatches
> 
> +The QEMU website is also maintained under source control.
> +
> +  git clone git://git.qemu.org/qemu-web.git
> +  https://www.qemu.org/2017/02/04/the-new-qemu-website-is-up/
> 
>  Bug reporting
>  =
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL] qemu-sparc updates

[Mark Cave-Ayland]

On 09/01/18 18:22, Peter Maydell wrote:


The following changes since commit 4124ea4f5bd367ca6412fb2dfe7ac4d80e1504d9:

   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20171229' into 
staging (2018-01-08 16:17:04 +)

are available in the git repository at:

   https://github.com/mcayland/qemu.git tags/qemu-sparc-signed

for you to fetch changes up to 6a52624720e5abc6a1f067a7e7b8239b428e0c95:

   sun4u_iommu: add trace event for IOMMU translations (2018-01-08 19:07:55 
+)


qemu-sparc update




Hi. This seems to crash in 'make check'. One of the crashes has a
memory corruption splat:

TEST: tests/device-introspect-test... (pid=20423)
   /sparc64/device/introspect/list: OK
   /sparc64/device/introspect/list-fields:  OK
   /sparc64/device/introspect/none: OK
   /sparc64/device/introspect/abstract: OK
   /sparc64/device/introspect/concrete:
*** Error in `sparc64-softmmu/qemu-system-spar
c64': corrupted double-linked list (not small): 0x010033b823a0 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb0b94)[0x3fff90ce0b94]
/lib64/libc.so.6(+0xb5b18)[0x3fff90ce5b18]
/lib64/libc.so.6(__libc_calloc-0x14b664)[0x3fff90ce9934]
/lib64/libglib-2.0.so.0(g_malloc0-0x100d54)[0x3fff97a634d4]
sparc64-softmmu/qemu-system-sparc64[0x1030a9bc]
sparc64-softmmu/qemu-system-sparc64[0x103062c8]
sparc64-softmmu/qemu-system-sparc64[0x103062a0]

Running it under valgrind with
QTEST_QEMU_BINARY='valgrind sparc64-softmmu/qemu-system-sparc64'
./tests/device-introspect-test -p /sparc64/device/introspect/concrete

gives this write-after-free:

==1931== Invalid write of size 8
==1931==at 0x55EA51: pci_host_bus_register (pci.c:331)
==1931==by 0x55ECBD: pci_bus_init (pci.c:393)
==1931==by 0x55EE18: pci_bus_new (pci.c:424)
==1931==by 0x55EEE2: pci_register_bus (pci.c:447)
==1931==by 0x55D14F: pci_pbm_init (apb.c:464)
==1931==by 0x69179B: object_init_with_type (object.c:353)
==1931==by 0x6919D0: object_initialize_with_type (object.c:384)
==1931==by 0x691E3B: object_new_with_type (object.c:492)
==1931==by 0x691E78: object_new (object.c:502)
==1931==by 0x479A3C: qmp_device_list_properties (qmp.c:537)
==1931==by 0x455479: qdev_device_help (qdev-monitor.c:279)
==1931==by 0x456C9E: qmp_device_add (qdev-monitor.c:802)
==1931==  Address 0x2ca7af08 is 1,528 bytes inside a block of size 3,312 free'd
==1931==at 0x4C2EDEB: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1931==by 0x691DC6: object_finalize (object.c:480)
==1931==by 0x692CBD: object_unref (object.c:911)
==1931==by 0x479B91: qmp_device_list_properties (qmp.c:572)
==1931==by 0x469EA0: qmp_marshal_device_list_properties (qmp-marshal.c:1393)
==1931==by 0x7A25D2: do_qmp_dispatch (qmp-dispatch.c:104)
==1931==by 0x7A2703: qmp_dispatch (qmp-dispatch.c:131)
==1931==by 0x39E36D: handle_qmp_command (monitor.c:3839)
==1931==by 0x7AA357: json_message_process_token (json-streamer.c:105)
==1931==by 0x7D70CB: json_lexer_feed_char (json-lexer.c:323)
==1931==by 0x7D7213: json_lexer_feed (json-lexer.c:373)
==1931==by 0x7AA3FE: json_message_parser_feed (json-streamer.c:124)
==1931==  Block was alloc'd at
==1931==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1931==by 0x1C004718: g_malloc (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==1931==by 0x691E1C: object_new_with_type (object.c:491)
==1931==by 0x691E78: object_new (object.c:502)
==1931==by 0x479A3C: qmp_device_list_properties (qmp.c:537)
==1931==by 0x469EA0: qmp_marshal_device_list_properties (qmp-marshal.c:1393)
==1931==by 0x7A25D2: do_qmp_dispatch (qmp-dispatch.c:104)
==1931==by 0x7A2703: qmp_dispatch (qmp-dispatch.c:131)
==1931==by 0x39E36D: handle_qmp_command (monitor.c:3839)
==1931==by 0x7AA357: json_message_process_token (json-streamer.c:105)
==1931==by 0x7D70CB: json_lexer_feed_char (json-lexer.c:323)
==1931==by 0x7D7213: json_lexer_feed (json-lexer.c:373)


Thanks for the hint - while it didn't crash locally, I was certainly 
able to reproduce the above trace in valgrind.


Turns out the issue was that thought I could move pci_register_bus() 
from realize to init in patch 10, but evidently not :)


I've moved it back and repushed the signed tag if you can try and apply 
the PR once again?



Many thanks,

Mark.

Re: [Qemu-devel] [PATCH v5 26/31] sdhci: remove the deprecated 'capareg' property

[Alistair Francis]

On Mon, Jan 8, 2018 at 7:42 AM, Philippe Mathieu-Daudé  wrote:
> All SDHCI consumers have been upgraded to set correct properties.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/sd/sdhci.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 34bda73b66..ae914a6ef9 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1194,9 +1194,7 @@ static void sdhci_init_readonly_registers(SDHCIState 
> *s, Error **errp)
>  }
>  s->version = (SDHC_HCVER_VENDOR << 8) | (s->spec_version - 1);
>
> -if (s->capareg == UINT64_MAX) {
> -sdhci_init_capareg(s, errp);
> -}
> +sdhci_init_capareg(s, errp);
>  }
>
>  static void sdhci_initfn(SDHCIState *s)
> @@ -1345,9 +1343,6 @@ static Property sdhci_properties[] = {
>  DEFINE_PROP_UINT8("bus-speed", SDHCIState, cap.sdr, 0),
>  DEFINE_PROP_UINT8("driver-strength", SDHCIState, cap.strength, 0),
>
> -/* capareg: deprecated */
> -DEFINE_PROP_UINT64("capareg", SDHCIState, capareg, UINT64_MAX),
> -
>  DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
>  DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, 
> pending_insert_quirk,
>   false),
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v5 09/31] sdhci: add a common class

[Alistair Francis]

On Mon, Jan 8, 2018 at 7:42 AM, Philippe Mathieu-Daudé  wrote:
> so this class can be inherited.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  include/hw/sd/sdhci.h | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 4a9c3e9175..a80b7c0424 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -124,4 +124,16 @@ typedef struct SDHCIState {
>  #define SYSBUS_SDHCI(obj)   \
>   OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
>
> +typedef struct {
> +/*< private >*/
> +BusClass parent_class;
> +/*< public >*/
> +DeviceRealize parent_realize;
> +} SDHCICommonClass;
> +
> +#define SYSBUS_SDHCI_COMMON_CLASS(klass) \
> +OBJECT_CLASS_CHECK(SDHCICommonClass, (klass), TYPE_SYSBUS_SDHCI)
> +#define SYSBUS_SDHCI_COMMON_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(SDHCICommonClass, (obj), TYPE_SYSBUS_SDHCI)
> +
>  #endif /* SDHCI_H */
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v5 05/31] sdhci: add DMA and 64-bit capabilities (Spec v2)

[Alistair Francis]

On Mon, Jan 8, 2018 at 7:42 AM, Philippe Mathieu-Daudé  wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sdhci-internal.h | 14 +++---
>  include/hw/sd/sdhci.h  |  4 
>  hw/sd/sdhci.c  | 40 ++--
>  3 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 0561e6eaf7..affbe4015c 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -89,12 +89,12 @@ FIELD(SDHC_PRNSTS, WRITE_PROTECT,  19, 1);
>  FIELD(SDHC_HOSTCTL, LED_CTRL,  0, 1);
>  FIELD(SDHC_HOSTCTL, DATATRANSFERWIDTH, 1, 1); /* SD mode only */
>  FIELD(SDHC_HOSTCTL, HIGH_SPEED,2, 1);
> -#define SDHC_CTRL_DMA_CHECK_MASK   0x18
> +FIELD(SDHC_HOSTCTL, DMA,   3, 2);
>  #define SDHC_CTRL_SDMA 0x00
> -#define SDHC_CTRL_ADMA1_32 0x08
> +#define SDHC_CTRL_ADMA1_32 0x08 /* NOT ALLOWED since v2 */
>  #define SDHC_CTRL_ADMA2_32 0x10
> -#define SDHC_CTRL_ADMA2_64 0x18
> -#define SDHC_DMA_TYPE(x)   ((x) & SDHC_CTRL_DMA_CHECK_MASK)
> +#define SDHC_CTRL_ADMA2_64 0x18 /* only v1 & v2 (v3 optional) */
> +#define SDHC_DMA_TYPE(x)   ((x) & R_SDHC_HOSTCTL_DMA_MASK)
>
>  /* R/W Power Control Register 0x0 */
>  #define SDHC_PWRCON0x29
> @@ -185,19 +185,19 @@ FIELD(SDHC_ACMD12ERRSTS, INDEX_ERR,4, 1);
>
>  /* HWInit Capabilities Register 0x05E80080 */
>  #define SDHC_CAPAB 0x40
> -#define SDHC_CAN_DO_ADMA2  0x0008
> -#define SDHC_CAN_DO_ADMA1  0x0010
> -#define SDHC_64_BIT_BUS_SUPPORT(1 << 28)
>  FIELD(SDHC_CAPAB, TOCLKFREQ,   0, 6);
>  FIELD(SDHC_CAPAB, TOUNIT,  7, 1);
>  FIELD(SDHC_CAPAB, BASECLKFREQ, 8, 8);
>  FIELD(SDHC_CAPAB, MAXBLOCKLENGTH, 16, 2);
> +FIELD(SDHC_CAPAB, ADMA2,  19, 1); /* since v2 */
> +FIELD(SDHC_CAPAB, ADMA1,  20, 1); /* v1 only? */
>  FIELD(SDHC_CAPAB, HIGHSPEED,  21, 1);
>  FIELD(SDHC_CAPAB, SDMA,   22, 1);
>  FIELD(SDHC_CAPAB, SUSPRESUME, 23, 1);
>  FIELD(SDHC_CAPAB, V33,24, 1);
>  FIELD(SDHC_CAPAB, V30,25, 1);
>  FIELD(SDHC_CAPAB, V18,26, 1);
> +FIELD(SDHC_CAPAB, BUS64BIT,   28, 1); /* since v2 */
>
>  /* HWInit Maximum Current Capabilities Register 0x0 */
>  #define SDHC_MAXCURR   0x48
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index c1602becd2..4a9c3e9175 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -103,6 +103,7 @@ typedef struct SDHCIState {
>  bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
>  uint8_t spec_version;
>  struct {
> +/* v1 */
>  uint8_t timeout_clk_freq, base_clk_freq_mhz;
>  bool timeout_clk_in_mhz;
>  uint16_t max_blk_len;
> @@ -110,6 +111,9 @@ typedef struct SDHCIState {
>  bool high_speed;
>  bool sdma;
>  bool v33, v30, v18;
> +/* v2 */
> +bool adma1, adma2;
> +bool bus64;
>  } cap;
>  } SDHCIState;
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 05681c86d6..56466e0427 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -38,24 +38,6 @@
>  #define TYPE_SDHCI_BUS "sdhci-bus"
>  #define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SDHCI_BUS)
>
> -/* Default SD/MMC host controller features information, which will be
> - * presented in CAPABILITIES register of generic SD host controller at reset.
> - * If not stated otherwise:
> - * 0 - not supported, 1 - supported, other - prohibited.
> - */
> -#define SDHC_CAPAB_64BITBUS   0ul/* 64-bit System Bus Support */
> -#define SDHC_CAPAB_ADMA1  1ul/* ADMA1 support */
> -#define SDHC_CAPAB_ADMA2  1ul/* ADMA2 support */
> -
> -/* Now check all parameters and calculate CAPABILITIES REGISTER value */
> -#if SDHC_CAPAB_64BITBUS > 1 || SDHC_CAPAB_ADMA2 > 1 || SDHC_CAPAB_ADMA1 > 1
> -#error Capabilities features can have value 0 or 1 only!
> -#endif
> -
> -#define SDHC_CAPAB_REG_DEFAULT \
> -   ((SDHC_CAPAB_64BITBUS << 28) | (SDHC_CAPAB_ADMA1 << 20) |   \
> -(SDHC_CAPAB_ADMA2 << 19))
> -
>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>
>  static void sdhci_check_capab_freq_range(SDHCIState *s, const char *desc,
> @@ -71,12 +53,22 @@ static void sdhci_check_capab_freq_range(SDHCIState *s, 
> const char *desc,
>  }
>  }
>
> +/* Default SD/MMC host controller features information, which will be
> + * presented in CAPABILITIES register of generic SD host controller at 
> reset. */
>  static void sdhci_init_capareg(SDHCIState *s, Error **errp)
>  {
>  uint64_t capareg = 0;
>  uint32_t val;
>
>  switch (s->spec_version) {
> +/* fallback */

This 

Re: [Qemu-devel] Call for GSoC & Outreachy 2018 mentors & project ideas

[Alistair Francis]

On Tue, Jan 2, 2018 at 12:23 PM, Stefan Hajnoczi  wrote:
> QEMU will apply to the Google Summer of Code
> (https://summerofcode.withgoogle.com/) and Outreachy
> (https://www.outreachy.org/) open source internship programs again
> this year.
>
> Do you want to mentor newcomers beginning open source development in
> our community?
>
> Please post your project ideas on the wiki by January 23rd:
> https://wiki.qemu.org/Google_Summer_of_Code_2018
>
> Project ideas must:
>  * Be doable in 12 weeks by someone fluent in C programming but not
> familiar with the codebase.
>  * Have a clear scope and few dependencies.
>  * Have a high chance of being merged.
>  * Consist of smaller steps that can be merged incrementally.
>
> Active QEMU, KVM, and Jailhouse contributors are invited to become
> mentors.  Mentoring is a volunteer activity that requires around 5
> hours per week to interact with your intern, review code, etc.  GSoC
> and Outreachy internships run from May through August.
>
> For background on why QEMU participates in open source internship
> programs and how it works, check out my KVM Forum presentation:
> https://www.youtube.com/watch?v=xNVCX7YMUL8
> https://vmsplice.net/~stefan/stefanha-kvm-forum-2016.pdf
>
> KVM and Jailhouse are invited to participate under the QEMU GSoC
> umbrella organization.
>
> Please let me know if you have any questions!

Hey,

Can anyone who has done this before chime in.

What do you think about getting someone to cleanup and improve the GDB
support in QEMU? Would that be the right difficulty of task for a GSoC
project?

Alistair

>
> Stefan
>

Re: [Qemu-devel] [PATCH 1/2] hw/intc/arm_gicv3: Make reserved register addresses RAZ/WI

[Alistair Francis]

On Wed, Dec 13, 2017 at 8:52 AM, Peter Maydell  wrote:
> The GICv3 specification says that reserved register addresses
> should RAZ/WI. This means we need to return MEMTX_OK, not MEMTX_ERROR,
> because now that we support generating external aborts the
> latter will cause an abort on new board models.
>
> Signed-off-by: Peter Maydell 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/intc/arm_gicv3_dist.c   | 13 +
>  hw/intc/arm_gicv3_its_common.c |  8 +++-
>  hw/intc/arm_gicv3_redist.c | 13 +
>  3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
> index 3ea3dd0..93fe936 100644
> --- a/hw/intc/arm_gicv3_dist.c
> +++ b/hw/intc/arm_gicv3_dist.c
> @@ -817,6 +817,13 @@ MemTxResult gicv3_dist_read(void *opaque, hwaddr offset, 
> uint64_t *data,
>"%s: invalid guest read at offset " TARGET_FMT_plx
>"size %u\n", __func__, offset, size);
>  trace_gicv3_dist_badread(offset, size, attrs.secure);
> +/* The spec requires that reserved registers are RAZ/WI;
> + * so use MEMTX_ERROR returns from leaf functions as a way to
> + * trigger the guest-error logging but don't return it to
> + * the caller, or we'll cause a spurious guest data abort.
> + */
> +r = MEMTX_OK;
> +*data = 0;
>  } else {
>  trace_gicv3_dist_read(offset, *data, size, attrs.secure);
>  }
> @@ -852,6 +859,12 @@ MemTxResult gicv3_dist_write(void *opaque, hwaddr 
> offset, uint64_t data,
>"%s: invalid guest write at offset " TARGET_FMT_plx
>"size %u\n", __func__, offset, size);
>  trace_gicv3_dist_badwrite(offset, data, size, attrs.secure);
> +/* The spec requires that reserved registers are RAZ/WI;
> + * so use MEMTX_ERROR returns from leaf functions as a way to
> + * trigger the guest-error logging but don't return it to
> + * the caller, or we'll cause a spurious guest data abort.
> + */
> +r = MEMTX_OK;
>  } else {
>  trace_gicv3_dist_write(offset, data, size, attrs.secure);
>  }
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 2bd2f0f..284c0a7 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -67,7 +67,8 @@ static MemTxResult gicv3_its_trans_read(void *opaque, 
> hwaddr offset,
>  MemTxAttrs attrs)
>  {
>  qemu_log_mask(LOG_GUEST_ERROR, "ITS read at offset 0x%"PRIx64"\n", 
> offset);
> -return MEMTX_ERROR;
> +*data = 0;
> +return MEMTX_OK;
>  }
>
>  static MemTxResult gicv3_its_trans_write(void *opaque, hwaddr offset,
> @@ -82,15 +83,12 @@ static MemTxResult gicv3_its_trans_write(void *opaque, 
> hwaddr offset,
>  if (ret <= 0) {
>  qemu_log_mask(LOG_GUEST_ERROR,
>"ITS: Error sending MSI: %s\n", strerror(-ret));
> -return MEMTX_DECODE_ERROR;
>  }
> -
> -return MEMTX_OK;
>  } else {
>  qemu_log_mask(LOG_GUEST_ERROR,
>"ITS write at bad offset 0x%"PRIx64"\n", offset);
> -return MEMTX_DECODE_ERROR;
>  }
> +return MEMTX_OK;
>  }
>
>  static const MemoryRegionOps gicv3_its_trans_ops = {
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 77e5cfa..8a8684d 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -455,6 +455,13 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr 
> offset, uint64_t *data,
>"size %u\n", __func__, offset, size);
>  trace_gicv3_redist_badread(gicv3_redist_affid(cs), offset,
> size, attrs.secure);
> +/* The spec requires that reserved registers are RAZ/WI;
> + * so use MEMTX_ERROR returns from leaf functions as a way to
> + * trigger the guest-error logging but don't return it to
> + * the caller, or we'll cause a spurious guest data abort.
> + */
> +r = MEMTX_OK;
> +*data = 0;
>  } else {
>  trace_gicv3_redist_read(gicv3_redist_affid(cs), offset, *data,
>  size, attrs.secure);
> @@ -505,6 +512,12 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr 
> offset, uint64_t data,
>"size %u\n", __func__, offset, size);
>  trace_gicv3_redist_badwrite(gicv3_redist_affid(cs), offset, data,
>  size, attrs.secure);
> +/* The spec requires that reserved registers are RAZ/WI;
> + * so use MEMTX_ERROR returns from leaf functions as a way to
> + * trigger the guest-error logging but don't return it to
> + * the caller, or we'll cause a 

Re: [Qemu-devel] [PATCH 2/2] hw/intc/arm_gic: reserved register addresses are RAZ/WI

[Alistair Francis]

On Wed, Dec 13, 2017 at 8:52 AM, Peter Maydell  wrote:
> The GICv2 specification says that reserved register addresses
> must RAZ/WI; now that we implement external abort handling
> for Arm CPUs this means we must return MEMTX_OK rather than
> MEMTX_ERROR, to avoid generating a spurious guest data abort.
>
> Signed-off-by: Peter Maydell 

Looks good to me.

Reviewed-by: Alistair Francis 

Alistair


> ---
>  hw/intc/arm_gic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 5a0e2a3..d701e49 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1261,7 +1261,8 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, 
> int offset,
>  default:
>  qemu_log_mask(LOG_GUEST_ERROR,
>"gic_cpu_read: Bad offset %x\n", (int)offset);
> -return MEMTX_ERROR;
> +*data = 0;
> +break;
>  }
>  return MEMTX_OK;
>  }
> @@ -1329,7 +1330,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, 
> int offset,
>  default:
>  qemu_log_mask(LOG_GUEST_ERROR,
>"gic_cpu_write: Bad offset %x\n", (int)offset);
> -return MEMTX_ERROR;
> +return MEMTX_OK;
>  }
>  gic_update(s);
>  return MEMTX_OK;
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH v1 02/21] RISC-V ELF Machine Definition

[Alistair Francis]

On Tue, Jan 2, 2018 at 4:44 PM, Michael Clark  wrote:
> Define RISC-V ELF machine EM_RISCV 243
>
> Signed-off-by: Michael Clark 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  include/elf.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/elf.h b/include/elf.h
> index e8a515c..8e457fc 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -112,6 +112,8 @@ typedef int64_t  Elf64_Sxword;
>
>  #define EM_UNICORE32110 /* UniCore32 */
>
> +#define EM_RISCV243 /* RISC-V */
> +
>  /*
>   * This is an interim value that we will use until the committee comes
>   * up with a final number.
> --
> 2.7.0
>
>

Re: [Qemu-devel] [RFC PATCH 12/12] slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:29:04 -0300, wrote:
> Access struct in6_addr with 'void *', then cast to 'u8 *' to avoid alignment
> issues.

Err, I don't understand the point. in6_addr's s6_addr is already a
uint8_t by the standard.  There is no non-byte access in the existing
code.

Samuel

Re: [Qemu-devel] [PATCH v1 01/21] RISC-V Maintainers

[Alistair Francis]

On Tue, Jan 2, 2018 at 4:44 PM, Michael Clark  wrote:
> Add Michael Clark, Sagar Karandikar and Bastian Koppelmann as
> RISC-V Maintainers.
>
> Signed-off-by: Michael Clark 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  MAINTAINERS | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73a..09a1314 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -209,6 +209,16 @@ F: hw/ppc/
>  F: include/hw/ppc/
>  F: disas/ppc.c
>
> +RISC-V
> +M: Michael Clark 
> +M: Sagar Karandikar 
> +M: Bastian Koppelmann 
> +S: Maintained
> +F: target/riscv/
> +F: hw/riscv/
> +F: include/hw/riscv/
> +F: disas/riscv.c
> +
>  S390
>  M: Richard Henderson 
>  M: Alexander Graf 
> --
> 2.7.0
>
>

Re: [Qemu-devel] [PATCH 09/12] slirp: add in6_dhcp_multicast()

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:29:01 -0300, wrote:
> Signed-off-by: Philippe Mathieu-Daudé 

Applied to my tree.

Thanks,
Samuel

> ---
>  slirp/dhcpv6.h | 3 +++
>  slirp/udp6.c   | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/dhcpv6.h b/slirp/dhcpv6.h
> index 9189cd3f2d..3373f6cb89 100644
> --- a/slirp/dhcpv6.h
> +++ b/slirp/dhcpv6.h
> @@ -17,6 +17,9 @@
>  0x00, 0x00, 0x00, 0x00,\
>  0x00, 0x01, 0x00, 0x02 } }
>  
> +#define in6_dhcp_multicast(a)\
> +in6_equal(a, &(struct in6_addr)ALLDHCP_MULTICAST)
> +
>  void dhcpv6_input(struct sockaddr_in6 *srcsas, struct mbuf *m);
>  
>  #endif
> diff --git a/slirp/udp6.c b/slirp/udp6.c
> index 9fa314bc2d..7c4a6b003a 100644
> --- a/slirp/udp6.c
> +++ b/slirp/udp6.c
> @@ -65,7 +65,7 @@ void udp6_input(struct mbuf *m)
>  /* handle DHCPv6 */
>  if (ntohs(uh->uh_dport) == DHCPV6_SERVER_PORT &&
>  (in6_equal(>ip_dst, >vhost_addr6) ||
> - in6_equal(>ip_dst, &(struct in6_addr)ALLDHCP_MULTICAST))) {
> + in6_dhcp_multicast(>ip_dst))) {
>  m->m_data += iphlen;
>  m->m_len -= iphlen;
>  dhcpv6_input(, m);
> -- 
> 2.15.1
> 

-- 
Samuel
 Créer une hiérarchie supplementaire pour remedier à un problème (?) de
 dispersion est d'une logique digne des Shadocks.
 * BT in: Guide du Cabaliste Usenet - La Cabale vote oui (les Shadocks aussi) *

Re: [Qemu-devel] [PATCH 08/12] slirp: removed unused code

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 20:02:14 +0100, wrote:
> On 08.01.2018 18:29, Philippe Mathieu-Daudé wrote:
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  slirp/ip.h | 13 -
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/slirp/ip.h b/slirp/ip.h
> > index e29ccd3c9f..71c3642cfe 100644
> > --- a/slirp/ip.h
> > +++ b/slirp/ip.h
> > @@ -233,17 +233,4 @@ struct ipasfrag {
> >  #define ipf_next ipf_link.next
> >  #define ipf_prev ipf_link.prev
> >  
> > -/*
> > - * Structure stored in mbuf in inpcb.ip_options
> > - * and passed to ip_output when ip options are in use.
> > - * The actual length of the options (including ipopt_dst)
> > - * is in m_len.
> > - */
> > -#define MAX_IPOPTLEN   40
> > -
> > -struct ipoption {
> > -   struct  in_addr ipopt_dst;  /* first-hop dst if source routed */
> > -   int8_t  ipopt_list[MAX_IPOPTLEN];   /* options proper */
> > -} QEMU_PACKED;
> > -
> >  #endif
> > 
> 
> Reviewed-by: Thomas Huth 

Applied to my tree.

Thanks,
Samuel

Re: [Qemu-devel] [PATCH 07/12] slirp: remove unnecessary

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 20:01:10 +0100, wrote:
> The subject is missing a word or two.

Applied to my tree with a more complete subject :)

Samuel

Re: [Qemu-devel] [PATCH 06/12] slirp: remove unused header

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 19:54:27 +0100, wrote:
> On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  slirp/slirp.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/slirp/slirp.h b/slirp/slirp.h
> > index 9a7287e7cc..447dc045a8 100644
> > --- a/slirp/slirp.h
> > +++ b/slirp/slirp.h
> > @@ -1,7 +1,6 @@
> >  #ifndef SLIRP_H
> >  #define SLIRP_H
> >  
> > -#include "qemu/host-utils.h"
> 
> This had been added by commit 87776ab72b02e3c99a042ab7a0a378bc457cc069
> which stated "There are some inclusions of qemu/host-utils.h in headers,
> but they are all necessary." ... I wonder why it is not necessary
> anymore today...?
> 
> Anyway, seems like it compiles now fine without this:
> 
> Tested-by: Thomas Huth 

Applied to my tree, thanks!

Samuel

Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Samuel Thibault]

Richard Henderson, on mar. 09 janv. 2018 08:33:22 -0800, wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> > Host: Mac OS 10.12.5
> > Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> > 
> >   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' 
> > of class or
> > structure 'ip6' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >   if (IN6_IS_ADDR_MULTICAST(>ip_src) ||
> >  ^~
> >   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 
> > 'IN6_IS_ADDR_MULTICAST'
> >   #define IN6_IS_ADDR_MULTICAST(a)((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

The compiler could be smarter to realize that it's actually a byte access 
indeed.

There are two things for me:

- The OS-provided implementation of IN6_IS_ADDR_MULTICAST could be doing
  32bit accesses and whatnot, so we are not supposed to use it on packed
  fields.

- With the proposal patch, the compiler could still emit the warning,
  since we are basically still passing the address of the packed field
  to a function which the compiler might not see either.  We are however
  sure that the function makes an aligned access.

So I'd say we should still rather do it to be on the safe side. With
Thomas Huth's comments applied.

BTW,

Thomas Huth  wrote:
> I think this might also be a bug in the macro definitions.

> > > #define IN6_IS_ADDR_MULTICAST(a)((a)->s6_addr[0] == 0xff)

> On Linux, it is defined like this:
> 
> #define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

Well, the standard says that IN6_IS_ADDR_MULTICAST takes a struct
in6_addr*, so it can use the s6_addr field.

Samuel

Re: [Qemu-devel] [PATCH 03/12] slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:28:55 -0300, wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:80:38: warning: taking address of packed member 'ip_src' 
> of class or
> structure 'ip6' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>   IN6_IS_ADDR_UNSPECIFIED(>ip_src)) {
>^~
>   /usr/include/netinet6/in6.h:238:42: note: expanded from macro 
> 'IN6_IS_ADDR_UNSPECIFIED'
>   ((*(const __uint32_t *)(const void *)(&(a)->s6_addr[0]) == 0) && \

I have applied it to my tree.

Thanks,
Samuel

Re: [Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:28:53 -0300, wrote:
>  struct mbuf_ptr {
>   struct mbuf *mptr;
>   uint32_t dummy;
> -} QEMU_PACKED;
> +};
>  #else
>  struct mbuf_ptr {
>   struct mbuf *mptr;
> -} QEMU_PACKED;
> +};

> @@ -199,7 +199,7 @@ struct ipovly {
>   uint16_tih_len; /* protocol length */
>   struct  in_addr ih_src; /* source internet address */
>   struct  in_addr ih_dst; /* destination internet address */
> -} QEMU_PACKED;
> +};

Did you actually try to change these structures?  The presence of the
"dummy" field should have hinted that it's not a trivial structure :)

mbuf_ptr is used in struct ipovly which is to have the same size as the
ipv4 header.  One would have to untangle that before removing the packed
attribute.

> @@ -215,7 +215,7 @@ struct ipq {
>   uint8_t ipq_p;  /* protocol of this fragment */
>   uint16_tipq_id; /* sequence id for reassembly */
>   struct  in_addr ipq_src,ipq_dst;
> -} QEMU_PACKED;
> +};

This one seems safe to me. I'd still rather see an actual test with
added holes in the structure to be sure :)

> @@ -225,7 +225,7 @@ struct ipq {
>  struct   ipasfrag {
>   struct qlink ipf_link;
>   struct ip ipf_ip;
> -} QEMU_PACKED;
> +};

Please see iptofrag and fragtoip in ip_input.c, they assume that ipf_ip
immediately follows ipf_link.  I guess using offsetof there should avoid
the issue.  Note however that slirp assumes that in a 32bit-aligned
ethernet frame it has enough room before the IP header to stuff the
ipf_link things.  We could add a build-time check that offsetof(ipf_ip)
<= 2 + ETH_HLEN

> @@ -136,7 +136,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>  struct ndpentry {
>  unsigned char   eth_addr[ETH_ALEN]; /* sender hardware address */
>  struct in6_addr ip_addr;/* sender IP address   */
> -} QEMU_PACKED;
> +};

This one should be safe.

Samuel

Re: [Qemu-devel] [PATCH v2 0/7] macro do/while (0) cleanup

[Eric Blake]

ping

On 12/01/2017 05:24 PM, Eric Blake wrote:
> Noticed this by chance in the tests/ directory, so I broadened
> it to a grep of the entire code base.  I suspect^wKNOW many of
> the bad macros were the victims of copy-and-paste from some
> other bad location (particularly given how many bit-rotten
> debug print macros were involved).
> 
> https://wiki.qemu.org/BiteSizedTasks#Bitrot_prevention is still
> left for someone else, for another day.
> 
> In v2:
> - add review tags
> - audit that the maint patch has no semantic change (all callers
> were indeed supplying ';' at the macro call-sites)
> - improved commit messages
> - more patches for some additional cleanups
> - tighten the checkpatch rule to also flag 'while (false);'
> 
> Eric Blake (7):
>   net: Drop unusual use of do { } while (0);
>   mips: Tweak location of ';' in macros
>   chardev: Use goto/label instead of do/break/while(0)
>   chardev: Clean up previous patch indentation
>   tests: Avoid 'do/while(false);' in vhost-user-bridge
>   maint: Fix macros with broken 'do/while(0);' usage
>   checkpatch: Enforce proper do/while (0) style
> 
>  tests/acpi-utils.h |  8 ++---
>  ui/sdl_zoom_template.h |  8 ++---
>  audio/paaudio.c|  4 +--
>  chardev/char-serial.c  | 75 
> --
>  hw/adc/stm32f2xx_adc.c |  2 +-
>  hw/block/m25p80.c  |  2 +-
>  hw/char/cadence_uart.c |  2 +-
>  hw/char/stm32f2xx_usart.c  |  2 +-
>  hw/display/cg3.c   |  2 +-
>  hw/display/dpcd.c  |  2 +-
>  hw/display/xlnx_dp.c   |  2 +-
>  hw/dma/pl330.c |  2 +-
>  hw/dma/xlnx-zynq-devcfg.c  |  2 +-
>  hw/dma/xlnx_dpdma.c|  2 +-
>  hw/i2c/i2c-ddc.c   |  2 +-
>  hw/misc/auxbus.c   |  2 +-
>  hw/misc/macio/mac_dbdma.c  |  4 +--
>  hw/misc/mmio_interface.c   |  2 +-
>  hw/misc/stm32f2xx_syscfg.c |  2 +-
>  hw/misc/zynq_slcr.c|  2 +-
>  hw/net/cadence_gem.c   |  2 +-
>  hw/net/pcnet.c | 20 ++---
>  hw/ssi/mss-spi.c   |  2 +-
>  hw/ssi/stm32f2xx_spi.c |  2 +-
>  hw/ssi/xilinx_spi.c|  2 +-
>  hw/ssi/xilinx_spips.c  |  2 +-
>  hw/timer/a9gtimer.c|  2 +-
>  hw/timer/cadence_ttc.c |  2 +-
>  hw/timer/mss-timer.c   |  2 +-
>  hw/timer/stm32f2xx_timer.c |  2 +-
>  hw/tpm/tpm_passthrough.c   |  2 +-
>  hw/tpm/tpm_tis.c   |  2 +-
>  migration/rdma.c   |  2 +-
>  target/arm/translate-a64.c |  2 +-
>  target/mips/msa_helper.c   | 34 +++--
>  target/s390x/kvm.c |  2 +-
>  tests/tcg/test-mmap.c  |  2 +-
>  tests/vhost-user-bridge.c  |  6 ++--
>  scripts/checkpatch.pl  |  5 
>  39 files changed, 119 insertions(+), 105 deletions(-)
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Eric Blake]

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/201 | 130 
> +
>  tests/qemu-iotests/201.out |   5 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 136 insertions(+)
>  create mode 100644 tests/qemu-iotests/201
>  create mode 100644 tests/qemu-iotests/201.out
> 

pep8 complains:

$ pep8 tests/qemu-iotests/201
tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1
tests/qemu-iotests/201:68:17: E128 continuation line under-indented for
visual indent
tests/qemu-iotests/201:69:17: E128 continuation line under-indented for
visual indent

I don't mind cleaning those up (if you don't have to respin because of
my comments on 5/6).

> +import os
> +import sys
> +import iotests
> +import time
> +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive
> +
> +nbd_port = '10900'
> +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp'

Is this still going to be safe when we improve the testsuite to run
multiple tests in parallel?  Wouldn't it be safer to dynamically choose
the port, instead of hard-coding one?

> +disk = os.path.join(iotests.test_dir, 'disk')
> +
> +class TestNbdServerRemove(iotests.QMPTestCase):
> +def setUp(self):
> +qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
> +
> +self.vm = iotests.VM().add_drive(disk)
> +self.vm.launch()
> +
> +address = {
> +'type': 'inet',
> +'data': {
> +'host': 'localhost',
> +'port': nbd_port

Again, for a one-shot test, this works; but it doesn't lend itself to
parallel testing.  Maybe do a loop with incrementing port numbers until
one succeeds, if we can reliably detect when a port is already in use?

> +def assertConnectFailed(self, qemu_io_output):
> +self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
> + "can't open device nbd+tcp://localhost:10900/exp: " 
> +

Worth parameterizing or filtering this assertion to match the rest of
the parameterization of nbd_port?

> + "Requested export not available\nserver reported: " 
> +
> + "export 'exp' not present")
> +
> +def do_test_connect_after_remove(self, force=None):
> +args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
> +self.assertReadOk(qemu_io(*args))
> +self.remove_export('exp', force)
> +self.assertExportNotFound('exp')
> +self.assertConnectFailed(qemu_io(*args))
> +
> +def test_connect_after_remove_default(self):
> +self.do_test_connect_after_remove()
> +
> +def test_connect_after_remove_safe(self):
> +self.do_test_connect_after_remove(False)
> +
> +def test_connect_after_remove_force(self):
> +self.do_test_connect_after_remove(True)

May need updates if my comments on 3/6 result in us having three states
rather than just 2 (the difference being whether there is a knob for
choosing to let existing clients gracefully disconnect with all pending
I/O completed, and/or failing the nbd-server-remove if a client is still
connected).

> +++ b/tests/qemu-iotests/201.out
> @@ -0,0 +1,5 @@
> +...
> +--
> +Ran 7 tests

I'm not a fan of python tests that are difficult to debug.  Your
additions to 147 in patch 4/6 are okay (hard to debug, but an
incremental addition); but is it possible to rewrite this test in a bit
more verbose manner?  See test 194 and this message for more details:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html

> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 3e688678dd..15df7678b3 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -197,3 +197,4 @@
>  197 rw auto quick
>  198 rw auto
>  200 rw auto
> +201 rw auto quick
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class

[Eric Blake]

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:

The subject says what, but there is no commit body that says why.

Presumably this makes writing the test in the next patch easier, but
some details in the commit message would make this obvious.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/iotests.py | 38 ++
>  1 file changed, 38 insertions(+)
> 

My python is not strong; it looks good overall, although I have a few
questions that may warrant a v3 before I give R-b.

> +class QemuIoInteractive:
> +def __init__(self, *args):
> +self.args = qemu_io_args + list(args)
> +self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
> +   stdout=subprocess.PIPE,
> +   stderr=subprocess.STDOUT)

Why STDOUT instead of STDERR?  Is the redirection intentional?


> +def _read_output(self):
> +pattern = 'qemu-io> '
> +n = len(pattern)
> +pos = 0
> +s = []
> +while pos != n:
> +c = self._p.stdout.read(1)
> +#check unexpected EOF

pep8 doesn't like this comment style (it wants space after #).  The file
is already unclean under pep8, but you shouldn't make it worse.

> +assert c != ''

Is assert really the nicest control flow when early EOF is present? Or
is it because we are only using this in tests, where we don't expect
early EOF, so asserting is just fine for our usage?

> +s.append(c)
> +if c == pattern[pos]:
> +pos += 1
> +else:
> +pos = 0
> +
> +return ''.join(s[:-n])

Is it necessary to use join() on the empty string here, compared to just
returning the array slice directly?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field

[Frediano Ziglio]

Thanks.
How is your hand?

Frediano

- Original Message -
> 
> On Sun, Dec 31, 2017 at 05:17:43AM -0500, Frediano Ziglio wrote:
> > ping
> > 
> > > 
> > > ping
> > > 
> > > > 
> > > > ping the series
> > > > 
> 
> Was on sick leave for a few weeks, back now, will process asap but will
> probably take a while nevertheless due to the big backlog I have now.
> 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Ala Hino]

On Tue, Jan 9, 2018 at 10:11 PM, Eric Blake  wrote:

> On 01/09/2018 01:58 PM, Ala Hino wrote:
>
> >>> I know this is debatable but I think the #1 purpose of image locking is
> >> to
> >>> prevent data corruption;
> >>
> >> Isn't potentially wrong output of 'qemu-img info' a form of data
> >> corruption? Not data as in disk content, but metadata is data, too.
> >>
> >>> #2 IMO is to reduce confusion and misinformation.
> >>> While inconsistent output of "qemu-img info" is misinformation, it not
> >> working
> >>> as before is actually confusion. Though the current behavior is indeed
> >> ideal,
> >>> the proposed patch is a bit more pragmatical.
> >>
> >> To be clear: If we want to minimise confusing by change, we have to
> >> disable locking by default, not only here but everywhere else. And
> >> therefore make it completely useless.
> >>
> >> The whole point of locking is to change something in order to make
> >> things safer. Yes, this may inconvenience users who want to do something
> >> unsafe. Tough luck. The only other option is not making things safer.
> >>
> >
> > Safer is better for sure.
> > It is not about doing something unsafe, it is about breaking a released
> > version -
> > RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will
> not
> > be able to create snapshots.
> > I see two options:
> > 1. As mentioned by Fam, settle on warning for good
>
> Which is a downgrade in qemu behavior, since we've already had releases
> where it was an error (and if you have to worry about THOSE qemu
> releases, then the warning doesn't buy you anything).
>

We test our code on RHEL, and currently require qemu-kvm-rhev >= 2.6.0.
On RHEL 7.4 qemu-kvm-rhev version is 2.9.0 - and everything works good.
However, on RHEL 7.5 the version is 2.10.0, which breaks RHV 4.1.
If we got qemu-kvm-rhev 2.10.0 in RHEL 7.4 repositories, we could detect the
failure earlier and maybe we would be able to adapt the code before
releasing 4.1.

>
> > 2. Conflict qemu with vdsm < 4.2
>
> If I'm understanding this option correctly, you are proposing that the
> distribution is set up so that you have to upgrade vdsm prior to being
> able to upgrade to newer qemu that enables locking (so you can use old
> vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get
> rejected on attempts to use old vdsm, new qemu).  That's outside the
> scope of qemu proper and falls instead into the distro's packaging
> scheme, but sounds like a better alternative than trying to fix new qemu
> to work around an issue that released qemu already has, all on behalf of
> vdsm where old vdsm plays nice with old qemu, and where new vdsm already
> knows how to play nice with both flavors of qemu.
>

I agree that this might be the better option.

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

Re: [Qemu-devel] [PATCH v2 4/6] iotest 147: add cases to test new @name parameter of nbd-server-add

[Eric Blake]

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/147 | 68 
> +-
>  tests/qemu-iotests/147.out |  4 +--
>  2 files changed, 57 insertions(+), 15 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCHv2] linux-user: Fix sched_get/setaffinity conversion

[Samuel Thibault]

sched_get/setaffinity linux-user syscalls were missing conversions for
little/big endian, which is hairy since longs may not be the same size
either.

For simplicity, this just introduces loops to convert bit by bit like is
done for select.

Signed-off-by: Samuel Thibault 
Reviewed-by: Laurent Vivier 

---
Difference from v1: bitmask computation was separated out into
target_to_host_cpu_mask()/host_to_target_cpu_mask().

 linux-user/syscall.c | 81 ++--
 1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 11c9116c4a..cac07419aa 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7716,6 +7716,73 @@ static TargetFdTrans target_inotify_trans = {
 };
 #endif
 
+static int target_to_host_cpu_mask(unsigned long *host_mask,
+   size_t host_size,
+   abi_ulong target_addr,
+   size_t target_size)
+{
+unsigned target_bits = sizeof(abi_ulong) * 8;
+unsigned host_bits = sizeof(*host_mask) * 8;
+abi_ulong *target_mask;
+unsigned i, j;
+
+assert(host_size >= target_size);
+
+target_mask = lock_user(VERIFY_READ, target_addr, target_size, 1);
+if (!target_mask) {
+return -TARGET_EFAULT;
+}
+memset(host_mask, 0, host_size);
+
+for (i = 0 ; i < target_size / sizeof(abi_ulong); i++) {
+unsigned bit = i * target_bits;
+abi_ulong val;
+
+__get_user(val, _mask[i]);
+for (j = 0; j < target_bits; j++, bit++) {
+if (val & (1UL << j)) {
+host_mask[bit / host_bits] |= 1UL << (bit % host_bits);
+}
+}
+}
+
+unlock_user(target_mask, target_addr, 0);
+return 0;
+}
+
+static int host_to_target_cpu_mask(const unsigned long *host_mask,
+   size_t host_size,
+   abi_ulong target_addr,
+   size_t target_size)
+{
+unsigned target_bits = sizeof(abi_ulong) * 8;
+unsigned host_bits = sizeof(*host_mask) * 8;
+abi_ulong *target_mask;
+unsigned i, j;
+
+assert(host_size >= target_size);
+
+target_mask = lock_user(VERIFY_WRITE, target_addr, target_size, 0);
+if (!target_mask) {
+return -TARGET_EFAULT;
+}
+
+for (i = 0 ; i < target_size / sizeof(abi_ulong); i++) {
+unsigned bit = i * target_bits;
+abi_ulong val = 0;
+
+for (j = 0; j < target_bits; j++, bit++) {
+if (host_mask[bit / host_bits] & (1UL << (bit % host_bits))) {
+val |= 1UL << j;
+}
+}
+__put_user(val, _mask[i]);
+}
+
+unlock_user(target_mask, target_addr, target_size);
+return 0;
+}
+
 /* do_syscall() should always have a single exit point at the end so
that actions, such as logging of syscall results, can be performed.
All errnos that do_syscall() returns must be -TARGET_. */
@@ -10353,6 +10420,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1);
 
 mask = alloca(mask_size);
+memset(mask, 0, mask_size);
 ret = get_errno(sys_sched_getaffinity(arg1, mask_size, mask));
 
 if (!is_error(ret)) {
@@ -10372,9 +10440,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 ret = arg2;
 }
 
-if (copy_to_user(arg3, mask, ret)) {
-goto efault;
-}
+ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2);
 }
 }
 break;
@@ -10392,13 +10458,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 break;
 }
 mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1);
-
 mask = alloca(mask_size);
-if (!lock_user_struct(VERIFY_READ, p, arg3, 1)) {
-goto efault;
+
+ret = target_to_host_cpu_mask(mask, mask_size, arg3, arg2);
+if (ret) {
+break;
 }
-memcpy(mask, p, arg2);
-unlock_user_struct(p, arg2, 0);
 
 ret = get_errno(sys_sched_setaffinity(arg1, mask_size, mask));
 }
-- 
2.15.1

Re: [Qemu-devel] [PATCH] qemu: improve hugepage allocation failure message

[Haozhong Zhang]

On 01/09/18 17:37 -0200, Marcelo Tosatti wrote:
> 
> Improve hugepage allocation failure message, indicating
> whats happening to the user.
> 
> Signed-off-by: Marcelo Tosatti 
> 
> diff --git a/exec.c b/exec.c
> index 4722e521d4..439abedb98 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1643,7 +1643,8 @@ static void *file_ram_alloc(RAMBlock *block,
>   block->flags & RAM_SHARED);
>  if (area == MAP_FAILED) {
>  error_setg_errno(errp, errno,
> - "unable to map backing store for guest RAM");
> + "unable to map backing store for guest RAM, "
> + "falling back to malloc based RAM allocation");
>  return NULL;
>  }
>  

The backing store here is not always hugetlbfs, e.g., it can be NVDIMM
and even regular files. In those cases, the failure of file_ram_alloc()
does not fall back to malloc.

Haozhong

Re: [Qemu-devel] [PATCH v6 0/9] Add memfd memory backend

[Eduardo Habkost]

On Fri, Dec 22, 2017 at 07:48:43PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Oct 23, 2017 at 4:18 PM, Marc-André Lureau
>  wrote:
> > Add a new Linux-specific memory backend, similar to hostmem-file,
> > except that it doesn't need file path. It also try to enforce memory
> > sealing if available. It is thus slightly easier and secure, and is
> > compatible with transparent huge-pages since Linux 4.8.
> >
> > Since Linux 4.14, memfd supports explicit hugetlb, however without
> > sealing.
> 
> A little update here, 4.16 is likely to support sealing with hugetlb
> too (patches are in -mm tree). This should make memfd the prefered
> backend for sharing memory with a vhost-user backend.
> 
> please review.
> thanks

I've queued patches 1-2 on machine-next.  I plan to review the
rest soon.  Sorry for the long delay.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/6] Replace has_dynamic_sysbus with list of allowed device types

[Eduardo Habkost]

Queued on machine-next.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Eric Blake]

On 01/09/2018 01:58 PM, Ala Hino wrote:

>>> I know this is debatable but I think the #1 purpose of image locking is
>> to
>>> prevent data corruption;
>>
>> Isn't potentially wrong output of 'qemu-img info' a form of data
>> corruption? Not data as in disk content, but metadata is data, too.
>>
>>> #2 IMO is to reduce confusion and misinformation.
>>> While inconsistent output of "qemu-img info" is misinformation, it not
>> working
>>> as before is actually confusion. Though the current behavior is indeed
>> ideal,
>>> the proposed patch is a bit more pragmatical.
>>
>> To be clear: If we want to minimise confusing by change, we have to
>> disable locking by default, not only here but everywhere else. And
>> therefore make it completely useless.
>>
>> The whole point of locking is to change something in order to make
>> things safer. Yes, this may inconvenience users who want to do something
>> unsafe. Tough luck. The only other option is not making things safer.
>>
> 
> Safer is better for sure.
> It is not about doing something unsafe, it is about breaking a released
> version -
> RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not
> be able to create snapshots.
> I see two options:
> 1. As mentioned by Fam, settle on warning for good

Which is a downgrade in qemu behavior, since we've already had releases
where it was an error (and if you have to worry about THOSE qemu
releases, then the warning doesn't buy you anything).

> 2. Conflict qemu with vdsm < 4.2

If I'm understanding this option correctly, you are proposing that the
distribution is set up so that you have to upgrade vdsm prior to being
able to upgrade to newer qemu that enables locking (so you can use old
vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get
rejected on attempts to use old vdsm, new qemu).  That's outside the
scope of qemu proper and falls instead into the distro's packaging
scheme, but sounds like a better alternative than trying to fix new qemu
to work around an issue that released qemu already has, all on behalf of
vdsm where old vdsm plays nice with old qemu, and where new vdsm already
knows how to play nice with both flavors of qemu.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.12 v4 0/3] nvdimm: fixes for (non-)dax backends

[Eduardo Habkost]

Queued on machine-next.  Thanks, and sorry for the long delay.


On Mon, Dec 11, 2017 at 03:28:03PM +0800, Haozhong Zhang wrote:
> Previous versions can be found at
>   v3: https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04785.html
>   v2: https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg01203.html
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05919.html
> 
> Changes in v4:
>  * Document new memory-backend-file option 'align' in qemu-options.hx. 
> (Eduardo Habkost)
>  * Add endian conversion when setting the unarmed flag in NFIT. (Stefan 
> Hajnoczi)
> 
> Changes in v3:
>  * Add an option 'align' to 'memory-backend-file' to address the
>failure when mmap device dax (patch 1).
>  * Remove device dax check, which needs to access sysfs and may not
>work with SELinux.
>  * Add a boolean option 'unarmed' to '-device nvdimm', which allows
>users to control the unarmed flag in guest ACPI NFIT. I don't make
>it as OnOffAuto, because of the remove of device dax check.
>  * Document new options added by this patch series.
> 
> Haozhong Zhang (3):
>   hostmem-file: add "align" option
>   nvdimm: add a macro for property "label-size"
>   nvdimm: add 'unarmed' option
> 
>  backends/hostmem-file.c | 41 -
>  docs/nvdimm.txt | 31 +++
>  exec.c  |  8 +++-
>  hw/acpi/nvdimm.c|  7 +++
>  hw/mem/nvdimm.c | 28 +++-
>  include/exec/memory.h   |  3 +++
>  include/hw/mem/nvdimm.h | 12 
>  memory.c|  2 ++
>  numa.c  |  2 +-
>  qemu-options.hx |  9 -
>  10 files changed, 138 insertions(+), 5 deletions(-)
> 
> -- 
> 2.14.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.11 v3 0/2] Check for the availability of a hotplug controller before adding a device

[Eduardo Habkost]

On Mon, Dec 18, 2017 at 12:59:50PM +0100, Thomas Huth wrote:
> On 02.11.2017 11:10, Thomas Huth wrote:
> > First patch is a small clean up to the error handling code in
> > qdev_device_add(), and the second patch adds a proper check for
> > the availability of a hotplug controller to prevent the possibility
> > of a crash with device_del.
> > 
> > The crash can currently be triggered for example like this:
> > 
> > $ s390x-softmmu/qemu-system-s390x -M none -nographic 
> > QEMU 2.10.50 monitor - type 'help' for more information
> > (qemu) device_add qemu-s390x-cpu,id=x
> > (qemu) device_del x
> > **
> > ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> > Aborted (core dumped)
> > 
> > v3:
> >  - Invert the logic of the second error check in the first patch
> >as suggested by Igor
> >  - Updated the patch description of the second patch with the current
> >way to crash QEMU
> > 
> > v2:
> >  - Do the check for the availability of the hotplug controller earlier
> >in qdev_device_add
> >  - Use common new err_dev_del handler in case of failure
> > 
> > Thomas Huth (2):
> >   qdev_monitor: Simplify error handling in qdev_device_add()
> >   qdev: Check for the availability of a hotplug controller before adding
> > a device
> > 
> >  hw/core/qdev.c | 28 
> >  include/hw/qdev-core.h |  1 +
> >  qdev-monitor.c | 21 +
> >  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> Ping!
> 
> Paolo or Eduardo, could one of you please pick up the two patches?

Very sorry for the long delay.  I just queued it on machine-next.
Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH] scripts: Remove fixed entries from the device-crash-test

[Eduardo Habkost]

On Mon, Dec 18, 2017 at 05:10:38PM +0100, Thomas Huth wrote:
> These are crashes / errors which have been fixed already in the past
> months. We can remove these from the device-crash-test script now.
> 
> Signed-off-by: Thomas Huth 

Queued on machine-next.  Sorry for the long delay.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Ala Hino]

On Tue, Jan 9, 2018 at 11:58 AM, Kevin Wolf  wrote:

> Am 09.01.2018 um 07:24 hat Fam Zheng geschrieben:
> > On Mon, 01/08 18:57, Kevin Wolf wrote:
> > > I'm not sure if going back to the old behaviour for a while now would
> be
> > > helpful, you'd just end up with an even more confusing set of qemu
> > > versions, for example:
> > >
> > > <= 2.9  - works without a warning
> > > 2.10 and 2.11   - errors out
> > > 2.12- prints a warning, but works
> > > >= 2.13 - errors out again
> >
> > What I had in mind is settle on warning for good. QEMU (including
> qemu-img) is a
> > low level tool that can be used in many ways that it isn't supposed to,
> this one
> > is not more harmful than others (e.g. "qemu-img snapshot ..." on
> iscsi:// qcow2
> > image) we allow siliently.
> >
> > I know this is debatable but I think the #1 purpose of image locking is
> to
> > prevent data corruption;
>
> Isn't potentially wrong output of 'qemu-img info' a form of data
> corruption? Not data as in disk content, but metadata is data, too.
>
> > #2 IMO is to reduce confusion and misinformation.
> > While inconsistent output of "qemu-img info" is misinformation, it not
> working
> > as before is actually confusion. Though the current behavior is indeed
> ideal,
> > the proposed patch is a bit more pragmatical.
>
> To be clear: If we want to minimise confusing by change, we have to
> disable locking by default, not only here but everywhere else. And
> therefore make it completely useless.
>
> The whole point of locking is to change something in order to make
> things safer. Yes, this may inconvenience users who want to do something
> unsafe. Tough luck. The only other option is not making things safer.
>

Safer is better for sure.
It is not about doing something unsafe, it is about breaking a released
version -
RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not
be able to create snapshots.
I see two options:
1. As mentioned by Fam, settle on warning for good
2. Conflict qemu with vdsm < 4.2

>
> We already successfully made a point that tools (libvirt with shared
> images, or libguestfs) need to update their qemu invocation for qemu
> proper. They didn't like it, but they could see the point, and it has
> been this way for two releases already. So I don't see a compelling
> reason why now we should give up again some of the safety we achieved
> long-term.
>
> If we were designing qemu-img from scratch, it would be an error. So
> that's what it should be in the long term. Tools should preferably use
> 'query-block' and friends rather 'qemu-img info' if the image is in use.
>
> Kevin
>

Re: [Qemu-devel] Some question about savem/qcow2 incremental snapshot

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefa...@gmail.com) wrote:
> On Mon, Dec 25, 2017 at 07:54:00AM +, He, Junyan wrote:
> > I am now focusing on snapshot optimization for Intel NVDimm kind memory. 
> > Different from the normal memory, the NVDimm may be 128G, 256G or even more 
> > for just one guest, and its speed is slower than the normal memory. So 
> > sometimes it may take several minutes to complete just one snapshot saving. 
> > Even with compression enabled, the snapshot point may consume more than 30G 
> > disk space. We decide to add incremental kind snapshot saving to resolve 
> > this. Just store difference between snapshot points to save time and disk 
> > space.
> > But the current snapshot/save_vm framework seems not to support this.
> > We need to add snapshot dependency and extra operations when we LOAD and 
> > DELETE the snapshot point.
> > Is that possible to modify the savevm framework and add some incremental 
> > snapshot support to QCOW2 format?
> 
> It sounds like you'd like to save incremental guest RAM snapshots based
> on the contents of earlier snapshots.  QEMU has no way of doing this at
> the moment.
> 
> In order to do this QEMU needs to keep dirty memory logging enabled at
> all times so it knows which pages have been written since the last
> snapshot.  This will affect guest performance.

Keeping the dirty logging going is something that COLO does, to send
incremental snapshots to the secondary.   As you say, it does slow
things down and you have to be able to keep the state between the
migration runs.

> Certain guest operations like rebooting or zeroing memory will defeat
> the incremental guest RAM snapshot feature.  It's worth thinking about
> these cases to make sure this feature would be worth it in real use
> cases.

But those probably wouldn't upset an NVDimm?

> What you're proposing isn't specific to NVDIMM.  Regular RAM use cases
> also benefit from incremental snapshots because less disk space is
> required for the savevm data.
> 
> Some questions about your use case:
> 
> 1. Does the guest have -device nvdimm?
> 
> 2. Is the qcow2 file on NVDIMM?
> 
> Stefan

Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add command for export removing. It is needed for cases when we
> don't want to keep export after the operation on it was completed.
> The other example is temporary node, created with blockdev-add.
> If we want to delete it we should firstly remove corresponding
> NBD export.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block.json | 18 ++
>  include/block/nbd.h |  3 ++-
>  blockdev-nbd.c  | 29 -
>  nbd/server.c| 25 ++---
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 503d4b287b..f83485c325 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -228,6 +228,24 @@
>'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @nbd-server-remove:
> +#
> +# Remove NBD export by name.
> +#
> +# @name: Export name.
> +#
> +# @force: Whether active connections to the export should be closed. If this
> +# parameter is false the export only becomes hidden from clients (new
> +# connections are impossible), and would be automatically freed after
> +# all clients are disconnected (default false).

Unstated, but if the parameter is true, existing clients are forcefully
disconnected, possibly losing pending transactions.

Do we want a third mode in the middle, where the server starts replying
to all existing clients with ESHUTDOWN errors for all new requests
rather than abruptly disconnecting (no new I/O, but no forced disconnect
and pending in-flight transactions can still complete gracefully)?

> +#
> +# Returns: error if the server is not running or export is not found.
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove', 'data': {'name': 'str', '*force': 'bool'} }
> +

If we're okay with just the bool parameter, then this patch looks good;
but if we want a third mode, then we want '*force' to be an enum type.
So tentative:

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] qemu: improve hugepage allocation failure message

[Eric Blake]

On 01/09/2018 01:37 PM, Marcelo Tosatti wrote:
> 
> Improve hugepage allocation failure message, indicating
> whats happening to the user.

s/whats/what's/

> 
> Signed-off-by: Marcelo Tosatti 
> 
> diff --git a/exec.c b/exec.c
> index 4722e521d4..439abedb98 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1643,7 +1643,8 @@ static void *file_ram_alloc(RAMBlock *block,
>   block->flags & RAM_SHARED);
>  if (area == MAP_FAILED) {
>  error_setg_errno(errp, errno,
> - "unable to map backing store for guest RAM");
> + "unable to map backing store for guest RAM, "
> + "falling back to malloc based RAM allocation");
>  return NULL;
>  }
>  
> 
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] qemu: improve hugepage allocation failure message

[Marcelo Tosatti]

Improve hugepage allocation failure message, indicating
whats happening to the user.

Signed-off-by: Marcelo Tosatti 

diff --git a/exec.c b/exec.c
index 4722e521d4..439abedb98 100644
--- a/exec.c
+++ b/exec.c
@@ -1643,7 +1643,8 @@ static void *file_ram_alloc(RAMBlock *block,
  block->flags & RAM_SHARED);
 if (area == MAP_FAILED) {
 error_setg_errno(errp, errno,
- "unable to map backing store for guest RAM");
+ "unable to map backing store for guest RAM, "
+ "falling back to malloc based RAM allocation");
 return NULL;
 }
 

[Qemu-devel] [PATCH v2 2.5/6] hmp: Add name parameter to nbd_server_add

[Eric Blake]

Extend the flexibility of the previous QMP patch to also work
in HMP.

Signed-off-by: Eric Blake 
---

In response to Dave's request.
I could also squash this into Vladimir's 2/6 if desired.

 hmp.c   | 3 ++-
 hmp-commands.hx | 9 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hmp.c b/hmp.c
index 80c5b01540..c9fcdc8593 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2221,10 +2221,11 @@ exit:
 void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
 {
 const char *device = qdict_get_str(qdict, "device");
+const char *name = qdict_get_try_str(qdict, "name");
 bool writable = qdict_get_try_bool(qdict, "writable", false);
 Error *local_err = NULL;

-qmp_nbd_server_add(device, false, NULL, true, writable, _err);
+qmp_nbd_server_add(device, !!name, name, true, writable, _err);

 if (local_err != NULL) {
 hmp_handle_error(mon, _err);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6d5ebdf6ab..b8b6fb9184 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1553,17 +1553,18 @@ ETEXI

 {
 .name   = "nbd_server_add",
-.args_type  = "writable:-w,device:B",
-.params = "nbd_server_add [-w] device",
+.args_type  = "writable:-w,device:B,name:s?",
+.params = "nbd_server_add [-w] device [name]",
 .help   = "export a block device via NBD",
 .cmd= hmp_nbd_server_add,
 },
 STEXI
-@item nbd_server_add @var{device}
+@item nbd_server_add @var{device} [ @var{name} ]
 @findex nbd_server_add
 Export a block device through QEMU's NBD server, which must be started
 beforehand with @command{nbd_server_start}.  The @option{-w} option makes the
-exported device writable too.
+exported device writable too.  The export name is controlled by @var{name},
+defaulting to @var{device}.
 ETEXI

 {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Eric Blake]

On 12/08/2017 11:33 AM, Dr. David Alan Gilbert wrote:

>> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict 
>> *qdict)
>>  bool writable = qdict_get_try_bool(qdict, "writable", false);
>>  Error *local_err = NULL;
>>  
>> -qmp_nbd_server_add(device, true, writable, _err);
>> +qmp_nbd_server_add(device, false, NULL, true, writable, _err);
> 
> I wont insist, but it would be nice if you wired up an optional
> parameter on HMP as well.

Can be done as a followup patch; I'm not sure how many people are
setting up NBD exports via HMP, and I'm also okay with just stating that
the full power requires use of QMP.  But I'll give such a followup patch
a quick try, to see whether it is easy after all.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Eric Blake]

On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
> 
> This also allows creating several exports per device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block.json |  9 +++--
>  blockdev-nbd.c  | 14 +-
>  hmp.c   |  5 +++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
>  #
>  # @device: The device name or node name of the node to be exported
>  #
> +# @name: Export name. If unspecified @device parameter used as export name.

s/unspecified/unspecified, the/
s/used as/is used as the/

Will tweak as part of applying to my NBD queue.
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v1 1/3] xlnx-zynqmp-rtc: Initial commit

[Alistair Francis]

Initial commit of the ZynqMP RTC device.

Signed-off-by: Alistair Francis 
---

 hw/timer/Makefile.objs |   1 +
 hw/timer/xlnx-zynqmp-rtc.c | 232 +
 include/hw/timer/xlnx-zynqmp-rtc.h |  84 ++
 3 files changed, 317 insertions(+)
 create mode 100644 hw/timer/xlnx-zynqmp-rtc.c
 create mode 100644 include/hw/timer/xlnx-zynqmp-rtc.h

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 8c19eac3b6..8b27a4b7ef 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -21,6 +21,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
 common-obj-$(CONFIG_IMX) += imx_gpt.o
 common-obj-$(CONFIG_LM32) += lm32_timer.o
 common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
+common-obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp-rtc.o
 
 obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c
new file mode 100644
index 00..40533220fc
--- /dev/null
+++ b/hw/timer/xlnx-zynqmp-rtc.c
@@ -0,0 +1,232 @@
+/*
+ * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
+ *
+ * Copyright (c) 2017 Xilinx Inc.
+ *
+ * Written-by: Alistair Francis 
+ *
+ * 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"
+#include "hw/sysbus.h"
+#include "hw/register.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "hw/timer/xlnx-zynqmp-rtc.h"
+
+#ifndef XLNX_ZYNQMP_RTC_ERR_DEBUG
+#define XLNX_ZYNQMP_RTC_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT_L(lvl, fmt, args...) do { \
+if (XLNX_ZYNQMP_RTC_ERR_DEBUG >= lvl) { \
+qemu_log("%s: " fmt, __func__, ## args); \
+} \
+} while (0);
+
+#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
+
+static void rtc_int_update_irq(XlnxZynqMPRTC *s)
+{
+bool pending = s->regs[R_RTC_INT_STATUS] & ~s->regs[R_RTC_INT_MASK];
+qemu_set_irq(s->irq_rtc_int, pending);
+}
+
+static void addr_error_int_update_irq(XlnxZynqMPRTC *s)
+{
+bool pending = s->regs[R_ADDR_ERROR] & ~s->regs[R_ADDR_ERROR_INT_MASK];
+qemu_set_irq(s->irq_addr_error_int, pending);
+}
+
+static void rtc_int_status_postw(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+rtc_int_update_irq(s);
+}
+
+static uint64_t rtc_int_en_prew(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+uint32_t val = val64;
+
+s->regs[R_RTC_INT_MASK] &= ~val;
+rtc_int_update_irq(s);
+return 0;
+}
+
+static uint64_t rtc_int_dis_prew(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+uint32_t val = val64;
+
+s->regs[R_RTC_INT_MASK] |= val;
+rtc_int_update_irq(s);
+return 0;
+}
+
+static void addr_error_postw(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+addr_error_int_update_irq(s);
+}
+
+static uint64_t addr_error_int_en_prew(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+uint32_t val = val64;
+
+s->regs[R_ADDR_ERROR_INT_MASK] &= ~val;
+addr_error_int_update_irq(s);
+return 0;
+}
+
+static uint64_t addr_error_int_dis_prew(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+uint32_t val = val64;
+
+s->regs[R_ADDR_ERROR_INT_MASK] |= val;
+addr_error_int_update_irq(s);
+return 0;
+}
+
+static const RegisterAccessInfo rtc_regs_info[] = {
+{   .name = "SET_TIME_WRITE",  .addr = A_SET_TIME_WRITE,
+},{ .name = "SET_TIME_READ",  .addr = A_SET_TIME_READ,
+.ro = 0x,
+},{ .name = "CALIB_WRITE",  .addr = A_CALIB_WRITE,
+},{ .name = "CALIB_READ",  .addr = A_CALIB_READ,
+.ro = 0x1f,
+},{ .name = "CURRENT_TIME",  .addr = A_CURRENT_TIME,
+.ro = 0x,
+},{ .name = 

[Qemu-devel] [PATCH v1 3/3] xlnx-zynqmp: Connect the RTC device

[Alistair Francis]

Signed-off-by: Alistair Francis 
---

 hw/arm/xlnx-zynqmp.c | 14 ++
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 325642058b..deef583c2a 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -50,6 +50,9 @@
 #define DPDMA_ADDR  0xfd4c
 #define DPDMA_IRQ   116
 
+#define RTC_ADDR0xffa6
+#define RTC_IRQ 26
+
 static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
 0xFF0B, 0xFF0C, 0xFF0D, 0xFF0E,
 };
@@ -183,6 +186,9 @@ static void xlnx_zynqmp_init(Object *obj)
 
 object_initialize(>dpdma, sizeof(s->dpdma), TYPE_XLNX_DPDMA);
 qdev_set_parent_bus(DEVICE(>dpdma), sysbus_get_default());
+
+object_initialize(>rtc, sizeof(s->rtc), TYPE_XLNX_ZYNQMP_RTC);
+qdev_set_parent_bus(DEVICE(>rtc), sysbus_get_default());
 }
 
 static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
@@ -454,6 +460,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
**errp)
  _abort);
 sysbus_mmio_map(SYS_BUS_DEVICE(>dpdma), 0, DPDMA_ADDR);
 sysbus_connect_irq(SYS_BUS_DEVICE(>dpdma), 0, gic_spi[DPDMA_IRQ]);
+
+object_property_set_bool(OBJECT(>rtc), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>rtc), 0, RTC_ADDR);
+sysbus_connect_irq(SYS_BUS_DEVICE(>rtc), 0, gic_spi[RTC_IRQ]);
 }
 
 static Property xlnx_zynqmp_props[] = {
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 3e6fb9b7bd..9e8c9e18dd 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -28,6 +28,7 @@
 #include "hw/ssi/xilinx_spips.h"
 #include "hw/dma/xlnx_dpdma.h"
 #include "hw/display/xlnx_dp.h"
+#include "hw/timer/xlnx-zynqmp-rtc.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
@@ -90,6 +91,7 @@ typedef struct XlnxZynqMPState {
 XlnxZynqMPQSPIPS qspi;
 XlnxDPState dp;
 XlnxDPDMAState dpdma;
+XlnxZynqMPRTC rtc;
 
 char *boot_cpu;
 ARMCPU *boot_cpu_ptr;
-- 
2.14.1

[Qemu-devel] [PATCH v1 2/3] xlnx-zynqmp-rtc: Add basic time support

[Alistair Francis]

Allow the guest to determine the time set from the QEMU command line.

Signed-off-by: Alistair Francis 
---

 hw/timer/xlnx-zynqmp-rtc.c | 24 
 include/hw/timer/xlnx-zynqmp-rtc.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c
index 40533220fc..eed17805ac 100644
--- a/hw/timer/xlnx-zynqmp-rtc.c
+++ b/hw/timer/xlnx-zynqmp-rtc.c
@@ -29,6 +29,7 @@
 #include "hw/register.h"
 #include "qemu/bitops.h"
 #include "qemu/log.h"
+#include "hw/ptimer.h"
 #include "hw/timer/xlnx-zynqmp-rtc.h"
 
 #ifndef XLNX_ZYNQMP_RTC_ERR_DEBUG
@@ -55,6 +56,13 @@ static void addr_error_int_update_irq(XlnxZynqMPRTC *s)
 qemu_set_irq(s->irq_addr_error_int, pending);
 }
 
+static uint64_t current_time_postr(RegisterInfo *reg, uint64_t val64)
+{
+XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
+
+return mktime(>current_tm);
+}
+
 static void rtc_int_status_postw(RegisterInfo *reg, uint64_t val64)
 {
 XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque);
@@ -111,11 +119,13 @@ static const RegisterAccessInfo rtc_regs_info[] = {
 {   .name = "SET_TIME_WRITE",  .addr = A_SET_TIME_WRITE,
 },{ .name = "SET_TIME_READ",  .addr = A_SET_TIME_READ,
 .ro = 0x,
+.post_read = current_time_postr,
 },{ .name = "CALIB_WRITE",  .addr = A_CALIB_WRITE,
 },{ .name = "CALIB_READ",  .addr = A_CALIB_READ,
 .ro = 0x1f,
 },{ .name = "CURRENT_TIME",  .addr = A_CURRENT_TIME,
 .ro = 0x,
+.post_read = current_time_postr,
 },{ .name = "CURRENT_TICK",  .addr = A_CURRENT_TICK,
 .ro = 0x,
 },{ .name = "ALARM",  .addr = A_ALARM,
@@ -155,6 +165,13 @@ static void rtc_reset(DeviceState *dev)
 register_reset(>regs_info[i]);
 }
 
+qemu_get_timedate(>current_tm, 0);
+
+DB_PRINT("Get time from host: %d-%d-%d %2d:%02d:%02d\n",
+ s->current_tm.tm_year, s->current_tm.tm_mon,
+ s->current_tm.tm_mday, s->current_tm.tm_hour,
+ s->current_tm.tm_min, s->current_tm.tm_sec);
+
 rtc_int_update_irq(s);
 addr_error_int_update_irq(s);
 }
@@ -203,6 +220,13 @@ static const VMStateDescription vmstate_rtc = {
 .minimum_version_id = 1,
 .fields = (VMStateField[]) {
 VMSTATE_UINT32_ARRAY(regs, XlnxZynqMPRTC, XLNX_ZYNQMP_RTC_R_MAX),
+VMSTATE_INT32(current_tm.tm_sec, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_min, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_hour, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_wday, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_mday, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_mon, XlnxZynqMPRTC),
+VMSTATE_INT32(current_tm.tm_year, XlnxZynqMPRTC),
 VMSTATE_END_OF_LIST(),
 }
 };
diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h 
b/include/hw/timer/xlnx-zynqmp-rtc.h
index 87649836cc..51a49094ad 100644
--- a/include/hw/timer/xlnx-zynqmp-rtc.h
+++ b/include/hw/timer/xlnx-zynqmp-rtc.h
@@ -79,6 +79,8 @@ typedef struct XlnxZynqMPRTC {
 qemu_irq irq_rtc_int;
 qemu_irq irq_addr_error_int;
 
+struct tm current_tm;
+
 uint32_t regs[XLNX_ZYNQMP_RTC_R_MAX];
 RegisterInfo regs_info[XLNX_ZYNQMP_RTC_R_MAX];
 } XlnxZynqMPRTC;
-- 
2.14.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/2] GICv2 & GICv3: RAZ/WI reserved addresses rather than aborting

[Andrew Jones]

On Tue, Jan 09, 2018 at 04:35:30PM +, Peter Maydell wrote:
> On 9 January 2018 at 16:29, Laszlo Ersek  wrote:
> > On 01/09/18 17:12, Peter Maydell wrote:
> >> On 9 January 2018 at 15:58, Laszlo Ersek  wrote:
> >>> Sorry, no clue about any of this -- where should I read up?
> >>
> >> I cc'd you mostly as a heads-up since the QEMU bug is UEFI affecting,
> >> not because I wanted to make you read the GIC specs :-)
> >
> > Thanks (and, thanks :) ) -- from patch #2, looks like GICv2 is affected
> > too, and the patch seems to be fixing commit a9d853533cc1
> > ("hw/intc/arm_gic: Switch to read/write callbacks with tx attributes",
> > 2015-05-12).
> >
> > Is that right? That commit was released with v2.4.0. Should I have
> > experienced the error? Is it KVM / hardware specific? What are the symptoms?
> 
> Happens under TCG emulation only. The bug got introduced with
> commit c79c0a314c43b78f6, which changed the effect of a device
> returning MEMTX_ERROR/MEMTX_DECODE_ERROR from "RAZ/WI" to
> "guest data abort". That's in general the right thing to do,
> but in the case of these device models we were returning those
> values for cases which aren't supposed to provoke aborts.
> 
> The symptom is that if your guest code is poorly behaved and
> tries to read or write offsets that don't correspond to documented
> GIC registers, it will take an abort that it didn't before.
> Linux is fine; this UEFI image I have lying around stopped working.

Thanks for pointing this out. I had just recently noticed that the
'gicv3-active' kvm-unit-tests test had stopped passing on TCG, getting
DABTs when attempting to write GICD_ICACTIVER. I was just about to
complain in this thread that that register is indeed documented, but
now I see the implementation of the test was always wrong. It was using
the wrong register base, RD vs. SGI...

drew

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2-12 12/15] s390x/tcg: fix size + content of STSI blocks

[Thomas Huth]

On 11.12.2017 14:47, David Hildenbrand wrote:
> All blocks are 4k in size, which is only true for two of them right now.
> Also some reserved fields were wrong, fix it and convert all reserved
> fields to u8.
> 
> This also fixes the LPAR part output in /proc/sysinfo under TCG. (for
> now, everything was indicated as 0)
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/cpu.h | 22 ++
>  1 file changed, 14 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v4 1/6] vhost: Move log_dirty check

[Dr. David Alan Gilbert]

* Igor Mammedov (imamm...@redhat.com) wrote:
> On Thu, 14 Dec 2017 15:20:10 +
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > On Wed, 13 Dec 2017 18:08:02 +
> > > "Dr. David Alan Gilbert (git)"  wrote:
> > > 
> > > > From: "Dr. David Alan Gilbert" 
> > > > 
> > > > Move the log_dirty check into vhost_section.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert 
> > > > ---
> > > >  hw/virtio/trace-events |  3 +++
> > > >  hw/virtio/vhost.c  | 20 +---
> > > >  2 files changed, 16 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > > > index 775461ae98..4a493bcd46 100644
> > > > --- a/hw/virtio/trace-events
> > > > +++ b/hw/virtio/trace-events
> > > > @@ -1,5 +1,8 @@
> > > >  # See docs/devel/tracing.txt for syntax documentation.
> > > >  
> > > > +# hw/virtio/vhost.c
> > > > +vhost_section(const char *name, int r) "%s:%d"
> > > > +
> > > >  # hw/virtio/virtio.c
> > > >  virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, 
> > > > unsigned out_num) "elem %p size %zd in_num %u out_num %u"
> > > >  virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned 
> > > > int idx) "vq %p elem %p len %u idx %u"
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index e4290ce93d..e923219e63 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "hw/virtio/virtio-access.h"
> > > >  #include "migration/blocker.h"
> > > >  #include "sysemu/dma.h"
> > > > +#include "trace.h"
> > > >  
> > > >  /* enabled until disconnected backend stabilizes */
> > > >  #define _VHOST_DEBUG 1
> > > > @@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener 
> > > > *listener,
> > > >   memory_listener);
> > > >  hwaddr start_addr = section->offset_within_address_space;
> > > >  ram_addr_t size = int128_get64(section->size);
> > > > -bool log_dirty =
> > > > -memory_region_get_dirty_log_mask(section->mr) & ~(1 << 
> > > > DIRTY_MEMORY_MIGRATION);
> > > >  int s = offsetof(struct vhost_memory, regions) +
> > > >  (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
> > > >  void *ram;
> > > >  
> > > >  dev->mem = g_realloc(dev->mem, s);
> > > >  
> > > > -if (log_dirty) {
> > > > -add = false;
> > > > -}
> > > > -
> > > >  assert(size);
> > > >  
> > > >  /* Optimize no-change case. At least cirrus_vga does this a lot at 
> > > > this time. */
> > > > @@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener 
> > > > *listener,
> > > >  
> > > >  static bool vhost_section(MemoryRegionSection *section)
> > > >  {
> > > > -return memory_region_is_ram(section->mr) &&
> > > > +bool result;
> > > > +bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
> > > > + ~(1 << DIRTY_MEMORY_MIGRATION);
> > > > +result = memory_region_is_ram(section->mr) &&
> > > >  !memory_region_is_rom(section->mr);
> > > > +
> > > > +/* Vhost doesn't handle any block which is doing dirty-tracking 
> > > > other
> > > > + * than migration; this typically fires on VGA areas.
> > > > + */
> > > > +result &= !log_dirty;
> > > before patch even if log_dirty, vhost_set_memory will still proceed
> > > and may remove dirty section from memmap and set memory_changed = true
> > > 
> > > but with this patch it will just ignore such section,
> > > I'm not sure it's right (it might be right with new approach add/nop
> > > but then this patch should go after new code in place and old one
> > > if gone).
> > 
> > I thought about that, but then I came to the conclusion that the whole
> > idea is that we're supposed to be ignoring these regions - so why
> > should they cause any change to the behaviour of vhost at all?
> > Thus this way seems safer.
> the thing is that with originally with old code backend will use memap
> with not yet dirty region, then after region becomes dirty current code
> will REMOVE dirty region from map and NOTIFY backend.
> 
> While this patch is fine for new approach as memmap is build from scratch,
> it doesn't look correct for current code since region that just became
> dirty will not be removed from memap and might be used backend.
> Whether it would really break something or not I'm not sure,
> maybe Michael may Ack this patch ii using dirty region is fine.

That's OK; I can add the check to vhost_region_add_section when I
initially create it, and then swing this change around nearer to the
end of the patchset, so that now we're using the new code.

Dave

> > 
> > Dave
> > 
> > > > +
> > > > +trace_vhost_section(section->mr->name, result);
> > > > +return result;
> > > >  }
> > > >  
> > > >  static void vhost_begin(MemoryListener *listener)

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Juan Quintela]

Peter Maydell  wrote:
> On 5 January 2018 at 09:59, Juan Quintela  wrote:
>> Eric Blake  wrote:
>>> On 01/03/2018 03:38 AM, Juan Quintela wrote:
 Hi

 This are the changes for migration that are already reviewed.

 Please, apply.

>>>
 Alexey Perevalov (6):
   migration: introduce postcopy-blocktime capability
   migration: add postcopy blocktime ctx into MigrationIncomingState
   migration: calculate vCPU blocktime on dst side
   migration: postcopy_blocktime documentation
   migration: add blocktime calculation into migration-test
   migration: add postcopy total blocktime into query-migrate
>>>
>>> I had unanswered questions about these patches in the v12 series, where
>>> I'm not sure if the interface is still quite right.
>>
>> To be fair, I had alroady integrated the patches before I saw your questions.
>>
>>> We're still early
>>> enough that we could adjust the interface after the fact depending on
>>> how the questions are answered;
>>
>> I think this is the best approach, so far I can see two questions:
>>
>> - do we want to make it conditional?  it requires some locking, but I
>>   haven't meassured it to see how slow/fast is it.
>>
>> - the other was documentation.
>>
>> I will like Alexey to answer.  Depending of how slow it is, I can agree
>> to make it non-optional.
>
> So have you come to a consensus on whether you'd like me to apply
> this pull request or not ?

My understanding is that you should apply it.  We will add a
documentation patch later.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/6] hw/sd/milkymist-memcard: split realize() out of SysBusDevice init()

[Philippe Mathieu-Daudé]

>> Creating the device (milkymist_memcard_create()) fails with an assertion:
>>   sysbus_mmio_map_common: Assertion `n >= 0 && n < dev->num_mmio' failed.
>
> Thanks for trying this.
>
> Odd 'make check-qtest-lm32' didn't catch this...

Oh it actually does... My bad!

  GTESTER check-qtest-lm32
qemu-system-lm32: hw/core/sysbus.c:131: sysbus_mmio_map_common:
Assertion `n >= 0 && n < dev->num_mmio' failed.
Broken pipe
tests/Makefile.include:854: recipe for target 'check-qtest-lm32' failed
make: *** [check-qtest-lm32] Error 1

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 5 January 2018 at 09:59, Juan Quintela  wrote:
> Eric Blake  wrote:
>> On 01/03/2018 03:38 AM, Juan Quintela wrote:
>>> Hi
>>>
>>> This are the changes for migration that are already reviewed.
>>>
>>> Please, apply.
>>>
>>
>>> Alexey Perevalov (6):
>>>   migration: introduce postcopy-blocktime capability
>>>   migration: add postcopy blocktime ctx into MigrationIncomingState
>>>   migration: calculate vCPU blocktime on dst side
>>>   migration: postcopy_blocktime documentation
>>>   migration: add blocktime calculation into migration-test
>>>   migration: add postcopy total blocktime into query-migrate
>>
>> I had unanswered questions about these patches in the v12 series, where
>> I'm not sure if the interface is still quite right.
>
> To be fair, I had alroady integrated the patches before I saw your questions.
>
>> We're still early
>> enough that we could adjust the interface after the fact depending on
>> how the questions are answered;
>
> I think this is the best approach, so far I can see two questions:
>
> - do we want to make it conditional?  it requires some locking, but I
>   haven't meassured it to see how slow/fast is it.
>
> - the other was documentation.
>
> I will like Alexey to answer.  Depending of how slow it is, I can agree
> to make it non-optional.

So have you come to a consensus on whether you'd like me to apply
this pull request or not ?

thanks
-- PMM

Re: [Qemu-devel] [PULL] qemu-sparc updates

[Peter Maydell]

On 8 January 2018 at 19:31, Mark Cave-Ayland
 wrote:
> Hi Peter,
>
> Here is the first set of SPARC updates for 2.12. Please pull.
>
>
> ATB,
>
> Mark.
>
>
> The following changes since commit 4124ea4f5bd367ca6412fb2dfe7ac4d80e1504d9:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20171229' into 
> staging (2018-01-08 16:17:04 +)
>
> are available in the git repository at:
>
>   https://github.com/mcayland/qemu.git tags/qemu-sparc-signed
>
> for you to fetch changes up to 6a52624720e5abc6a1f067a7e7b8239b428e0c95:
>
>   sun4u_iommu: add trace event for IOMMU translations (2018-01-08 19:07:55 
> +)
>
> 
> qemu-sparc update
>
> 

Hi. This seems to crash in 'make check'. One of the crashes has a
memory corruption splat:

TEST: tests/device-introspect-test... (pid=20423)
  /sparc64/device/introspect/list: OK
  /sparc64/device/introspect/list-fields:  OK
  /sparc64/device/introspect/none: OK
  /sparc64/device/introspect/abstract: OK
  /sparc64/device/introspect/concrete:
*** Error in `sparc64-softmmu/qemu-system-spar
c64': corrupted double-linked list (not small): 0x010033b823a0 ***
=== Backtrace: =
/lib64/libc.so.6(+0xb0b94)[0x3fff90ce0b94]
/lib64/libc.so.6(+0xb5b18)[0x3fff90ce5b18]
/lib64/libc.so.6(__libc_calloc-0x14b664)[0x3fff90ce9934]
/lib64/libglib-2.0.so.0(g_malloc0-0x100d54)[0x3fff97a634d4]
sparc64-softmmu/qemu-system-sparc64[0x1030a9bc]
sparc64-softmmu/qemu-system-sparc64[0x103062c8]
sparc64-softmmu/qemu-system-sparc64[0x103062a0]

Running it under valgrind with
QTEST_QEMU_BINARY='valgrind sparc64-softmmu/qemu-system-sparc64'
./tests/device-introspect-test -p /sparc64/device/introspect/concrete

gives this write-after-free:

==1931== Invalid write of size 8
==1931==at 0x55EA51: pci_host_bus_register (pci.c:331)
==1931==by 0x55ECBD: pci_bus_init (pci.c:393)
==1931==by 0x55EE18: pci_bus_new (pci.c:424)
==1931==by 0x55EEE2: pci_register_bus (pci.c:447)
==1931==by 0x55D14F: pci_pbm_init (apb.c:464)
==1931==by 0x69179B: object_init_with_type (object.c:353)
==1931==by 0x6919D0: object_initialize_with_type (object.c:384)
==1931==by 0x691E3B: object_new_with_type (object.c:492)
==1931==by 0x691E78: object_new (object.c:502)
==1931==by 0x479A3C: qmp_device_list_properties (qmp.c:537)
==1931==by 0x455479: qdev_device_help (qdev-monitor.c:279)
==1931==by 0x456C9E: qmp_device_add (qdev-monitor.c:802)
==1931==  Address 0x2ca7af08 is 1,528 bytes inside a block of size 3,312 free'd
==1931==at 0x4C2EDEB: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1931==by 0x691DC6: object_finalize (object.c:480)
==1931==by 0x692CBD: object_unref (object.c:911)
==1931==by 0x479B91: qmp_device_list_properties (qmp.c:572)
==1931==by 0x469EA0: qmp_marshal_device_list_properties (qmp-marshal.c:1393)
==1931==by 0x7A25D2: do_qmp_dispatch (qmp-dispatch.c:104)
==1931==by 0x7A2703: qmp_dispatch (qmp-dispatch.c:131)
==1931==by 0x39E36D: handle_qmp_command (monitor.c:3839)
==1931==by 0x7AA357: json_message_process_token (json-streamer.c:105)
==1931==by 0x7D70CB: json_lexer_feed_char (json-lexer.c:323)
==1931==by 0x7D7213: json_lexer_feed (json-lexer.c:373)
==1931==by 0x7AA3FE: json_message_parser_feed (json-streamer.c:124)
==1931==  Block was alloc'd at
==1931==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1931==by 0x1C004718: g_malloc (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==1931==by 0x691E1C: object_new_with_type (object.c:491)
==1931==by 0x691E78: object_new (object.c:502)
==1931==by 0x479A3C: qmp_device_list_properties (qmp.c:537)
==1931==by 0x469EA0: qmp_marshal_device_list_properties (qmp-marshal.c:1393)
==1931==by 0x7A25D2: do_qmp_dispatch (qmp-dispatch.c:104)
==1931==by 0x7A2703: qmp_dispatch (qmp-dispatch.c:131)
==1931==by 0x39E36D: handle_qmp_command (monitor.c:3839)
==1931==by 0x7AA357: json_message_process_token (json-streamer.c:105)
==1931==by 0x7D70CB: json_lexer_feed_char (json-lexer.c:323)
==1931==by 0x7D7213: json_lexer_feed (json-lexer.c:373)



thanks
-- PMM

[Qemu-devel] [PULL v3] target/xtensa updates

[Max Filippov]

Hi Peter,

please pull the following batch of updates for the target/xtensa.
Changes v2->v3:
- Don't use g_malloc_n, use g_new instead.
Changes v1->v2:
- Drop no longer used function option_bits_enabled.

The following changes since commit 0a0dc59d27527b78a195c2d838d28b7b49e5a639:

  Update version for v2.11.0 release (2017-12-13 14:31:09 +)

are available in the git repository at:

  git://github.com/OSLL/qemu-xtensa.git tags/20180109-xtensa

for you to fetch changes up to 5a6539e627faf9251e1db78238b9f9b870610518:

  target/xtensa: implement disassembler (2018-01-09 09:55:39 -0800)


target/xtensa updates:

- add libisa to the xtensa target;
- change xtensa instruction translator to use it;
- switch existing xtensa cores to use it;
- add support for a number of instructions: salt/saltu, const16,
  GPIO32 group, debug mode and MMU-related;
- add disassembler for Xtensa.


Max Filippov (16):
  target/xtensa: pass actual frame size to the entry helper
  target/xtensa: import libisa source
  target/xtensa: extract core opcode translators
  target/xtensa: extract FPU2000 opcode translators
  target/xtensa: update import_core.sh script for libisa
  target/xtensa: switch dc232b to libisa
  target/xtensa: switch dc233c to libisa
  target/xtensa: switch fsf to libisa
  target/xtensa: use libisa for instruction decoding
  target/xtensa: tests: fix memctl SR test
  target/xtensa: drop DisasContext::litbase
  target/xtensa: add internal/noop SRs and opcodes
  target/xtensa: implement salt/saltu
  target/xtensa: implement GPIO32
  target/xtensa: implement const16
  target/xtensa: implement disassembler

 MAINTAINERS| 1 +
 disas/Makefile.objs| 1 +
 disas/xtensa.c |   133 +
 include/disas/bfd.h| 1 +
 include/hw/xtensa/xtensa-isa.h |   838 ++
 target/xtensa/Makefile.objs| 1 +
 target/xtensa/core-dc232b.c| 4 +
 target/xtensa/core-dc232b/xtensa-modules.c | 14105 +
 target/xtensa/core-dc233c.c| 4 +
 target/xtensa/core-dc233c/xtensa-modules.c | 15232 +++
 target/xtensa/core-fsf.c   | 5 +
 target/xtensa/core-fsf/xtensa-modules.c|  9841 +
 target/xtensa/cpu.c| 9 +
 target/xtensa/cpu.h|31 +
 target/xtensa/helper.c |37 +
 target/xtensa/import_core.sh   |15 +
 target/xtensa/op_helper.c  | 2 +-
 target/xtensa/translate.c  |  5900 +++
 target/xtensa/xtensa-isa-internal.h|   231 +
 target/xtensa/xtensa-isa.c |  1745 +++
 target/xtensa/xtensa-isa.h | 1 +
 tests/tcg/xtensa/test_sr.S | 2 +-
 22 files changed, 45955 insertions(+), 2184 deletions(-)
 create mode 100644 disas/xtensa.c
 create mode 100644 include/hw/xtensa/xtensa-isa.h
 create mode 100644 target/xtensa/core-dc232b/xtensa-modules.c
 create mode 100644 target/xtensa/core-dc233c/xtensa-modules.c
 create mode 100644 target/xtensa/core-fsf/xtensa-modules.c
 create mode 100644 target/xtensa/xtensa-isa-internal.h
 create mode 100644 target/xtensa/xtensa-isa.c
 create mode 100644 target/xtensa/xtensa-isa.h

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH 2/6] hw/sd/milkymist-memcard: split realize() out of SysBusDevice init()

[Philippe Mathieu-Daudé]

Hi Michael,

> Am 2018-01-03 17:23, schrieb Philippe Mathieu-Daudé:
>>
>> Create the SDCard in the realize() function.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/sd/milkymist-memcard.c | 24 +++-
>>  1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
>> index 5de3e00e2f..5df3a0f815 100644
>> --- a/hw/sd/milkymist-memcard.c
>> +++ b/hw/sd/milkymist-memcard.c
>> @@ -255,24 +255,29 @@ static void milkymist_memcard_reset(DeviceState *d)
>>  static int milkymist_memcard_init(SysBusDevice *dev)
>>  {
>>  MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
>> -DriveInfo *dinfo;
>> +
>> +memory_region_init_io(>regs_region, OBJECT(s), _mmio_ops,
>> s,
>> +"milkymist-memcard", R_MAX * 4);
>> +sysbus_init_mmio(dev, >regs_region);
>> +
>> +return 0;
>> +}
>
>
> Creating the device (milkymist_memcard_create()) fails with an assertion:
>   sysbus_mmio_map_common: Assertion `n >= 0 && n < dev->num_mmio' failed.

Thanks for trying this.

Odd 'make check-qtest-lm32' didn't catch this...

> because dev->num_mmio is still 0. Seems that sysbus_init_mmio() hasn't been
> called yet. Do we have to move the milkymist_memcard_init() to
> .instance_init?

Your patch seems correct :)

Do you have some milky one prebuilt image that I can use for testing?

Regards,

Phil.

>
> Eg.
>
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index 5df3a0f815..21c0e91926 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -252,15 +252,14 @@ static void milkymist_memcard_reset(DeviceState *d)
>  }
>  }
>
> -static int milkymist_memcard_init(SysBusDevice *dev)
> +static void milkymist_memcard_init(Object *obj)
>  {
> -MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
> +MilkymistMemcardState *s = MILKYMIST_MEMCARD(obj);
> +SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>
>  memory_region_init_io(>regs_region, OBJECT(s), _mmio_ops, s,
>  "milkymist-memcard", R_MAX * 4);
>  sysbus_init_mmio(dev, >regs_region);
> -
> -return 0;
>  }
>
>  static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
> @@ -300,9 +299,7 @@ static const VMStateDescription
> vmstate_milkymist_memcard = {
>  static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -k->init = milkymist_memcard_init;
>  dc->realize = milkymist_memcard_realize;
>  dc->reset = milkymist_memcard_reset;
>  dc->vmsd = _milkymist_memcard;
> @@ -314,6 +311,7 @@ static const TypeInfo milkymist_memcard_info = {
>  .name  = TYPE_MILKYMIST_MEMCARD,
>  .parent= TYPE_SYS_BUS_DEVICE,
>  .instance_size = sizeof(MilkymistMemcardState),
> +.instance_init = milkymist_memcard_init,
>  .class_init= milkymist_memcard_class_init,
>  };
>
> -michael
>
>
>> +
>> +static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
>> +{
>> +MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
>>  BlockBackend *blk;
>> +DriveInfo *dinfo;
>>
>>  /* FIXME use a qdev drive property instead of drive_get_next() */
>>  dinfo = drive_get_next(IF_SD);
>>  blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
>>  s->card = sd_init(blk, false);
>>  if (s->card == NULL) {
>> -return -1;
>> +error_setg(errp, "failed to init SD card");
>> +return;
>>  }
>> -
>>  s->enabled = blk && blk_is_inserted(blk);
>> -
>> -memory_region_init_io(>regs_region, OBJECT(s), _mmio_ops,
>> s,
>> -"milkymist-memcard", R_MAX * 4);
>> -sysbus_init_mmio(dev, >regs_region);
>> -
>> -return 0;
>>  }
>>
>>  static const VMStateDescription vmstate_milkymist_memcard = {
>> @@ -298,6 +303,7 @@ static void
>> milkymist_memcard_class_init(ObjectClass *klass, void *data)
>>  SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>>
>>  k->init = milkymist_memcard_init;
>> +dc->realize = milkymist_memcard_realize;
>>  dc->reset = milkymist_memcard_reset;
>>  dc->vmsd = _milkymist_memcard;
>>  /* Reason: init() method uses drive_get_next() */

Re: [Qemu-devel] [PATCH] scsi: Don't check uninitialized local variable

[Eric Blake]

On 01/09/2018 12:05 PM, Paolo Bonzini wrote:
> On 08/01/2018 04:09, Fam Zheng wrote:
>> After the out label there is a check on iTask.task but it is not
>> initialized yet.
>>
>> Fixes: e38bc23454ef763deb4405ebdee6a1081aa00bc8
>> Signed-off-by: Fam Zheng 
>> ---
>>  block/iscsi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 5c0a9e55b6..1cb8cc93c5 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -659,8 +659,7 @@ static int64_t coroutine_fn 
>> iscsi_co_get_block_status(BlockDriverState *bs,
>>  int64_t ret;
>>  
>>  if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
>> -ret = -EINVAL;
>> -goto out;
>> +return -EINVAL;
>>  }
>>  
>>  /* default to all sectors allocated */
>>
> 
> Queued, thanks.

I thought we wanted Peter's version, not Fam's.
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg01237.html

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL v2 00/16] target/xtensa updates

[Max Filippov]

On Tue, Jan 9, 2018 at 9:44 AM, Peter Maydell  wrote:
> /Users/pm215/src/qemu-for-merges/target/xtensa/helper.c:63:26: error:
> implicit declaration of function 'g_malloc_n' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
> config->opcode_ops = g_malloc_n(opcodes, sizeof(XtensaOpcodeOps *));
>  ^
>
> g_malloc_n() only came in in glib 2.24, so this doesn't build on
> glib 2.22 (our minimum required version).

Sorry about that, fixed.

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v6 21/29] hw/s390x: Replace fprintf(stderr, "*\n" with error_report()

[Alistair Francis]

On Tue, Jan 9, 2018 at 9:45 AM, Cornelia Huck  wrote:
> On Wed, 20 Dec 2017 09:24:41 -0800
> Alistair Francis  wrote:
>
>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> error_report(). The functions were renamed with these commands and then
>> compiler issues where manually fixed.
>>
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N;N; {s|fprintf(stderr, 
>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>> find ./* -type f -exec sed -i \
>> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>> {} +
>>
>> Some lines where then manually tweaked to pass checkpatch.
>>
>> One fprintf(stderr, was manually converted to a
>> qemu_log_mask(LOG_GUEST_ERROR,
>>
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Thomas Huth 
>> ---
>> V3:
>>  - Use a qemu_log_mask(LOG_GUEST_ERROR,
>> V2:
>>  - Split hw patch into individual directories
>>
>>  hw/s390x/virtio-ccw.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index 38f6a8afc9..3d8f26949b 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -426,8 +426,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>   * passes us zeroes for those we don't support.
>>   */
>>  if (features.features) {
>> -fprintf(stderr, "Guest bug: features[%i]=%x (expected 
>> 0)\n",
>> -features.index, features.features);
>> +qemu_log_mask(LOG_GUEST_ERROR,
>> +  "Guest bug: features[%i]=%x (expected 0)",
>> +  features.index, features.features);
>>  /* XXX: do a unit check here? */
>>  }
>>  }
>
> I'll queue this to s390-next so this doesn't get lost.
>
> I'll also tweak the commit message, as nothing is converted to
> error_report().

Thanks!

Alistair