[Qemu-devel] [PATCH] gtk: initialize zoom_to_fit

[Gerd Hoffmann]

Fixes: CID 1395988
Signed-off-by: Gerd Hoffmann 
---
 ui/gtk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 3ddb5fe162..ec935fff90 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2136,7 +2136,7 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, 
VirtualConsole *vc,
   QemuConsole *con, int idx,
   GSList *group, GtkWidget *view_menu)
 {
-bool zoom_to_fit;
+bool zoom_to_fit = false;
 
 vc->label = qemu_console_get_label(con);
 vc->s = s;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 4/4] cputlb: read CPUTLBEntry.addr_write atomically

[Emilio G. Cota]

On Thu, Oct 04, 2018 at 00:01:47 -0400, Emilio G. Cota wrote:
> Speedup over master
(snip)
> That is, a 5% average slowdown, with a max slowdown of ~14% for
> mcf :-(

png chart:
  https://imgur.com/a/5Jghi6Q

E.

Re: [Qemu-devel] [PATCH v2 4/4] cputlb: read CPUTLBEntry.addr_write atomically

[Emilio G. Cota]

On Wed, Oct 03, 2018 at 16:04:54 -0400, Emilio G. Cota wrote:
> Updates can come from other threads, so readers that do not
> take tlb_lock must use atomic_read to avoid undefined
> behaviour (UB).
> 
> This and the previous commit result in a small performance decrease,
> but this is a fair price for removing UB.
(snip)
> That is, a ~2% slowdown for the aarch64 bootup+shutdown test.

I've run more tests. This slowdown is much more pronounced on
memory-heavy workloads. These are the numbers for SPEC06int:

Speedup over master

  1.05 +-+--+++++++---++++++--+-+
   | +++  ||  +++   |
   |tlb-lock-noatomic  +++|  **|   |+++ |
   |  +atomic   |     |  **##  | |  |
 1 +-+..+++...++##.***#...|..**|#..**|+-+
   |### ***++ ***# *+*# +++  **+#  +++ **## |
   |# # *+*#  *|*# *+*#  ||  ** # **## **|# |
   |# # * *#+ *+*# * *#  ||  ** # **+#+**|# +**  ++###  |
  0.95 +-+..#.#.*.*#..*.*#.*.*#.***#.**.#.**.#.**|#..**##***+#+-+
   |# # * *#  * *# * *# *|*# ** # ** # **+#  **+#* * #  |
   |# # * *#  * *# * *# *|*# ** # ** # ** #+ ** #* * #  |
   0.9 +-+***.#..+++*.*#..*.*#.*.*#.*+*#.**.#.**.#.**.#+**|..**.#*.*.#+-+
   |  * * #***##* *#  * *# * *# * *# ** # ** # ** # **## ** #* * #  |
   |  * * #* *+#* *#   +++* *# * *# * *# ** # ** # ** # **|# ** #* * #  |
   |  * * #* * #* *# ***# * *# * *# *+*# ** # ** # ** # **+# ** #* * #  |
  0.85 +-+*.*.#*.*.#*.*#.*.*#+*.*#.*.*#.*.*#.**.#.**.#.**.#.**.#.**.#*.*.#+-+
   |  * * #* * #* *# * *# * *# * *# * *# ** # ** # ** # ** # ** #* * #  |
   |  * * #* * #* *# * *# * *# * *# * *# ** # ** # ** # ** # ** #* * #  |
   |  * * #* * #* *# * *# * *# * *# * *# ** # ** # ** # ** # ** #* * #  |
   0.8 +-+***##***##***#-***#-***#-***#-***#-**##-**##-**##-**##-**##***##+-+
401.bzi403.g429445.g456.462.libq464.h471.omn4483.xalancbgeomean

That is, a 5% average slowdown, with a max slowdown of ~14% for
mcf :-(

I'll profile tomorrow and see where the slowdown comes from.
If the lock is the issue, we might be better off shifting
all the work to the cross-vCPU call (e.g. doing a round of
synchronous cross-vCPU calls via run_on_cpu), if the assumption
that those calls are very rare is correct.

Emilio

[Qemu-devel] [PULL 0/4] Python queue, 2018-10-03

[Eduardo Habkost]

The following changes since commit dafd95053611aa14dda40266857608d12ddce658:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging 
(2018-10-02 18:27:18 +0100)

are available in the Git repository at:

  git://github.com/ehabkost/qemu.git tags/python-next-pull-request

for you to fetch changes up to cd0c3da730bc8369f0b6c1c6bf81babd9851060d:

  scripts/device-crash-test: Remove entries for serial devices (2018-10-03 
23:08:51 -0300)


Python queue, 2018-10-03

* Remove fixed serial device errors from device-crash-test
* Remove unnecessary Python 2.6 compatibility code



Eduardo Habkost (3):
  device-crash-test: No need for sys.path hack
  Revert "docker.py: Python 2.6 argparse compatibility"
  Revert "tests: migration/guestperf Python 2.6 argparse compatibility"

Thomas Huth (1):
  scripts/device-crash-test: Remove entries for serial devices

 scripts/device-crash-test  | 6 --
 tests/docker/docker.py | 4 +---
 tests/migration/guestperf/shell.py | 8 +++-
 3 files changed, 4 insertions(+), 14 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PULL 3/4] Revert "tests: migration/guestperf Python 2.6 argparse compatibility"

[Eduardo Habkost]

This reverts commit 0ea47d0f36112f0f38661e2e430edf32737c7f43.

scripts/argparse.py was removed from the tree, so we don't
need this hack anymore.

Signed-off-by: Eduardo Habkost 
Message-Id: <20180618225131.13113-4-ehabk...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Eduardo Habkost 
---
 tests/migration/guestperf/shell.py | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/migration/guestperf/shell.py 
b/tests/migration/guestperf/shell.py
index a6b8cec1e0..61d2abbaad 100644
--- a/tests/migration/guestperf/shell.py
+++ b/tests/migration/guestperf/shell.py
@@ -19,14 +19,12 @@ from __future__ import print_function
 #
 
 
-import os
-import os.path
-import sys
-sys.path.append(os.path.join(os.path.dirname(__file__),
- '..', '..', '..', 'scripts'))
 import argparse
 import fnmatch
+import os
+import os.path
 import platform
+import sys
 import logging
 
 from guestperf.hardware import Hardware
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PULL 2/4] Revert "docker.py: Python 2.6 argparse compatibility"

[Eduardo Habkost]

This reverts commit c2d3189667409561772e8c1e5615c5166cd8aa2c.

scripts/argparse.py was removed from the tree, so we don't need
this hack anymore.

Signed-off-by: Eduardo Habkost 
Message-Id: <20180618225131.13113-3-ehabk...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Eduardo Habkost 
---
 tests/docker/docker.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index d3006d4dae..44d5f7493b 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -14,14 +14,12 @@
 from __future__ import print_function
 import os
 import sys
-sys.path.append(os.path.join(os.path.dirname(__file__),
- '..', '..', 'scripts'))
-import argparse
 import subprocess
 import json
 import hashlib
 import atexit
 import uuid
+import argparse
 import tempfile
 import re
 import signal
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PULL 4/4] scripts/device-crash-test: Remove entries for serial devices

[Eduardo Habkost]

From: Thomas Huth 

The problem with the various serial devices has been fixed a while
ago in commit 47c4f85a0c27888e12af827471cfef87deb49821 ("hw/char/serial:
Allow disconnected chardevs") already, so we can remove these entries
from the "ignore" list in the device-crash-test script now.

Signed-off-by: Thomas Huth 
Message-Id: <1538403190-27146-1-git-send-email-th...@redhat.com>
Signed-off-by: Eduardo Habkost 
---
 scripts/device-crash-test | 5 -
 1 file changed, 5 deletions(-)

diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index da0bc5edd0..930200b034 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -98,7 +98,6 @@ ERROR_WHITELIST = [
 {'device':'isa-ipmi-bt', 'expected':True}, # IPMI device 
requires a bmc attribute to be set
 {'device':'isa-ipmi-kcs', 'expected':True},# IPMI device 
requires a bmc attribute to be set
 {'device':'isa-parallel', 'expected':True},# Can't create 
serial device, empty char device
-{'device':'isa-serial', 'expected':True},  # Can't create 
serial device, empty char device
 {'device':'ivshmem', 'expected':True}, # You must specify 
either 'shm' or 'chardev'
 {'device':'ivshmem-doorbell', 'expected':True},# You must specify 
a 'chardev'
 {'device':'ivshmem-plain', 'expected':True},   # You must specify 
a 'memdev'
@@ -109,9 +108,6 @@ ERROR_WHITELIST = [
 {'device':'pc-dimm', 'expected':True}, # 'memdev' property 
is not set
 {'device':'pci-bridge', 'expected':True},  # Bridge chassis 
not specified. Each bridge is required to be assigned a unique chassis id > 0.
 {'device':'pci-bridge-seat', 'expected':True}, # Bridge chassis 
not specified. Each bridge is required to be assigned a unique chassis id > 0.
-{'device':'pci-serial', 'expected':True},  # Can't create 
serial device, empty char device
-{'device':'pci-serial-2x', 'expected':True},   # Can't create 
serial device, empty char device
-{'device':'pci-serial-4x', 'expected':True},   # Can't create 
serial device, empty char device
 {'device':'pxa2xx-dma', 'expected':True},  # channels value 
invalid
 {'device':'pxb', 'expected':True}, # Bridge chassis 
not specified. Each bridge is required to be assigned a unique chassis id > 0.
 {'device':'scsi-block', 'expected':True},  # drive property 
not set
@@ -217,7 +213,6 @@ ERROR_WHITELIST = [
 {'exitcode':-6, 'log':r"Object .* is not an instance of type 
generic-pc-machine", 'loglevel':logging.ERROR},
 {'exitcode':-6, 'log':r"Object .* is not an instance of type e500-ccsr", 
'loglevel':logging.ERROR},
 {'exitcode':-6, 'log':r"vmstate_register_with_alias_id: Assertion 
`!se->compat \|\| se->instance_id == 0' failed", 'loglevel':logging.ERROR},
-{'exitcode':-11, 'device':'isa-serial', 'loglevel':logging.ERROR, 
'expected':True},
 
 # everything else (including SIGABRT and SIGSEGV) will be a fatal error:
 {'exitcode':None, 'fatal':True, 'loglevel':logging.FATAL},
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PULL 1/4] device-crash-test: No need for sys.path hack

[Eduardo Habkost]

The device-crash-test script is already inside the 'scripts'
directory, there's no need to add the directory manually to
sys.path.

Signed-off-by: Eduardo Habkost 
Message-Id: <20180618225131.13113-2-ehabk...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Eduardo Habkost 
---
 scripts/device-crash-test | 1 -
 1 file changed, 1 deletion(-)

diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 7045594bd4..da0bc5edd0 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -35,7 +35,6 @@ import random
 import argparse
 from itertools import chain
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'scripts'))
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [RFC] target/xtensa: rework zero overhead loops implementation

[Max Filippov]

Don't invalidate TB with the end of zero overhead loop when LBEG or LEND
change. Instead encode the distance from the TB start to the LEND in the
TB flags and generate loopback code when offset of the next PC from the
TB start equals that distance. Distance not greater than the maximal
instruction length is encoded literally, greater distances are capped at
the target page size and encoded as the maximal instruction length plus
the greatest power of 2 that is not bigger than the distance.

Although this change adds dynamic TB search at the end of each zero
overhead loop the resulting emulation speed is about 10% higher in
uClibc-ng and LTP tests.

Signed-off-by: Max Filippov 
---
 dtc   |  2 +-
 target/xtensa/cpu.h   | 11 ++
 target/xtensa/helper.h|  2 --
 target/xtensa/op_helper.c | 24 -
 target/xtensa/translate.c | 53 ---
 5 files changed, 34 insertions(+), 58 deletions(-)

diff --git a/dtc b/dtc
index 88f18909db73..e54388015af1 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 88f18909db731a627456f26d779445f84e449536
+Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 34e5ccd9f1d6..ad76d75aadde 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -694,6 +694,8 @@ static inline int cpu_mmu_index(CPUXtensaState *env, bool 
ifetch)
 #define XTENSA_TBFLAG_CWOE 0x4
 #define XTENSA_TBFLAG_CALLINC_MASK 0x18
 #define XTENSA_TBFLAG_CALLINC_SHIFT 19
+#define XTENSA_TBFLAG_LEND_MASK 0xfe0
+#define XTENSA_TBFLAG_LEND_SHIFT 21
 
 static inline void cpu_get_tb_cpu_state(CPUXtensaState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
@@ -706,6 +708,15 @@ static inline void cpu_get_tb_cpu_state(CPUXtensaState 
*env, target_ulong *pc,
 *flags |= xtensa_get_ring(env);
 if (env->sregs[PS] & PS_EXCM) {
 *flags |= XTENSA_TBFLAG_EXCM;
+} else if (xtensa_option_enabled(env->config, XTENSA_OPTION_LOOP)) {
+target_ulong lend_dist = env->sregs[LEND] - env->pc;
+
+if (lend_dist > (1u << TARGET_PAGE_BITS)) {
+lend_dist = MAX_INSN_LENGTH + 31 - TARGET_PAGE_BITS;
+} else if (lend_dist > MAX_INSN_LENGTH) {
+lend_dist = MAX_INSN_LENGTH + 31 - clz32(lend_dist);
+}
+*flags |= lend_dist << XTENSA_TBFLAG_LEND_SHIFT;
 }
 if (xtensa_option_enabled(env->config, XTENSA_OPTION_EXTENDED_L32R) &&
 (env->sregs[LITBASE] & 1)) {
diff --git a/target/xtensa/helper.h b/target/xtensa/helper.h
index 10153c245360..2ebba0b2c2bf 100644
--- a/target/xtensa/helper.h
+++ b/target/xtensa/helper.h
@@ -12,8 +12,6 @@ DEF_HELPER_2(rotw, void, env, i32)
 DEF_HELPER_3(window_check, noreturn, env, i32, i32)
 DEF_HELPER_1(restore_owb, void, env)
 DEF_HELPER_2(movsp, void, env, i32)
-DEF_HELPER_2(wsr_lbeg, void, env, i32)
-DEF_HELPER_2(wsr_lend, void, env, i32)
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_1(simcall, void, env)
 #endif
diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
index e4b42ab3e56c..078aeb6c2c94 100644
--- a/target/xtensa/op_helper.c
+++ b/target/xtensa/op_helper.c
@@ -107,13 +107,6 @@ static void tb_invalidate_virtual_addr(CPUXtensaState 
*env, uint32_t vaddr)
 }
 }
 
-#else
-
-static void tb_invalidate_virtual_addr(CPUXtensaState *env, uint32_t vaddr)
-{
-tb_invalidate_phys_addr(vaddr);
-}
-
 #endif
 
 void HELPER(exception)(CPUXtensaState *env, uint32_t excp)
@@ -370,23 +363,6 @@ void HELPER(movsp)(CPUXtensaState *env, uint32_t pc)
 }
 }
 
-void HELPER(wsr_lbeg)(CPUXtensaState *env, uint32_t v)
-{
-if (env->sregs[LBEG] != v) {
-tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
-env->sregs[LBEG] = v;
-}
-}
-
-void HELPER(wsr_lend)(CPUXtensaState *env, uint32_t v)
-{
-if (env->sregs[LEND] != v) {
-tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
-env->sregs[LEND] = v;
-tb_invalidate_virtual_addr(env, env->sregs[LEND] - 1);
-}
-}
-
 void HELPER(dump_state)(CPUXtensaState *env)
 {
 XtensaCPU *cpu = xtensa_env_get_cpu(env);
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 46e13384488e..c48285ce207e 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -54,7 +54,7 @@ struct DisasContext {
 int cring;
 int ring;
 uint32_t lbeg;
-uint32_t lend;
+uint32_t lend_dist;
 
 bool sar_5bit;
 bool sar_m32_5bit;
@@ -431,14 +431,13 @@ static void gen_callwi(DisasContext *dc, int callinc, 
uint32_t dest, int slot)
 
 static bool gen_check_loop_end(DisasContext *dc, int slot)
 {
-if (option_enabled(dc, XTENSA_OPTION_LOOP) &&
-!(dc->base.tb->flags & XTENSA_TBFLAG_EXCM) &&
-dc->base.pc_next == dc->lend) {
+if (dc->lend_dist && dc->lend_dist <= MAX_INSN_LENGTH &&
+dc->base.pc_next - dc->base.pc_first == dc->lend_dist) {
 TCGLabel *label = 

Re: [Qemu-devel] [PATCH v2 4/4] softfloat: Specialize udiv_qrnnd for ppc64

[David Gibson]

On Wed, Oct 03, 2018 at 01:07:11PM -0500, Richard Henderson wrote:
> The ISA has a 128/64-bit division instruction, though it assumes the
> low 64-bits of the numerator are 0, and so requires a bit more fixup
> than a full 128-bit division insn.
> 
> Cc: qemu-...@nongnu.org
> Cc: Alexander Graf 
> Cc: David Gibson 
> Signed-off-by: Richard Henderson 

Reviewed-by: David Gibson 

> ---
>  include/fpu/softfloat-macros.h | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
> index e702607b43..001bf4f23c 100644
> --- a/include/fpu/softfloat-macros.h
> +++ b/include/fpu/softfloat-macros.h
> @@ -632,6 +632,22 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t 
> n1,
>  asm("dlgr %0, %1" : "+r"(n) : "r"(d));
>  *r = n >> 64;
>  return n;
> +#elif defined(_ARCH_PPC64)
> +/* From Power ISA 3.0B, programming note for divdeu.  */
> +uint64_t q1, q2, Q, r1, r2, R;
> +asm("divdeu %0,%2,%4; divdu %1,%3,%4"
> +: "="(q1), "=r"(q2)
> +: "r"(n1), "r"(n0), "r"(d));
> +r1 = -(q1 * d); /* low part of (n1<<64) - (q1 * d) */
> +r2 = n0 - (q2 * d);
> +Q = q1 + q2;
> +R = r2 + r1;
> +if (R < r2 || R >= d) { /* overflow implies R > d */
> +Q += 1;
> +R -= d;
> +}
> +*r = R;
> +return Q;
>  #else
>  uint64_t d0, d1, q0, q1, r1, r0, m;
>  

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


signature.asc
Description: PGP signature

[Qemu-devel] [PULL 4/5] qemu-nbd: drop old-style negotiation

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

Use new-style negotiation always, with default "" (empty) export name
if it is not specified with '-x' option.

qemu as client can manage either style since 2.6.0, commit 69b49502d8

For comparison:

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

Furthermore, if a client that only speaks oldstyle still needs to
communicate to qemu, nbdkit remains available to perform the
translation between the two protocols.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20181003170228.95973-2-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
[eblake: enhance commit message]
Signed-off-by: Eric Blake 
---
 qemu-nbd.c | 25 ++---
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 66e023f7fa4..6aaebe7d938 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -56,7 +56,6 @@
 #define MBR_SIZE 512

 static NBDExport *exp;
-static bool newproto;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -84,8 +83,8 @@ static void usage(const char *name)
 "  -e, --shared=NUM  device can be shared by NUM clients (default 
'1')\n"
 "  -t, --persistent  don't exit on the last connection\n"
 "  -v, --verbose display extra debugging information\n"
-"  -x, --export-name=NAMEexpose export by name\n"
-"  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -x, --export-name=NAMEexpose export by name (default is empty string)\n"
+"  -D, --description=TEXTexport a human-readable description\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET   offset into the image\n"
@@ -355,8 +354,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,

 nb_fds++;
 nbd_update_server_watch();
-nbd_client_new(newproto ? NULL : exp, cioc,
-   tlscreds, NULL, nbd_client_closed);
+nbd_client_new(NULL, cioc, tlscreds, NULL, nbd_client_closed);
 }

 static void nbd_update_server_watch(void)
@@ -550,7 +548,7 @@ int main(int argc, char **argv)
 Error *local_err = NULL;
 BlockdevDetectZeroesOptions detect_zeroes = 
BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
 QDict *options = NULL;
-const char *export_name = NULL;
+const char *export_name = ""; /* Default export name */
 const char *export_description = NULL;
 const char *tlscredsid = NULL;
 bool imageOpts = false;
@@ -809,11 +807,6 @@ int main(int argc, char **argv)
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
 }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
 tlscreds = nbd_get_tls_creds(tlscredsid, _err);
 if (local_err) {
 error_report("Failed to get TLS creds %s",
@@ -1014,14 +1007,8 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 exit(EXIT_FAILURE);
 }
-if (export_name) {
-nbd_export_set_name(exp, export_name);
-nbd_export_set_description(exp, export_description);
-newproto = true;
-} else if (export_description) {
-error_report("Export description requires an export name");
-exit(EXIT_FAILURE);
-}
+nbd_export_set_name(exp, export_name);
+nbd_export_set_description(exp, export_description);

 if (device) {
 int ret;
-- 
2.17.1

[Qemu-devel] [PULL 1/5] nbd: Don't take address of fields in packed structs

[Eric Blake]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

This patch was produced with the following spatch script:
@@
expression E;
@@
-be16_to_cpus();
+E = be16_to_cpu(E);
@@
expression E;
@@
-be32_to_cpus();
+E = be32_to_cpu(E);
@@
expression E;
@@
-be64_to_cpus();
+E = be64_to_cpu(E);
@@
expression E;
@@
-cpu_to_be16s();
+E = cpu_to_be16(E);
@@
expression E;
@@
-cpu_to_be32s();
+E = cpu_to_be32(E);
@@
expression E;
@@
-cpu_to_be64s();
+E = cpu_to_be64(E);

Signed-off-by: Peter Maydell 
Message-Id: <20180927164200.15097-1-peter.mayd...@linaro.org>
Reviewed-by: Eric Blake 
[eblake: rebase, and squash in missed changes]
Signed-off-by: Eric Blake 
---
 nbd/client.c | 44 ++--
 nbd/server.c | 24 
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 40b74d9761f..b4d457a19ad 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -117,10 +117,10 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
uint32_t opt,
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be64_to_cpus(>magic);
-be32_to_cpus(>option);
-be32_to_cpus(>type);
-be32_to_cpus(>length);
+reply->magic = be64_to_cpu(reply->magic);
+reply->option = be32_to_cpu(reply->option);
+reply->type = be32_to_cpu(reply->type);
+reply->length = be32_to_cpu(reply->length);

 trace_nbd_receive_option_reply(reply->option, 
nbd_opt_lookup(reply->option),
reply->type, nbd_rep_lookup(reply->type),
@@ -396,7 +396,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
 return -1;
 }
 len -= sizeof(type);
-be16_to_cpus();
+type = be16_to_cpu(type);
 switch (type) {
 case NBD_INFO_EXPORT:
 if (len != sizeof(info->size) + sizeof(info->flags)) {
@@ -410,13 +410,13 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
*wantname,
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be64_to_cpus(>size);
+info->size = be64_to_cpu(info->size);
 if (nbd_read(ioc, >flags, sizeof(info->flags), errp) < 0) {
 error_prepend(errp, "failed to read info flags: ");
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be16_to_cpus(>flags);
+info->flags = be16_to_cpu(info->flags);
 trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
 break;

@@ -433,7 +433,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be32_to_cpus(>min_block);
+info->min_block = be32_to_cpu(info->min_block);
 if (!is_power_of_2(info->min_block)) {
 error_setg(errp, "server minimum block size %" PRIu32
" is not a power of two", info->min_block);
@@ -447,7 +447,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be32_to_cpus(>opt_block);
+info->opt_block = be32_to_cpu(info->opt_block);
 if (!is_power_of_2(info->opt_block) ||
 info->opt_block < info->min_block) {
 error_setg(errp, "server preferred block size %" PRIu32
@@ -461,7 +461,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
 nbd_send_opt_abort(ioc);
 return -1;
 }
-be32_to_cpus(>max_block);
+info->max_block = be32_to_cpu(info->max_block);
 if (info->max_block < info->min_block) {
 error_setg(errp, "server maximum block size %" PRIu32
" is not valid", info->max_block);
@@ -668,7 +668,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
*ioc,
 if (nbd_read(ioc, _id, sizeof(received_id), errp) < 0) {
 return -1;
 }
-be32_to_cpus(_id);
+received_id = be32_to_cpu(received_id);

 reply.length -= sizeof(received_id);
 name = g_malloc(reply.length + 1);
@@ -872,13 +872,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name,
 error_prepend(errp, "Failed to read export length: ");
 goto fail;
 }
-be64_to_cpus(>size);
+info->size = be64_to_cpu(info->size);

 if (nbd_read(ioc, >flags, sizeof(info->flags), errp) < 0) {
 error_prepend(errp, "Failed to read export flags: ");
 goto fail;
 }
-

[Qemu-devel] [PULL 2/5] nbd/server: fix NBD_CMD_CACHE

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

We should not go to structured-read branch on CACHE command, fix that.

Bug introduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE"
with the whole feature and affects 3.0.0 release.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
CC: qemu-sta...@nongnu.org
Message-Id: <20181003144738.70670-1-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
[eblake: commit message typo fix]
Signed-off-by: Eric Blake 
---
 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 98d0fa25158..4fb247b1166 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2177,7 +2177,8 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient 
*client, NBDRequest *request,
 }

 if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) &&
-request->len) {
+request->len && request->type != NBD_CMD_CACHE)
+{
 return nbd_co_send_sparse_read(client, request->handle, request->from,
data, request->len, errp);
 }
-- 
2.17.1

[Qemu-devel] [PULL 3/5] qemu-nbd: Document --tls-creds

[Eric Blake]

Commit 145614a1 introduced --tls-creds and documented it in
qemu-nbd.texi, but forgot to document it in 'qemu-nbd --help'.

Signed-off-by: Eric Blake 
Message-Id: <20181003180426.602765-1-ebl...@redhat.com>
Reviewed-by: John Snow 
---
 qemu-nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..66e023f7fa4 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -94,6 +94,7 @@ static void usage(const char *name)
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
 "passwords and/or encryption keys\n"
+"  --tls-creds=IDuse id of an earlier --object to provide TLS\n"
 "  -T, --trace [[enable=]][,events=][,file=]\n"
 "specify tracing options\n"
 "  --forkfork off the server process and exit the parent\n"
-- 
2.17.1

[Qemu-devel] [PULL 0/5] NBD patches through 2018-10-03

[Eric Blake]

The following changes since commit dafd95053611aa14dda40266857608d12ddce658:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging 
(2018-10-02 18:27:18 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-10-03

for you to fetch changes up to 7f7dfe2a53446072c136d349e3150c84d322b2bc:

  nbd/server: drop old-style negotiation (2018-10-03 15:52:32 -0500)


nbd patches for 2018-10-03

Fix bug in NBD_CMD_CACHE, drop support for oldstyle NBD server,
minor build and doc fixes

- Vladimir Sementsov-Ogievskiy: 0/2 server: drop old-style negotiation
- Eric Blake: qemu-nbd: Document --tls-creds
- Vladimir Sementsov-Ogievskiy: nbd/server: fix NBD_CMD_CACHE
- Peter Maydell: nbd: Don't take address of fields in packed structs


Eric Blake (1):
  qemu-nbd: Document --tls-creds

Peter Maydell (1):
  nbd: Don't take address of fields in packed structs

Vladimir Sementsov-Ogievskiy (3):
  nbd/server: fix NBD_CMD_CACHE
  qemu-nbd: drop old-style negotiation
  nbd/server: drop old-style negotiation

 include/block/nbd.h |  3 +-
 blockdev-nbd.c  |  3 +-
 nbd/client.c| 44 ++---
 nbd/server.c| 80 +++--
 qemu-nbd.c  | 26 +
 5 files changed, 60 insertions(+), 96 deletions(-)

-- 
2.17.1

[Qemu-devel] [PULL 5/5] nbd/server: drop old-style negotiation

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy 

After the previous commit, nbd_client_new's first parameter is always
NULL. Let's drop it with all corresponding old-style negotiation code
path which is unreachable now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20181003170228.95973-3-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
[eblake: re-wrap short line]
Signed-off-by: Eric Blake 
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c  |  3 +--
 nbd/server.c| 53 +
 qemu-nbd.c  |  2 +-
 4 files changed, 18 insertions(+), 43 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4638c839f51..0129d1a4b46 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -308,8 +308,7 @@ void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

-void nbd_client_new(NBDExport *exp,
-QIOChannelSocket *sioc,
+void nbd_client_new(QIOChannelSocket *sioc,
 QCryptoTLSCreds *tlscreds,
 const char *tlsaclname,
 void (*close_fn)(NBDClient *, bool));
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1ef11041a73..1d170c80b82 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -36,8 +36,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,
gpointer opaque)
 {
 qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-nbd_client_new(NULL, cioc,
-   nbd_server->tlscreds, NULL,
+nbd_client_new(cioc, nbd_server->tlscreds, NULL,
nbd_blockdev_client_closed);
 }

diff --git a/nbd/server.c b/nbd/server.c
index 4fb247b1166..a1eda0114fa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1253,7 +1253,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, 
Error **errp)
 const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
   NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
   NBD_FLAG_SEND_WRITE_ZEROES | 
NBD_FLAG_SEND_CACHE);
-bool oldStyle;

 /* Old style negotiation header, no room for options
 [ 0 ..   7]   passwd   ("NBDMAGIC")
@@ -1274,33 +1273,19 @@ static coroutine_fn int nbd_negotiate(NBDClient 
*client, Error **errp)
 trace_nbd_negotiate_begin();
 memcpy(buf, "NBDMAGIC", 8);

-oldStyle = client->exp != NULL && !client->tlscreds;
-if (oldStyle) {
-trace_nbd_negotiate_old_style(client->exp->size,
-  client->exp->nbdflags | myflags);
-stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
-stq_be_p(buf + 16, client->exp->size);
-stl_be_p(buf + 24, client->exp->nbdflags | myflags);
+stq_be_p(buf + 8, NBD_OPTS_MAGIC);
+stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES);

-if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
-error_prepend(errp, "write failed: ");
-return -EINVAL;
-}
-} else {
-stq_be_p(buf + 8, NBD_OPTS_MAGIC);
-stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES);
-
-if (nbd_write(client->ioc, buf, 18, errp) < 0) {
-error_prepend(errp, "write failed: ");
-return -EINVAL;
-}
-ret = nbd_negotiate_options(client, myflags, errp);
-if (ret != 0) {
-if (ret < 0) {
-error_prepend(errp, "option negotiation failed: ");
-}
-return ret;
+if (nbd_write(client->ioc, buf, 18, errp) < 0) {
+error_prepend(errp, "write failed: ");
+return -EINVAL;
+}
+ret = nbd_negotiate_options(client, myflags, errp);
+if (ret != 0) {
+if (ret < 0) {
+error_prepend(errp, "option negotiation failed: ");
 }
+return ret;
 }

 assert(!client->optlen);
@@ -2396,13 +2381,8 @@ static void nbd_client_receive_next_request(NBDClient 
*client)
 static coroutine_fn void nbd_co_client_start(void *opaque)
 {
 NBDClient *client = opaque;
-NBDExport *exp = client->exp;
 Error *local_err = NULL;

-if (exp) {
-nbd_export_get(exp);
-QTAILQ_INSERT_TAIL(>clients, client, next);
-}
 qemu_co_mutex_init(>send_lock);

 if (nbd_negotiate(client, _err)) {
@@ -2417,13 +2397,11 @@ static coroutine_fn void nbd_co_client_start(void 
*opaque)
 }

 /*
- * Create a new client listener on the given export @exp, using the
- * given channel @sioc.  Begin servicing it in a coroutine.  When the
- * connection closes, call @close_fn with an indication of whether the
- * client completed negotiation.
+ * Create a new client listener using the given channel @sioc.
+ * Begin servicing it in a coroutine.  When the connection closes, call
+ * @close_fn with an indication of whether the client completed negotiation.
  */
-void 

Re: [Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Eric Blake]

On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:

It's unexpected behavior that without -x option qemu-nbd do old-style
negotiation. Let's use "" as a default name instead (as it is already
done if tls is used) and therefore, drop old-style negotiation from
Qemu NBD server.

Vladimir Sementsov-Ogievskiy (2):
   qemu-nbd: drop old-style negotiation
   nbd/server: drop old-style negotiation

  include/block/nbd.h |  3 +--
  blockdev-nbd.c  |  2 +-
  nbd/server.c| 53 +
  qemu-nbd.c  | 25 +
  4 files changed, 23 insertions(+), 60 deletions(-)



Thanks; applying this series to my NBD queue.

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

Re: [Qemu-devel] [PATCH v2] bitmap: Update count after a merge

[John Snow]

On 10/02/2018 07:33 PM, John Snow wrote:
> From: Eric Blake 
> 
> We need an accurate count of the number of bits set in a bitmap
> after a merge. In particular, since the merge operation short-circuits
> a merge from an empty source, if you have bitmaps A, B, and C where
> B started empty, then merge C into B, and B into A, an inaccurate
> count meant that A did not get the contents of C.
> 
> In the worst case, we may falsely regard the bitmap as empty when
> it has had new writes merged into it.
> 
> Fixes: be58721db
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> Signed-off-by: John Snow 
> ---
> 
> v2: based off of Eric's cover letter, now rebased properly
> on top of the jsnow/bitmaps staging branch to use the
> correct bitmap target (result).
> 
> 
>  util/hbitmap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index d5aca5159f..8d402c59d9 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -759,6 +759,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, 
> HBitmap *result)
>  }
>  }
>  
> +/* Recompute the dirty count */
> +result->count = hb_count_between(result, 0, result->size - 1);
> +
>  return true;
>  }
>  
> 

Tests on the way, but for now...

Thanks, applied to my bitmaps tree:

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

--js

Re: [Qemu-devel] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Eric Blake]

On 10/3/18 12:55 PM, Eric Blake wrote:

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

qemu as client can manage either style since 2.6.0, commit 69b49502d8

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

Signed-off-by: Eric Blake 
---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
---
  qemu-nbd.c | 37 +++--
  1 file changed, 23 insertions(+), 14 deletions(-)


But I still forgot to document it in qemu-nbd.texi.

I'm dropping this patch in favor of Vladimir's stronger series, now that 
Rich has agreed with my observation that nbdkit can bridge the needs of 
any non-qemu client that still only talks oldstyle.


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

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-nbd: Document --tls-creds

[John Snow]

On 10/03/2018 04:42 PM, Eric Blake wrote:
> On 10/3/18 3:35 PM, John Snow wrote:
>>
>>
>> On 10/03/2018 02:04 PM, Eric Blake wrote:
>>> Commit 145614a1 introduced --tls-creds, but forgot to document
>>> it in 'qemu-nbd --help'.
>>>
>>> Signed-off-by: Eric Blake 
>>>
> 
>> Reviewed-by: John Snow 
>>
>> Do we have a manpage/texi that needs to update, too?
> 
> No; commit 145614a1 already covered that in qemu-nbd.texi.  It's just
> the --help output that needs it. I can tweak the commit message to
> mention that, if it helps.
> 

Not worth a re-spin.

--js

Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

On 10/02/2018 07:02 PM, John Snow wrote:
> based on: jsnow/bitmaps staging branch
> 
> This series builds on a previous standalone patch and adjusts
> the permission for all (or most) of the QMP bitmap commands.
> 
> V4:
>  - Replace "in-use" with "in use"
>  - Replace "user_modifiable" version with "user_locked"
>  - Remove more usages of frozen-and-or-locked in NBD.
> 
> John Snow (6):
>   block/dirty-bitmaps: add user_locked status checker
>   block/dirty-bitmaps: fix merge permissions
>   block/dirty-bitmaps: allow clear on disabled bitmaps
>   block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>   block/backup: prohibit backup from using in use bitmaps
>   nbd: forbid use of frozen bitmaps
> 
>  block/dirty-bitmap.c   | 13 +---
>  blockdev.c | 75 
> +++---
>  include/block/dirty-bitmap.h   |  1 +
>  migration/block-dirty-bitmap.c | 10 ++
>  nbd/server.c   |  4 +--
>  5 files changed, 48 insertions(+), 55 deletions(-)
> 

Thanks, applied to my bitmaps tree: [with edits suggested by Vladimir];

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

--js

Re: [Qemu-devel] [PATCH] qemu-nbd: Document --tls-creds

[Eric Blake]

On 10/3/18 1:04 PM, Eric Blake wrote:

Commit 145614a1 introduced --tls-creds, but forgot to document
it in 'qemu-nbd --help'.

Signed-off-by: Eric Blake 

---
Sadly, 'git grep -i "qemu.nbd.*tls"' has no hits, making me wonder
if our iotests are even covering this.

Noticed while writing my other patches for defaulting to newstyle.
---
  qemu-nbd.c | 1 +
  1 file changed, 1 insertion(+)


Thanks; applied to my NBD queue.

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

Re: [Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Richard W.M. Jones]

FWIW I don't have anything to add - agree with what's been said already.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-nbd: Document --tls-creds

[Eric Blake]

On 10/3/18 3:35 PM, John Snow wrote:



On 10/03/2018 02:04 PM, Eric Blake wrote:

Commit 145614a1 introduced --tls-creds, but forgot to document
it in 'qemu-nbd --help'.

Signed-off-by: Eric Blake 




Reviewed-by: John Snow 

Do we have a manpage/texi that needs to update, too?


No; commit 145614a1 already covered that in qemu-nbd.texi.  It's just 
the --help output that needs it. I can tweak the commit message to 
mention that, if it helps.


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

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-nbd: Document --tls-creds

[John Snow]

On 10/03/2018 02:04 PM, Eric Blake wrote:
> Commit 145614a1 introduced --tls-creds, but forgot to document
> it in 'qemu-nbd --help'.
> 
> Signed-off-by: Eric Blake 
> 
> ---
> Sadly, 'git grep -i "qemu.nbd.*tls"' has no hits, making me wonder
> if our iotests are even covering this.
> 
> Noticed while writing my other patches for defaulting to newstyle.
> ---
>  qemu-nbd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 51b9d38c727..66e023f7fa4 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -94,6 +94,7 @@ static void usage(const char *name)
>  "General purpose options:\n"
>  "  --object type,id=ID,...   define an object such as 'secret' for 
> providing\n"
>  "passwords and/or encryption keys\n"
> +"  --tls-creds=IDuse id of an earlier --object to provide TLS\n"
>  "  -T, --trace [[enable=]][,events=][,file=]\n"
>  "specify tracing options\n"
>  "  --forkfork off the server process and exit the 
> parent\n"
> 

Reviewed-by: John Snow 

Do we have a manpage/texi that needs to update, too?

Re: [Qemu-devel] [PATCH v2 2/4] cputlb: fix assert_cpu_is_self macro

[Richard Henderson]

On 10/3/18 3:04 PM, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota 
> ---
>  accel/tcg/cputlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 13/13] target/arm: Add v8M stack checks for MSR to SP_NS

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Updating the NS stack pointer via MSR to SP_NS should include
> a check whether the new SP value is below the stack limit.
> No other kinds of update to the various stack pointer and
> limit registers via MSR should perform a check.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for the VLDM/VSTM
> (aka VPUSH/VPOP) instructions. This code is currently
> unreachable because we haven't yet implemented M profile
> floating point support, but since the change is simple,
> we add it now because otherwise we're likely to forget to
> do it later.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 12 
>  1 file changed, 12 insertions(+)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 11/13] target/arm: Add v8M stack checks for Thumb push/pop

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add v8M stack checks for the 16-bit Thumb push/pop
> encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for the VLDM/VSTM
> (aka VPUSH/VPOP) instructions. This code is currently
> unreachable because we haven't yet implemented M profile
> floating point support, but since the change is simple,
> we add it now because otherwise we're likely to forget to
> do it later.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 12 
>  1 file changed, 12 insertions(+)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 10/13] target/arm: Add v8M stack checks for T32 load/store single

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add v8M stack checks for the instructions in the T32
> "load/store single" encoding class: these are the
> "immediate pre-indexed" and "immediate, post-indexed"
> LDR and STR instructions.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 23 ++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 09/13] target/arm: Add v8M stack checks for Thumb2 LDM/STM

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDM (T2 encoding)
>  * STM (T2 encoding)
> 
> This includes the 32-bit encodings of the instructions listed
> in v8M ARM ARM rule R_YVWT as
>  * LDM, LDMIA, LDMFD
>  * LDMDB, LDMEA
>  * POP (multiple registers)
>  * PUSH (muliple registers)
>  * STM, STMIA, STMEA
>  * STMDB, STMFD
> 
> We perform the stack limit before doing any other part
> of the load or store.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDRD (immediate)
>  * STRD (immediate)
> 
> Loads and stores are more complicated than ADD/SUB/MOV, because we
> must ensure that memory accesses below the stack limit are not
> performed, so we can't simply do the check when we actually update
> SP.
> 
> For these instructions, if the stack limit check triggers
> we must not:
>  * perform any memory access below the SP limit
>  * update PC, SP or the load/store base register
> but it is IMPDEF whether we:
>  * perform any accesses above or equal to the SP limit
>  * update destination registers for loads
> 
> For QEMU we choose to always check the limit before doing any other
> part of the load or store, so we won't update any registers or
> perform any memory accesses.
> 
> It is UNKNOWN whether the limit check triggers for a load or store
> where the initial SP value is below the limit and one of the stores
> would be below the limit, but the writeback moves SP to above the
> limit.  For QEMU we choose to trigger the check in this situation.
> 
> Note that limit checks happen only for loads and stores which update
> SP via writeback; they do not happen for loads and stores which
> simply use SP as a base register.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 27 +--
>  1 file changed, 25 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 07/13] target/arm: Add v8M stack limit checks on NS function calls

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Check the v8M stack limits when pushing the frame for a
> non-secure function call via BLXNS.
> 
> In order to be able to generate the exception we need to
> promote raise_exception() from being local to op_helper.c
> so we can call it from helper.c.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/internals.h | 9 +
>  target/arm/helper.c| 4 
>  target/arm/op_helper.c | 4 ++--
>  3 files changed, 15 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 06/13] target/arm: Add v8M stack checks on exception entry

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add checks for breaches of the v8M stack limit when the
> stack pointer is decremented to push the exception frame
> for exception entry.
> 
> Note that the exception-entry case is unique in that the
> stack pointer is updated to be the limit value if the limit
> is hit (per rule R_ZLZG).
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.c | 54 ++---
>  1 file changed, 46 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson 

r~

[Qemu-devel] [PATCH v2 0/4] per-TLB lock

[Emilio G. Cota]

v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00395.html

Changes since v1:

- Rebase on master

- Expand lock usage to other tlb_table/tlb_v_table updates, which I
  missed in v1

- Fix assert_cpu_is_self macro

- Add comment on why the owner thread doesn't need to use atomic_set
  for updates

- Add more calls to assert_cpu_is_self macro, which together with
  the added comment should make the code simpler to understand

- Include perf numbers in the last patch

The series is checkpatch-clean. You can fetch the code from:
  https://github.com/cota/qemu/tree/tlb-lock-v2

Thanks,

Emilio

[Qemu-devel] [PATCH v2 3/4] cputlb: serialize tlb updates with env->tlb_lock

[Emilio G. Cota]

Currently we rely on atomic operations for cross-CPU invalidations.
There are two cases that these atomics miss: cross-CPU invalidations
can race with either (1) vCPU threads flushing their TLB, which
happens via memset, or (2) vCPUs calling tlb_reset_dirty on their TLB,
which updates .addr_write with a regular store. This results in
undefined behaviour, since we're mixing regular and atomic ops
on concurrent accesses.

Fix it by using tlb_lock, a per-vCPU lock. All updaters of tlb_table
and the corresponding victim cache now hold the lock.
The readers that do not hold tlb_lock must use atomic reads when
reading .addr_write, since this field can be updated by other threads;
the conversion to atomic reads is done in the next patch.

Note that an alternative fix would be to expand the use of atomic ops.
However, in the case of TLB flushes this would have a huge performance
impact, since (1) TLB flushes can happen very frequently and (2) we
currently use a full memory barrier to flush each TLB entry, and a TLB
has many entries. Instead, acquiring the lock is barely slower than a
full memory barrier since it is uncontended, and with a single lock
acquisition we can flush the entire TLB.

Signed-off-by: Emilio G. Cota 
---
 include/exec/cpu-defs.h |   2 +
 accel/tcg/cputlb.c  | 153 ++--
 2 files changed, 87 insertions(+), 68 deletions(-)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index a171ffc1a4..bcc40c8ef5 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -142,6 +142,8 @@ typedef struct CPUIOTLBEntry {
 
 #define CPU_COMMON_TLB \
 /* The meaning of the MMU modes is defined in the target code. */   \
+/* tlb_lock serializes updates to tlb_table and tlb_v_table */  \
+QemuMutex tlb_lock; \
 CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE];  \
 CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE];   \
 CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];\
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f6b388c961..142a9cdf9e 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -75,6 +75,9 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
 
 void tlb_init(CPUState *cpu)
 {
+CPUArchState *env = cpu->env_ptr;
+
+qemu_mutex_init(>tlb_lock);
 }
 
 /* flush_all_helper: run fn across all cpus
@@ -129,8 +132,17 @@ static void tlb_flush_nocheck(CPUState *cpu)
 atomic_set(>tlb_flush_count, env->tlb_flush_count + 1);
 tlb_debug("(count: %zu)\n", tlb_flush_count());
 
+/*
+ * tlb_table/tlb_v_table updates from any thread must hold tlb_lock.
+ * However, updates from the owner thread (as is the case here; see the
+ * above assert_cpu_is_self) do not need atomic_set because all reads
+ * that do not hold the lock are performed by the same owner thread.
+ */
+qemu_mutex_lock(>tlb_lock);
 memset(env->tlb_table, -1, sizeof(env->tlb_table));
 memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
+qemu_mutex_unlock(>tlb_lock);
+
 cpu_tb_jmp_cache_clear(cpu);
 
 env->vtlb_index = 0;
@@ -182,6 +194,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, 
run_on_cpu_data data)
 
 tlb_debug("start: mmu_idx:0x%04lx\n", mmu_idx_bitmask);
 
+qemu_mutex_lock(>tlb_lock);
 for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
 
 if (test_bit(mmu_idx, _idx_bitmask)) {
@@ -191,6 +204,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, 
run_on_cpu_data data)
 memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
 }
 }
+qemu_mutex_unlock(>tlb_lock);
 
 cpu_tb_jmp_cache_clear(cpu);
 
@@ -247,22 +261,36 @@ static inline bool tlb_hit_page_anyprot(CPUTLBEntry 
*tlb_entry,
tlb_hit_page(tlb_entry->addr_code, page);
 }
 
-static inline void tlb_flush_entry(CPUTLBEntry *tlb_entry, target_ulong page)
+/* Called with tlb_lock held */
+static inline void tlb_flush_entry_locked(CPUTLBEntry *tlb_entry,
+  target_ulong page)
 {
 if (tlb_hit_page_anyprot(tlb_entry, page)) {
 memset(tlb_entry, -1, sizeof(*tlb_entry));
 }
 }
 
-static inline void tlb_flush_vtlb_page(CPUArchState *env, int mmu_idx,
-   target_ulong page)
+/* Called with tlb_lock held */
+static inline void tlb_flush_vtlb_page_locked(CPUArchState *env, int mmu_idx,
+  target_ulong page)
 {
 int k;
+
+assert_cpu_is_self(ENV_GET_CPU(env));
 for (k = 0; k < CPU_VTLB_SIZE; k++) {
-tlb_flush_entry(>tlb_v_table[mmu_idx][k], page);
+tlb_flush_entry_locked(>tlb_v_table[mmu_idx][k], page);
 }
 }
 
+static inline void tlb_flush_vtlb_page(CPUArchState *env, int mmu_idx,
+   target_ulong page)
+{
+assert_cpu_is_self(ENV_GET_CPU(env));
+ 

[Qemu-devel] ACPI PCI hotplug table updates

[open sorcerer]

Hi,

I am digging into an issue where qmp_device_del does not actually delete
devices when a guest OS is in prelaunch. This seems to be due to the guest
OS not handling ACPI events because it is not currently running. If I
assume correctly, qmp should allow you to add/remove devices while the host
is down, or if not possible, publish an error message.

I think fixing this issue is as simple as making sure that the VM is in a
safe state to ignore the hotplug ACPI dance but eject the disk, something
like:

prelaunch, preconfig, shutdown: ignore acpi and deal with cleaning devices
other non-running: bubble up error
running: default behavior

I was trying to validate that this change would be safe (keep in mind I am
learning ACPI in little pieces while digging) using GDB, and code
inspection. While stepping through with GDB i noticed that the PCI slots
are controlled by memory region and the opaque acpi pci hp state object. I
was unable this far to find any code executed that modifies the ACPI tables
beyond just the pci hotplug state.

I also tried to test using "while true; do acpidump | md5; sleep 1; done"
in the guest OS and then add/remove a virtio-blk-pci device (which
exercised the ACPI callbacks via piix4 callbacks). The output of the
acpidump -> md5 was consistent during each phase of the data collection
which I believe implied that the acpi tables were not modified by the PCI
hotplug.

Can someone help me understand:

1. Are the ACPI tables not modified when doing PCI hotplug?
2. Do the general changes proposed seem safe?
3. Are there resources or documentation I can read to help me understand
this problem further? I have skimmed through alot of different documents
and watched some youtube videos, but the ACPI documentation is hard to read
and sift through and the youtube videos are generally too high level.

Thanks.

[Qemu-devel] [PATCH v2 4/4] cputlb: read CPUTLBEntry.addr_write atomically

[Emilio G. Cota]

Updates can come from other threads, so readers that do not
take tlb_lock must use atomic_read to avoid undefined
behaviour (UB).

This and the previous commit result in a small performance decrease,
but this is a fair price for removing UB.

Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz

- Before:
 Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs):

   7482.981146  task-clock (msec) #0.998 CPUs utilized  
  ( +-  0.09% )
31,565,219,958  cycles#4.218 GHz
  ( +-  0.09% )
57,102,517,194  instructions  #1.81  insns per cycle
  ( +-  0.07% )
10,255,768,012  branches  # 1370.546 M/sec  
  ( +-  0.07% )
   172,980,542  branch-misses #1.69% of all branches
  ( +-  0.11% )

   7.494710830 seconds time elapsed 
 ( +-  0.09% )

- After:
 Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs):

   7649.735155  task-clock (msec) #0.999 CPUs utilized  
  ( +-  0.13% )
32,262,593,483  cycles#4.217 GHz
  ( +-  0.13% )
58,487,065,236  instructions  #1.81  insns per cycle
  ( +-  0.06% )
10,561,549,557  branches  # 1380.643 M/sec  
  ( +-  0.06% )
   173,995,793  branch-misses #1.65% of all branches
  ( +-  0.12% )

   7.660611466 seconds time elapsed 
 ( +-  0.13% )

That is, a ~2% slowdown for the aarch64 bootup+shutdown test.

Signed-off-by: Emilio G. Cota 
---
 accel/tcg/softmmu_template.h | 16 ++--
 include/exec/cpu_ldst.h  |  2 +-
 include/exec/cpu_ldst_template.h |  2 +-
 accel/tcg/cputlb.c   | 15 +--
 4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index f060a693d4..1e50263871 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -277,7 +277,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
 {
 unsigned mmu_idx = get_mmuidx(oi);
 int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+target_ulong tlb_addr =
+atomic_read(>tlb_table[mmu_idx][index].addr_write);
 unsigned a_bits = get_alignment_bits(get_memop(oi));
 uintptr_t haddr;
 
@@ -292,7 +293,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
 tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
  mmu_idx, retaddr);
 }
-tlb_addr = env->tlb_table[mmu_idx][index].addr_write & 
~TLB_INVALID_MASK;
+tlb_addr = atomic_read(>tlb_table[mmu_idx][index].addr_write) &
+~TLB_INVALID_MASK;
 }
 
 /* Handle an IO access.  */
@@ -321,7 +323,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
cannot evict the first.  */
 page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK;
 index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write;
+tlb_addr2 = atomic_read(>tlb_table[mmu_idx][index2].addr_write);
 if (!tlb_hit_page(tlb_addr2, page2)
 && !VICTIM_TLB_HIT(addr_write, page2)) {
 tlb_fill(ENV_GET_CPU(env), page2, DATA_SIZE, MMU_DATA_STORE,
@@ -354,7 +356,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
 {
 unsigned mmu_idx = get_mmuidx(oi);
 int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+target_ulong tlb_addr =
+atomic_read(>tlb_table[mmu_idx][index].addr_write);
 unsigned a_bits = get_alignment_bits(get_memop(oi));
 uintptr_t haddr;
 
@@ -369,7 +372,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
 tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
  mmu_idx, retaddr);
 }
-tlb_addr = env->tlb_table[mmu_idx][index].addr_write & 
~TLB_INVALID_MASK;
+tlb_addr = atomic_read(>tlb_table[mmu_idx][index].addr_write) &
+~TLB_INVALID_MASK;
 }
 
 /* Handle an IO access.  */
@@ -398,7 +402,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
addr, DATA_TYPE val,
cannot evict the first.  */
 page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK;
 index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write;
+tlb_addr2 = atomic_read(>tlb_table[mmu_idx][index2].addr_write);
 if 

[Qemu-devel] [PATCH v2 2/4] cputlb: fix assert_cpu_is_self macro

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota 
---
 accel/tcg/cputlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 502eea2850..f6b388c961 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -58,9 +58,9 @@
 } \
 } while (0)
 
-#define assert_cpu_is_self(this_cpu) do { \
+#define assert_cpu_is_self(cpu) do {  \
 if (DEBUG_TLB_GATE) { \
-g_assert(!cpu->created || qemu_cpu_is_self(cpu)); \
+g_assert(!(cpu)->created || qemu_cpu_is_self(cpu));   \
 } \
 } while (0)
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 1/4] exec: introduce tlb_init

[Emilio G. Cota]

Paves the way for the addition of a per-TLB lock.

Signed-off-by: Emilio G. Cota 
---
 include/exec/exec-all.h | 8 
 accel/tcg/cputlb.c  | 4 
 exec.c  | 1 +
 3 files changed, 13 insertions(+)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5f78125582..815e5b1e83 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -99,6 +99,11 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 
 #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
 /* cputlb.c */
+/**
+ * tlb_init - initialize a CPU's TLB
+ * @cpu: CPU whose TLB should be initialized
+ */
+void tlb_init(CPUState *cpu);
 /**
  * tlb_flush_page:
  * @cpu: CPU whose TLB should be flushed
@@ -258,6 +263,9 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
 void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
  uintptr_t retaddr);
 #else
+static inline void tlb_init(CPUState *cpu)
+{
+}
 static inline void tlb_flush_page(CPUState *cpu, target_ulong addr)
 {
 }
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f4702ce91f..502eea2850 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -73,6 +73,10 @@ QEMU_BUILD_BUG_ON(sizeof(target_ulong) > 
sizeof(run_on_cpu_data));
 QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
 #define ALL_MMUIDX_BITS ((1 << NB_MMU_MODES) - 1)
 
+void tlb_init(CPUState *cpu)
+{
+}
+
 /* flush_all_helper: run fn across all cpus
  *
  * If the wait flag is set then the src cpu's helper will be queued as
diff --git a/exec.c b/exec.c
index d0821e69aa..4fd831ef06 100644
--- a/exec.c
+++ b/exec.c
@@ -965,6 +965,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 tcg_target_initialized = true;
 cc->tcg_initialize();
 }
+tlb_init(cpu);
 
 #ifndef CONFIG_USER_ONLY
 if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 05/13] target/arm: Add some comments in Thumb decode

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add some comments to the Thumb decoder indicating what bits
> of the instruction have been decoded at various points in
> the code.
> 
> This is not an exhaustive set of comments; we're gradually
> adding comments as we work with particular bits of the code.
> 
> Signed-off-by: Peter Maydell 
> ---
> Specifically, I figured these out as I was going through looking
> for the insns which write SP. These comments turn out not to
> be relevant to those instructions, but I don't want to throw
> them away.
> ---
>  target/arm/translate.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH v2] bitmap: Update count after a merge

[Eric Blake]

On 10/3/18 2:57 PM, John Snow wrote:


    - I always forget to update this field.. We definitely should add some
generic check on it somewhere, at least in tests.


My suggestion (in another thread) was to enhance
x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the
checksum, to make it easier to write such tests.



I suppose this is in preference to query-block? It would make it easier.


Well, query-block remains the canonical way for users to access the 
count, while x-debug-block-dirty-bitmap-sha256 is for debugging only 
(basically, for our unit tests, as no one outside of qemu will get much 
use out of it).  But yes, writing a test that uses bitmaps on an 
unconnected BDS is easier than a test that has to use a named device, 
since query-block won't list nodes that aren't connected to a named 
device, and requires a lot more parsing to get at the actual bitmap 
information in comparison to the debug command directly telling you 
about a single bitmap.


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

Re: [Qemu-devel] [PATCH 04/13] target/arm: Add v8M stack checks on ADD/SUB/MOV of SP

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Add code to insert calls to a helper function to do the stack
> limit checking when we handle these forms of instruction
> that write to SP:
>  * ADD (SP plus immediate)
>  * ADD (SP plus register)
>  * SUB (SP minus immediate)
>  * SUB (SP minus register)
>  * MOV (register)
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.h|  2 ++
>  target/arm/internals.h | 14 
>  target/arm/op_helper.c | 19 ++
>  target/arm/translate.c | 80 +-
>  4 files changed, 106 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH v2] bitmap: Update count after a merge

[John Snow]

On 10/03/2018 10:49 AM, Eric Blake wrote:
> On 10/3/18 9:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 03.10.2018 02:33, John Snow wrote:
>>> From: Eric Blake 
>>>
>>> We need an accurate count of the number of bits set in a bitmap
>>> after a merge. In particular, since the merge operation short-circuits
>>> a merge from an empty source, if you have bitmaps A, B, and C where
>>> B started empty, then merge C into B, and B into A, an inaccurate
>>> count meant that A did not get the contents of C.
>>>
>>> In the worst case, we may falsely regard the bitmap as empty when
>>> it has had new writes merged into it.
>>>
>>> Fixes: be58721db
>>> CC: qemu-sta...@nongnu.org
>>> Signed-off-by: Eric Blake 
>>> Signed-off-by: John Snow 
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>
>> Hm, I rememberd:
>> commit 3260cdfffbf00f33923f5f9f6bef45932d7ac28b
>> Author: Liang Li 
>> Date:   Wed Feb 7 11:35:49 2018 -0500
>>
>>       hbitmap: fix missing restore count when finish deserialization
>>
>>       The .count of HBitmap is forgot to set in function
>>       hbitmap_deserialize_finish, let's set it to the right value.
>>
>>
>>    - I always forget to update this field.. We definitely should add some
>> generic check on it somewhere, at least in tests.
> 
> My suggestion (in another thread) was to enhance
> x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the
> checksum, to make it easier to write such tests.
> 

I suppose this is in preference to query-block? It would make it easier.

--js

Re: [Qemu-devel] [PATCH 03/13] target/arm: Move v7m_using_psp() to internals.h

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> We're going to want v7m_using_psp() in op_helper.c in the
> next patch, so move it from helper.c to internals.h.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/internals.h | 15 +++
>  target/arm/helper.c| 12 
>  2 files changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 02/13] target/arm: Define new EXCP type for v8M stack overflows

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> Define EXCP_STKOF, and arrange for it to cause us to take
> a UsageFault with CFSR.STKOF set.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/cpu.h| 2 ++
>  target/arm/helper.c | 5 +
>  2 files changed, 7 insertions(+)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH] target/arm: Don't read r4 from v8M exception stackframe twice

[Richard Henderson]

On 10/2/18 10:03 AM, Peter Maydell wrote:
> A cut-and-paste error meant we were reading r4 from the v8M
> callee-saves exception stack frame twice. This is harmless
> since it just meant we did two memory accesses to the same
> location, but it's unnecessary. Delete it.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [Qemu-devel] [PATCH 01/13] target/arm: Define new TBFLAG for v8M stack checking

[Richard Henderson]

On 10/2/18 11:35 AM, Peter Maydell wrote:
> The Arm v8M architecture includes hardware stack limit checking.
> When certain instructions update the stack pointer, if the new
> value of SP is below the limit set in the associated limit register
> then an exception is taken. Add a TB flag that tracks whether
> the limit-checking code needs to be emitted.
> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/cpu.h   |  7 +++
>  target/arm/translate.h |  1 +
>  target/arm/helper.c| 10 ++
>  target/arm/translate.c |  1 +
>  4 files changed, 19 insertions(+)

Reviewed-by: Richard Henderson 

r~

[Qemu-devel] [PATCH v3 9/9] target/s390x: Check HAVE_ATOMIC128 and HAVE_CMPXCHG128 at translate

[Richard Henderson]

Cc: qemu-s3...@nongnu.org
Signed-off-by: Richard Henderson 
---
 target/s390x/mem_helper.c | 40 +++
 target/s390x/translate.c  | 25 +---
 2 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index b5858d2fa2..490c43e6e6 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1420,9 +1420,7 @@ void HELPER(cdsg_parallel)(CPUS390XState *env, uint64_t 
addr,
 Int128 oldv;
 bool fail;
 
-if (!HAVE_CMPXCHG128) {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_CMPXCHG128);
 
 mem_idx = cpu_mmu_index(env, false);
 oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
@@ -2115,16 +2113,17 @@ uint64_t HELPER(lpq_parallel)(CPUS390XState *env, 
uint64_t addr)
 {
 uintptr_t ra = GETPC();
 uint64_t hi, lo;
+int mem_idx;
+TCGMemOpIdx oi;
+Int128 v;
 
-if (HAVE_ATOMIC128) {
-int mem_idx = cpu_mmu_index(env, false);
-TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
-Int128 v = helper_atomic_ldo_be_mmu(env, addr, oi, ra);
-hi = int128_gethi(v);
-lo = int128_getlo(v);
-} else {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_ATOMIC128);
+
+mem_idx = cpu_mmu_index(env, false);
+oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
+v = helper_atomic_ldo_be_mmu(env, addr, oi, ra);
+hi = int128_gethi(v);
+lo = int128_getlo(v);
 
 env->retxl = lo;
 return hi;
@@ -2145,15 +2144,16 @@ void HELPER(stpq_parallel)(CPUS390XState *env, uint64_t 
addr,
uint64_t low, uint64_t high)
 {
 uintptr_t ra = GETPC();
+int mem_idx;
+TCGMemOpIdx oi;
+Int128 v;
 
-if (HAVE_ATOMIC128) {
-int mem_idx = cpu_mmu_index(env, false);
-TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
-Int128 v = int128_make128(low, high);
-helper_atomic_sto_be_mmu(env, addr, v, oi, ra);
-} else {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_ATOMIC128);
+
+mem_idx = cpu_mmu_index(env, false);
+oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
+v = int128_make128(low, high);
+helper_atomic_sto_be_mmu(env, addr, v, oi, ra);
 }
 
 /* Execute instruction.  This instruction executes an insn modified with
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 7fad3ad8e9..57fe74e4a0 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -44,6 +44,7 @@
 #include "trace-tcg.h"
 #include "exec/translator.h"
 #include "exec/log.h"
+#include "qemu/atomic128.h"
 
 
 /* Information that (most) every instruction needs to manipulate.  */
@@ -2032,6 +2033,7 @@ static DisasJumpType op_cdsg(DisasContext *s, DisasOps *o)
 int r3 = get_field(s->fields, r3);
 int d2 = get_field(s->fields, d2);
 int b2 = get_field(s->fields, b2);
+DisasJumpType ret = DISAS_NEXT;
 TCGv_i64 addr;
 TCGv_i32 t_r1, t_r3;
 
@@ -2039,17 +2041,20 @@ static DisasJumpType op_cdsg(DisasContext *s, DisasOps 
*o)
 addr = get_address(s, 0, b2, d2);
 t_r1 = tcg_const_i32(r1);
 t_r3 = tcg_const_i32(r3);
-if (tb_cflags(s->base.tb) & CF_PARALLEL) {
+if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
+gen_helper_cdsg(cpu_env, addr, t_r1, t_r3);
+} else if (HAVE_CMPXCHG128) {
 gen_helper_cdsg_parallel(cpu_env, addr, t_r1, t_r3);
 } else {
-gen_helper_cdsg(cpu_env, addr, t_r1, t_r3);
+gen_helper_exit_atomic(cpu_env);
+ret = DISAS_NORETURN;
 }
 tcg_temp_free_i64(addr);
 tcg_temp_free_i32(t_r1);
 tcg_temp_free_i32(t_r3);
 
 set_cc_static(s);
-return DISAS_NEXT;
+return ret;
 }
 
 static DisasJumpType op_csst(DisasContext *s, DisasOps *o)
@@ -3036,10 +3041,13 @@ static DisasJumpType op_lpd(DisasContext *s, DisasOps 
*o)
 
 static DisasJumpType op_lpq(DisasContext *s, DisasOps *o)
 {
-if (tb_cflags(s->base.tb) & CF_PARALLEL) {
+if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
+gen_helper_lpq(o->out, cpu_env, o->in2);
+} else if (HAVE_ATOMIC128) {
 gen_helper_lpq_parallel(o->out, cpu_env, o->in2);
 } else {
-gen_helper_lpq(o->out, cpu_env, o->in2);
+gen_helper_exit_atomic(cpu_env);
+return DISAS_NORETURN;
 }
 return_low128(o->out2);
 return DISAS_NEXT;
@@ -4462,10 +4470,13 @@ static DisasJumpType op_stmh(DisasContext *s, DisasOps 
*o)
 
 static DisasJumpType op_stpq(DisasContext *s, DisasOps *o)
 {
-if (tb_cflags(s->base.tb) & CF_PARALLEL) {
+if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
+gen_helper_stpq(cpu_env, o->in2, o->out2, o->out);
+} else if (HAVE_ATOMIC128) {
 gen_helper_stpq_parallel(cpu_env, o->in2, o->out2, o->out);
 } else {
-gen_helper_stpq(cpu_env, o->in2, o->out2, o->out);
+

Re: [Qemu-devel] [PATCH] target/arm: Correct condition for v8M callee stack push

[Richard Henderson]

On 10/2/18 9:59 AM, Peter Maydell wrote:
> In v7m_exception_taken() we were incorrectly using a
> "LR bit EXCRET.ES is 1" check when it should be 0
> (compare the pseudocode ExceptionTaken() function).
> This meant we didn't stack the callee-saved registers
> when tailchaining from a NonSecure to a Secure exception.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

[Qemu-devel] [PATCH v3 3/9] target/arm: Convert to HAVE_CMPXCHG128

[Richard Henderson]

Reviewed-by: Emilio G. Cota 
Signed-off-by: Richard Henderson 
---
 target/arm/helper-a64.c | 259 +---
 1 file changed, 133 insertions(+), 126 deletions(-)

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 7f6ad3000b..6e4e1b8a19 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -30,6 +30,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "qemu/int128.h"
+#include "qemu/atomic128.h"
 #include "tcg.h"
 #include "fpu/softfloat.h"
 #include  /* For crc32 */
@@ -509,189 +510,195 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, 
uint32_t bytes)
 return crc32c(acc, buf, bytes) ^ 0x;
 }
 
-/* Returns 0 on success; 1 otherwise.  */
-static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
-   uint64_t new_lo, uint64_t new_hi,
-   bool parallel, uintptr_t ra)
+uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
+ uint64_t new_lo, uint64_t new_hi)
 {
-Int128 oldv, cmpv, newv;
+Int128 cmpv = int128_make128(env->exclusive_val, env->exclusive_high);
+Int128 newv = int128_make128(new_lo, new_hi);
+Int128 oldv;
+uintptr_t ra = GETPC();
+uint64_t o0, o1;
 bool success;
 
-cmpv = int128_make128(env->exclusive_val, env->exclusive_high);
-newv = int128_make128(new_lo, new_hi);
-
-if (parallel) {
-#ifndef CONFIG_ATOMIC128
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-#else
-int mem_idx = cpu_mmu_index(env, false);
-TCGMemOpIdx oi = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
-oldv = helper_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv, oi, ra);
-success = int128_eq(oldv, cmpv);
-#endif
-} else {
-uint64_t o0, o1;
-
 #ifdef CONFIG_USER_ONLY
-/* ??? Enforce alignment.  */
-uint64_t *haddr = g2h(addr);
+/* ??? Enforce alignment.  */
+uint64_t *haddr = g2h(addr);
 
-helper_retaddr = ra;
-o0 = ldq_le_p(haddr + 0);
-o1 = ldq_le_p(haddr + 1);
-oldv = int128_make128(o0, o1);
+helper_retaddr = ra;
+o0 = ldq_le_p(haddr + 0);
+o1 = ldq_le_p(haddr + 1);
+oldv = int128_make128(o0, o1);
 
-success = int128_eq(oldv, cmpv);
-if (success) {
-stq_le_p(haddr + 0, int128_getlo(newv));
-stq_le_p(haddr + 1, int128_gethi(newv));
-}
-helper_retaddr = 0;
-#else
-int mem_idx = cpu_mmu_index(env, false);
-TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
-TCGMemOpIdx oi1 = make_memop_idx(MO_LEQ, mem_idx);
-
-o0 = helper_le_ldq_mmu(env, addr + 0, oi0, ra);
-o1 = helper_le_ldq_mmu(env, addr + 8, oi1, ra);
-oldv = int128_make128(o0, o1);
-
-success = int128_eq(oldv, cmpv);
-if (success) {
-helper_le_stq_mmu(env, addr + 0, int128_getlo(newv), oi1, ra);
-helper_le_stq_mmu(env, addr + 8, int128_gethi(newv), oi1, ra);
-}
-#endif
+success = int128_eq(oldv, cmpv);
+if (success) {
+stq_le_p(haddr + 0, int128_getlo(newv));
+stq_le_p(haddr + 1, int128_gethi(newv));
 }
+helper_retaddr = 0;
+#else
+int mem_idx = cpu_mmu_index(env, false);
+TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
+TCGMemOpIdx oi1 = make_memop_idx(MO_LEQ, mem_idx);
+
+o0 = helper_le_ldq_mmu(env, addr + 0, oi0, ra);
+o1 = helper_le_ldq_mmu(env, addr + 8, oi1, ra);
+oldv = int128_make128(o0, o1);
+
+success = int128_eq(oldv, cmpv);
+if (success) {
+helper_le_stq_mmu(env, addr + 0, int128_getlo(newv), oi1, ra);
+helper_le_stq_mmu(env, addr + 8, int128_gethi(newv), oi1, ra);
+}
+#endif
 
 return !success;
 }
 
-uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
-  uint64_t new_lo, uint64_t new_hi)
-{
-return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false, GETPC());
-}
-
 uint64_t HELPER(paired_cmpxchg64_le_parallel)(CPUARMState *env, uint64_t addr,
   uint64_t new_lo, uint64_t new_hi)
-{
-return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true, GETPC());
-}
-
-static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
-   uint64_t new_lo, uint64_t new_hi,
-   bool parallel, uintptr_t ra)
 {
 Int128 oldv, cmpv, newv;
+uintptr_t ra = GETPC();
 bool success;
+int mem_idx;
+TCGMemOpIdx oi;
 
-/* high and low need to be switched here because this is not actually a
- * 128bit store but two doublewords stored consecutively
- */
-cmpv = int128_make128(env->exclusive_high, env->exclusive_val);
-newv = int128_make128(new_hi, new_lo);
-
-if (parallel) {
-#ifndef 

[Qemu-devel] [PATCH v3 5/9] target/ppc: Convert to HAVE_CMPXCHG128 and HAVE_ATOMIC128

[Richard Henderson]

Reviewed-by: Emilio G. Cota 
Signed-off-by: Richard Henderson 
---
 target/ppc/helper.h |   2 +-
 target/ppc/mem_helper.c |  33 ++--
 target/ppc/translate.c  | 115 +---
 3 files changed, 88 insertions(+), 62 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index ef64248bc4..7a1481fd0b 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -800,7 +800,7 @@ DEF_HELPER_4(dscliq, void, env, fprp, fprp, i32)
 DEF_HELPER_1(tbegin, void, env)
 DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
 
-#if defined(TARGET_PPC64) && defined(CONFIG_ATOMIC128)
+#ifdef TARGET_PPC64
 DEF_HELPER_FLAGS_3(lq_le_parallel, TCG_CALL_NO_WG, i64, env, tl, i32)
 DEF_HELPER_FLAGS_3(lq_be_parallel, TCG_CALL_NO_WG, i64, env, tl, i32)
 DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
index 8f0d86d104..a1485fad9b 100644
--- a/target/ppc/mem_helper.c
+++ b/target/ppc/mem_helper.c
@@ -25,6 +25,7 @@
 #include "exec/cpu_ldst.h"
 #include "tcg.h"
 #include "internal.h"
+#include "qemu/atomic128.h"
 
 //#define DEBUG_OP
 
@@ -215,11 +216,15 @@ target_ulong helper_lscbx(CPUPPCState *env, target_ulong 
addr, uint32_t reg,
 return i;
 }
 
-#if defined(TARGET_PPC64) && defined(CONFIG_ATOMIC128)
+#ifdef TARGET_PPC64
 uint64_t helper_lq_le_parallel(CPUPPCState *env, target_ulong addr,
uint32_t opidx)
 {
-Int128 ret = helper_atomic_ldo_le_mmu(env, addr, opidx, GETPC());
+Int128 ret;
+
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_ATOMIC128);
+ret = helper_atomic_ldo_le_mmu(env, addr, opidx, GETPC());
 env->retxh = int128_gethi(ret);
 return int128_getlo(ret);
 }
@@ -227,7 +232,11 @@ uint64_t helper_lq_le_parallel(CPUPPCState *env, 
target_ulong addr,
 uint64_t helper_lq_be_parallel(CPUPPCState *env, target_ulong addr,
uint32_t opidx)
 {
-Int128 ret = helper_atomic_ldo_be_mmu(env, addr, opidx, GETPC());
+Int128 ret;
+
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_ATOMIC128);
+ret = helper_atomic_ldo_be_mmu(env, addr, opidx, GETPC());
 env->retxh = int128_gethi(ret);
 return int128_getlo(ret);
 }
@@ -235,14 +244,22 @@ uint64_t helper_lq_be_parallel(CPUPPCState *env, 
target_ulong addr,
 void helper_stq_le_parallel(CPUPPCState *env, target_ulong addr,
 uint64_t lo, uint64_t hi, uint32_t opidx)
 {
-Int128 val = int128_make128(lo, hi);
+Int128 val;
+
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_ATOMIC128);
+val = int128_make128(lo, hi);
 helper_atomic_sto_le_mmu(env, addr, val, opidx, GETPC());
 }
 
 void helper_stq_be_parallel(CPUPPCState *env, target_ulong addr,
 uint64_t lo, uint64_t hi, uint32_t opidx)
 {
-Int128 val = int128_make128(lo, hi);
+Int128 val;
+
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_ATOMIC128);
+val = int128_make128(lo, hi);
 helper_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
 }
 
@@ -252,6 +269,9 @@ uint32_t helper_stqcx_le_parallel(CPUPPCState *env, 
target_ulong addr,
 {
 bool success = false;
 
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_CMPXCHG128);
+
 if (likely(addr == env->reserve_addr)) {
 Int128 oldv, cmpv, newv;
 
@@ -271,6 +291,9 @@ uint32_t helper_stqcx_be_parallel(CPUPPCState *env, 
target_ulong addr,
 {
 bool success = false;
 
+/* We will have raised EXCP_ATOMIC from the translator.  */
+assert(HAVE_CMPXCHG128);
+
 if (likely(addr == env->reserve_addr)) {
 Int128 oldv, cmpv, newv;
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 881743571b..4e59dd5f42 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -33,6 +33,7 @@
 #include "trace-tcg.h"
 #include "exec/translator.h"
 #include "exec/log.h"
+#include "qemu/atomic128.h"
 
 
 #define CPU_SINGLE_STEP 0x1
@@ -2654,22 +2655,22 @@ static void gen_lq(DisasContext *ctx)
 hi = cpu_gpr[rd];
 
 if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
-#ifdef CONFIG_ATOMIC128
-TCGv_i32 oi = tcg_temp_new_i32();
-if (ctx->le_mode) {
-tcg_gen_movi_i32(oi, make_memop_idx(MO_LEQ, ctx->mem_idx));
-gen_helper_lq_le_parallel(lo, cpu_env, EA, oi);
+if (HAVE_ATOMIC128) {
+TCGv_i32 oi = tcg_temp_new_i32();
+if (ctx->le_mode) {
+tcg_gen_movi_i32(oi, make_memop_idx(MO_LEQ, ctx->mem_idx));
+gen_helper_lq_le_parallel(lo, cpu_env, EA, oi);
+} else {
+tcg_gen_movi_i32(oi, make_memop_idx(MO_BEQ, ctx->mem_idx));
+gen_helper_lq_be_parallel(lo, cpu_env, EA, oi);
+}
+tcg_temp_free_i32(oi);
+tcg_gen_ld_i64(hi, cpu_env, 

[Qemu-devel] [PATCH v3 1/9] tcg: Split CONFIG_ATOMIC128

[Richard Henderson]

GCC7+ will no longer advertise support for 16-byte __atomic operations
if only cmpxchg is supported, as for x86_64.  Fortunately, x86_64 still
has support for __sync_compare_and_swap_16 and we can make use of that.
AArch64 does not have, nor ever has had such support, so open-code it.

Reviewed-by: Emilio G. Cota 
Signed-off-by: Richard Henderson 
---
 accel/tcg/atomic_template.h |  20 -
 include/qemu/atomic128.h| 155 
 tcg/tcg.h   |  16 ++--
 accel/tcg/cputlb.c  |   3 +-
 accel/tcg/user-exec.c   |   5 +-
 configure   |  19 +
 6 files changed, 204 insertions(+), 14 deletions(-)
 create mode 100644 include/qemu/atomic128.h

diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index d751bcba48..efde12fdb2 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -100,19 +100,24 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, 
target_ulong addr,
 DATA_TYPE ret;
 
 ATOMIC_TRACE_RMW;
+#if DATA_SIZE == 16
+ret = atomic16_cmpxchg(haddr, cmpv, newv);
+#else
 ret = atomic_cmpxchg__nocheck(haddr, cmpv, newv);
+#endif
 ATOMIC_MMU_CLEANUP;
 return ret;
 }
 
 #if DATA_SIZE >= 16
+#if HAVE_ATOMIC128
 ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
 {
 ATOMIC_MMU_DECLS;
 DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
 
 ATOMIC_TRACE_LD;
-__atomic_load(haddr, , __ATOMIC_RELAXED);
+val = atomic16_read(haddr);
 ATOMIC_MMU_CLEANUP;
 return val;
 }
@@ -124,9 +129,10 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
 DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
 
 ATOMIC_TRACE_ST;
-__atomic_store(haddr, , __ATOMIC_RELAXED);
+atomic16_set(haddr, val);
 ATOMIC_MMU_CLEANUP;
 }
+#endif
 #else
 ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
@@ -228,19 +234,24 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, 
target_ulong addr,
 DATA_TYPE ret;
 
 ATOMIC_TRACE_RMW;
+#if DATA_SIZE == 16
+ret = atomic16_cmpxchg(haddr, BSWAP(cmpv), BSWAP(newv));
+#else
 ret = atomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv));
+#endif
 ATOMIC_MMU_CLEANUP;
 return BSWAP(ret);
 }
 
 #if DATA_SIZE >= 16
+#if HAVE_ATOMIC128
 ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
 {
 ATOMIC_MMU_DECLS;
 DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
 
 ATOMIC_TRACE_LD;
-__atomic_load(haddr, , __ATOMIC_RELAXED);
+val = atomic16_read(haddr);
 ATOMIC_MMU_CLEANUP;
 return BSWAP(val);
 }
@@ -253,9 +264,10 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
 
 ATOMIC_TRACE_ST;
 val = BSWAP(val);
-__atomic_store(haddr, , __ATOMIC_RELAXED);
+atomic16_set(haddr, val);
 ATOMIC_MMU_CLEANUP;
 }
+#endif
 #else
 ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
diff --git a/include/qemu/atomic128.h b/include/qemu/atomic128.h
new file mode 100644
index 00..fdea225132
--- /dev/null
+++ b/include/qemu/atomic128.h
@@ -0,0 +1,155 @@
+/*
+ * Simple interface for 128-bit atomic operations.
+ *
+ * Copyright (C) 2018 Linaro, Ltd.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * See docs/devel/atomics.txt for discussion about the guarantees each
+ * atomic primitive is meant to provide.
+ */
+
+#ifndef QEMU_ATOMIC128_H
+#define QEMU_ATOMIC128_H
+
+/*
+ * GCC is a house divided about supporting large atomic operations.
+ *
+ * For hosts that only have large compare-and-swap, a legalistic reading
+ * of the C++ standard means that one cannot implement __atomic_read on
+ * read-only memory, and thus all atomic operations must synchronize
+ * through libatomic.
+ *
+ * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878
+ *
+ * This interpretation is not especially helpful for QEMU.
+ * For softmmu, all RAM is always read/write from the hypervisor.
+ * For user-only, if the guest doesn't implement such an __atomic_read
+ * then the host need not worry about it either.
+ *
+ * Moreover, using libatomic is not an option, because its interface is
+ * built for std::atomic, and requires that *all* accesses to such an
+ * object go through the library.  In our case we do not have an object
+ * in the C/C++ sense, but a view of memory as seen by the guest.
+ * The guest may issue a large atomic operation and then access those
+ * pieces using word-sized accesses.  From the hypervisor, we have no
+ * way to connect those two actions.
+ *
+ * Therefore, special case each platform.
+ */
+
+#if defined(CONFIG_ATOMIC128)
+static inline Int128 atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
+{
+return atomic_cmpxchg__nocheck(ptr, cmp, new);
+}
+# define HAVE_CMPXCHG128 1
+#elif defined(CONFIG_CMPXCHG128)
+static 

[Qemu-devel] [PATCH v3 7/9] target/s390x: Split do_cdsg, do_lpq, do_stpq

[Richard Henderson]

Cc: qemu-s3...@nongnu.org
Signed-off-by: Richard Henderson 
---
 target/s390x/mem_helper.c | 128 ++
 1 file changed, 61 insertions(+), 67 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index e106f61b4e..b5858d2fa2 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1380,57 +1380,58 @@ uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, 
uint32_t r2,
 return cc;
 }
 
-static void do_cdsg(CPUS390XState *env, uint64_t addr,
-uint32_t r1, uint32_t r3, bool parallel)
+void HELPER(cdsg)(CPUS390XState *env, uint64_t addr,
+  uint32_t r1, uint32_t r3)
 {
 uintptr_t ra = GETPC();
 Int128 cmpv = int128_make128(env->regs[r1 + 1], env->regs[r1]);
 Int128 newv = int128_make128(env->regs[r3 + 1], env->regs[r3]);
 Int128 oldv;
+uint64_t oldh, oldl;
 bool fail;
 
-if (parallel) {
-#if !HAVE_CMPXCHG128
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-#else
-int mem_idx = cpu_mmu_index(env, false);
-TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
-oldv = helper_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv, oi, ra);
-fail = !int128_eq(oldv, cmpv);
-#endif
-} else {
-uint64_t oldh, oldl;
+check_alignment(env, addr, 16, ra);
 
-check_alignment(env, addr, 16, ra);
+oldh = cpu_ldq_data_ra(env, addr + 0, ra);
+oldl = cpu_ldq_data_ra(env, addr + 8, ra);
 
-oldh = cpu_ldq_data_ra(env, addr + 0, ra);
-oldl = cpu_ldq_data_ra(env, addr + 8, ra);
-
-oldv = int128_make128(oldl, oldh);
-fail = !int128_eq(oldv, cmpv);
-if (fail) {
-newv = oldv;
-}
-
-cpu_stq_data_ra(env, addr + 0, int128_gethi(newv), ra);
-cpu_stq_data_ra(env, addr + 8, int128_getlo(newv), ra);
+oldv = int128_make128(oldl, oldh);
+fail = !int128_eq(oldv, cmpv);
+if (fail) {
+newv = oldv;
 }
 
+cpu_stq_data_ra(env, addr + 0, int128_gethi(newv), ra);
+cpu_stq_data_ra(env, addr + 8, int128_getlo(newv), ra);
+
 env->cc_op = fail;
 env->regs[r1] = int128_gethi(oldv);
 env->regs[r1 + 1] = int128_getlo(oldv);
 }
 
-void HELPER(cdsg)(CPUS390XState *env, uint64_t addr,
-  uint32_t r1, uint32_t r3)
-{
-do_cdsg(env, addr, r1, r3, false);
-}
-
 void HELPER(cdsg_parallel)(CPUS390XState *env, uint64_t addr,
uint32_t r1, uint32_t r3)
 {
-do_cdsg(env, addr, r1, r3, true);
+uintptr_t ra = GETPC();
+Int128 cmpv = int128_make128(env->regs[r1 + 1], env->regs[r1]);
+Int128 newv = int128_make128(env->regs[r3 + 1], env->regs[r3]);
+int mem_idx;
+TCGMemOpIdx oi;
+Int128 oldv;
+bool fail;
+
+if (!HAVE_CMPXCHG128) {
+cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
+}
+
+mem_idx = cpu_mmu_index(env, false);
+oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
+oldv = helper_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv, oi, ra);
+fail = !int128_eq(oldv, cmpv);
+
+env->cc_op = fail;
+env->regs[r1] = int128_gethi(oldv);
+env->regs[r1 + 1] = int128_getlo(oldv);
 }
 
 static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,
@@ -2097,16 +2098,25 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
 #endif
 
 /* load pair from quadword */
-static uint64_t do_lpq(CPUS390XState *env, uint64_t addr, bool parallel)
+uint64_t HELPER(lpq)(CPUS390XState *env, uint64_t addr)
 {
 uintptr_t ra = GETPC();
 uint64_t hi, lo;
 
-if (!parallel) {
-check_alignment(env, addr, 16, ra);
-hi = cpu_ldq_data_ra(env, addr + 0, ra);
-lo = cpu_ldq_data_ra(env, addr + 8, ra);
-} else if (HAVE_ATOMIC128) {
+check_alignment(env, addr, 16, ra);
+hi = cpu_ldq_data_ra(env, addr + 0, ra);
+lo = cpu_ldq_data_ra(env, addr + 8, ra);
+
+env->retxl = lo;
+return hi;
+}
+
+uint64_t HELPER(lpq_parallel)(CPUS390XState *env, uint64_t addr)
+{
+uintptr_t ra = GETPC();
+uint64_t hi, lo;
+
+if (HAVE_ATOMIC128) {
 int mem_idx = cpu_mmu_index(env, false);
 TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
 Int128 v = helper_atomic_ldo_be_mmu(env, addr, oi, ra);
@@ -2120,27 +2130,23 @@ static uint64_t do_lpq(CPUS390XState *env, uint64_t 
addr, bool parallel)
 return hi;
 }
 
-uint64_t HELPER(lpq)(CPUS390XState *env, uint64_t addr)
-{
-return do_lpq(env, addr, false);
-}
-
-uint64_t HELPER(lpq_parallel)(CPUS390XState *env, uint64_t addr)
-{
-return do_lpq(env, addr, true);
-}
-
 /* store pair to quadword */
-static void do_stpq(CPUS390XState *env, uint64_t addr,
-uint64_t low, uint64_t high, bool parallel)
+void HELPER(stpq)(CPUS390XState *env, uint64_t addr,
+  uint64_t low, uint64_t high)
 {
 uintptr_t ra = GETPC();
 
-if (!parallel) {
-check_alignment(env, addr, 16, ra);
-

[Qemu-devel] [PATCH v3 4/9] target/arm: Check HAVE_CMPXCHG128 at translate time

[Richard Henderson]

Reviewed-by: Emilio G. Cota 
Signed-off-by: Richard Henderson 
---
 target/arm/helper-a64.c| 16 
 target/arm/translate-a64.c | 38 ++
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 6e4e1b8a19..61799d20e1 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -563,9 +563,7 @@ uint64_t HELPER(paired_cmpxchg64_le_parallel)(CPUARMState 
*env, uint64_t addr,
 int mem_idx;
 TCGMemOpIdx oi;
 
-if (!HAVE_CMPXCHG128) {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_CMPXCHG128);
 
 mem_idx = cpu_mmu_index(env, false);
 oi = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
@@ -635,9 +633,7 @@ uint64_t HELPER(paired_cmpxchg64_be_parallel)(CPUARMState 
*env, uint64_t addr,
 int mem_idx;
 TCGMemOpIdx oi;
 
-if (!HAVE_CMPXCHG128) {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_CMPXCHG128);
 
 mem_idx = cpu_mmu_index(env, false);
 oi = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
@@ -663,9 +659,7 @@ void HELPER(casp_le_parallel)(CPUARMState *env, uint32_t 
rs, uint64_t addr,
 int mem_idx;
 TCGMemOpIdx oi;
 
-if (!HAVE_CMPXCHG128) {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_CMPXCHG128);
 
 mem_idx = cpu_mmu_index(env, false);
 oi = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
@@ -686,9 +680,7 @@ void HELPER(casp_be_parallel)(CPUARMState *env, uint32_t 
rs, uint64_t addr,
 int mem_idx;
 TCGMemOpIdx oi;
 
-if (!HAVE_CMPXCHG128) {
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-}
+assert(HAVE_CMPXCHG128);
 
 mem_idx = cpu_mmu_index(env, false);
 oi = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8ca3876707..77ee8d9085 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -37,6 +37,7 @@
 
 #include "trace-tcg.h"
 #include "translate-a64.h"
+#include "qemu/atomic128.h"
 
 static TCGv_i64 cpu_X[32];
 static TCGv_i64 cpu_pc;
@@ -2082,26 +2083,27 @@ static void gen_store_exclusive(DisasContext *s, int 
rd, int rt, int rt2,
get_mem_index(s),
MO_64 | MO_ALIGN | s->be_data);
 tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, cpu_exclusive_val);
-} else if (s->be_data == MO_LE) {
-if (tb_cflags(s->base.tb) & CF_PARALLEL) {
+} else if (tb_cflags(s->base.tb) & CF_PARALLEL) {
+if (!HAVE_CMPXCHG128) {
+gen_helper_exit_atomic(cpu_env);
+s->base.is_jmp = DISAS_NORETURN;
+} else if (s->be_data == MO_LE) {
 gen_helper_paired_cmpxchg64_le_parallel(tmp, cpu_env,
 cpu_exclusive_addr,
 cpu_reg(s, rt),
 cpu_reg(s, rt2));
 } else {
-gen_helper_paired_cmpxchg64_le(tmp, cpu_env, 
cpu_exclusive_addr,
-   cpu_reg(s, rt), cpu_reg(s, 
rt2));
-}
-} else {
-if (tb_cflags(s->base.tb) & CF_PARALLEL) {
 gen_helper_paired_cmpxchg64_be_parallel(tmp, cpu_env,
 cpu_exclusive_addr,
 cpu_reg(s, rt),
 cpu_reg(s, rt2));
-} else {
-gen_helper_paired_cmpxchg64_be(tmp, cpu_env, 
cpu_exclusive_addr,
-   cpu_reg(s, rt), cpu_reg(s, 
rt2));
 }
+} else if (s->be_data == MO_LE) {
+gen_helper_paired_cmpxchg64_le(tmp, cpu_env, cpu_exclusive_addr,
+   cpu_reg(s, rt), cpu_reg(s, rt2));
+} else {
+gen_helper_paired_cmpxchg64_be(tmp, cpu_env, cpu_exclusive_addr,
+   cpu_reg(s, rt), cpu_reg(s, rt2));
 }
 } else {
 tcg_gen_atomic_cmpxchg_i64(tmp, cpu_exclusive_addr, cpu_exclusive_val,
@@ -2171,14 +2173,18 @@ static void gen_compare_and_swap_pair(DisasContext *s, 
int rs, int rt,
 }
 tcg_temp_free_i64(cmp);
 } else if (tb_cflags(s->base.tb) & CF_PARALLEL) {
-TCGv_i32 tcg_rs = tcg_const_i32(rs);
-
-if (s->be_data == MO_LE) {
-gen_helper_casp_le_parallel(cpu_env, tcg_rs, addr, t1, t2);
+if (HAVE_CMPXCHG128) {
+TCGv_i32 tcg_rs = tcg_const_i32(rs);
+if (s->be_data == MO_LE) {
+gen_helper_casp_le_parallel(cpu_env, tcg_rs, addr, t1, t2);
+} else {
+gen_helper_casp_be_parallel(cpu_env, tcg_rs, 

[Qemu-devel] [PATCH v3 8/9] target/s390x: Skip wout, cout helpers if op helper does not return

[Richard Henderson]

When op raises an exception, it may not have initialized the output
temps that would be written back by wout or cout.

Cc: qemu-s3...@nongnu.org
Signed-off-by: Richard Henderson 
---
 target/s390x/translate.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 7363aabf3a..7fad3ad8e9 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, 
DisasContext *s)
 if (insn->help_op) {
 ret = insn->help_op(s, );
 }
-if (insn->help_wout) {
-insn->help_wout(s, , );
-}
-if (insn->help_cout) {
-insn->help_cout(s, );
+if (ret != DISAS_NORETURN) {
+if (insn->help_wout) {
+insn->help_wout(s, , );
+}
+if (insn->help_cout) {
+insn->help_cout(s, );
+}
 }
 
 /* Free any temporaries created by the helpers.  */
-- 
2.17.1

[Qemu-devel] [PATCH v3 0/9] tcg: Reorg 128-bit atomic operations

[Richard Henderson]

For v2, and history, see
  http://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg04533.html

Changes since v2:
  * Fixed a typo noticed by Emilio.
  * Brought the target/s390x changes back, as the patches with
which they conflicted are now in mainline.


r~


Richard Henderson (9):
  tcg: Split CONFIG_ATOMIC128
  target/i386: Convert to HAVE_CMPXCHG128
  target/arm: Convert to HAVE_CMPXCHG128
  target/arm: Check HAVE_CMPXCHG128 at translate time
  target/ppc: Convert to HAVE_CMPXCHG128 and HAVE_ATOMIC128
  target/s390x: Convert to HAVE_CMPXCHG128 and HAVE_ATOMIC128
  target/s390x: Split do_cdsg, do_lpq, do_stpq
  target/s390x: Skip wout, cout helpers if op helper does not return
  target/s390x: Check HAVE_ATOMIC128 and HAVE_CMPXCHG128 at translate

 accel/tcg/atomic_template.h |  20 ++-
 include/qemu/atomic128.h| 155 ++
 target/ppc/helper.h |   2 +-
 tcg/tcg.h   |  16 ++-
 accel/tcg/cputlb.c  |   3 +-
 accel/tcg/user-exec.c   |   5 +-
 target/arm/helper-a64.c | 251 ++--
 target/arm/translate-a64.c  |  38 +++---
 target/i386/mem_helper.c|   9 +-
 target/ppc/mem_helper.c |  33 -
 target/ppc/translate.c  | 115 +
 target/s390x/mem_helper.c   | 202 +
 target/s390x/translate.c|  37 --
 configure   |  19 +++
 14 files changed, 561 insertions(+), 344 deletions(-)
 create mode 100644 include/qemu/atomic128.h

-- 
2.17.1

[Qemu-devel] [PATCH v3 6/9] target/s390x: Convert to HAVE_CMPXCHG128 and HAVE_ATOMIC128

[Richard Henderson]

Cc: qemu-s3...@nongnu.org
Signed-off-by: Richard Henderson 
---
 target/s390x/mem_helper.c | 92 +--
 1 file changed, 41 insertions(+), 51 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index bacae4f503..e106f61b4e 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -25,6 +25,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "qemu/int128.h"
+#include "qemu/atomic128.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/storage-keys.h"
@@ -1389,7 +1390,7 @@ static void do_cdsg(CPUS390XState *env, uint64_t addr,
 bool fail;
 
 if (parallel) {
-#ifndef CONFIG_ATOMIC128
+#if !HAVE_CMPXCHG128
 cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
 #else
 int mem_idx = cpu_mmu_index(env, false);
@@ -1435,9 +1436,7 @@ void HELPER(cdsg_parallel)(CPUS390XState *env, uint64_t 
addr,
 static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,
 uint64_t a2, bool parallel)
 {
-#if !defined(CONFIG_USER_ONLY) || defined(CONFIG_ATOMIC128)
 uint32_t mem_idx = cpu_mmu_index(env, false);
-#endif
 uintptr_t ra = GETPC();
 uint32_t fc = extract32(env->regs[0], 0, 8);
 uint32_t sc = extract32(env->regs[0], 8, 8);
@@ -1465,18 +1464,20 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t 
r3, uint64_t a1,
 probe_write(env, a2, 0, mem_idx, ra);
 #endif
 
-/* Note that the compare-and-swap is atomic, and the store is atomic, but
-   the complete operation is not.  Therefore we do not need to assert 
serial
-   context in order to implement this.  That said, restart early if we 
can't
-   support either operation that is supposed to be atomic.  */
+/*
+ * Note that the compare-and-swap is atomic, and the store is atomic,
+ * but the complete operation is not.  Therefore we do not need to
+ * assert serial context in order to implement this.  That said,
+ * restart early if we can't support either operation that is supposed
+ * to be atomic.
+ */
 if (parallel) {
-int mask = 0;
-#if !defined(CONFIG_ATOMIC64)
-mask = -8;
-#elif !defined(CONFIG_ATOMIC128)
-mask = -16;
+uint32_t max = 2;
+#ifdef CONFIG_ATOMIC64
+max = 3;
 #endif
-if (((4 << fc) | (1 << sc)) & mask) {
+if ((HAVE_CMPXCHG128 ? 0 : fc + 2 > max) ||
+(HAVE_ATOMIC128  ? 0 : sc > max)) {
 cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
 }
 }
@@ -1546,16 +1547,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, 
uint64_t a1,
 Int128 cv = int128_make128(env->regs[r3 + 1], env->regs[r3]);
 Int128 ov;
 
-if (parallel) {
-#ifdef CONFIG_ATOMIC128
-TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
-ov = helper_atomic_cmpxchgo_be_mmu(env, a1, cv, nv, oi, ra);
-cc = !int128_eq(ov, cv);
-#else
-/* Note that we asserted !parallel above.  */
-g_assert_not_reached();
-#endif
-} else {
+if (!parallel) {
 uint64_t oh = cpu_ldq_data_ra(env, a1 + 0, ra);
 uint64_t ol = cpu_ldq_data_ra(env, a1 + 8, ra);
 
@@ -1567,6 +1559,13 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, 
uint64_t a1,
 
 cpu_stq_data_ra(env, a1 + 0, int128_gethi(nv), ra);
 cpu_stq_data_ra(env, a1 + 8, int128_getlo(nv), ra);
+} else if (HAVE_CMPXCHG128) {
+TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
+ov = helper_atomic_cmpxchgo_be_mmu(env, a1, cv, nv, oi, ra);
+cc = !int128_eq(ov, cv);
+} else {
+/* Note that we asserted !parallel above.  */
+g_assert_not_reached();
 }
 
 env->regs[r3 + 0] = int128_gethi(ov);
@@ -1596,18 +1595,16 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t 
r3, uint64_t a1,
 cpu_stq_data_ra(env, a2, svh, ra);
 break;
 case 4:
-if (parallel) {
-#ifdef CONFIG_ATOMIC128
+if (!parallel) {
+cpu_stq_data_ra(env, a2 + 0, svh, ra);
+cpu_stq_data_ra(env, a2 + 8, svl, ra);
+} else if (HAVE_ATOMIC128) {
 TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
 Int128 sv = int128_make128(svl, svh);
 helper_atomic_sto_be_mmu(env, a2, sv, oi, ra);
-#else
+} else {
 /* Note that we asserted !parallel above.  */
 g_assert_not_reached();
-#endif
-} else {
-cpu_stq_data_ra(env, a2 + 0, svh, ra);
-cpu_stq_data_ra(env, a2 + 8, svl, ra);
 }
 break;
 default:
@@ -2105,21 +2102,18 @@ static uint64_t do_lpq(CPUS390XState *env, uint64_t 
addr, bool 

[Qemu-devel] [PATCH v3 2/9] target/i386: Convert to HAVE_CMPXCHG128

[Richard Henderson]

Reviewed-by: Emilio G. Cota 
Signed-off-by: Richard Henderson 
---
 target/i386/mem_helper.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/i386/mem_helper.c b/target/i386/mem_helper.c
index 30c26b9d9c..6cc53bcb40 100644
--- a/target/i386/mem_helper.c
+++ b/target/i386/mem_helper.c
@@ -23,6 +23,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "qemu/int128.h"
+#include "qemu/atomic128.h"
 #include "tcg.h"
 
 void helper_cmpxchg8b_unlocked(CPUX86State *env, target_ulong a0)
@@ -137,10 +138,7 @@ void helper_cmpxchg16b(CPUX86State *env, target_ulong a0)
 
 if ((a0 & 0xf) != 0) {
 raise_exception_ra(env, EXCP0D_GPF, ra);
-} else {
-#ifndef CONFIG_ATOMIC128
-cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
-#else
+} else if (HAVE_CMPXCHG128) {
 int eflags = cpu_cc_compute_all(env, CC_OP);
 
 Int128 cmpv = int128_make128(env->regs[R_EAX], env->regs[R_EDX]);
@@ -159,7 +157,8 @@ void helper_cmpxchg16b(CPUX86State *env, target_ulong a0)
 eflags &= ~CC_Z;
 }
 CC_SRC = eflags;
-#endif
+} else {
+cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
 }
 }
 #endif
-- 
2.17.1

Re: [Qemu-devel] [PATCH] qapi/misc.json: Remove superfluous words in CpuModelExpansionType

[Eduardo Habkost]

On Wed, Oct 03, 2018 at 12:46:05PM +0200, Kashyap Chamarthy wrote:
> While at it, s/QMU/QEMU in @CpuDefinitionInfo.
> 
> Signed-off-by: Kashyap Chamarthy 

Reviewed-by: Eduardo Habkost 

Will this go through the QAPI tree?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 18/18] block/backup: use fleecing-hook instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

01.10.2018 13:29, Vladimir Sementsov-Ogievskiy wrote:
> Drop write notifiers and use filter node instead. Changes:
>
> 1. copy-before-writes now handled by filter node, so, drop all
> is_write_notifier arguments.
>
> 2. we don't have intersecting requests, so their handling is dropped.
> Instead, synchronization works as follows:
> when backup or fleecing-hook starts copying of some area it firstly
> clears copy-bitmap bits, and nobody touches areas, not marked with
> dirty bits in copy-bitmap, so there no intersection. Also, read
> requests are marked serializing, to not interfer with guest writes and
> not read changed data from source (before reading we clear
> corresponding bit in copy-bitmap, so, this area is not more handled by
> fleecing-hook).
>
> 3. To sync with in-flight requests we no just drain hook node, we don't
> need rw-lock.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>

There is a problem in this patch: to be sure, that if backup job started 
to read from source, guest will not write to the area, I use
1. dirty bitmap: if it is set this means there must not be any in flight 
write requests on source on this region
2. serialized read: we start it, so until it finishes nobody writes

But backup may restart copy operation if it failed. And of course, 
second try of read may appear after guest write.. I see the following 
way to fix:

don't retry the whole operation, but only read. Then all there reads 
will be serialized and no yield point between them, so, guest can't 
write until we finish..

in the same time, retrying of copy_range() operation should be OK as is, 
as it keeps read serialized requests during the whole operation.

so, a kind of refactoring needed here.

Or, is it better to create external interface for creating serialized 
requests? something like bdrv_co_pblockwrite, which will block writes on 
this region, by creating special serialized request, until paired 
bdrv_co_punblockwrite()?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[Eric Blake]

On 10/3/18 1:28 PM, John Snow wrote:



Thanks, I'll just make these edits and trust that Eric is fine with it
as well.


Yes, works for me



--js



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

Re: [Qemu-devel] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Vladimir Sementsov-Ogievskiy]

03.10.2018 20:55, Eric Blake wrote:

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

qemu as client can manage either style since 2.6.0, commit 69b49502d8

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

Signed-off-by: Eric Blake 
---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
---
 qemu-nbd.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..e35c97e7ca4 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -56,7 +56,7 @@
 #define MBR_SIZE 512

 static NBDExport *exp;
-static bool newproto;
+static bool oldstyle;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -84,8 +84,9 @@ static void usage(const char *name)
 "  -e, --shared=NUM  device can be shared by NUM clients (default 
'1')\n"
 "  -t, --persistent  don't exit on the last connection\n"
 "  -v, --verbose display extra debugging information\n"
-"  -x, --export-name=NAMEexpose export by name\n"
-"  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -x, --export-name=NAMEexpose export by name (default "")\n"
+"  -D, --description=TEXTexpose a human-readable description of export\n"
+"  -O, --oldstyleforce oldstyle (not with -x, -D, or 
--tls-creds)\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET   offset into the image\n"
@@ -354,7 +355,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,

 nb_fds++;
 nbd_update_server_watch();
-nbd_client_new(newproto ? NULL : exp, cioc,
+nbd_client_new(oldstyle ? exp : NULL, cioc,
tlscreds, NULL, nbd_client_closed);
 }

@@ -502,7 +503,7 @@ int main(int argc, char **argv)
 off_t fd_size;
 QemuOpts *sn_opts = NULL;
 const char *sn_id_or_name = NULL;
-const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
+const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:O";
 struct option lopt[] = {
 { "help", no_argument, NULL, 'h' },
 { "version", no_argument, NULL, 'V' },
@@ -529,6 +530,7 @@ int main(int argc, char **argv)
 { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
 { "export-name", required_argument, NULL, 'x' },
 { "description", required_argument, NULL, 'D' },
+{ "oldstyle", no_argument, NULL, 'O' },
 { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
 { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
 { "trace", required_argument, NULL, 'T' },
@@ -723,6 +725,9 @@ int main(int argc, char **argv)
 case 'D':
 export_description = optarg;
 break;
+case 'O':
+oldstyle = true;
+break;
 case 'v':
 verbose = 1;
 break;
@@ -799,7 +804,16 @@ int main(int argc, char **argv)
 }
 }

+if (!oldstyle && !export_name) {
+/* Set the default NBD protocol export name, to favor new style. */
+export_name = "";
+}

this




+
 if (tlscredsid) {
+if (oldstyle) {
+error_report("TLS is incompatible with oldstyle");
+exit(EXIT_FAILURE);
+}
 if (sockpath) {
 error_report("TLS is only supported with IPv4/IPv6");
 exit(EXIT_FAILURE);
@@ -808,11 +822,6 @@ int main(int argc, char **argv)
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
 }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
 tlscreds = nbd_get_tls_creds(tlscredsid, _err);
 if (local_err) {
 error_report("Failed to get TLS creds %s",
@@ -1013,13 +1022,13 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 exit(EXIT_FAILURE);
 }
+if (oldstyle && (export_name || export_description)) {
+error_report("oldstyle negotiation cannot set export details");
+exit(EXIT_FAILURE);
+}

and this are very simple option checks, so, it is better to place them together 
and after getopt loop, before the other more difficult logic.

but it's a nit-picking, so, with or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy 




 if 

Re: [Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker

[John Snow]

On 10/03/2018 08:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.10.2018 02:02, John Snow wrote:
>> Instead of both frozen and qmp_locked checks, wrap it into one check.
>> frozen implies the bitmap is split in two (for backup), and shouldn't
>> be modified. qmp_locked implies it's being used by another operation,
>> like being exported over NBD. In both cases it means we shouldn't allow
>> the user to modify it in any meaningful way.
>>
>> Replace any usages where we check both frozen and qmp_locked with the
>> new check.
>>
>> Signed-off-by: John Snow 
>> ---
>>  block/dirty-bitmap.c   |  6 ++
>>  blockdev.c | 29 -
>>  include/block/dirty-bitmap.h   |  1 +
>>  migration/block-dirty-bitmap.c | 10 ++
>>  4 files changed, 17 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8ac933cf1c..85bc668f6a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>>  return bitmap->successor;
>>  }
>>  
>> +/* Both conditions disallow user-modification via QMP. */
>> +bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
>> +return (bdrv_dirty_bitmap_frozen(bitmap) ||
>> +bdrv_dirty_bitmap_qmp_locked(bitmap));
> 
> hmm, extra parentheses

I'm fond of them, but I'll take them out.

> 
>> +}
>> +
>>  void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool 
>> qmp_locked)
>>  {
>>  qemu_mutex_lock(bitmap->mutex);
> 
> [...]
> 
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
>>  goto fail;
>>  }
>>  
>> -if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> -error_report("Can't migrate frozen dirty bitmap: '%s",
>> - bdrv_dirty_bitmap_name(bitmap));
>> -goto fail;
>> -}
>> -
>> -if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
>> -error_report("Can't migrate locked dirty bitmap: '%s",
>> +if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>> +error_report("Can't migrate a bitmap that is in use: '%s'",
> 
> "by another operation" like in other cases?
> 

Oh, sure. Can't hurt.

>>   bdrv_dirty_bitmap_name(bitmap));
>>  goto fail;
>>  }
> 
> anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
> -- 
> Best regards,
> Vladimir
> 

Thanks, I'll just make these edits and trust that Eric is fine with it
as well.

--js

[Qemu-devel] [RFC PATCH v2 1/3] acceptance tests: Add SeaBIOS boot and debug console checking test

[Philippe Mathieu-Daudé]

This test boots SeaBIOS and check the debug console (I/O port on the ISA bus)
reports enough information on the initialized devices.

Example:

$ avocado run tests/acceptance/boot_firmware.py
JOB ID : 3dac2e738c941747ec01f043092b882a8370a92f
JOB LOG: /home/phil/avocado/job-results/job-2018-10-03T19.42-3dac2e7/job.log
 (1/1) tests/acceptance/boot_firmware.py:BootFirmware.test_seabios: PASS (0.27 
s)
RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 0.56 s

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/boot_firmware.py | 74 +++
 1 file changed, 74 insertions(+)
 create mode 100644 tests/acceptance/boot_firmware.py

diff --git a/tests/acceptance/boot_firmware.py 
b/tests/acceptance/boot_firmware.py
new file mode 100644
index 00..669e4849f6
--- /dev/null
+++ b/tests/acceptance/boot_firmware.py
@@ -0,0 +1,74 @@
+# coding=utf-8
+#
+# Functional test that boots SeaBIOS and checks the console
+#
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+#  Philippe Mathieu-Daudé 
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import logging
+
+from avocado_qemu import Test
+from avocado.utils.wait import wait_for
+
+
+def read_console_for_string(console, expected_string, logger, 
ignore_ansi=True):
+msg = console.readline()
+if len(msg) == 0:
+return False
+if logger:
+logger.debug(msg.strip() if ignore_ansi and not '\x1b' in msg else msg)
+return expected_string in msg
+
+
+class BootFirmware(Test):
+"""
+Boots a firmware and checks via a console it is operational
+
+:avocado: enable
+"""
+
+def test_seabios(self):
+"""
+Boots SeaBIOS on a default PC machine, checks the debug console
+
+:avocado: tags=arch:x86_64
+:avocado: tags=maxtime:5s
+:avocado: tags=quick
+"""
+debugcon_path = os.path.join(self.workdir, 'debugconsole.log')
+serial_logger = logging.getLogger('serial')
+debugcon_logger = logging.getLogger('debugcon')
+
+self.vm.set_machine('pc')
+self.vm.set_console()
+self.vm.add_args('-nographic',
+ '-net', 'none',
+ '-global', 'isa-debugcon.iobase=0x402',
+ '-debugcon', 'file:%s' % debugcon_path)
+self.vm.launch()
+console = self.vm.console_socket.makefile()
+
+# serial console checks
+if not wait_for(read_console_for_string, timeout=5, step=0,
+args=(console, 'No bootable device.', serial_logger)):
+self.fail("SeaBIOS failed to boot")
+
+# debug console checks
+expected = [
+'Running on QEMU (i440fx)',
+'Turning on vga text mode console',
+'Found 1 lpt ports',
+'Found 1 serial ports',
+'PS2 keyboard initialized',
+]
+with open(debugcon_path) as debugcon:
+content = debugcon.readlines()
+for line in content: # TODO use FDDrainer
+debugcon_logger.debug(line.strip())
+for exp in expected:
+self.assertIn(exp + '\n', content)
-- 
2.17.1

[Qemu-devel] [RFC PATCH v2 3/3] acceptance tests: Add EDK2 ArmVirtQemu boot and console checking test

[Philippe Mathieu-Daudé]

This test boots EDK2 ArmVirtQemu and check the debug console (PL011) reports 
enough
information on the initialized devices.

$ avocado run -p qemu_bin=aarch64-softmmu/qemu-system-aarch64 
tests/acceptance/boot_firmware.py
JOB ID : cb1c5bd9e0312483eabeffbb37885a5273ef23bf
JOB LOG: /home/phil/avocado/job-results/job-2018-10-03T19.39-cb1c5bd/job.log
 (1/1) tests/acceptance/boot_firmware.py:BootFirmware.test_ovmf_virt: PASS 
(5.02 s)
RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 5.30 s

Signed-off-by: Philippe Mathieu-Daudé 
---
The '-p' option requires Avocado >= 65.0
---
 tests/acceptance/boot_firmware.py | 36 +++
 1 file changed, 36 insertions(+)

diff --git a/tests/acceptance/boot_firmware.py 
b/tests/acceptance/boot_firmware.py
index 2053b1a4b6..42f0672963 100644
--- a/tests/acceptance/boot_firmware.py
+++ b/tests/acceptance/boot_firmware.py
@@ -121,3 +121,39 @@ class BootFirmware(Test):
 debugcon_logger.debug(line.strip())
 for exp in expected:
 self.assertIn(exp + '\r\n', content)
+
+def test_ovmf_virt(self):
+"""
+Boots OVMF on the default virt machine, checks the debug console
+
+:avocado: tags=arch:aarch64
+:avocado: tags=maxtime:20s
+"""
+image_url = ('http://snapshots.linaro.org/components/kernel/'
+'leg-virt-tianocore-edk2-upstream/latest/'
+'QEMU-AARCH64/DEBUG_GCC5/QEMU_EFI.img.gz')
+image_path_gz = self.fetch_asset(image_url)
+image_path = os.path.join(self.workdir, 'flash.img')
+
+# kludge until Avocado support gzip files
+import gzip, shutil
+with gzip.open(image_path_gz) as gz, open(image_path, 'wb') as img:
+shutil.copyfileobj(gz, img)
+
+serial_path = os.path.join(self.workdir, 'serial.log')
+self.vm.set_machine('virt')
+self.vm.add_args('-nographic',
+ '-cpu', 'cortex-a57',
+ '-m', '1G', # 1GB min to boot fw?
+ '-drive', 'file=%s,format=raw,if=pflash' % image_path,
+ '-chardev', 'file,path=%s,id=console' % serial_path,
+ '-serial', 'chardev:console')
+self.vm.launch()
+serial_logger = logging.getLogger('serial')
+
+# serial console checks
+if not wait_for(read_console_for_string, timeout=15, step=0,
+args=(open(serial_path),
+  'Start PXE over IPv4',
+  serial_logger)):
+self.fail("OVMF failed to boot")
-- 
2.17.1

Re: [Qemu-devel] ACPI PCI hotplug table updates

[Michael S. Tsirkin]

On Wed, Oct 03, 2018 at 10:44:20AM -0700, open sorcerer wrote:
> Hi,
> 
> I am digging into an issue where qmp_device_del does not actually delete
> devices when a guest OS is in prelaunch.

What exactly is meant by prelaunch? E.g. is it prelaunch while bios is
doing the pci bus scan?

> This seems to be due to the guest OS
> not handling ACPI events because it is not currently running. If I assume
> correctly, qmp should allow you to add/remove devices while the host is down,
> or if not possible, publish an error message.
> 
> I think fixing this issue is as simple as making sure that the VM is in a safe
> state to ignore the hotplug ACPI dance but eject the disk, something like:
> 
> prelaunch, preconfig, shutdown: ignore acpi and deal with cleaning devices
> other non-running: bubble up error
> running: default behavior
> 
> I was trying to validate that this change would be safe (keep in mind I am
> learning ACPI in little pieces while digging) using GDB, and code inspection.
> While stepping through with GDB i noticed that the PCI slots are controlled by
> memory region and the opaque acpi pci hp state object. I was unable this far 
> to
> find any code executed that modifies the ACPI tables beyond just the pci
> hotplug state.
> 
> I also tried to test using "while true; do acpidump | md5; sleep 1; done" in
> the guest OS and then add/remove a virtio-blk-pci device (which exercised the
> ACPI callbacks via piix4 callbacks). The output of the acpidump -> md5 was
> consistent during each phase of the data collection which I believe implied
> that the acpi tables were not modified by the PCI hotplug.
> 
> Can someone help me understand:
> 
> 1. Are the ACPI tables not modified when doing PCI hotplug?

Yes.

> 2. Do the general changes proposed seem safe?
> 3. Are there resources or documentation I can read to help me understand this
> problem further? I have skimmed through alot of different documents and 
> watched
> some youtube videos, but the ACPI documentation is hard to read and sift
> through and the youtube videos are generally too high level.
> 
> Thanks.
> 

We are generally trying to move away from ACPI hotplug to native
PCIE hotplug. You can read up on that in the pci express spec.

-- 
MST

[Qemu-devel] [RFC PATCH v2 2/3] acceptance tests: Add EDK2 OVMF boot and debug console checking test

[Philippe Mathieu-Daudé]

This test boots OVMF and check the debug console (I/O port on the ISA bus)
report enough information on the initialized devices.

$ avocado --show=app,debugcon run tests/acceptance/boot_firmware.py
 (1/1) tests/acceptance/boot_firmware.py:BootFirmware.test_ovmf_pc:
debugcon: SecCoreStartupWithStack(0xFFFCC000, 0x82)
debugcon: SEC: Normal boot
...
debugcon: [Bds]Booting EFI Internal Shell
PASS (5.27 s)
JOB TIME   : 5.60 s

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/boot_firmware.py | 49 +++
 1 file changed, 49 insertions(+)

diff --git a/tests/acceptance/boot_firmware.py 
b/tests/acceptance/boot_firmware.py
index 669e4849f6..2053b1a4b6 100644
--- a/tests/acceptance/boot_firmware.py
+++ b/tests/acceptance/boot_firmware.py
@@ -72,3 +72,52 @@ class BootFirmware(Test):
 debugcon_logger.debug(line.strip())
 for exp in expected:
 self.assertIn(exp + '\n', content)
+
+def test_ovmf_pc(self):
+"""
+Boots OVMF on the default PC machine, checks the debug console
+
+:avocado: tags=arch:x86_64
+:avocado: tags=maxtime:10s
+"""
+debugcon_path = os.path.join(self.workdir, 'debugconsole.log')
+serial_logger = logging.getLogger('serial')
+debugcon_logger = logging.getLogger('debugcon')
+
+self.vm.set_machine('pc')
+self.vm.set_console()
+self.vm.add_args('-nographic',
+ '-net', 'none',
+ '-global', 'isa-debugcon.iobase=0x402',
+ '-debugcon', 'file:%s' % debugcon_path,
+ '--bios', '/usr/share/OVMF/OVMF_CODE.fd')
+self.vm.launch()
+console = self.vm.console_socket.makefile()
+
+# serial console checks
+if not wait_for(read_console_for_string, timeout=10, step=0,
+args=(console, 'EDK II', serial_logger)):
+self.fail("OVMF failed to boot")
+
+# debug console checks
+expected = [
+'SEC: Normal boot',
+'S3 support was detected on QEMU',
+'Platform PEI Firmware Volume Initialization',
+'DXE IPL Entry',
+'Installing FVB for EMU Variable support',
+'SmbiosCreateTable: Initialize 32-bit entry point structure',
+'PlatformBootManagerBeforeConsole',
+'OnRootBridgesConnected: root bridges have been connected, '
+'installing ACPI tables',
+'Found LPC Bridge device',
+'PlatformBootManagerAfterConsole',
+'EfiBootManagerConnectAll',
+'[Bds]Booting EFI Internal Shell',
+]
+with open(debugcon_path) as debugcon:
+content = debugcon.readlines()
+for line in content: # TODO use FDDrainer
+debugcon_logger.debug(line.strip())
+for exp in expected:
+self.assertIn(exp + '\r\n', content)
-- 
2.17.1

[Qemu-devel] [RFC PATCH v2 0/3] acceptance tests: Test firmware checking debug console output

[Philippe Mathieu-Daudé]

Hi,

This RFC series add simple acceptance tests which boot SeaBIOS and EDK2 on
the pc and virt/aarch64 default machines

Still PoC but can be useful for the Avocado team to test the multi-arch targets.

Since v1: https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg03780.html
- more Pythonic context managers (Cleber)
- use wait_for (Cleber)
- use workdir (Cleber)
- use Avocado 65.0 "-p" option for aarch64 (Cleber)
- fixed Q35 incorrect description in cover (Laszlo)
- use ArmVirtQemu name (Laszlo)

Next:
- address Laszlo comments about correct QEMU options and flash images
- use FDDrainer
- refactor common code in setUp()
- use new avocado.utils.archive gzip features (Cleber)
- test x86_64/aarch64 'arch' tags (Cleber)
- build EDK2 flash images (Laszlo)
- include variable store flash image (Laszlo)
- use virtual disk with UEFI shell script (Laszlo)
- improve OVMF binary selection (releases / snapshots) (Laszlo)
- check ISO images (Laszlo -> Cleber)
- add aexpect tests (Cleber?)

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  acceptance tests: Add SeaBIOS boot and debug console checking test
  acceptance tests: Add EDK2 OVMF boot and debug console checking test
  acceptance tests: Add EDK2 ArmVirtQemu boot and console checking test

 tests/acceptance/boot_firmware.py | 159 ++
 1 file changed, 159 insertions(+)
 create mode 100644 tests/acceptance/boot_firmware.py

-- 
2.17.1

Re: [Qemu-devel] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Vladimir Sementsov-Ogievskiy]

03.10.2018 20:55, Eric Blake wrote:
> Oldstyle NBD negotiation cannot perform any of the extensions that
> we have recently been relying on.  While you can always pass -x ""
> to get newstyle negotiation, these days, it is better to just default
> to newstyle (with an empty export name) and fall back to oldstyle
> only on an explicit request.
>
> qemu as client can manage either style since 2.6.0, commit 69b49502d8
>
> For comparison:
>
> nbdkit 1.3 switched its default to newstyle (Jan 2018):
> https://github.com/libguestfs/nbdkit/commit/b2a8aecc
> https://github.com/libguestfs/nbdkit/commit/8158e773
>
> nbd 3.10 dropped oldstyle long ago (Mar 2015):
> https://github.com/NetworkBlockDevice/nbd/commit/36940193
>
> Signed-off-by: Eric Blake 
> ---
> v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
> ---
>   qemu-nbd.c | 37 +++--
>   1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 51b9d38c727..e35c97e7ca4 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -56,7 +56,7 @@
>   #define MBR_SIZE 512
>
>   static NBDExport *exp;
> -static bool newproto;
> +static bool oldstyle;
>   static int verbose;
>   static char *srcpath;
>   static SocketAddress *saddr;
> @@ -84,8 +84,9 @@ static void usage(const char *name)
>   "  -e, --shared=NUM  device can be shared by NUM clients (default 
> '1')\n"
>   "  -t, --persistent  don't exit on the last connection\n"
>   "  -v, --verbose display extra debugging information\n"
> -"  -x, --export-name=NAMEexpose export by name\n"
> -"  -D, --description=TEXTwith -x, also export a human-readable 
> description\n"
> +"  -x, --export-name=NAMEexpose export by name (default "")\n"
> +"  -D, --description=TEXTexpose a human-readable description of export\n"
> +"  -O, --oldstyleforce oldstyle (not with -x, -D, or 
> --tls-creds)\n"
>   "\n"
>   "Exposing part of the image:\n"
>   "  -o, --offset=OFFSET   offset into the image\n"
> @@ -354,7 +355,7 @@ static void nbd_accept(QIONetListener *listener, 
> QIOChannelSocket *cioc,
>
>   nb_fds++;
>   nbd_update_server_watch();
> -nbd_client_new(newproto ? NULL : exp, cioc,
> +nbd_client_new(oldstyle ? exp : NULL, cioc,
>  tlscreds, NULL, nbd_client_closed);
>   }
>
> @@ -502,7 +503,7 @@ int main(int argc, char **argv)
>   off_t fd_size;
>   QemuOpts *sn_opts = NULL;
>   const char *sn_id_or_name = NULL;
> -const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
> +const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:O";
>   struct option lopt[] = {
>   { "help", no_argument, NULL, 'h' },
>   { "version", no_argument, NULL, 'V' },
> @@ -529,6 +530,7 @@ int main(int argc, char **argv)
>   { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
>   { "export-name", required_argument, NULL, 'x' },
>   { "description", required_argument, NULL, 'D' },
> +{ "oldstyle", no_argument, NULL, 'O' },
>   { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
>   { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>   { "trace", required_argument, NULL, 'T' },
> @@ -723,6 +725,9 @@ int main(int argc, char **argv)
>   case 'D':
>   export_description = optarg;
>   break;
> +case 'O':
> +oldstyle = true;
> +break;
>   case 'v':
>   verbose = 1;
>   break;
> @@ -799,7 +804,16 @@ int main(int argc, char **argv)
>   }
>   }
>
> +if (!oldstyle && !export_name) {
> +/* Set the default NBD protocol export name, to favor new style. */
> +export_name = "";
> +}
this


> +
>   if (tlscredsid) {
> +if (oldstyle) {
> +error_report("TLS is incompatible with oldstyle");
> +exit(EXIT_FAILURE);
> +}
>   if (sockpath) {
>   error_report("TLS is only supported with IPv4/IPv6");
>   exit(EXIT_FAILURE);
> @@ -808,11 +822,6 @@ int main(int argc, char **argv)
>   error_report("TLS is not supported with a host device");
>   exit(EXIT_FAILURE);
>   }
> -if (!export_name) {
> -/* Set the default NBD protocol export name, since
> - * we *must* use new style protocol for TLS */
> -export_name = "";
> -}
>   tlscreds = nbd_get_tls_creds(tlscredsid, _err);
>   if (local_err) {
>   error_report("Failed to get TLS creds %s",
> @@ -1013,13 +1022,13 @@ int main(int argc, char **argv)
>   error_report_err(local_err);
>   exit(EXIT_FAILURE);
>   }
> +if (oldstyle && (export_name || export_description)) {
> +error_report("oldstyle negotiation cannot set export details");
> +exit(EXIT_FAILURE);
> +}

and this are simple option 

Re: [Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Eric Blake]

On 10/3/18 12:59 PM, Vladimir Sementsov-Ogievskiy wrote:

03.10.2018 20:32, Eric Blake wrote:
On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:
It's unexpected behavior that without -x option qemu-nbd do old-style
negotiation. Let's use "" as a default name instead (as it is already
done if tls is used) and therefore, drop old-style negotiation from
Qemu NBD server.


Hmm, your email quoting style changed from prior emails that used to 
prepend '>' when quoting, making it harder to tell where the text you 
are quoting ends,...




Oddly enough, I wrote a similar patch in parallel, and am only now just seeing 
your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow 
explicit fallback to oldstyle, while you ripped it out altogether).  The client 
can negotiate either style, so we don't need an option on the client side; 
rather, this is all about what the server should do by default.

https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html

Does anyone have a preference between the two? Here's the last time it was 
discussed:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html



and your reply begins.


Hm, I think everyone who like new-style should answer that he don't care 
(except stricter way is a bit better: don't have options and code which we 
don't need).
But if there is someone who has client which support only old-style negotiation his 
unhappiness (in case of strict way) will outweigh all our "bits":)

I'm from the first group). But let's chose your patch


My argument in favor of your patch over mine: nbdkit is a GREAT testbed 
for forcing all sorts of integration testing scenarios, including 
oldstyle servers.  Also, it includes a plugin for translating between 
new and oldstyle at will.  That is, if we ever legitimately encounter a 
client that can only talk oldstyle, but qemu only talks newstyle, we 
just tell the user to connect:


old client => nbdkit -o nbd => newstyle qemu

and then qemu doesn't have to worry about oldstyle because nbdkit does 
instead.


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

Re: [Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock

[Emilio G. Cota]

On Wed, Oct 03, 2018 at 19:05:51 +0200, Paolo Bonzini wrote:
> On 03/10/2018 19:02, Emilio G. Cota wrote:
> >> For reads I agree, but you may actually get a torn read if the writer
> >> doesn't use atomic_set.
> >
> > But you cannot get a torn read if all reads that don't hold the lock
> > are coming from the same thread that performed the write.
> 
> Ah, so you are relying on copy_tlb_helper(_locked) being invoked only
> from the vCPU thread (as opposed to someone else doing tlb_flush)?

Yes. tlb_flush_nocheck is always run by the owner thread--tlb_flush
checks for this, and if !qemu_cpu_is_self(cpu) then it schedules
async work on the owner thread.

> Maybe it's worth adding a comment if that's what I missed.

I'll send a v2 with an updated comment and a debug-only assert
in the copy helper.

Thanks,

E.

[Qemu-devel] [PATCH v2 1/4] softfloat: Fix division

[Richard Henderson]

The __udiv_qrnnd primitive that we nicked from gmp requires its
inputs to be normalized.  We were not doing that.  Because the
inputs are nearly normalized already, finishing that is trivial.

Replace div128to64 with a "proper" udiv_qrnnd, so that this
remains a reusable primitive.

Fixes: cf07323d494
Fixes: https://bugs.launchpad.net/qemu/+bug/1793119
Signed-off-by: Richard Henderson 
---
 include/fpu/softfloat-macros.h |  7 ---
 fpu/softfloat.c| 35 ++
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index edc682139e..03312471b2 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -619,7 +619,8 @@ static inline uint64_t estimateDiv128To64(uint64_t a0, 
uint64_t a1, uint64_t b)
  *
  * Licensed under the GPLv2/LGPLv3
  */
-static inline uint64_t div128To64(uint64_t n0, uint64_t n1, uint64_t d)
+static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
+  uint64_t n0, uint64_t d)
 {
 uint64_t d0, d1, q0, q1, r1, r0, m;
 
@@ -658,8 +659,8 @@ static inline uint64_t div128To64(uint64_t n0, uint64_t n1, 
uint64_t d)
 }
 r0 -= m;
 
-/* Return remainder in LSB */
-return (q1 << 32) | q0 | (r0 != 0);
+*r = r0;
+return (q1 << 32) | q0;
 }
 
 /*
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index a06b6ef7e4..97ef66d570 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1112,19 +1112,38 @@ static FloatParts div_floats(FloatParts a, FloatParts 
b, float_status *s)
 bool sign = a.sign ^ b.sign;
 
 if (a.cls == float_class_normal && b.cls == float_class_normal) {
-uint64_t temp_lo, temp_hi;
+uint64_t n0, n1, q, r;
 int exp = a.exp - b.exp;
+
+/*
+ * We want a 2*N / N-bit division to produce exactly an N-bit
+ * result, so that we do not lose any precision and so that we
+ * do not have to renormalize afterward.  If A.frac < B.frac,
+ * then division would produce an (N-1)-bit result; shift A left
+ * by one to produce the an N-bit result, and decrement the
+ * exponent to match.
+ *
+ * The udiv_qrnnd algorithm that we're using requires normalization,
+ * i.e. the msb of the denominator must be set.  Since we know that
+ * DECOMPOSED_BINARY_POINT is msb-1, the inputs must be shifted left
+ * by one (more), and the remainder must be shifted right by one.
+ */
 if (a.frac < b.frac) {
 exp -= 1;
-shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 1,
-  _hi, _lo);
+shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 2, , 
);
 } else {
-shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT,
-  _hi, _lo);
+shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 1, , 
);
 }
-/* LSB of quot is set if inexact which roundandpack will use
- * to set flags. Yet again we re-use a for the result */
-a.frac = div128To64(temp_lo, temp_hi, b.frac);
+q = udiv_qrnnd(, n1, n0, b.frac << 1);
+
+/*
+ * Set lsb if there is a remainder, to set inexact.
+ * As mentioned above, to find the actual value of the remainder we
+ * would need to shift right, but (1) we are only concerned about
+ * non-zero-ness, and (2) the remainder will always be even because
+ * both inputs to the division primitive are even.
+ */
+a.frac = q | (r != 0);
 a.sign = sign;
 a.exp = exp;
 return a;
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/4] softfloat: Specialize udiv_qrnnd for x86_64

[Richard Henderson]

The ISA has a 128/64-bit division instruction.

Signed-off-by: Richard Henderson 
---
 include/fpu/softfloat-macros.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 03312471b2..6d58615709 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -622,6 +622,11 @@ static inline uint64_t estimateDiv128To64(uint64_t a0, 
uint64_t a1, uint64_t b)
 static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
   uint64_t n0, uint64_t d)
 {
+#if defined(__x86_64__)
+uint64_t q;
+asm("divq %4" : "=a"(q), "=d"(*r) : "0"(n0), "1"(n1), "rm"(d));
+return q;
+#else
 uint64_t d0, d1, q0, q1, r1, r0, m;
 
 d0 = (uint32_t)d;
@@ -661,6 +666,7 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
 
 *r = r0;
 return (q1 << 32) | q0;
+#endif
 }
 
 /*
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/4] softfloat: Specialize udiv_qrnnd for ppc64

[Richard Henderson]

The ISA has a 128/64-bit division instruction, though it assumes the
low 64-bits of the numerator are 0, and so requires a bit more fixup
than a full 128-bit division insn.

Cc: qemu-...@nongnu.org
Cc: Alexander Graf 
Cc: David Gibson 
Signed-off-by: Richard Henderson 
---
 include/fpu/softfloat-macros.h | 16 
 1 file changed, 16 insertions(+)

diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index e702607b43..001bf4f23c 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -632,6 +632,22 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
 asm("dlgr %0, %1" : "+r"(n) : "r"(d));
 *r = n >> 64;
 return n;
+#elif defined(_ARCH_PPC64)
+/* From Power ISA 3.0B, programming note for divdeu.  */
+uint64_t q1, q2, Q, r1, r2, R;
+asm("divdeu %0,%2,%4; divdu %1,%3,%4"
+: "="(q1), "=r"(q2)
+: "r"(n1), "r"(n0), "r"(d));
+r1 = -(q1 * d); /* low part of (n1<<64) - (q1 * d) */
+r2 = n0 - (q2 * d);
+Q = q1 + q2;
+R = r2 + r1;
+if (R < r2 || R >= d) { /* overflow implies R > d */
+Q += 1;
+R -= d;
+}
+*r = R;
+return Q;
 #else
 uint64_t d0, d1, q0, q1, r1, r0, m;
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/4] softfloat: Specialize udiv_qrnnd for s390x

[Richard Henderson]

The ISA has a 128/64-bit division instruction.

Cc: qemu-s3...@nongnu.org
Cc: Cornelia Huck 
Cc: David Hildenbrand 
Signed-off-by: Richard Henderson 
---
 include/fpu/softfloat-macros.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 6d58615709..e702607b43 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -626,6 +626,12 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
 uint64_t q;
 asm("divq %4" : "=a"(q), "=d"(*r) : "0"(n0), "1"(n1), "rm"(d));
 return q;
+#elif defined(__s390x__)
+/* Need to use a TImode type to get an even register pair for DLGR.  */
+unsigned __int128 n = (unsigned __int128)n1 << 64 | n0;
+asm("dlgr %0, %1" : "+r"(n) : "r"(d));
+*r = n >> 64;
+return n;
 #else
 uint64_t d0, d1, q0, q1, r1, r0, m;
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 0/4] softfloat: Fix division

[Richard Henderson]

Changes from v1:
  * Preserve udiv_qrnnd as a separate division primitive that
could be used as a building block for float128 division.
  * Include asm fragments for x86_64, s390x, and ppc64.


r~


Richard Henderson (4):
  softfloat: Fix division
  softfloat: Specialize udiv_qrnnd for x86_64
  softfloat: Specialize udiv_qrnnd for s390x
  softfloat: Specialize udiv_qrnnd for ppc64

 include/fpu/softfloat-macros.h | 35 +++---
 fpu/softfloat.c| 35 ++
 2 files changed, 59 insertions(+), 11 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH] qemu-nbd: Document --tls-creds

[Eric Blake]

Commit 145614a1 introduced --tls-creds, but forgot to document
it in 'qemu-nbd --help'.

Signed-off-by: Eric Blake 

---
Sadly, 'git grep -i "qemu.nbd.*tls"' has no hits, making me wonder
if our iotests are even covering this.

Noticed while writing my other patches for defaulting to newstyle.
---
 qemu-nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..66e023f7fa4 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -94,6 +94,7 @@ static void usage(const char *name)
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
 "passwords and/or encryption keys\n"
+"  --tls-creds=IDuse id of an earlier --object to provide TLS\n"
 "  -T, --trace [[enable=]][,events=][,file=]\n"
 "specify tracing options\n"
 "  --forkfork off the server process and exit the parent\n"
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Vladimir Sementsov-Ogievskiy]

03.10.2018 20:32, Eric Blake wrote:
On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:
It's unexpected behavior that without -x option qemu-nbd do old-style
negotiation. Let's use "" as a default name instead (as it is already
done if tls is used) and therefore, drop old-style negotiation from
Qemu NBD server.

Oddly enough, I wrote a similar patch in parallel, and am only now just seeing 
your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow 
explicit fallback to oldstyle, while you ripped it out altogether).  The client 
can negotiate either style, so we don't need an option on the client side; 
rather, this is all about what the server should do by default.

https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html

Does anyone have a preference between the two? Here's the last time it was 
discussed:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html

Hm, I think everyone who like new-style should answer that he don't care 
(except stricter way is a bit better: don't have options and code which we 
don't need).
But if there is someone who has client which support only old-style negotiation 
his unhappiness (in case of strict way) will outweigh all our "bits":)

I'm from the first group). But let's chose your patch



Vladimir Sementsov-Ogievskiy (2):
   qemu-nbd: drop old-style negotiation
   nbd/server: drop old-style negotiation

  include/block/nbd.h |  3 +--
  blockdev-nbd.c  |  2 +-
  nbd/server.c| 53 +
  qemu-nbd.c  | 25 +
  4 files changed, 23 insertions(+), 60 deletions(-)





--
Best regards,
Vladimir

[Qemu-devel] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Eric Blake]

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

qemu as client can manage either style since 2.6.0, commit 69b49502d8

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

Signed-off-by: Eric Blake 
---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
---
 qemu-nbd.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..e35c97e7ca4 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -56,7 +56,7 @@
 #define MBR_SIZE 512

 static NBDExport *exp;
-static bool newproto;
+static bool oldstyle;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -84,8 +84,9 @@ static void usage(const char *name)
 "  -e, --shared=NUM  device can be shared by NUM clients (default 
'1')\n"
 "  -t, --persistent  don't exit on the last connection\n"
 "  -v, --verbose display extra debugging information\n"
-"  -x, --export-name=NAMEexpose export by name\n"
-"  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -x, --export-name=NAMEexpose export by name (default "")\n"
+"  -D, --description=TEXTexpose a human-readable description of export\n"
+"  -O, --oldstyleforce oldstyle (not with -x, -D, or 
--tls-creds)\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET   offset into the image\n"
@@ -354,7 +355,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,

 nb_fds++;
 nbd_update_server_watch();
-nbd_client_new(newproto ? NULL : exp, cioc,
+nbd_client_new(oldstyle ? exp : NULL, cioc,
tlscreds, NULL, nbd_client_closed);
 }

@@ -502,7 +503,7 @@ int main(int argc, char **argv)
 off_t fd_size;
 QemuOpts *sn_opts = NULL;
 const char *sn_id_or_name = NULL;
-const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
+const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:O";
 struct option lopt[] = {
 { "help", no_argument, NULL, 'h' },
 { "version", no_argument, NULL, 'V' },
@@ -529,6 +530,7 @@ int main(int argc, char **argv)
 { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
 { "export-name", required_argument, NULL, 'x' },
 { "description", required_argument, NULL, 'D' },
+{ "oldstyle", no_argument, NULL, 'O' },
 { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
 { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
 { "trace", required_argument, NULL, 'T' },
@@ -723,6 +725,9 @@ int main(int argc, char **argv)
 case 'D':
 export_description = optarg;
 break;
+case 'O':
+oldstyle = true;
+break;
 case 'v':
 verbose = 1;
 break;
@@ -799,7 +804,16 @@ int main(int argc, char **argv)
 }
 }

+if (!oldstyle && !export_name) {
+/* Set the default NBD protocol export name, to favor new style. */
+export_name = "";
+}
+
 if (tlscredsid) {
+if (oldstyle) {
+error_report("TLS is incompatible with oldstyle");
+exit(EXIT_FAILURE);
+}
 if (sockpath) {
 error_report("TLS is only supported with IPv4/IPv6");
 exit(EXIT_FAILURE);
@@ -808,11 +822,6 @@ int main(int argc, char **argv)
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
 }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
 tlscreds = nbd_get_tls_creds(tlscredsid, _err);
 if (local_err) {
 error_report("Failed to get TLS creds %s",
@@ -1013,13 +1022,13 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 exit(EXIT_FAILURE);
 }
+if (oldstyle && (export_name || export_description)) {
+error_report("oldstyle negotiation cannot set export details");
+exit(EXIT_FAILURE);
+}
 if (export_name) {
 nbd_export_set_name(exp, export_name);
 nbd_export_set_description(exp, export_description);
-newproto = true;
-} else if (export_description) {
-error_report("Export description requires an export name");
-exit(EXIT_FAILURE);
 }

 if (device) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Eric Blake]

On 10/3/18 12:38 PM, Vladimir Sementsov-Ogievskiy wrote:

03.10.2018 20:19, Eric Blake wrote:

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.




+    if (oldstyle && (export_name || export_description)) {
+    error_report("oldstyle negotiation cannot set export details");
+    exit(EXIT_FAILURE);
+    }
  if (export_name) {
  nbd_export_set_name(exp, export_name);
  nbd_export_set_description(exp, export_description);
  newproto = true;


a bit strange, to have both newproto and oldstyle variables which are 
opposite by value.


Yeah. I added a function-local 'oldstyle' before noticing that there was 
a file static 'newproto'. We could squash this in:


diff --git i/qemu-nbd.c w/qemu-nbd.c
index 8571a4f93d8..2b0f7de86d0 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -56,7 +56,7 @@
 #define MBR_SIZE 512

 static NBDExport *exp;
-static bool newproto;
+static bool oldstyle;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -355,7 +355,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,


 nb_fds++;
 nbd_update_server_watch();
-nbd_client_new(newproto ? NULL : exp, cioc,
+nbd_client_new(oldstyle ? exp : NULL, cioc,
tlscreds, NULL, nbd_client_closed);
 }

@@ -553,7 +553,6 @@ int main(int argc, char **argv)
 QDict *options = NULL;
 const char *export_name = NULL;
 const char *export_description = NULL;
-bool oldstyle = false;
 const char *tlscredsid = NULL;
 bool imageOpts = false;
 bool writethrough = true;
@@ -1030,7 +1029,6 @@ int main(int argc, char **argv)
 if (export_name) {
 nbd_export_set_name(exp, export_name);
 nbd_export_set_description(exp, export_description);
-newproto = true;
 }

 if (device) {


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

Re: [Qemu-devel] [PATCH] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Vladimir Sementsov-Ogievskiy]

03.10.2018 20:19, Eric Blake wrote:

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

Signed-off-by: Eric Blake 
---
  qemu-nbd.c | 27 +++
  1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..8571a4f93d8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -86,6 +86,7 @@ static void usage(const char *name)
  "  -v, --verbose display extra debugging information\n"
  "  -x, --export-name=NAMEexpose export by name\n"
  "  -D, --description=TEXTwith -x, also export a human-readable 
description\n"


If we prefer this way, -x and -D descriptions should be updated a bit, 
like in my patch



+"  -O, --oldstyleforce oldstyle negotiation\n"
  "\n"
  "Exposing part of the image:\n"
  "  -o, --offset=OFFSET   offset into the image\n"
@@ -529,6 +530,7 @@ int main(int argc, char **argv)
  { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
  { "export-name", required_argument, NULL, 'x' },
  { "description", required_argument, NULL, 'D' },
+{ "oldstyle", no_argument, NULL, 'O' },
  { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
  { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
  { "trace", required_argument, NULL, 'T' },
@@ -551,6 +553,7 @@ int main(int argc, char **argv)
  QDict *options = NULL;
  const char *export_name = NULL;
  const char *export_description = NULL;
+bool oldstyle = false;
  const char *tlscredsid = NULL;
  bool imageOpts = false;
  bool writethrough = true;
@@ -723,6 +726,9 @@ int main(int argc, char **argv)
  case 'D':
  export_description = optarg;
  break;
+case 'O':
+oldstyle = true;
+break;
  case 'v':
  verbose = 1;
  break;
@@ -799,7 +805,16 @@ int main(int argc, char **argv)
  }
  }

+if (!oldstyle && !export_name) {
+/* Set the default NBD protocol export name, to favor new style. */
+export_name = "";
+}
+
  if (tlscredsid) {
+if (oldstyle) {
+error_report("TLS is incompatible with oldstyle");
+exit(EXIT_FAILURE);
+}
  if (sockpath) {
  error_report("TLS is only supported with IPv4/IPv6");
  exit(EXIT_FAILURE);
@@ -808,11 +823,6 @@ int main(int argc, char **argv)
  error_report("TLS is not supported with a host device");
  exit(EXIT_FAILURE);
  }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
  tlscreds = nbd_get_tls_creds(tlscredsid, _err);
  if (local_err) {
  error_report("Failed to get TLS creds %s",
@@ -1013,13 +1023,14 @@ int main(int argc, char **argv)
  error_report_err(local_err);
  exit(EXIT_FAILURE);
  }
+if (oldstyle && (export_name || export_description)) {
+error_report("oldstyle negotiation cannot set export details");
+exit(EXIT_FAILURE);
+}
  if (export_name) {
  nbd_export_set_name(exp, export_name);
  nbd_export_set_description(exp, export_description);
  newproto = true;


a bit strange, to have both newproto and oldstyle variables which are 
opposite by value.



-} else if (export_description) {
-error_report("Export description requires an export name");
-exit(EXIT_FAILURE);
  }

  if (device) {



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] nbd/server: drop old-style negotiation

[Eric Blake]

On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:

After the previous commit, nbd_client_new first parameter is always
NULL. Let's drop it with all corresponding old-style negotiation code
path which is unreachable now.


Being able to force oldstyle negotiation for interoperability testing 
may still be useful. But as fewer and fewer interesting clients exist 
that want oldstyle (after all, extensions are only usable with 
newstyle), I'm finding it hard to justify that we need qemu to be the 
oldstyle server for such interoperability testing (and I can always keep 
an older qemu binary around).




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |  3 +--
  blockdev-nbd.c  |  2 +-
  nbd/server.c| 53 +
  qemu-nbd.c  |  2 +-
  4 files changed, 18 insertions(+), 42 deletions(-)




+++ b/blockdev-nbd.c
@@ -36,7 +36,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,
 gpointer opaque)
  {
  qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-nbd_client_new(NULL, cioc,
+nbd_client_new(cioc,
 nbd_server->tlscreds, NULL,


Could rewrap into fewer lines, but that's cosmetic.

Reviewed-by: Eric Blake 

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

Re: [Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Eric Blake]

On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:

It's unexpected behavior that without -x option qemu-nbd do old-style
negotiation. Let's use "" as a default name instead (as it is already
done if tls is used) and therefore, drop old-style negotiation from
Qemu NBD server.


Oddly enough, I wrote a similar patch in parallel, and am only now just 
seeing your mail. Yours is a bit stronger than mine (I added 'qemu-nbd 
-O' to allow explicit fallback to oldstyle, while you ripped it out 
altogether).  The client can negotiate either style, so we don't need an 
option on the client side; rather, this is all about what the server 
should do by default.


https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html

Does anyone have a preference between the two? Here's the last time it 
was discussed:


https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html



Vladimir Sementsov-Ogievskiy (2):
   qemu-nbd: drop old-style negotiation
   nbd/server: drop old-style negotiation

  include/block/nbd.h |  3 +--
  blockdev-nbd.c  |  2 +-
  nbd/server.c| 53 +
  qemu-nbd.c  | 25 +
  4 files changed, 23 insertions(+), 60 deletions(-)



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

Re: [Qemu-devel] [PATCH] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Eric Blake]

On 10/3/18 12:19 PM, Eric Blake wrote:

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193


Oh, I should also add:

qemu as client can manage either style since 2.6.0, commit 69b49502d8


+++ b/qemu-nbd.c
@@ -86,6 +86,7 @@ static void usage(const char *name)
  "  -v, --verbose display extra debugging information\n"
  "  -x, --export-name=NAMEexpose export by name\n"
  "  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -O, --oldstyleforce oldstyle negotiation\n"


and maybe I should touch up the -x and -D wording, to at least mention 
that -x defaults to "" without -O.


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

Re: [Qemu-devel] [PATCH 1/2] qemu-nbd: drop old-style negotiation

[Eric Blake]

On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:

Use new-style negotiation always, with default "" (empty) export name
if it is not specified with '-x' option.


If we like this approach (over mine of keeping oldstyle, but via an 
explicit -O option), then this commit message should add the following:


qemu as client can manage either style since 2.6.0, commit 69b49502d8

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193



Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qemu-nbd.c | 25 ++---
  1 file changed, 6 insertions(+), 19 deletions(-)



If we like your patch over mine,
Reviewed-by: Eric Blake 

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

Re: [Qemu-devel] [PATCH] fpu/softfloat: Replace countLeadingZeros32/64 with clz32/64

[Richard Henderson]

On 9/28/18 2:01 AM, Thomas Huth wrote:
> Our minimum required compiler for compiling QEMU is GCC 4.1 these days,
> so we can drop the support for compilers which do not provide the
> __builtin_clz*() functions yet. Since the countLeadingZeros32/64 are
> then identical to the clz32/64 functions, and we do not have to sync
> the softloat 2 codebase with upstream anymore (softloat 3 is a complete
> rewrite) we can simply replace the functions with our QEMU versions.
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Thomas Huth 
> ---
>  fpu/softfloat.c| 26 ++---
>  include/fpu/softfloat-macros.h | 87 
> --
>  2 files changed, 13 insertions(+), 100 deletions(-)

Queued, thanks.


r~

Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler

[David Hildenbrand]

On 03/10/2018 08:29, David Gibson wrote:
> On Wed, Sep 26, 2018 at 11:42:13AM +0200, David Hildenbrand wrote:
>> The unplug and unplug_request handlers are special: They are not
>> executed when unrealizing a device, but rather trigger the removal of a
>> device from device_del() via object_unparent() - to effectively
>> unrealize a device.
>>
>> If such a device has a child bus and another device attached to
>> that bus (e.g. how virtio devices are created with their proxy device),
>> we will not get a call to the unplug handler. As we want to support
>> hotplug handlers (and especially also some unplug logic to undo resource
>> assignment) for such devices, we cannot simply call the unplug handler
>> when unrealizing - it has a different semantic ("trigger removal").
>>
>> To handle this scenario, we need a do_unplug handler, that will be
>> executed for all devices with a hotplug handler.
>>
>> While at it, introduce hotplug_fn_nofail and fix a spelling mistake in
>> a comment.
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/core/hotplug.c| 10 ++
>>  hw/core/qdev.c   |  6 ++
>>  include/hw/hotplug.h | 26 --
>>  3 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>> index 2253072d0e..e7a68d5160 100644
>> --- a/hw/core/hotplug.c
>> +++ b/hw/core/hotplug.c
>> @@ -45,6 +45,16 @@ void hotplug_handler_post_plug(HotplugHandler 
>> *plug_handler,
>>  }
>>  }
>>  
>> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
>> + DeviceState *plugged_dev)
> 
> Hrm.  I really dislike things named "do_X".  The "do" rarely adds any
> useful meaning.  And when there's also something called just plain
> "X", it's *always* unclear how they relate to each other.
> 
> That's doubly true when it's a general interface like this, rather
> than just some local functions.
> 

Yes, the naming is not the best, but I didn't want to rename all unplug
handlers before we have an agreement on how to proceed. My concept would
be that

1. unplug() is renamed to trigger_unplug(). unplug_request() remains.
2. do_unplug() is renamed to pre_unplug() (just like pre_plug())
3. we might have in addition unplug() after realize(false) - just like
plug()

trigger_unplug() would perform checks and result in object_unparent().
>From there, all cleanup/unplugging would be performed via the unrealize
chain, just like we have for realize() now. That would allow to properly
unplug complete device hierarchies.

But Igor rather wants one hotplug handler chain, and no dedicated
hotplug handlers for other devices in the device hierarchy. As long as
there are no other opinions I guess I will have to go into the direction
Igor proposes to get virtio-pmem and friends upstream.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

[Eric Blake]

Oldstyle NBD negotiation cannot perform any of the extensions that
we have recently been relying on.  While you can always pass -x ""
to get newstyle negotiation, these days, it is better to just default
to newstyle (with an empty export name) and fall back to oldstyle
only on an explicit request.

For comparison:

nbdkit 1.3 switched its default to newstyle (Jan 2018):
https://github.com/libguestfs/nbdkit/commit/b2a8aecc
https://github.com/libguestfs/nbdkit/commit/8158e773

nbd 3.10 dropped oldstyle long ago (Mar 2015):
https://github.com/NetworkBlockDevice/nbd/commit/36940193

Signed-off-by: Eric Blake 
---
 qemu-nbd.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c727..8571a4f93d8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -86,6 +86,7 @@ static void usage(const char *name)
 "  -v, --verbose display extra debugging information\n"
 "  -x, --export-name=NAMEexpose export by name\n"
 "  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -O, --oldstyleforce oldstyle negotiation\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET   offset into the image\n"
@@ -529,6 +530,7 @@ int main(int argc, char **argv)
 { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
 { "export-name", required_argument, NULL, 'x' },
 { "description", required_argument, NULL, 'D' },
+{ "oldstyle", no_argument, NULL, 'O' },
 { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
 { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
 { "trace", required_argument, NULL, 'T' },
@@ -551,6 +553,7 @@ int main(int argc, char **argv)
 QDict *options = NULL;
 const char *export_name = NULL;
 const char *export_description = NULL;
+bool oldstyle = false;
 const char *tlscredsid = NULL;
 bool imageOpts = false;
 bool writethrough = true;
@@ -723,6 +726,9 @@ int main(int argc, char **argv)
 case 'D':
 export_description = optarg;
 break;
+case 'O':
+oldstyle = true;
+break;
 case 'v':
 verbose = 1;
 break;
@@ -799,7 +805,16 @@ int main(int argc, char **argv)
 }
 }

+if (!oldstyle && !export_name) {
+/* Set the default NBD protocol export name, to favor new style. */
+export_name = "";
+}
+
 if (tlscredsid) {
+if (oldstyle) {
+error_report("TLS is incompatible with oldstyle");
+exit(EXIT_FAILURE);
+}
 if (sockpath) {
 error_report("TLS is only supported with IPv4/IPv6");
 exit(EXIT_FAILURE);
@@ -808,11 +823,6 @@ int main(int argc, char **argv)
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
 }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
 tlscreds = nbd_get_tls_creds(tlscredsid, _err);
 if (local_err) {
 error_report("Failed to get TLS creds %s",
@@ -1013,13 +1023,14 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 exit(EXIT_FAILURE);
 }
+if (oldstyle && (export_name || export_description)) {
+error_report("oldstyle negotiation cannot set export details");
+exit(EXIT_FAILURE);
+}
 if (export_name) {
 nbd_export_set_name(exp, export_name);
 nbd_export_set_description(exp, export_description);
 newproto = true;
-} else if (export_description) {
-error_report("Export description requires an export name");
-exit(EXIT_FAILURE);
 }

 if (device) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock

[Paolo Bonzini]

On 03/10/2018 19:02, Emilio G. Cota wrote:
>> For reads I agree, but you may actually get a torn read if the writer
>> doesn't use atomic_set.
>
> But you cannot get a torn read if all reads that don't hold the lock
> are coming from the same thread that performed the write.

Ah, so you are relying on copy_tlb_helper(_locked) being invoked only
from the vCPU thread (as opposed to someone else doing tlb_flush)?
Maybe it's worth adding a comment if that's what I missed.

Paolo

[Qemu-devel] [PATCH 0/2] nbd server: drop old-style negotiation

[Vladimir Sementsov-Ogievskiy]

It's unexpected behavior that without -x option qemu-nbd do old-style
negotiation. Let's use "" as a default name instead (as it is already
done if tls is used) and therefore, drop old-style negotiation from
Qemu NBD server.

Vladimir Sementsov-Ogievskiy (2):
  qemu-nbd: drop old-style negotiation
  nbd/server: drop old-style negotiation

 include/block/nbd.h |  3 +--
 blockdev-nbd.c  |  2 +-
 nbd/server.c| 53 +
 qemu-nbd.c  | 25 +
 4 files changed, 23 insertions(+), 60 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] qemu-nbd: drop old-style negotiation

[Vladimir Sementsov-Ogievskiy]

Use new-style negotiation always, with default "" (empty) export name
if it is not specified with '-x' option.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-nbd.c | 25 ++---
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 51b9d38c72..57e79e30ea 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -56,7 +56,6 @@
 #define MBR_SIZE 512
 
 static NBDExport *exp;
-static bool newproto;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -84,8 +83,8 @@ static void usage(const char *name)
 "  -e, --shared=NUM  device can be shared by NUM clients (default 
'1')\n"
 "  -t, --persistent  don't exit on the last connection\n"
 "  -v, --verbose display extra debugging information\n"
-"  -x, --export-name=NAMEexpose export by name\n"
-"  -D, --description=TEXTwith -x, also export a human-readable 
description\n"
+"  -x, --export-name=NAMEexpose export by name (default is empty string)\n"
+"  -D, --description=TEXTexport a human-readable description\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET   offset into the image\n"
@@ -354,8 +353,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,
 
 nb_fds++;
 nbd_update_server_watch();
-nbd_client_new(newproto ? NULL : exp, cioc,
-   tlscreds, NULL, nbd_client_closed);
+nbd_client_new(NULL, cioc, tlscreds, NULL, nbd_client_closed);
 }
 
 static void nbd_update_server_watch(void)
@@ -549,7 +547,7 @@ int main(int argc, char **argv)
 Error *local_err = NULL;
 BlockdevDetectZeroesOptions detect_zeroes = 
BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
 QDict *options = NULL;
-const char *export_name = NULL;
+const char *export_name = ""; /* Default export name */
 const char *export_description = NULL;
 const char *tlscredsid = NULL;
 bool imageOpts = false;
@@ -808,11 +806,6 @@ int main(int argc, char **argv)
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
 }
-if (!export_name) {
-/* Set the default NBD protocol export name, since
- * we *must* use new style protocol for TLS */
-export_name = "";
-}
 tlscreds = nbd_get_tls_creds(tlscredsid, _err);
 if (local_err) {
 error_report("Failed to get TLS creds %s",
@@ -1013,14 +1006,8 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 exit(EXIT_FAILURE);
 }
-if (export_name) {
-nbd_export_set_name(exp, export_name);
-nbd_export_set_description(exp, export_description);
-newproto = true;
-} else if (export_description) {
-error_report("Export description requires an export name");
-exit(EXIT_FAILURE);
-}
+nbd_export_set_name(exp, export_name);
+nbd_export_set_description(exp, export_description);
 
 if (device) {
 int ret;
-- 
2.18.0

[Qemu-devel] [PATCH 2/2] nbd/server: drop old-style negotiation

[Vladimir Sementsov-Ogievskiy]

After the previous commit, nbd_client_new first parameter is always
NULL. Let's drop it with all corresponding old-style negotiation code
path which is unreachable now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c  |  2 +-
 nbd/server.c| 53 +
 qemu-nbd.c  |  2 +-
 4 files changed, 18 insertions(+), 42 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4638c839f5..0129d1a4b4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -308,8 +308,7 @@ void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);
 
-void nbd_client_new(NBDExport *exp,
-QIOChannelSocket *sioc,
+void nbd_client_new(QIOChannelSocket *sioc,
 QCryptoTLSCreds *tlscreds,
 const char *tlsaclname,
 void (*close_fn)(NBDClient *, bool));
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1ef11041a7..8913d8ff73 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -36,7 +36,7 @@ static void nbd_accept(QIONetListener *listener, 
QIOChannelSocket *cioc,
gpointer opaque)
 {
 qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-nbd_client_new(NULL, cioc,
+nbd_client_new(cioc,
nbd_server->tlscreds, NULL,
nbd_blockdev_client_closed);
 }
diff --git a/nbd/server.c b/nbd/server.c
index 1fba21414b..e87f881525 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1253,7 +1253,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, 
Error **errp)
 const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
   NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
   NBD_FLAG_SEND_WRITE_ZEROES | 
NBD_FLAG_SEND_CACHE);
-bool oldStyle;
 
 /* Old style negotiation header, no room for options
 [ 0 ..   7]   passwd   ("NBDMAGIC")
@@ -1274,33 +1273,19 @@ static coroutine_fn int nbd_negotiate(NBDClient 
*client, Error **errp)
 trace_nbd_negotiate_begin();
 memcpy(buf, "NBDMAGIC", 8);
 
-oldStyle = client->exp != NULL && !client->tlscreds;
-if (oldStyle) {
-trace_nbd_negotiate_old_style(client->exp->size,
-  client->exp->nbdflags | myflags);
-stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
-stq_be_p(buf + 16, client->exp->size);
-stl_be_p(buf + 24, client->exp->nbdflags | myflags);
+stq_be_p(buf + 8, NBD_OPTS_MAGIC);
+stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES);
 
-if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
-error_prepend(errp, "write failed: ");
-return -EINVAL;
-}
-} else {
-stq_be_p(buf + 8, NBD_OPTS_MAGIC);
-stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES);
-
-if (nbd_write(client->ioc, buf, 18, errp) < 0) {
-error_prepend(errp, "write failed: ");
-return -EINVAL;
-}
-ret = nbd_negotiate_options(client, myflags, errp);
-if (ret != 0) {
-if (ret < 0) {
-error_prepend(errp, "option negotiation failed: ");
-}
-return ret;
+if (nbd_write(client->ioc, buf, 18, errp) < 0) {
+error_prepend(errp, "write failed: ");
+return -EINVAL;
+}
+ret = nbd_negotiate_options(client, myflags, errp);
+if (ret != 0) {
+if (ret < 0) {
+error_prepend(errp, "option negotiation failed: ");
 }
+return ret;
 }
 
 assert(!client->optlen);
@@ -2396,13 +2381,8 @@ static void nbd_client_receive_next_request(NBDClient 
*client)
 static coroutine_fn void nbd_co_client_start(void *opaque)
 {
 NBDClient *client = opaque;
-NBDExport *exp = client->exp;
 Error *local_err = NULL;
 
-if (exp) {
-nbd_export_get(exp);
-QTAILQ_INSERT_TAIL(>clients, client, next);
-}
 qemu_co_mutex_init(>send_lock);
 
 if (nbd_negotiate(client, _err)) {
@@ -2417,13 +2397,11 @@ static coroutine_fn void nbd_co_client_start(void 
*opaque)
 }
 
 /*
- * Create a new client listener on the given export @exp, using the
- * given channel @sioc.  Begin servicing it in a coroutine.  When the
- * connection closes, call @close_fn with an indication of whether the
- * client completed negotiation.
+ * Create a new client listener using the given channel @sioc.
+ * Begin servicing it in a coroutine.  When the connection closes, call
+ * @close_fn with an indication of whether the client completed negotiation.
  */
-void nbd_client_new(NBDExport *exp,
-QIOChannelSocket *sioc,
+void nbd_client_new(QIOChannelSocket *sioc,
 QCryptoTLSCreds *tlscreds,
 const char *tlsaclname,
 

Re: [Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock

[Emilio G. Cota]

On Wed, Oct 03, 2018 at 17:52:32 +0200, Paolo Bonzini wrote:
> On 03/10/2018 17:48, Emilio G. Cota wrote:
> >> it's probably best to do all atomic_set instead of just the memberwise 
> >> copy.
> > Atomics aren't necessary here, as long as the copy is protected by the
> > lock. This allows other vCPUs to see a consistent view of the data (since
> > they always acquire the TLB lock), and since copy_tlb is only called
> > by the vCPU that owns the TLB, regular reads from this vCPU will always
> > see consistent data.
> 
> For reads I agree, but you may actually get a torn read if the writer
> doesn't use atomic_set.

But you cannot get a torn read if all reads that don't hold the lock
are coming from the same thread that performed the write.

> That's because in order to avoid UB all reads or writes that are
> concurrent with another write must be atomic, and the write must be
> atomic too.

Agreed, and that's why there's a patch adding atomic_read to all
.addr_write reads not protected by the lock.

> The lock does prevent write-write concurrent accesses, but
> not read-write, so the write must be atomic here.

But here we have no concurrent read-write, because the non-owner
thread always holds the lock, and the owner thread doesn't need
to synchronize with itself.

I'm appending a small example that might help make my point;
.addr_write corresponds to .written_by_all, and the other
fields correspond to .written_by_owner.

Thanks,

E.

---
/*
 * baz.c
 * gcc -O3 -fsanitize=thread -o baz baz.c
 */
#include 
#include 
#include 
#include 

#define atomic_read(ptr)   __atomic_load_n(ptr, __ATOMIC_RELAXED)
#define atomic_set(ptr, i) __atomic_store_n(ptr, i, __ATOMIC_RELAXED)

#define N_ITER 100

struct entry {
int written_by_all;
int written_by_owner;
};

static struct entry entry;
static pthread_mutex_t lock;

void *owner_thread(void *arg)
{
int i;

for (i = 0; i < N_ITER; i++) {
if (i % 2) {
volatile int a __attribute__((unused));
volatile int b __attribute__((unused));

a = atomic_read(_by_all);
b = entry.written_by_owner;
} else {
pthread_mutex_lock();
entry.written_by_all++;
entry.written_by_owner++;
pthread_mutex_unlock();
}
}
return NULL;
}

void *other_thread(void *arg)
{
int i;

for (i = 0; i < N_ITER; i++) {
pthread_mutex_lock();
atomic_set(_by_all, entry.written_by_all + 1);
pthread_mutex_unlock();
}
return NULL;
}

int main(int argc, char *argv[])
{
pthread_t threads[2];
int i;

pthread_mutex_init(, NULL);
if (pthread_create([0], NULL, owner_thread, NULL)) {
exit(1);
}
if (pthread_create([1], NULL, other_thread, NULL)) {
exit(1);
}
for (i = 0; i < 2; i++) {
pthread_join(threads[i], NULL);
}
return 0;
}

[Qemu-devel] Hotplug handler

[Sameeh Jubran]

Hi all,

I am trying to get the hotplug handler of a pci device in Qemu using
"qdev_get_hotplug_handler" function. This function simply tries to get
the hotplug handler from the parent bus. For some reason it's always
null. Why it is not initialized?

Thanks!

Re: [Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[John Snow]

On 10/03/2018 08:28 AM, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> If the bitmap is frozen, we shouldn't touch it.
>>
>> Signed-off-by: John Snow 
>> ---
>>   blockdev.c | 12 ++--
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index d0febfca79..098d4c337f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup
>> *backup, JobTxn *txn,
>>   bdrv_unref(target_bs);
>>   goto out;
>>   }
>> -    if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
>> +    if (bdrv_dirty_bitmap_user_locked(bmap)) {
>>   error_setg(errp,
>> -   "Bitmap '%s' is currently locked and cannot be
>> used for "
>> -   "backup", backup->bitmap);
>> +   "Bitmap '%s' is currently in use by another
>> operation"
>> +   " and cannot be used for backup",
>> backup->bitmap);
>>   goto out;
> 
> Is this right?  Why can we not have two parallel backups utilizing the
> same unchanging locked bitmap as its source?  Of course, to do that,
> we'd need the condition of being locked to be a ref-counter (increment
> for each backup that reuses the bitmap, decrement when the backup
> finishes, and it is unlocked when the counter is 0) rather than a bool.
> So, without that larger refactoring, this is a conservative approach
> that is a little too strict, but allows for a simpler implementation.
> And the user can always work around the limitation by cloning the locked
> bitmap into another temporary bitmap, and starting the second parallel
> backup with the second backup instead of the original.
> 
> Weak Reviewed-by: Eric Blake 
> 

Vladimir gave a good recounting of the reasons. My principal
justification here is that:

- FROZEN implies that the bitmap has been split; which means there is a
pending operating to re-suture them into one bitmap which may occur at
an indeterminate time in the future that we cannot account for in the
following job code, and

- QMP_LOCKED only implies that the bitmap is in use by, say, the NBD
fleecing operation with no further pending actions.

Here, in do_drive_backup, we check only that we are not qmp_locked, but
I argue we ought to check against frozen as well. It's likely to fail
because the BDS is already in use by another job, but this check is
strictly more correct.

In the opposite case, we don't want to split a bitmap that is being used
by someone else -- we're about to fork this bitmap (which means that the
bitmap referenced by this named handle will be CLEARED) which can alter
what the NBD process is doing, which is also bad.

For now, this is correct.

--js

Re: [Qemu-devel] [RFC PATCH 0/3] acceptance tests: Test firmware checking debug console output

[Cleber Rosa]

On 10/3/18 11:59 AM, Laszlo Ersek wrote:
> On 10/03/18 17:20, Cleber Rosa wrote:
>> On 10/3/18 3:13 AM, Laszlo Ersek wrote:
>>> On 10/03/18 02:23, Cleber Rosa wrote:
 On 9/28/18 6:51 AM, Laszlo Ersek wrote:
>>>
>   I'm not sure if Avocado provides disk image preparation utilities, but
>   perhaps (a) we could use the vvfat driver (*shudder*) or (b) we could
>   preformat a small image, and track it as a binary file in git.
>

 So far we've added support for generating ISO images (with pure Python).
 I'm not sure if that's useful here.  We can think about trying to add
 the same thing for vvfat.
>>>
>>> The ability to generate ISO images (natively at that!) seems useful.
>>> UEFI-readable ISO images need an extension on top: the ISO9660
>>> filesystem has to get the ElTorito extension, and the ElTorito boot
>>> image should be a FAT filesystem. Under UEFI, what's visible isn't the
>>> ISO9660 filesystem itself, but the contents of the embedded ElTorito
>>> boot image.
>>>
>>> In terms of shell utilities, this usually involves:
>>>
>>> - creating and populating the FAT filesystem image (with guestfish, or
>>>   with mkdosfs+mtools),
>>>
>>
>> Is FAT12 an option here?  The reason I ask is that there may be code
>> FAT12 capable code ready to be incorporated:
>>
>> https://github.com/clalancette/pyfat
> 
> Theoretically, I should answer "yes". For two reasons:
> 
> (1) In "13.3 File System Format", the UEFI-2.7 spec writes,
> 
> "[...] EFI encompasses the use of FAT32 for a system partition, and
> FAT12 or FAT16 for removable media. [...]"
> 
> (2) When invoking mkdosfs without the "-F" option, mkdosfs chooses the
> smallest FAT filesystem variant that can accommodate the requested size.
> We successfully format UEFI-readable ISO images that don't exceed e.g.
> 3MB in final size. This implies (and I have now actually checked, with
> "dosfsck -v") that their embedded ElTorito image is FAT12. edk2 has no
> trouble reading that.
> 
> 
> However... the maximum volume size for FAT12 appears to be 32 MB,
> according to wikipedia:
> 
>   https://en.wikipedia.org/wiki/File_Allocation_Table#FAT12
> 
> It doesn't look good for the long term. For example, I can imagine a
> test case where you place a kernel executable (containing a UEFI stub)
> and an initial ramdisk on the UEFI-readable ISO. Using the RHEL-7.5
> kernel and the matching initrd from my laptop as an example: that's
> already 6.2MB + 25MB.
> 
> So, technically, FAT12 should be fine; in practice, it could prove limiting.
> 

Nice, thanks for the detailed info.  I think it's safe to start with
that, and improve the original project with FAT16/32 support.

Regards,
- Cleber.

> [...]
> 
> Thanks!
> Laszlo
> 

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]

Re: [Qemu-devel] [RFC PATCH 0/3] acceptance tests: Test firmware checking debug console output

[Laszlo Ersek]

On 10/03/18 17:20, Cleber Rosa wrote:
> On 10/3/18 3:13 AM, Laszlo Ersek wrote:
>> On 10/03/18 02:23, Cleber Rosa wrote:
>>> On 9/28/18 6:51 AM, Laszlo Ersek wrote:
>>
   I'm not sure if Avocado provides disk image preparation utilities, but
   perhaps (a) we could use the vvfat driver (*shudder*) or (b) we could
   preformat a small image, and track it as a binary file in git.

>>>
>>> So far we've added support for generating ISO images (with pure Python).
>>> I'm not sure if that's useful here.  We can think about trying to add
>>> the same thing for vvfat.
>>
>> The ability to generate ISO images (natively at that!) seems useful.
>> UEFI-readable ISO images need an extension on top: the ISO9660
>> filesystem has to get the ElTorito extension, and the ElTorito boot
>> image should be a FAT filesystem. Under UEFI, what's visible isn't the
>> ISO9660 filesystem itself, but the contents of the embedded ElTorito
>> boot image.
>>
>> In terms of shell utilities, this usually involves:
>>
>> - creating and populating the FAT filesystem image (with guestfish, or
>>   with mkdosfs+mtools),
>>
> 
> Is FAT12 an option here?  The reason I ask is that there may be code
> FAT12 capable code ready to be incorporated:
> 
> https://github.com/clalancette/pyfat

Theoretically, I should answer "yes". For two reasons:

(1) In "13.3 File System Format", the UEFI-2.7 spec writes,

"[...] EFI encompasses the use of FAT32 for a system partition, and
FAT12 or FAT16 for removable media. [...]"

(2) When invoking mkdosfs without the "-F" option, mkdosfs chooses the
smallest FAT filesystem variant that can accommodate the requested size.
We successfully format UEFI-readable ISO images that don't exceed e.g.
3MB in final size. This implies (and I have now actually checked, with
"dosfsck -v") that their embedded ElTorito image is FAT12. edk2 has no
trouble reading that.


However... the maximum volume size for FAT12 appears to be 32 MB,
according to wikipedia:

  https://en.wikipedia.org/wiki/File_Allocation_Table#FAT12

It doesn't look good for the long term. For example, I can imagine a
test case where you place a kernel executable (containing a UEFI stub)
and an initial ramdisk on the UEFI-readable ISO. Using the RHEL-7.5
kernel and the matching initrd from my laptop as an example: that's
already 6.2MB + 25MB.

So, technically, FAT12 should be fine; in practice, it could prove limiting.

[...]

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock

[Paolo Bonzini]

On 03/10/2018 17:48, Emilio G. Cota wrote:
>> it's probably best to do all atomic_set instead of just the memberwise copy.
> Atomics aren't necessary here, as long as the copy is protected by the
> lock. This allows other vCPUs to see a consistent view of the data (since
> they always acquire the TLB lock), and since copy_tlb is only called
> by the vCPU that owns the TLB, regular reads from this vCPU will always
> see consistent data.

For reads I agree, but you may actually get a torn read if the writer
doesn't use atomic_set.

That's because in order to avoid UB all reads or writes that are
concurrent with another write must be atomic, and the write must be
atomic too.  The lock does prevent write-write concurrent accesses, but
not read-write, so the write must be atomic here.

Paolo