[Bug 1885350] Re: RISCV dynamic rounding mode is not behaving correctly
[Expired for QEMU because there has been no activity for 60 days.]
** Changed in: qemu
Status: Incomplete => Expired
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1885350
Title:
RISCV dynamic rounding mode is not behaving correctly
Status in QEMU:
Expired
Bug description:
Hello,
I’ve gone through the RISC-V code in latest QEMU release
(qemu-5.0.0-rc2) and when checking the Floating point encodings I
found the rounding mode is only updated if the opcode field “rm” is
changed “ctx->frm == rm”. But according to RISC-V Volume I:
Unprivileged ISA, there’s a dynamic mode when rm=7 where the rounding
mode is set with frm value.
So for the same rm value (=7) and when changing frm value seeking
different rounding modes, and according to the below code, the
rounding mode won’t be updated. Please correct me if I got this
implementation wrong.
static void gen_set_rm(DisasContext *ctx, int rm)
{
TCGv_i32 t0;
if (ctx->frm == rm) {
return;
}
ctx->frm = rm;
t0 = tcg_const_i32(rm);
gen_helper_set_rounding_mode(cpu_env, t0);
tcg_temp_free_i32(t0);
}
My testcase:
I set statically the rm field in the instruction to 7 and before this
execution I changed the value of frm field in fcsr register. For the 1st time
it worked (according to the code above, the rm is updated so the round mode
will also be updated). But when changing fcsr register an re-execute the
instruction, there's no difference and the rounding mode is the same like the
previous frm value.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1885350/+subscriptions
[Bug 1886306] Re: qemu running slow when the window is in background
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1886306 Title: qemu running slow when the window is in background Status in QEMU: Expired Bug description: Reported by on IRC: QEMU almost freezes when running with `GDK_BACKEND=x11` set and the parameter `gl=on` added to the `-display` option. GDK_BACKEND=x11 qemu-system-x86_64 -nodefaults -no-user-config -enable-kvm -machine q35 -cpu host -m 4G -display gtk,gl=on -vga std -usb -device usb-kbd -drive file=/tmp/Win10.qcow2,media=disk,format=qcow2 -drive file=~/Downloads/Win10_2004_EnglishInternational_x64.iso,media=cdrom Leaving out `GDK_BACKEND=x11` or `gl=on` fixes the issue. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1886306/+subscriptions
[Bug 1924738] Re: Failed to restore domain - error load load virtio-balloon:virtio
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1924738 Title: Failed to restore domain - error load load virtio-balloon:virtio Status in QEMU: Expired Bug description: I noticed a domain restore error on my virtual machines. I can't reproduce the error on a test virtual machine. sudo virsh save linux2020 /var/lib/libvirt/qemu/save/linux2020.save Domain 'linux2020' saved to /var/lib/libvirt/qemu/save/linux2020.save sudo virsh restore /var/lib/libvirt/qemu/save/linux2020.save error: Failed to restore domain from /var/lib/libvirt/qemu/save/linux2020.save error: внутренняя ошибка: QEMU неожиданно завершил работу монитора: qemu-system-x86_64: -chardev socket,id=charchannel0,fd=52,server,nowait: warning: short-form boolean option 'server' deprecated Please use server=on instead qemu-system-x86_64: -chardev socket,id=charchannel0,fd=52,server,nowait: warning: short-form boolean option 'nowait' deprecated Please use wait=off instead qemu-system-x86_64: -spice port=5900,addr=0.0.0.0,disable-ticketing,image-compression=off,seamless-migration=on: warning: short-form boolean option 'disable-ticketing' deprecated Please use disable-ticketing=on instead 2021-04-16T09:47:15.037700Z qemu-system-x86_64: VQ 0 size 0x80 < last_avail_idx 0x0 - used_idx 0x 2021-04-16T09:47:15.037737Z qemu-system-x86_64: Failed to load virtio-balloon:virtio 2021-04-16T09:47:15.037744Z qemu-system-x86_64: error while loading state for instance 0x0 of device ':00:02.0/virtio-balloon' 2021-04-16T09:47:15.037849Z qemu-system-x86_64: load of migration failed: Operation not permitted If in the machine configuration replace hvm to hvm the virtual machine is recovering normally To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1924738/+subscriptions
Re: [PATCH 00/17] target/riscv: Use tcg_constant_*
On 2021/7/16 上午12:15, Richard Henderson wrote: On 7/15/21 4:21 AM, LIU Zhiwei wrote: Also on a side note, could you give me some advice for the following question? I have been supporting running 32bit application on qemu-riscv64. After this patch set, it is hard to define a method, such as gpr_dst_s or gpr_dst_u, to extend the destination register. I can only extend the destination register(ext32s or ext32u) in each instruction with scattered code. Can we just omit the extension of the destination register? It's hard to give advice on code that I haven't seen. In general I would think that the destination register need not be extended for 32-bit mode, unless the architecture says otherwise. (What does the architecture say about the contents of the registers when transitioning from a 32-bit mode user program to a 64-bit mode kernel?) As privileged specification says, "Whenever XLEN in any mode is set to a value less than the widest supported XLEN, all operations must ignore source operand register bits above the configured XLEN, and must sign-extend results to fill the entire widest supported XLEN in the destination register. Similarly, pc bits above XLEN are ignored, and when the pc is written, it is sign-extended to fill the widest supported XLEN." If we want to strictly obey the spec, we should 1) Ignore MSB 32bits for source register, and sign-extend the destination register. 2) Always use 32bit operation(TCG 32bit OP). I want to still use TCG 64bit OP and just extend the source to 64bit by ext32s or ext32u. Is is OK? Thanks, Zhiwei r~
Re: [PATCH v6 2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation
在 2021/7/17 3:36, Peter Xu 写道:
On Fri, Jul 16, 2021 at 07:13:47PM +0800, huang...@chinatelecom.cn wrote:
+static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
+{
+int64_t msec = 0;
+int64_t start_time;
+DirtyPageRecord dirty_pages;
[1]
+
+dirtyrate_global_dirty_log_start();
+
+/*
+ * 1'round of log sync may return all 1 bits with
+ * KVM_DIRTY_LOG_INITIALLY_SET enable
+ * skip it unconditionally and start dirty tracking
+ * from 2'round of log sync
+ */
+dirtyrate_global_dirty_log_sync();
+
+/*
+ * reset page protect manually and unconditionally.
+ * this make sure kvm dirty log be cleared if
+ * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
+ */
+dirtyrate_manual_reset_protect();
+
[2]
+record_dirtypages_bitmap(_pages, true);
[3]
+
+start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+DirtyStat.start_time = start_time / 1000;
+
+msec = config.sample_period_seconds * 1000;
+msec = set_sample_page_period(msec, start_time);
+DirtyStat.calc_time = msec / 1000;
+
+/* fetch dirty bitmap from kvm and stop dirty tracking */
I don't think it really fetched anything.. So I think we need:
dirtyrate_global_dirty_log_sync();
It seems to be there in older versions but not in the latest two versions.
yes, latest version dropped this because dirtyrate_global_dirty_log_stop
below already do the sync before stop dirty log, which is recommended in
patchset of "support dirtyrate at the granualrity of vcpu" and make
dirtyrate more accurate. the older version do not consider this. :)
Please still remember to smoke the patches before posting, because without the
log sync we'll read nothing.
+dirtyrate_global_dirty_log_stop();
+
+record_dirtypages_bitmap(_pages, false);
[4]
I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
taking it for every function. Then we can move the bql operations out of
dirtyrate_global_dirty_log_stop() in this patch.
yeah, take bql at [1] and release at [2] is reasonable.
but if we try to take bql at [3], it will sleep for calculation time in
set_sample_page_period which is configured by user, which may be a heavy
overhead.
how about we take bql at [1] and release at [2], ingore bql at [3]/[4]
and let it be the same as older versoin. since we only copy
total_dirty_pages to local var in "get_dirtyrate" thread and maybe we
don't need bql.
Thanks,
+
+do_calculate_dirtyrate_bitmap(dirty_pages);
+}
[PATCH 1/2] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
It turns out you can do this directly from Python ... and because of
this, you don't need to worry about setting the fds, or spawning another
process.
Doing this is helpful because it allows QEMUMonitorProtocol to keep its
file descriptor and socket object as private implementation details,
which allows me to construct a different implementation -- a Synchronous
wrapper around my Async QMP library.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 44 +++---
python/qemu/qmp/__init__.py| 21 +++-
2 files changed, 18 insertions(+), 47 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 971ed7e8c6a..da47e2704cf 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -205,48 +205,22 @@ def add_fd(self, fd: int, fdset: int,
def send_fd_scm(self, fd: Optional[int] = None,
file_path: Optional[str] = None) -> int:
"""
-Send an fd or file_path to socket_scm_helper.
+Send an fd or file_path to the remote via SCM_RIGHTS.
-Exactly one of fd and file_path must be given.
-If it is file_path, the helper will open that file and pass its own fd.
+Exactly one of fd and file_path must be given. If it is
+file_path, the file will be opened read-only and the new file
+descriptor will be sent to the remote.
"""
-# In iotest.py, the qmp should always use unix socket.
-assert self._qmp.is_scm_available()
-if self._socket_scm_helper is None:
-raise QEMUMachineError("No path to socket_scm_helper set")
-if not os.path.exists(self._socket_scm_helper):
-raise QEMUMachineError("%s does not exist" %
- self._socket_scm_helper)
-
-# This did not exist before 3.4, but since then it is
-# mandatory for our purpose
-if hasattr(os, 'set_inheritable'):
-os.set_inheritable(self._qmp.get_sock_fd(), True)
-if fd is not None:
-os.set_inheritable(fd, True)
-
-fd_param = ["%s" % self._socket_scm_helper,
-"%d" % self._qmp.get_sock_fd()]
-
if file_path is not None:
assert fd is None
-fd_param.append(file_path)
+with open(file_path, "rb") as passfile:
+fd = passfile.fileno()
+self._qmp.send_fd_scm(fd)
else:
assert fd is not None
-fd_param.append(str(fd))
+self._qmp.send_fd_scm(fd)
-proc = subprocess.run(
-fd_param,
-stdin=subprocess.DEVNULL,
-stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
-check=False,
-close_fds=False,
-)
-if proc.stdout:
-LOG.debug(proc.stdout)
-
-return proc.returncode
+return 0
@staticmethod
def _remove_if_exists(path: str) -> None:
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 269516a79b9..8a1710f3a10 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -21,6 +21,7 @@
import json
import logging
import socket
+import struct
from types import TracebackType
from typing import (
Any,
@@ -406,18 +407,14 @@ def settimeout(self, timeout: Optional[float]) -> None:
raise ValueError(msg)
self.__sock.settimeout(timeout)
-def get_sock_fd(self) -> int:
+def send_fd_scm(self, fd: int) -> None:
"""
-Get the socket file descriptor.
-
-@return The file descriptor number.
-"""
-return self.__sock.fileno()
-
-def is_scm_available(self) -> bool:
+Send a file descriptor to the remote via SCM_RIGHTS.
"""
-Check if the socket allows for SCM_RIGHTS.
+if self.__sock.family != socket.AF_UNIX:
+raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
-@return True if SCM_RIGHTS is available, otherwise False.
-"""
-return self.__sock.family == socket.AF_UNIX
+self.__sock.sendmsg(
+[b' '],
+[(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+)
--
2.31.1
[PATCH 2/2] python, iotests: remove socket_scm_helper
It's not used anymore, now.
Signed-off-by: John Snow
---
tests/qemu-iotests/socket_scm_helper.c | 136 -
python/qemu/machine/machine.py | 3 -
python/qemu/machine/qtest.py | 2 -
tests/Makefile.include | 1 -
tests/meson.build | 4 -
tests/qemu-iotests/iotests.py | 3 -
tests/qemu-iotests/meson.build | 5 -
tests/qemu-iotests/testenv.py | 8 +-
8 files changed, 1 insertion(+), 161 deletions(-)
delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
delete mode 100644 tests/qemu-iotests/meson.build
diff --git a/tests/qemu-iotests/socket_scm_helper.c
b/tests/qemu-iotests/socket_scm_helper.c
deleted file mode 100644
index eb76d31aa94..000
--- a/tests/qemu-iotests/socket_scm_helper.c
+++ /dev/null
@@ -1,136 +0,0 @@
-/*
- * SCM_RIGHTS with unix socket help program for test
- *
- * Copyright IBM, Inc. 2013
- *
- * Authors:
- * Wenchao Xia
- *
- * This work is licensed under the terms of the GNU LGPL, version 2 or later.
- * See the COPYING.LIB file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include
-#include
-
-/* #define SOCKET_SCM_DEBUG */
-
-/*
- * @fd and @fd_to_send will not be checked for validation in this function,
- * a blank will be sent as iov data to notify qemu.
- */
-static int send_fd(int fd, int fd_to_send)
-{
-struct msghdr msg;
-struct iovec iov[1];
-int ret;
-char control[CMSG_SPACE(sizeof(int))];
-struct cmsghdr *cmsg;
-
-memset(, 0, sizeof(msg));
-memset(control, 0, sizeof(control));
-
-/* Send a blank to notify qemu */
-iov[0].iov_base = (void *)" ";
-iov[0].iov_len = 1;
-
-msg.msg_iov = iov;
-msg.msg_iovlen = 1;
-
-msg.msg_control = control;
-msg.msg_controllen = sizeof(control);
-
-cmsg = CMSG_FIRSTHDR();
-
-cmsg->cmsg_len = CMSG_LEN(sizeof(int));
-cmsg->cmsg_level = SOL_SOCKET;
-cmsg->cmsg_type = SCM_RIGHTS;
-memcpy(CMSG_DATA(cmsg), _to_send, sizeof(int));
-
-do {
-ret = sendmsg(fd, , 0);
-} while (ret < 0 && errno == EINTR);
-
-if (ret < 0) {
-fprintf(stderr, "Failed to send msg, reason: %s\n", strerror(errno));
-}
-
-return ret;
-}
-
-/* Convert string to fd number. */
-static int get_fd_num(const char *fd_str, bool silent)
-{
-int sock;
-char *err;
-
-errno = 0;
-sock = strtol(fd_str, , 10);
-if (errno) {
-if (!silent) {
-fprintf(stderr, "Failed in strtol for socket fd, reason: %s\n",
-strerror(errno));
-}
-return -1;
-}
-if (!*fd_str || *err || sock < 0) {
-if (!silent) {
-fprintf(stderr, "bad numerical value for socket fd '%s'\n",
fd_str);
-}
-return -1;
-}
-
-return sock;
-}
-
-/*
- * To make things simple, the caller needs to specify:
- * 1. socket fd.
- * 2. path of the file to be sent.
- */
-int main(int argc, char **argv, char **envp)
-{
-int sock, fd, ret;
-
-#ifdef SOCKET_SCM_DEBUG
-int i;
-for (i = 0; i < argc; i++) {
-fprintf(stderr, "Parameter %d: %s\n", i, argv[i]);
-}
-#endif
-
-if (argc != 3) {
-fprintf(stderr,
-"Usage: %s < socket-fd > < file-path >\n",
-argv[0]);
-return EXIT_FAILURE;
-}
-
-
-sock = get_fd_num(argv[1], false);
-if (sock < 0) {
-return EXIT_FAILURE;
-}
-
-fd = get_fd_num(argv[2], true);
-if (fd < 0) {
-/* Now only open a file in readonly mode for test purpose. If more
- precise control is needed, use python script in file operation,
which
- is supposed to fork and exec this program. */
-fd = open(argv[2], O_RDONLY);
-if (fd < 0) {
-fprintf(stderr, "Failed to open file '%s'\n", argv[2]);
-return EXIT_FAILURE;
-}
-}
-
-ret = send_fd(sock, fd);
-if (ret < 0) {
-close(fd);
-return EXIT_FAILURE;
-}
-
-close(fd);
-return EXIT_SUCCESS;
-}
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index da47e2704cf..7541b8d8ae9 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -93,7 +93,6 @@ def __init__(self,
name: Optional[str] = None,
base_temp_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper: Optional[str] = None,
sock_dir: Optional[str] = None,
drain_console: bool = False,
console_log: Optional[str] = None,
@@ -107,7 +106,6 @@ def __init__(self,
@param name: prefix for socket and log file names (default: qemu-PID)
@param base_temp_dir: default location where temp files are created
@param monitor_address: address for QMP monitor
-@param socket_scm_helper: helper program, required for
[PATCH 0/2] remove socket_scm_helper
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-socket-scm-helper CI: https://gitlab.com/jsnow/qemu/-/pipelines/338595177 19 insertions(+) 208 deletions(-) Seems good to me ...? --js John Snow (2): python/qmp: add send_fd_scm directly to QEMUMonitorProtocol python, iotests: remove socket_scm_helper tests/qemu-iotests/socket_scm_helper.c | 136 - python/qemu/machine/machine.py | 47 ++--- python/qemu/machine/qtest.py | 2 - python/qemu/qmp/__init__.py| 21 ++-- tests/Makefile.include | 1 - tests/meson.build | 4 - tests/qemu-iotests/iotests.py | 3 - tests/qemu-iotests/meson.build | 5 - tests/qemu-iotests/testenv.py | 8 +- 9 files changed, 19 insertions(+), 208 deletions(-) delete mode 100644 tests/qemu-iotests/socket_scm_helper.c delete mode 100644 tests/qemu-iotests/meson.build -- 2.31.1
[PATCH v2 11/11] accel/tcg: Push trace info building into atomic_common.c.inc
Use trace_mem_get_info instead of trace_mem_build_info,
using the TCGMemOpIdx that we already have. Do this in
the atomic_trace_*_pre function as common subroutines.
Signed-off-by: Richard Henderson
---
accel/tcg/atomic_template.h | 48 +--
accel/tcg/atomic_common.c.inc | 37 ++-
2 files changed, 37 insertions(+), 48 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 6ee0158c5f..d89af4cc1e 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -77,10 +77,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
- ATOMIC_MMU_IDX);
+uint16_t info = atomic_trace_rmw_pre(env, addr, oi);
-atomic_trace_rmw_pre(env, addr, info);
#if DATA_SIZE == 16
ret = atomic16_cmpxchg(haddr, cmpv, newv);
#else
@@ -99,10 +97,8 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr,
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
PAGE_READ, retaddr);
DATA_TYPE val;
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
- ATOMIC_MMU_IDX);
+uint16_t info = atomic_trace_ld_pre(env, addr, oi);
-atomic_trace_ld_pre(env, addr, info);
val = atomic16_read(haddr);
ATOMIC_MMU_CLEANUP;
atomic_trace_ld_post(env, addr, info);
@@ -114,10 +110,8 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val,
{
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
PAGE_WRITE, retaddr);
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, true,
- ATOMIC_MMU_IDX);
+uint16_t info = atomic_trace_st_pre(env, addr, oi);
-atomic_trace_st_pre(env, addr, info);
atomic16_set(haddr, val);
ATOMIC_MMU_CLEANUP;
atomic_trace_st_post(env, addr, info);
@@ -130,10 +124,8 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong
addr, ABI_TYPE val,
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
- ATOMIC_MMU_IDX);
+uint16_t info = atomic_trace_rmw_pre(env, addr, oi);
-atomic_trace_rmw_pre(env, addr, info);
ret = qatomic_xchg__nocheck(haddr, val);
ATOMIC_MMU_CLEANUP;
atomic_trace_rmw_post(env, addr, info);
@@ -147,9 +139,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong
addr, \
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE, \
PAGE_READ | PAGE_WRITE, retaddr); \
DATA_TYPE ret; \
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
- ATOMIC_MMU_IDX); \
-atomic_trace_rmw_pre(env, addr, info); \
+uint16_t info = atomic_trace_rmw_pre(env, addr, oi);\
ret = qatomic_##X(haddr, val); \
ATOMIC_MMU_CLEANUP; \
atomic_trace_rmw_post(env, addr, info); \
@@ -182,9 +172,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong
addr, \
XDATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE, \
PAGE_READ | PAGE_WRITE, retaddr); \
XDATA_TYPE cmp, old, new, val = xval; \
-uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
- ATOMIC_MMU_IDX); \
-atomic_trace_rmw_pre(env, addr, info); \
+uint16_t info = atomic_trace_rmw_pre(env, addr, oi);\
smp_mb(); \
cmp = qatomic_read__nocheck(haddr); \
do {\
@@ -228,10 +216,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
-uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
- ATOMIC_MMU_IDX);
+uint16_t info = atomic_trace_rmw_pre(env, addr, oi);
-atomic_trace_rmw_pre(env, addr, info);
#if DATA_SIZE == 16
ret =
[PATCH v2 10/11] trace: Fold mem-internal.h into mem.h
Since the last thing that mem.h does is include mem-internal.h,
the symbols are not actually private.
Signed-off-by: Richard Henderson
---
trace/mem-internal.h | 50
trace/mem.h | 50 ++--
2 files changed, 39 insertions(+), 61 deletions(-)
delete mode 100644 trace/mem-internal.h
diff --git a/trace/mem-internal.h b/trace/mem-internal.h
deleted file mode 100644
index 8b72b678fa..00
--- a/trace/mem-internal.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Helper functions for guest memory tracing
- *
- * Copyright (C) 2016 Lluís Vilanova
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#ifndef TRACE__MEM_INTERNAL_H
-#define TRACE__MEM_INTERNAL_H
-
-#define TRACE_MEM_SZ_SHIFT_MASK 0xf /* size shift mask */
-#define TRACE_MEM_SE (1ULL << 4)/* sign extended (y/n) */
-#define TRACE_MEM_BE (1ULL << 5)/* big endian (y/n) */
-#define TRACE_MEM_ST (1ULL << 6)/* store (y/n) */
-#define TRACE_MEM_MMU_SHIFT 8 /* mmu idx */
-
-static inline uint16_t trace_mem_build_info(
-int size_shift, bool sign_extend, MemOp endianness,
-bool store, unsigned int mmu_idx)
-{
-uint16_t res;
-
-res = size_shift & TRACE_MEM_SZ_SHIFT_MASK;
-if (sign_extend) {
-res |= TRACE_MEM_SE;
-}
-if (endianness == MO_BE) {
-res |= TRACE_MEM_BE;
-}
-if (store) {
-res |= TRACE_MEM_ST;
-}
-#ifdef CONFIG_SOFTMMU
-res |= mmu_idx << TRACE_MEM_MMU_SHIFT;
-#endif
-return res;
-}
-
-static inline uint16_t trace_mem_get_info(MemOp op,
- unsigned int mmu_idx,
- bool store)
-{
-return trace_mem_build_info(op & MO_SIZE, !!(op & MO_SIGN),
-op & MO_BSWAP, store,
-mmu_idx);
-}
-
-#endif /* TRACE__MEM_INTERNAL_H */
diff --git a/trace/mem.h b/trace/mem.h
index 9644f592b4..2f27e7bdf0 100644
--- a/trace/mem.h
+++ b/trace/mem.h
@@ -12,24 +12,52 @@
#include "tcg/tcg.h"
-
-/**
- * trace_mem_get_info:
- *
- * Return a value for the 'info' argument in guest memory access traces.
- */
-static uint16_t trace_mem_get_info(MemOp op, unsigned int mmu_idx, bool store);
+#define TRACE_MEM_SZ_SHIFT_MASK 0xf /* size shift mask */
+#define TRACE_MEM_SE (1ULL << 4)/* sign extended (y/n) */
+#define TRACE_MEM_BE (1ULL << 5)/* big endian (y/n) */
+#define TRACE_MEM_ST (1ULL << 6)/* store (y/n) */
+#define TRACE_MEM_MMU_SHIFT 8 /* mmu idx */
/**
* trace_mem_build_info:
*
* Return a value for the 'info' argument in guest memory access traces.
*/
-static uint16_t trace_mem_build_info(int size_shift, bool sign_extend,
- MemOp endianness, bool store,
- unsigned int mmuidx);
+static inline uint16_t trace_mem_build_info(int size_shift, bool sign_extend,
+MemOp endianness, bool store,
+unsigned int mmu_idx)
+{
+uint16_t res;
+
+res = size_shift & TRACE_MEM_SZ_SHIFT_MASK;
+if (sign_extend) {
+res |= TRACE_MEM_SE;
+}
+if (endianness == MO_BE) {
+res |= TRACE_MEM_BE;
+}
+if (store) {
+res |= TRACE_MEM_ST;
+}
+#ifdef CONFIG_SOFTMMU
+res |= mmu_idx << TRACE_MEM_MMU_SHIFT;
+#endif
+return res;
+}
-#include "trace/mem-internal.h"
+/**
+ * trace_mem_get_info:
+ *
+ * Return a value for the 'info' argument in guest memory access traces.
+ */
+static inline uint16_t trace_mem_get_info(MemOp op,
+ unsigned int mmu_idx,
+ bool store)
+{
+return trace_mem_build_info(op & MO_SIZE, !!(op & MO_SIGN),
+op & MO_BSWAP, store,
+mmu_idx);
+}
#endif /* TRACE__MEM_H */
--
2.25.1
[PATCH v2 09/11] accel/tcg: Expand ATOMIC_MMU_LOOKUP_*
Unify the parameters of atomic_mmu_lookup between cputlb.c and
user-exec.c. Call the function directly, and remove the macros.
Signed-off-by: Richard Henderson
---
accel/tcg/atomic_template.h | 41 +
accel/tcg/cputlb.c | 7 +--
accel/tcg/user-exec.c | 12 ++-
3 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index ae6b6a03be..6ee0158c5f 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -74,7 +74,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong
addr,
ABI_TYPE cmpv, ABI_TYPE newv,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
+ PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -95,7 +96,9 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong
addr,
ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
+ PAGE_READ, retaddr);
+DATA_TYPE val;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -109,7 +112,8 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr,
void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
+ PAGE_WRITE, retaddr);
uint16_t info = trace_mem_build_info(SHIFT, false, 0, true,
ATOMIC_MMU_IDX);
@@ -123,7 +127,8 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
+ PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -139,7 +144,8 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong
addr, ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
-DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;\
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE, \
+ PAGE_READ | PAGE_WRITE, retaddr); \
DATA_TYPE ret; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
ATOMIC_MMU_IDX); \
@@ -161,7 +167,8 @@ GEN_ATOMIC_HELPER(xor_fetch)
#undef GEN_ATOMIC_HELPER
-/* These helpers are, as a whole, full barriers. Within the helper,
+/*
+ * These helpers are, as a whole, full barriers. Within the helper,
* the leading barrier is explicit and the trailing barrier is within
* cmpxchg primitive.
*
@@ -172,7 +179,8 @@ GEN_ATOMIC_HELPER(xor_fetch)
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE xval, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
-XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
+XDATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE, \
+ PAGE_READ | PAGE_WRITE, retaddr); \
XDATA_TYPE cmp, old, new, val = xval; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
ATOMIC_MMU_IDX); \
@@ -217,7 +225,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
+DATA_TYPE *haddr = atomic_mmu_lookup(env, addr, oi, DATA_SIZE,
+ PAGE_READ | PAGE_WRITE, retaddr);
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT,
[PATCH v2 05/11] tcg: Rename helper_atomic_*_mmu and provide for user-only
Always provide the atomic interface using TCGMemOpIdx oi and uintptr_t retaddr. Rename from helper_* to cpu_* so as to (mostly) match the exec/cpu_ldst.h functions, and to emphasize that they are not callable from TCG directly. Signed-off-by: Richard Henderson --- include/tcg/tcg.h | 78 --- accel/tcg/cputlb.c| 8 ++-- accel/tcg/user-exec.c | 59 -- target/arm/helper-a64.c | 8 ++-- target/i386/tcg/mem_helper.c | 15 +-- target/m68k/op_helper.c | 19 +++-- target/ppc/mem_helper.c | 16 +++ target/s390x/tcg/mem_helper.c | 19 - 8 files changed, 104 insertions(+), 118 deletions(-) diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h index 25dd19d6e1..44ccd86f3e 100644 --- a/include/tcg/tcg.h +++ b/include/tcg/tcg.h @@ -1341,31 +1341,32 @@ void helper_be_stq_mmu(CPUArchState *env, target_ulong addr, uint64_t val, # define helper_ret_stl_mmu helper_le_stl_mmu # define helper_ret_stq_mmu helper_le_stq_mmu #endif +#endif /* CONFIG_SOFTMMU */ -uint32_t helper_atomic_cmpxchgb_mmu(CPUArchState *env, target_ulong addr, +uint32_t cpu_atomic_cmpxchgb_mmu(CPUArchState *env, target_ulong addr, + uint32_t cmpv, uint32_t newv, + TCGMemOpIdx oi, uintptr_t retaddr); +uint32_t cpu_atomic_cmpxchgw_le_mmu(CPUArchState *env, target_ulong addr, uint32_t cmpv, uint32_t newv, TCGMemOpIdx oi, uintptr_t retaddr); -uint32_t helper_atomic_cmpxchgw_le_mmu(CPUArchState *env, target_ulong addr, - uint32_t cmpv, uint32_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); -uint32_t helper_atomic_cmpxchgl_le_mmu(CPUArchState *env, target_ulong addr, - uint32_t cmpv, uint32_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); -uint64_t helper_atomic_cmpxchgq_le_mmu(CPUArchState *env, target_ulong addr, - uint64_t cmpv, uint64_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); -uint32_t helper_atomic_cmpxchgw_be_mmu(CPUArchState *env, target_ulong addr, - uint32_t cmpv, uint32_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); -uint32_t helper_atomic_cmpxchgl_be_mmu(CPUArchState *env, target_ulong addr, - uint32_t cmpv, uint32_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); -uint64_t helper_atomic_cmpxchgq_be_mmu(CPUArchState *env, target_ulong addr, - uint64_t cmpv, uint64_t newv, - TCGMemOpIdx oi, uintptr_t retaddr); +uint32_t cpu_atomic_cmpxchgl_le_mmu(CPUArchState *env, target_ulong addr, +uint32_t cmpv, uint32_t newv, +TCGMemOpIdx oi, uintptr_t retaddr); +uint64_t cpu_atomic_cmpxchgq_le_mmu(CPUArchState *env, target_ulong addr, +uint64_t cmpv, uint64_t newv, +TCGMemOpIdx oi, uintptr_t retaddr); +uint32_t cpu_atomic_cmpxchgw_be_mmu(CPUArchState *env, target_ulong addr, +uint32_t cmpv, uint32_t newv, +TCGMemOpIdx oi, uintptr_t retaddr); +uint32_t cpu_atomic_cmpxchgl_be_mmu(CPUArchState *env, target_ulong addr, +uint32_t cmpv, uint32_t newv, +TCGMemOpIdx oi, uintptr_t retaddr); +uint64_t cpu_atomic_cmpxchgq_be_mmu(CPUArchState *env, target_ulong addr, +uint64_t cmpv, uint64_t newv, +TCGMemOpIdx oi, uintptr_t retaddr); #define GEN_ATOMIC_HELPER(NAME, TYPE, SUFFIX) \ -TYPE helper_atomic_ ## NAME ## SUFFIX ## _mmu \ +TYPE cpu_atomic_ ## NAME ## SUFFIX ## _mmu\ (CPUArchState *env, target_ulong addr, TYPE val, \ TCGMemOpIdx oi, uintptr_t retaddr); @@ -1411,31 +1412,22 @@ GEN_ATOMIC_HELPER_ALL(xchg) #undef GEN_ATOMIC_HELPER_ALL #undef GEN_ATOMIC_HELPER -#endif /* CONFIG_SOFTMMU */ -/* - * These aren't really a "proper" helpers because TCG cannot manage Int128. - * However, use the same format as the others, for use by the backends. - * - * The cmpxchg functions are only defined if HAVE_CMPXCHG128; - * the ld/st functions are only defined if HAVE_ATOMIC128, - * as defined by . - */ -Int128 helper_atomic_cmpxchgo_le_mmu(CPUArchState *env, target_ulong addr, - Int128 cmpv, Int128 newv, - TCGMemOpIdx oi, uintptr_t retaddr); -Int128 helper_atomic_cmpxchgo_be_mmu(CPUArchState *env,
[PATCH v2 06/11] accel/tcg: Standardize atomic helpers on softmmu api
Reduce the amount of code duplication by always passing the TCGMemOpIdx argument to helper_atomic_*. This is not currently used for user-only, but it's easy to ignore. Signed-off-by: Richard Henderson --- accel/tcg/tcg-runtime.h | 46 --- accel/tcg/cputlb.c| 32 accel/tcg/user-exec.c | 26 - tcg/tcg-op.c | 47 --- accel/tcg/atomic_common.c.inc | 70 +++ 5 files changed, 78 insertions(+), 143 deletions(-) diff --git a/accel/tcg/tcg-runtime.h b/accel/tcg/tcg-runtime.h index 91a5b7e85f..37cbd722bf 100644 --- a/accel/tcg/tcg-runtime.h +++ b/accel/tcg/tcg-runtime.h @@ -39,8 +39,6 @@ DEF_HELPER_FLAGS_1(exit_atomic, TCG_CALL_NO_WG, noreturn, env) DEF_HELPER_FLAGS_3(memset, TCG_CALL_NO_RWG, ptr, ptr, int, ptr) #endif /* IN_HELPER_PROTO */ -#ifdef CONFIG_SOFTMMU - DEF_HELPER_FLAGS_5(atomic_cmpxchgb, TCG_CALL_NO_WG, i32, env, tl, i32, i32, i32) DEF_HELPER_FLAGS_5(atomic_cmpxchgw_be, TCG_CALL_NO_WG, @@ -88,50 +86,6 @@ DEF_HELPER_FLAGS_5(atomic_cmpxchgq_le, TCG_CALL_NO_WG, TCG_CALL_NO_WG, i32, env, tl, i32, i32) #endif /* CONFIG_ATOMIC64 */ -#else - -DEF_HELPER_FLAGS_4(atomic_cmpxchgb, TCG_CALL_NO_WG, i32, env, tl, i32, i32) -DEF_HELPER_FLAGS_4(atomic_cmpxchgw_be, TCG_CALL_NO_WG, i32, env, tl, i32, i32) -DEF_HELPER_FLAGS_4(atomic_cmpxchgw_le, TCG_CALL_NO_WG, i32, env, tl, i32, i32) -DEF_HELPER_FLAGS_4(atomic_cmpxchgl_be, TCG_CALL_NO_WG, i32, env, tl, i32, i32) -DEF_HELPER_FLAGS_4(atomic_cmpxchgl_le, TCG_CALL_NO_WG, i32, env, tl, i32, i32) -#ifdef CONFIG_ATOMIC64 -DEF_HELPER_FLAGS_4(atomic_cmpxchgq_be, TCG_CALL_NO_WG, i64, env, tl, i64, i64) -DEF_HELPER_FLAGS_4(atomic_cmpxchgq_le, TCG_CALL_NO_WG, i64, env, tl, i64, i64) -#endif - -#ifdef CONFIG_ATOMIC64 -#define GEN_ATOMIC_HELPERS(NAME) \ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), b), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), w_le), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), w_be), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), l_le), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), l_be), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), q_le), \ - TCG_CALL_NO_WG, i64, env, tl, i64)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), q_be), \ - TCG_CALL_NO_WG, i64, env, tl, i64) -#else -#define GEN_ATOMIC_HELPERS(NAME) \ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), b), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), w_le), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), w_be), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), l_le), \ - TCG_CALL_NO_WG, i32, env, tl, i32)\ -DEF_HELPER_FLAGS_3(glue(glue(atomic_, NAME), l_be), \ - TCG_CALL_NO_WG, i32, env, tl, i32) -#endif /* CONFIG_ATOMIC64 */ - -#endif /* CONFIG_SOFTMMU */ - GEN_ATOMIC_HELPERS(fetch_add) GEN_ATOMIC_HELPERS(fetch_and) GEN_ATOMIC_HELPERS(fetch_or) diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 2ff72e0aed..ea6fd06834 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -2725,38 +2725,6 @@ void cpu_stq_le_data(CPUArchState *env, target_ulong ptr, uint64_t val) #include "atomic_template.h" #endif -/* Second set of helpers are directly callable from TCG as helpers. */ - -#undef EXTRA_ARGS -#undef ATOMIC_NAME -#undef ATOMIC_MMU_LOOKUP_RW -#undef ATOMIC_MMU_LOOKUP_R -#undef ATOMIC_MMU_LOOKUP_W - -#define EXTRA_ARGS , TCGMemOpIdx oi -#define ATOMIC_NAME(X) HELPER(glue(glue(atomic_ ## X, SUFFIX), END)) -#define ATOMIC_MMU_LOOKUP_RW \ -atomic_mmu_lookup(env, addr, oi, DATA_SIZE, PAGE_READ | PAGE_WRITE, GETPC()) -#define ATOMIC_MMU_LOOKUP_R \ -atomic_mmu_lookup(env, addr, oi, DATA_SIZE, PAGE_READ, GETPC()) -#define ATOMIC_MMU_LOOKUP_W \ -atomic_mmu_lookup(env, addr, oi, DATA_SIZE, PAGE_WRITE, GETPC()) - -#define DATA_SIZE 1 -#include "atomic_template.h" - -#define DATA_SIZE 2 -#include "atomic_template.h" - -#define DATA_SIZE 4 -#include "atomic_template.h" - -#ifdef CONFIG_ATOMIC64 -#define DATA_SIZE 8 -#include "atomic_template.h" -#endif -#undef ATOMIC_MMU_IDX - /* Code access functions. */ static uint64_t full_ldub_code(CPUArchState *env, target_ulong addr, diff --git
[PATCH v2 04/11] qemu/atomic: Add aligned_{int64,uint64}_t types
Use it to avoid some clang-12 -Watomic-alignment errors,
forcing some structures to be aligned and as a pointer when
we have ensured that the address is aligned.
Signed-off-by: Richard Henderson
---
accel/tcg/atomic_template.h | 4 ++--
include/qemu/atomic.h | 14 +-
include/qemu/stats64.h | 2 +-
softmmu/timers-state.h | 2 +-
linux-user/hppa/cpu_loop.c | 2 +-
util/qsp.c | 4 ++--
6 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index afa8a9daf3..d347462af5 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -28,8 +28,8 @@
# define SHIFT 4
#elif DATA_SIZE == 8
# define SUFFIX q
-# define DATA_TYPE uint64_t
-# define SDATA_TYPE int64_t
+# define DATA_TYPE aligned_uint64_t
+# define SDATA_TYPE aligned_int64_t
# define BSWAP bswap64
# define SHIFT 3
#elif DATA_SIZE == 4
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index a45f115fe1..6e9d8b3882 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -238,7 +238,19 @@
_oldn; \
})
-/* Abstractions to access atomically (i.e. "once") i64/u64 variables */
+/*
+ * Abstractions to access atomically (i.e. "once") i64/u64 variables.
+ *
+ * The i386 abi is odd in that by default members are only aligned to
+ * 4 bytes, which means that 8-byte types can wind up mis-aligned.
+ * Clang will then warn about this, and emit a call into libatomic.
+ *
+ * Use of these types in structures when they will be used with atomic
+ * operations can avoid this.
+ */
+typedef int64_t aligned_int64_t __attribute__((aligned(8)));
+typedef uint64_t aligned_uint64_t __attribute__((aligned(8)));
+
#ifdef CONFIG_ATOMIC64
/* Use __nocheck because sizeof(void *) might be < sizeof(u64) */
#define qatomic_read_i64 qatomic_read__nocheck
diff --git a/include/qemu/stats64.h b/include/qemu/stats64.h
index fdd3d1b8f9..802402254b 100644
--- a/include/qemu/stats64.h
+++ b/include/qemu/stats64.h
@@ -21,7 +21,7 @@
typedef struct Stat64 {
#ifdef CONFIG_ATOMIC64
-uint64_t value;
+aligned_uint64_t value;
#else
uint32_t low, high;
uint32_t lock;
diff --git a/softmmu/timers-state.h b/softmmu/timers-state.h
index 8c262ce139..94bb7394c5 100644
--- a/softmmu/timers-state.h
+++ b/softmmu/timers-state.h
@@ -47,7 +47,7 @@ typedef struct TimersState {
int64_t last_delta;
/* Compensate for varying guest execution speed. */
-int64_t qemu_icount_bias;
+aligned_int64_t qemu_icount_bias;
int64_t vm_clock_warp_start;
int64_t cpu_clock_offset;
diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c
index 3aaaf3337c..82d8183821 100644
--- a/linux-user/hppa/cpu_loop.c
+++ b/linux-user/hppa/cpu_loop.c
@@ -82,7 +82,7 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
o64 = *(uint64_t *)g2h(cs, old);
n64 = *(uint64_t *)g2h(cs, new);
#ifdef CONFIG_ATOMIC64
-r64 = qatomic_cmpxchg__nocheck((uint64_t *)g2h(cs, addr),
+r64 = qatomic_cmpxchg__nocheck((aligned_uint64_t *)g2h(cs,
addr),
o64, n64);
ret = r64 != o64;
#else
diff --git a/util/qsp.c b/util/qsp.c
index bacc5fa2f6..8562b14a87 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -83,8 +83,8 @@ typedef struct QSPCallSite QSPCallSite;
struct QSPEntry {
void *thread_ptr;
const QSPCallSite *callsite;
-uint64_t n_acqs;
-uint64_t ns;
+aligned_uint64_t n_acqs;
+aligned_uint64_t ns;
unsigned int n_objs; /* count of coalesced objs; only used for reporting */
};
typedef struct QSPEntry QSPEntry;
--
2.25.1
[PATCH v2 07/11] accel/tcg: Fold EXTRA_ARGS into atomic_template.h
All instances of EXTRA_ARGS are now identical.
Signed-off-by: Richard Henderson
---
accel/tcg/atomic_template.h | 36
accel/tcg/cputlb.c | 1 -
accel/tcg/user-exec.c | 1 -
3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index d347462af5..52fb26a274 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -71,7 +71,8 @@
#endif
ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
- ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
+ ABI_TYPE cmpv, ABI_TYPE newv,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
@@ -92,7 +93,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong
addr,
#if DATA_SIZE >= 16
#if HAVE_ATOMIC128
-ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
+ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
@@ -106,8 +108,8 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr EXTRA_ARGS)
return val;
}
-void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
- ABI_TYPE val EXTRA_ARGS)
+void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
@@ -121,8 +123,8 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
}
#endif
#else
-ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
- ABI_TYPE val EXTRA_ARGS)
+ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
@@ -139,7 +141,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong
addr,
#define GEN_ATOMIC_HELPER(X)\
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
-ABI_TYPE val EXTRA_ARGS)\
+ABI_TYPE val, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
ATOMIC_MMU_DECLS; \
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;\
@@ -173,7 +175,7 @@ GEN_ATOMIC_HELPER(xor_fetch)
*/
#define GEN_ATOMIC_HELPER_FN(X, FN, XDATA_TYPE, RET)\
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
-ABI_TYPE xval EXTRA_ARGS) \
+ABI_TYPE xval, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
ATOMIC_MMU_DECLS; \
XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
@@ -218,7 +220,8 @@ GEN_ATOMIC_HELPER_FN(umax_fetch, MAX, DATA_TYPE, new)
#endif
ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
- ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
+ ABI_TYPE cmpv, ABI_TYPE newv,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
@@ -239,7 +242,8 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
#if DATA_SIZE >= 16
#if HAVE_ATOMIC128
-ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
+ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
@@ -253,8 +257,8 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr EXTRA_ARGS)
return BSWAP(val);
}
-void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
- ABI_TYPE val EXTRA_ARGS)
+void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
@@ -270,8 +274,8 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
}
#endif
#else
-ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
- ABI_TYPE val EXTRA_ARGS)
+ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
+ TCGMemOpIdx oi, uintptr_t retaddr)
{
ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
@@ -288,7 +292,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env,
[PATCH v2 02/11] qemu/atomic: Simplify typeof_strip_qual
The right-hand side of the comma operator has the type quals
stripped without also undergoing implicit promotion.
Signed-off-by: Richard Henderson
---
include/qemu/atomic.h | 41 -
1 file changed, 4 insertions(+), 37 deletions(-)
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 99d6030095..55d75fc757 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -18,47 +18,14 @@
/* Compiler barrier */
#define barrier() ({ asm volatile("" ::: "memory"); (void)0; })
-/* The variable that receives the old value of an atomically-accessed
+/*
+ * The variable that receives the old value of an atomically-accessed
* variable must be non-qualified, because atomic builtins return values
* through a pointer-type argument as in __atomic_load(, , MODEL).
*
- * This macro has to handle types smaller than int manually, because of
- * implicit promotion. int and larger types, as well as pointers, can be
- * converted to a non-qualified type just by applying a binary operator.
+ * Handle this by evaluating an expression involving the comma operator.
*/
-#define typeof_strip_qual(expr)
\
- typeof(
\
-__builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(expr), bool) ||
\
-__builtin_types_compatible_p(typeof(expr), const bool) ||
\
-__builtin_types_compatible_p(typeof(expr), volatile bool) ||
\
-__builtin_types_compatible_p(typeof(expr), const volatile bool),
\
-(bool)1,
\
-__builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(expr), signed char) ||
\
-__builtin_types_compatible_p(typeof(expr), const signed char) ||
\
-__builtin_types_compatible_p(typeof(expr), volatile signed char) ||
\
-__builtin_types_compatible_p(typeof(expr), const volatile signed
char),\
-(signed char)1,
\
-__builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(expr), unsigned char) ||
\
-__builtin_types_compatible_p(typeof(expr), const unsigned char) ||
\
-__builtin_types_compatible_p(typeof(expr), volatile unsigned char) ||
\
-__builtin_types_compatible_p(typeof(expr), const volatile unsigned
char), \
-(unsigned char)1,
\
-__builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(expr), signed short) ||
\
-__builtin_types_compatible_p(typeof(expr), const signed short) ||
\
-__builtin_types_compatible_p(typeof(expr), volatile signed short) ||
\
-__builtin_types_compatible_p(typeof(expr), const volatile signed
short), \
-(signed short)1,
\
-__builtin_choose_expr(
\
- __builtin_types_compatible_p(typeof(expr), unsigned short) ||
\
-__builtin_types_compatible_p(typeof(expr), const unsigned short) ||
\
-__builtin_types_compatible_p(typeof(expr), volatile unsigned short) ||
\
-__builtin_types_compatible_p(typeof(expr), const volatile unsigned
short), \
-(unsigned short)1,
\
- (expr)+0))
+#define typeof_strip_qual(expr) typeof((void)0, (expr))
#ifdef __ATOMIC_RELAXED
/* For C11 atomic ops */
--
2.25.1
[PATCH v2 01/11] qemu/atomic: Use macros for CONFIG_ATOMIC64
Clang warnings about questionable atomic usage get localized
to the inline function in atomic.h. By using a macro, we get
the full traceback to the original use that caused the warning.
Signed-off-by: Richard Henderson
---
include/qemu/atomic.h | 25 +
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 3ccf84fd46..99d6030095 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -457,26 +457,11 @@
/* Abstractions to access atomically (i.e. "once") i64/u64 variables */
#ifdef CONFIG_ATOMIC64
-static inline int64_t qatomic_read_i64(const int64_t *ptr)
-{
-/* use __nocheck because sizeof(void *) might be < sizeof(u64) */
-return qatomic_read__nocheck(ptr);
-}
-
-static inline uint64_t qatomic_read_u64(const uint64_t *ptr)
-{
-return qatomic_read__nocheck(ptr);
-}
-
-static inline void qatomic_set_i64(int64_t *ptr, int64_t val)
-{
-qatomic_set__nocheck(ptr, val);
-}
-
-static inline void qatomic_set_u64(uint64_t *ptr, uint64_t val)
-{
-qatomic_set__nocheck(ptr, val);
-}
+/* Use __nocheck because sizeof(void *) might be < sizeof(u64) */
+#define qatomic_read_i64 qatomic_read__nocheck
+#define qatomic_read_u64 qatomic_read__nocheck
+#define qatomic_set_i64 qatomic_set__nocheck
+#define qatomic_set_u64 qatomic_set__nocheck
static inline void qatomic64_init(void)
{
--
2.25.1
[PATCH v2 00/11] Atomic cleanup + clang-12 build fix
This is intended to fix building with clang-12 on i386.
Version 2 bears little relation to version 1, in that I no longer
turn off the warning, which merely hid the problem until link time
failed to find libatomic symbols.
In the process, I found bugs wrt handling of guest memory in target/
with respect to atomics, fixed by unifying the api between softmmu
and user-only and removing some ifdefs under target/.
Unification of the api allowed some further cleanups.
I think that patches 1-6 fix all of the bugs, and that 7-11 are only
cleanup and could be left to next cycle.
r~
Richard Henderson (11):
qemu/atomic: Use macros for CONFIG_ATOMIC64
qemu/atomic: Simplify typeof_strip_qual
qemu/atomic: Remove pre-C11 atomic fallbacks
qemu/atomic: Add aligned_{int64,uint64}_t types
tcg: Rename helper_atomic_*_mmu and provide for user-only
accel/tcg: Standardize atomic helpers on softmmu api
accel/tcg: Fold EXTRA_ARGS into atomic_template.h
accel/tcg: Remove ATOMIC_MMU_DECLS
accel/tcg: Expand ATOMIC_MMU_LOOKUP_*
trace: Fold mem-internal.h into mem.h
accel/tcg: Push trace info building into atomic_common.c.inc
configure | 7 -
accel/tcg/atomic_template.h | 141 -
accel/tcg/tcg-runtime.h | 46 --
include/qemu/atomic.h | 284 --
include/qemu/stats64.h| 2 +-
include/tcg/tcg.h | 78 +-
softmmu/timers-state.h| 2 +-
trace/mem-internal.h | 50 --
trace/mem.h | 50 --
accel/tcg/cputlb.c| 49 +-
accel/tcg/user-exec.c | 41 ++---
linux-user/hppa/cpu_loop.c| 2 +-
target/arm/helper-a64.c | 8 +-
target/i386/tcg/mem_helper.c | 15 +-
target/m68k/op_helper.c | 19 +--
target/ppc/mem_helper.c | 16 +-
target/s390x/tcg/mem_helper.c | 19 +--
tcg/tcg-op.c | 47 +-
util/qsp.c| 4 +-
accel/tcg/atomic_common.c.inc | 107 +++--
20 files changed, 324 insertions(+), 663 deletions(-)
delete mode 100644 trace/mem-internal.h
--
2.25.1
[PATCH v2 08/11] accel/tcg: Remove ATOMIC_MMU_DECLS
All definitions are now empty.
Signed-off-by: Richard Henderson
---
accel/tcg/atomic_template.h | 12
accel/tcg/cputlb.c | 1 -
accel/tcg/user-exec.c | 1 -
3 files changed, 14 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 52fb26a274..ae6b6a03be 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -74,7 +74,6 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong
addr,
ABI_TYPE cmpv, ABI_TYPE newv,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
@@ -96,7 +95,6 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong
addr,
ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -111,7 +109,6 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr,
void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, true,
ATOMIC_MMU_IDX);
@@ -126,7 +123,6 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
@@ -143,7 +139,6 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong
addr, ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
-ATOMIC_MMU_DECLS; \
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;\
DATA_TYPE ret; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
@@ -177,7 +172,6 @@ GEN_ATOMIC_HELPER(xor_fetch)
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE xval, TCGMemOpIdx oi, uintptr_t retaddr) \
{ \
-ATOMIC_MMU_DECLS; \
XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
XDATA_TYPE cmp, old, new, val = xval; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,\
@@ -223,7 +217,6 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
@@ -245,7 +238,6 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env,
target_ulong addr,
ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
ATOMIC_MMU_IDX);
@@ -260,7 +252,6 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong
addr,
void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, true,
ATOMIC_MMU_IDX);
@@ -277,7 +268,6 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr, ABI_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-ATOMIC_MMU_DECLS;
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
ABI_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
@@ -294,7 +284,6 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong
addr, ABI_TYPE val,
ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val, TCGMemOpIdx oi,
[PATCH v2 03/11] qemu/atomic: Remove pre-C11 atomic fallbacks
We now require c11, so the fallbacks are now dead code
Signed-off-by: Richard Henderson
---
configure | 7 --
include/qemu/atomic.h | 204 +++---
2 files changed, 10 insertions(+), 201 deletions(-)
diff --git a/configure b/configure
index 4d0a2bfdd8..628d596be8 100755
--- a/configure
+++ b/configure
@@ -3902,18 +3902,11 @@ cat > $TMPC << EOF
int main(void)
{
uint64_t x = 0, y = 0;
-#ifdef __ATOMIC_RELAXED
y = __atomic_load_n(, __ATOMIC_RELAXED);
__atomic_store_n(, y, __ATOMIC_RELAXED);
__atomic_compare_exchange_n(, , x, 0, __ATOMIC_RELAXED,
__ATOMIC_RELAXED);
__atomic_exchange_n(, y, __ATOMIC_RELAXED);
__atomic_fetch_add(, y, __ATOMIC_RELAXED);
-#else
- typedef char is_host64[sizeof(void *) >= sizeof(uint64_t) ? 1 : -1];
- __sync_lock_test_and_set(, y);
- __sync_val_compare_and_swap(, y, 0);
- __sync_fetch_and_add(, y);
-#endif
return 0;
}
EOF
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 55d75fc757..a45f115fe1 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -27,8 +27,9 @@
*/
#define typeof_strip_qual(expr) typeof((void)0, (expr))
-#ifdef __ATOMIC_RELAXED
-/* For C11 atomic ops */
+#ifndef __ATOMIC_RELAXED
+#error "Expecting C11 atomic ops"
+#endif
/* Manual memory barriers
*
@@ -206,193 +207,8 @@
#define qatomic_xor(ptr, n) \
((void) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST))
-#else /* __ATOMIC_RELAXED */
-
-#ifdef __alpha__
-#define smp_read_barrier_depends() asm volatile("mb":::"memory")
-#endif
-
-#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
-
-/*
- * Because of the strongly ordered storage model, wmb() and rmb() are nops
- * here (a compiler barrier only). QEMU doesn't do accesses to write-combining
- * qemu memory or non-temporal load/stores from C code.
- */
-#define smp_mb_release() barrier()
-#define smp_mb_acquire() barrier()
-
-/*
- * __sync_lock_test_and_set() is documented to be an acquire barrier only,
- * but it is a full barrier at the hardware level. Add a compiler barrier
- * to make it a full barrier also at the compiler level.
- */
-#define qatomic_xchg(ptr, i)(barrier(), __sync_lock_test_and_set(ptr, i))
-
-#elif defined(_ARCH_PPC)
-
-/*
- * We use an eieio() for wmb() on powerpc. This assumes we don't
- * need to order cacheable and non-cacheable stores with respect to
- * each other.
- *
- * smp_mb has the same problem as on x86 for not-very-new GCC
- * (http://patchwork.ozlabs.org/patch/126184/, Nov 2011).
- */
-#define smp_wmb() ({ asm volatile("eieio" ::: "memory"); (void)0; })
-#if defined(__powerpc64__)
-#define smp_mb_release() ({ asm volatile("lwsync" ::: "memory"); (void)0; })
-#define smp_mb_acquire() ({ asm volatile("lwsync" ::: "memory"); (void)0; })
-#else
-#define smp_mb_release() ({ asm volatile("sync" ::: "memory"); (void)0; })
-#define smp_mb_acquire() ({ asm volatile("sync" ::: "memory"); (void)0; })
-#endif
-#define smp_mb() ({ asm volatile("sync" ::: "memory"); (void)0; })
-
-#endif /* _ARCH_PPC */
-
-/*
- * For (host) platforms we don't have explicit barrier definitions
- * for, we use the gcc __sync_synchronize() primitive to generate a
- * full barrier. This should be safe on all platforms, though it may
- * be overkill for smp_mb_acquire() and smp_mb_release().
- */
-#ifndef smp_mb
-#define smp_mb() __sync_synchronize()
-#endif
-
-#ifndef smp_mb_acquire
-#define smp_mb_acquire() __sync_synchronize()
-#endif
-
-#ifndef smp_mb_release
-#define smp_mb_release() __sync_synchronize()
-#endif
-
-#ifndef smp_read_barrier_depends
-#define smp_read_barrier_depends() barrier()
-#endif
-
-#ifndef signal_barrier
-#define signal_barrier()barrier()
-#endif
-
-/* These will only be atomic if the processor does the fetch or store
- * in a single issue memory operation
- */
-#define qatomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
-#define qatomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile*) (p)) = (i))
-
-#define qatomic_read(ptr) qatomic_read__nocheck(ptr)
-#define qatomic_set(ptr, i) qatomic_set__nocheck(ptr,i)
-
-/**
- * qatomic_rcu_read - reads a RCU-protected pointer to a local variable
- * into a RCU read-side critical section. The pointer can later be safely
- * dereferenced within the critical section.
- *
- * This ensures that the pointer copy is invariant thorough the whole critical
- * section.
- *
- * Inserts memory barriers on architectures that require them (currently only
- * Alpha) and documents which pointers are protected by RCU.
- *
- * qatomic_rcu_read also includes a compiler barrier to ensure that
- * value-speculative optimizations (e.g. VSS: Value Speculation
- * Scheduling) does not perform the data read before the pointer read
- * by speculating the value of the pointer.
- *
- * Should match qatomic_rcu_set(), qatomic_xchg(), qatomic_cmpxchg().
- */
-#define qatomic_rcu_read(ptr)({
[PATCH v2 22/24] python/aqmp: add asyncio_run compatibility wrapper
As a convenience. It isn't used by the library itself, but it is used by the test suite. It will also come in handy for users of the library still on Python 3.6. Signed-off-by: John Snow --- python/qemu/aqmp/util.py | 19 +++ 1 file changed, 19 insertions(+) diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py index 70ef94ad600..de0df44cbd7 100644 --- a/python/qemu/aqmp/util.py +++ b/python/qemu/aqmp/util.py @@ -137,6 +137,25 @@ async def wait_closed(writer: asyncio.StreamWriter) -> None: await flush(writer) +def asyncio_run(coro: Coroutine[Any, Any, T], *, debug: bool = False) -> T: +""" +Python 3.6-compatible `asyncio.run` wrapper. + +:param coro: A coroutine to execute now. +:return: The return value from the coroutine. +""" +if sys.version_info >= (3, 7): +return asyncio.run(coro, debug=debug) + +# Python 3.6 +loop = asyncio.get_event_loop() +loop.set_debug(debug) +ret = loop.run_until_complete(coro) +loop.close() + +return ret + + # # Section: Logging & Debugging # -- 2.31.1
[PATCH v2 23/24] python/aqmp: add scary message
Add a warning whenever AQMP is used to steer people gently away from using it for the time-being. Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 14 ++ 1 file changed, 14 insertions(+) diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index ef2903fa7fc..321ea5c5c4b 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -21,6 +21,8 @@ # This work is licensed under the terms of the GNU GPL, version 2. See # the COPYING file in the top-level directory. +import warnings + from .error import AQMPError from .events import EventListener from .message import Message @@ -28,6 +30,18 @@ from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient +_WMSG = """ + +The Asynchronous QMP library is currently in development and its API +should be considered highly fluid and subject to change. It should +not be used by any other scripts checked into the QEMU tree. + +Proceed with caution! +""" + +warnings.warn(_WMSG, FutureWarning) + + # The order of these fields impact the Sphinx documentation order. __all__ = ( # Classes, most to least important -- 2.31.1
[PATCH v2 21/24] python/aqmp: add _raw() execution interface
This is added in anticipation of wanting it for a synchronous wrapper
for the iotest interface. Normally, execute() and execute_msg() both
raise QMP errors in the form of Python exceptions.
Many iotests expect the entire reply as-is. To reduce churn there, add a
private execution interface that will ease transition churn. However, I
do not wish to encourage its use, so it will remain a private interface.
Signed-off-by: John Snow
---
python/qemu/aqmp/qmp_client.py | 51 ++
1 file changed, 51 insertions(+)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 879348feaaa..82e9dab124c 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -484,6 +484,57 @@ async def _execute(self, msg: Message, assign_id: bool =
True) -> Message:
exec_id = await self._issue(msg)
return await self._reply(exec_id)
+@upper_half
+@require(Runstate.RUNNING)
+async def _raw(
+self,
+msg: Union[Message, Mapping[str, object], bytes],
+assign_id: bool = True,
+) -> Message:
+"""
+Issue a raw `Message` to the QMP server and await a reply.
+
+:param msg:
+A Message to send to the server. It may be a `Message`, any
+Mapping (including Dict), or raw bytes.
+:param assign_id:
+Assign an arbitrary execution ID to this message. If
+`False`, the existing id must either be absent (and no other
+such pending execution may omit an ID) or a string. If it is
+a string, it must not start with '__aqmp#' and no other such
+pending execution may currently be using that ID.
+
+:return: Execution reply from the server.
+
+:raise ExecInterruptedError:
+When the reply could not be retrieved because the connection
+was lost, or some other problem.
+:raise TypeError:
+When assign_id is `False`, an ID is given, and it is not a string.
+:raise ValueError:
+When assign_id is `False`, but the ID is not usable;
+Either because it starts with '__aqmp#' or it is already in-use.
+"""
+# 1. convert generic Mapping or bytes to a QMP Message
+# 2. copy Message objects so that we assign an ID only to the copy.
+msg = Message(msg)
+
+exec_id = msg.get('id')
+if not assign_id and 'id' in msg:
+if not isinstance(exec_id, str):
+raise TypeError(f"ID ('{exec_id}') must be a string.")
+if exec_id.startswith('__aqmp#'):
+raise ValueError(
+f"ID ('{exec_id}') must not start with '__aqmp#'."
+)
+
+if not assign_id and exec_id in self._pending:
+raise ValueError(
+f"ID '{exec_id}' is in-use and cannot be used."
+)
+
+return await self._execute(msg, assign_id=assign_id)
+
@upper_half
@require(Runstate.RUNNING)
async def execute_msg(self, msg: Message) -> object:
--
2.31.1
[PATCH v2 20/24] python/aqmp: add execute() interfaces
Add execute() and execute_msg().
_execute() is split into _issue() and _reply() halves so that
hypothetical subclasses of QMP that want to support different execution
paradigms can do so.
I anticipate a synchronous interface may have need of separating the
send/reply phases. However, I do not wish to expose that interface here
and want to actively discourage it, so they remain private interfaces.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 4 +-
python/qemu/aqmp/qmp_client.py | 202 +++--
2 files changed, 198 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 7bd66a48bed..ef2903fa7fc 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -25,7 +25,7 @@
from .events import EventListener
from .message import Message
from .protocol import ConnectError, Runstate
-from .qmp_client import QMPClient
+from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
# The order of these fields impact the Sphinx documentation order.
@@ -39,4 +39,6 @@
# Exceptions, most generic to most explicit
'AQMPError',
'ConnectError',
+'ExecuteError',
+'ExecInterruptedError',
)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index fa0cc7c5ae5..879348feaaa 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -7,8 +7,7 @@
accept an incoming connection from that server.
"""
-# The import workarounds here are fixed in the next commit.
-import asyncio # pylint: disable=unused-import # noqa
+import asyncio
import logging
from typing import (
Dict,
@@ -22,8 +21,8 @@
from .error import AQMPError, ProtocolError
from .events import Events
from .message import Message
-from .models import Greeting
-from .protocol import AsyncProtocol
+from .models import ErrorResponse, Greeting
+from .protocol import AsyncProtocol, Runstate, require
from .util import (
bottom_half,
exception_summary,
@@ -65,11 +64,32 @@ class NegotiationError(_WrappedProtocolError):
"""
+class ExecuteError(AQMPError):
+"""
+Exception raised by `QMPClient.execute()` on RPC failure.
+
+:param error_response: The RPC error response object.
+:param sent: The sent RPC message that caused the failure.
+:param received: The raw RPC error reply received.
+"""
+def __init__(self, error_response: ErrorResponse,
+ sent: Message, received: Message):
+super().__init__(error_response.error.desc)
+#: The sent `Message` that caused the failure
+self.sent: Message = sent
+#: The received `Message` that indicated failure
+self.received: Message = received
+#: The parsed error response
+self.error: ErrorResponse = error_response
+#: The QMP error class
+self.error_class: str = error_response.error.class_
+
+
class ExecInterruptedError(AQMPError):
"""
-Exception raised when an RPC is interrupted.
+Exception raised by `execute()` (et al) when an RPC is interrupted.
-This error is raised when an execute() statement could not be
+This error is raised when an `execute()` statement could not be
completed. This can occur because the connection itself was
terminated before a reply was received.
@@ -112,6 +132,27 @@ class ServerParseError(_MsgProtocolError):
"""
+class BadReplyError(_MsgProtocolError):
+"""
+An execution reply was successfully routed, but not understood.
+
+If a QMP message is received with an 'id' field to allow it to be
+routed, but is otherwise malformed, this exception will be raised.
+
+A reply message is malformed if it is missing either the 'return' or
+'error' keys, or if the 'error' value has missing keys or members of
+the wrong type.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The malformed reply that was received.
+:param sent: The message that was sent that prompted the error.
+"""
+def __init__(self, error_message: str, msg: Message, sent: Message):
+super().__init__(error_message, msg)
+#: The sent `Message` that caused the failure
+self.sent = sent
+
+
class QMPClient(AsyncProtocol[Message], Events):
"""
Implements a QMP client connection.
@@ -174,6 +215,9 @@ def __init__(self, name: Optional[str] = None) -> None:
# Cached Greeting, if one was awaited.
self._greeting: Optional[Greeting] = None
+# Command ID counter
+self._execute_id = 0
+
# Incoming RPC reply messages.
self._pending: Dict[
Union[str, None],
@@ -363,12 +407,135 @@ def _do_send(self, msg: Message) -> None:
assert self._writer is not None
self._writer.write(bytes(msg))
+@upper_half
+def _get_exec_id(self) -> str:
+exec_id = f"__aqmp#{self._execute_id:05d}"
+
[PATCH v2 17/24] python/aqmp: add QMP protocol support
The star of our show!
Add most of the QMP protocol, sans support for actually executing
commands. No problem, that happens in the next several commits.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 2 +
python/qemu/aqmp/qmp_client.py | 264 +
2 files changed, 266 insertions(+)
create mode 100644 python/qemu/aqmp/qmp_client.py
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 084eb5ab3e8..7bd66a48bed 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -25,11 +25,13 @@
from .events import EventListener
from .message import Message
from .protocol import ConnectError, Runstate
+from .qmp_client import QMPClient
# The order of these fields impact the Sphinx documentation order.
__all__ = (
# Classes, most to least important
+'QMPClient',
'Message',
'EventListener',
'Runstate',
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
new file mode 100644
index 000..000ff59c7a7
--- /dev/null
+++ b/python/qemu/aqmp/qmp_client.py
@@ -0,0 +1,264 @@
+"""
+QMP Protocol Implementation
+
+This module provides the `QMPClient` class, which can be used to connect
+and send commands to a QMP server such as QEMU. The QMP class can be
+used to either connect to a listening server, or used to listen and
+accept an incoming connection from that server.
+"""
+
+import logging
+from typing import (
+Dict,
+List,
+Mapping,
+Optional,
+)
+
+from .error import ProtocolError
+from .events import Events
+from .message import Message
+from .models import Greeting
+from .protocol import AsyncProtocol
+from .util import (
+bottom_half,
+exception_summary,
+pretty_traceback,
+upper_half,
+)
+
+
+class _WrappedProtocolError(ProtocolError):
+"""
+Abstract exception class for Protocol errors that wrap an Exception.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+def __init__(self, error_message: str, exc: Exception):
+super().__init__(error_message)
+self.exc = exc
+
+def __str__(self) -> str:
+return f"{self.error_message}: {self.exc!s}"
+
+
+class GreetingError(_WrappedProtocolError):
+"""
+An exception occurred during the Greeting phase.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+
+
+class NegotiationError(_WrappedProtocolError):
+"""
+An exception occurred during the Negotiation phase.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+
+
+class QMPClient(AsyncProtocol[Message], Events):
+"""
+Implements a QMP client connection.
+
+QMP can be used to establish a connection as either the transport
+client or server, though this class always acts as the QMP client.
+
+:param name: Optional nickname for the connection, used for logging.
+
+Basic script-style usage looks like this::
+
+ qmp = QMPClient('my_virtual_machine_name')
+ await qmp.connect(('127.0.0.1', 1234))
+ ...
+ res = await qmp.execute('block-query')
+ ...
+ await qmp.disconnect()
+
+Basic async client-style usage looks like this::
+
+ class Client:
+ def __init__(self, name: str):
+ self.qmp = QMPClient(name)
+
+ async def watch_events(self):
+ try:
+ async for event in self.qmp.events:
+ print(f"Event: {event['event']}")
+ except asyncio.CancelledError:
+ return
+
+ async def run(self, address='/tmp/qemu.socket'):
+ await self.qmp.connect(address)
+ asyncio.create_task(self.watch_events())
+ await self.qmp.runstate_changed.wait()
+ await self.disconnect()
+
+See `aqmp.events` for more detail on event handling patterns.
+"""
+#: Logger object used for debugging messages.
+logger = logging.getLogger(__name__)
+
+# Read buffer limit; large enough to accept query-qmp-schema
+_limit = (256 * 1024)
+
+def __init__(self, name: Optional[str] = None) -> None:
+super().__init__(name)
+Events.__init__(self)
+
+#: Whether or not to await a greeting after establishing a connection.
+self.await_greeting: bool = True
+
+#: Whether or not to perform capabilities negotiation upon connection.
+#: Implies `await_greeting`.
+self.negotiate: bool = True
+
+# Cached Greeting, if one was awaited.
+self._greeting: Optional[Greeting] = None
+
+@upper_half
+async def _establish_session(self) -> None:
+"""
+Initiate the QMP session.
+
+Wait for the QMP greeting and perform capabilities negotiation.
+
+:raise GreetingError: When the greeting is
[PATCH v2 19/24] python/aqmp: Add message routing to QMP protocol
Add the ability to handle and route messages in qmp_protocol.py. The
interface for actually sending anything still isn't added until next
commit.
Signed-off-by: John Snow
---
python/qemu/aqmp/qmp_client.py | 122 -
1 file changed, 120 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 000ff59c7a7..fa0cc7c5ae5 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -7,15 +7,19 @@
accept an incoming connection from that server.
"""
+# The import workarounds here are fixed in the next commit.
+import asyncio # pylint: disable=unused-import # noqa
import logging
from typing import (
Dict,
List,
Mapping,
Optional,
+Union,
+cast,
)
-from .error import ProtocolError
+from .error import AQMPError, ProtocolError
from .events import Events
from .message import Message
from .models import Greeting
@@ -61,6 +65,53 @@ class NegotiationError(_WrappedProtocolError):
"""
+class ExecInterruptedError(AQMPError):
+"""
+Exception raised when an RPC is interrupted.
+
+This error is raised when an execute() statement could not be
+completed. This can occur because the connection itself was
+terminated before a reply was received.
+
+The true cause of the interruption will be available via `disconnect()`.
+"""
+
+
+class _MsgProtocolError(ProtocolError):
+"""
+Abstract error class for protocol errors that have a `Message` object.
+
+This Exception class is used for protocol errors where the `Message`
+was mechanically understood, but was found to be inappropriate or
+malformed.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The QMP `Message` that caused the error.
+"""
+def __init__(self, error_message: str, msg: Message):
+super().__init__(error_message)
+#: The received `Message` that caused the error.
+self.msg: Message = msg
+
+def __str__(self) -> str:
+return "\n".join([
+super().__str__(),
+f" Message was: {str(self.msg)}\n",
+])
+
+
+class ServerParseError(_MsgProtocolError):
+"""
+The Server sent a `Message` indicating parsing failure.
+
+i.e. A reply has arrived from the server, but it is missing the "ID"
+field, indicating a parsing error.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The QMP `Message` that caused the error.
+"""
+
+
class QMPClient(AsyncProtocol[Message], Events):
"""
Implements a QMP client connection.
@@ -106,6 +157,9 @@ async def run(self, address='/tmp/qemu.socket'):
# Read buffer limit; large enough to accept query-qmp-schema
_limit = (256 * 1024)
+# Type alias for pending execute() result items
+_PendingT = Union[Message, ExecInterruptedError]
+
def __init__(self, name: Optional[str] = None) -> None:
super().__init__(name)
Events.__init__(self)
@@ -120,6 +174,12 @@ def __init__(self, name: Optional[str] = None) -> None:
# Cached Greeting, if one was awaited.
self._greeting: Optional[Greeting] = None
+# Incoming RPC reply messages.
+self._pending: Dict[
+Union[str, None],
+'asyncio.Queue[QMPClient._PendingT]'
+] = {}
+
@upper_half
async def _establish_session(self) -> None:
"""
@@ -132,6 +192,9 @@ async def _establish_session(self) -> None:
:raise EOFError: When the server unexpectedly hangs up.
:raise OSError: For underlying stream errors.
"""
+self._greeting = None
+self._pending = {}
+
if self.await_greeting or self.negotiate:
self._greeting = await self._get_greeting()
@@ -203,10 +266,33 @@ async def _negotiate(self) -> None:
self.logger.debug("%s:\n%s\n", emsg, pretty_traceback())
raise
+@bottom_half
+async def _bh_disconnect(self) -> None:
+try:
+await super()._bh_disconnect()
+finally:
+if self._pending:
+self.logger.debug("Cancelling pending executions")
+keys = self._pending.keys()
+for key in keys:
+self.logger.debug("Cancelling execution '%s'", key)
+self._pending[key].put_nowait(
+ExecInterruptedError("Disconnected")
+)
+
+self.logger.debug("QMP Disconnected.")
+
+@upper_half
+def _cleanup(self) -> None:
+super()._cleanup()
+assert not self._pending
+
@bottom_half
async def _on_message(self, msg: Message) -> None:
"""
Add an incoming message to the appropriate queue/handler.
+
+:raise ServerParseError: When Message indicates server parse failure.
"""
# Incoming messages are not fully parsed/validated here;
[PATCH v2 11/24] python/aqmp: add _cb_inbound and _cb_inbound logging hooks
Add hooks designed to log/filter incoming/outgoing messages. The primary
intent for these is to be able to support iotests which may want to log
messages with specific filters for reproducible output.
Another use is for plugging into Urwid frameworks; all messages in/out
can be automatically added to a rendering list for the purposes of a
qmp-shell like tool.
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 50 +---
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 86002a52654..6f83d3e3922 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -176,6 +176,11 @@ class AsyncProtocol(Generic[T]):
can be written after the super() call.
- `_on_message`:
Actions to be performed when a message is received.
+ - `_cb_outbound`:
+ Logging/Filtering hook for all outbound messages.
+ - `_cb_inbound`:
+ Logging/Filtering hook for all inbound messages.
+ This hook runs *before* `_on_message()`.
:param name:
Name used for logging messages, if any. By default, messages
@@ -732,6 +737,43 @@ async def _bh_recv_message(self) -> None:
# Section: Message I/O
#
+@upper_half
+@bottom_half
+def _cb_outbound(self, msg: T) -> T:
+"""
+Callback: outbound message hook.
+
+This is intended for subclasses to be able to add arbitrary
+hooks to filter or manipulate outgoing messages. The base
+implementation does nothing but log the message without any
+manipulation of the message.
+
+:param msg: raw outbound message
+:return: final outbound message
+"""
+self.logger.debug("--> %s", str(msg))
+return msg
+
+@upper_half
+@bottom_half
+def _cb_inbound(self, msg: T) -> T:
+"""
+Callback: inbound message hook.
+
+This is intended for subclasses to be able to add arbitrary
+hooks to filter or manipulate incoming messages. The base
+implementation does nothing but log the message without any
+manipulation of the message.
+
+This method does not "handle" incoming messages; it is a filter.
+The actual "endpoint" for incoming messages is `_on_message()`.
+
+:param msg: raw inbound message
+:return: processed inbound message
+"""
+self.logger.debug("<-- %s", str(msg))
+return msg
+
@upper_half
@bottom_half
async def _do_recv(self) -> T:
@@ -760,8 +802,8 @@ async def _recv(self) -> T:
:return: A single (filtered, processed) protocol message.
"""
-# A forthcoming commit makes this method less trivial.
-return await self._do_recv()
+message = await self._do_recv()
+return self._cb_inbound(message)
@upper_half
@bottom_half
@@ -791,7 +833,7 @@ async def _send(self, msg: T) -> None:
:raise OSError: For problems with the underlying stream.
"""
-# A forthcoming commit makes this method less trivial.
+msg = self._cb_outbound(msg)
self._do_send(msg)
@bottom_half
@@ -806,6 +848,6 @@ async def _on_message(self, msg: T) -> None:
directly cause the loop to halt, so logic may be best-kept
to a minimum if at all possible.
-:param msg: The incoming message
+:param msg: The incoming message, already logged/filtered.
"""
# Nothing to do in the abstract case.
--
2.31.1
[PATCH v2 24/24] python/aqmp: add AsyncProtocol unit tests
This tests most of protocol.py -- From a hacked up Coverage.py run, it's
at about 86%. There's a few error cases that aren't very well tested
yet, they're hard to induce artificially so far. I'm working on it.
Signed-off-by: John Snow
---
python/tests/null_proto.py | 67 ++
python/tests/protocol.py | 458 +
2 files changed, 525 insertions(+)
create mode 100644 python/tests/null_proto.py
create mode 100644 python/tests/protocol.py
diff --git a/python/tests/null_proto.py b/python/tests/null_proto.py
new file mode 100644
index 000..c697efc0001
--- /dev/null
+++ b/python/tests/null_proto.py
@@ -0,0 +1,67 @@
+import asyncio
+
+from qemu.aqmp.protocol import AsyncProtocol
+
+
+class NullProtocol(AsyncProtocol[None]):
+"""
+NullProtocol is a test mockup of an AsyncProtocol implementation.
+
+It adds a fake_session instance variable that enables a code path
+that bypasses the actual connection logic, but still allows the
+reader/writers to start.
+
+Because the message type is defined as None, an asyncio.Event named
+'trigger_input' is created that prohibits the reader from
+incessantly being able to yield None; this input can be poked to
+simulate an incoming message.
+
+For testing symmetry with do_recv, an interface is added to "send" a
+Null message.
+
+For testing purposes, a "simulate_disconnection" method is also
+added which allows us to trigger a bottom half disconnect without
+injecting any real errors into the reader/writer loops; in essence
+it performs exactly half of what disconnect() normally does.
+"""
+def __init__(self, name=None):
+self.fake_session = False
+self.trigger_input: asyncio.Event
+super().__init__(name)
+
+async def _establish_session(self):
+self.trigger_input = asyncio.Event()
+await super()._establish_session()
+
+async def _do_accept(self, address, ssl=None):
+if not self.fake_session:
+await super()._do_accept(address, ssl)
+
+async def _do_connect(self, address, ssl=None):
+if not self.fake_session:
+await super()._do_connect(address, ssl)
+
+async def _do_recv(self) -> None:
+await self.trigger_input.wait()
+self.trigger_input.clear()
+
+def _do_send(self, msg: None) -> None:
+pass
+
+async def send_msg(self) -> None:
+await self._outgoing.put(None)
+
+async def simulate_disconnect(self) -> None:
+# Simulates a bottom half disconnect, e.g. schedules a
+# disconnection but does not wait for it to complete. This is
+# used to put the loop into the DISCONNECTING state without
+# fully quiescing it back to IDLE; this is normally something
+# you cannot coax AsyncProtocol to do on purpose, but it will be
+# similar to what happens with an unhandled Exception in the
+# reader/writer.
+#
+# Under normal circumstances, the library design requires you to
+# await on disconnect(), which awaits the disconnect task and
+# returns bottom half errors as a pre-condition to allowing the
+# loop to return back to IDLE.
+self._schedule_disconnect()
diff --git a/python/tests/protocol.py b/python/tests/protocol.py
new file mode 100644
index 000..2374d01365e
--- /dev/null
+++ b/python/tests/protocol.py
@@ -0,0 +1,458 @@
+import asyncio
+from contextlib import contextmanager
+import os
+import socket
+from tempfile import TemporaryDirectory
+
+import avocado
+
+from qemu.aqmp import ConnectError, Runstate
+from qemu.aqmp.protocol import StateError
+from qemu.aqmp.util import asyncio_run, create_task
+
+# An Avocado bug prevents us from defining this testing class in-line here:
+from null_proto import NullProtocol
+
+
+def run_as_task(coro, allow_cancellation=False):
+# This helper runs a given coroutine as a task, wrapping it in a
+# try...except that allows it to be cancelled gracefully.
+async def _runner():
+try:
+await coro
+except asyncio.CancelledError:
+if allow_cancellation:
+return
+raise
+return create_task(_runner())
+
+
+@contextmanager
+def jammed_socket():
+# This method opens up a random TCP port on localhost, then jams it.
+socks = []
+
+try:
+sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+sock.bind(('127.0.0.1', 0))
+sock.listen(1)
+address = sock.getsockname()
+
+socks.append(sock)
+
+# I don't *fully* understand why, but it takes *two* un-accepted
+# connections to start jamming the socket.
+for _ in range(2):
+sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+sock.connect(address)
+socks.append(sock)
+
+yield address
+
+
[PATCH v2 14/24] python/aqmp: add well-known QMP object models
The QMP spec doesn't define very many objects that are iron-clad in
their format, but there are a few. This module makes it trivial to
validate them without relying on an external third-party library.
Signed-off-by: John Snow
---
python/qemu/aqmp/models.py | 133 +
1 file changed, 133 insertions(+)
create mode 100644 python/qemu/aqmp/models.py
diff --git a/python/qemu/aqmp/models.py b/python/qemu/aqmp/models.py
new file mode 100644
index 000..24c94123ac0
--- /dev/null
+++ b/python/qemu/aqmp/models.py
@@ -0,0 +1,133 @@
+"""
+QMP Data Models
+
+This module provides simplistic data classes that represent the few
+structures that the QMP spec mandates; they are used to verify incoming
+data to make sure it conforms to spec.
+"""
+# pylint: disable=too-few-public-methods
+
+from collections import abc
+from typing import (
+Any,
+Mapping,
+Optional,
+Sequence,
+)
+
+
+class Model:
+"""
+Abstract data model, representing some QMP object of some kind.
+
+:param raw: The raw object to be validated.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+self._raw = raw
+
+def _check_key(self, key: str) -> None:
+if key not in self._raw:
+raise KeyError(f"'{self._name}' object requires '{key}' member")
+
+def _check_value(self, key: str, type_: type, typestr: str) -> None:
+assert key in self._raw
+if not isinstance(self._raw[key], type_):
+raise TypeError(
+f"'{self._name}' member '{key}' must be a {typestr}"
+)
+
+def _check_member(self, key: str, type_: type, typestr: str) -> None:
+self._check_key(key)
+self._check_value(key, type_, typestr)
+
+@property
+def _name(self) -> str:
+return type(self).__name__
+
+def __repr__(self) -> str:
+return f"{self._name}({self._raw!r})"
+
+
+class Greeting(Model):
+"""
+Defined in qmp-spec.txt, section 2.2, "Server Greeting".
+
+:param raw: The raw Greeting object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'QMP' member
+self.QMP: QMPGreeting # pylint: disable=invalid-name
+
+self._check_member('QMP', abc.Mapping, "JSON object")
+self.QMP = QMPGreeting(self._raw['QMP'])
+
+
+class QMPGreeting(Model):
+"""
+Defined in qmp-spec.txt, section 2.2, "Server Greeting".
+
+:param raw: The raw QMPGreeting object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'version' member
+self.version: Mapping[str, object]
+#: 'capabilities' member
+self.capabilities: Sequence[object]
+
+self._check_member('version', abc.Mapping, "JSON object")
+self.version = self._raw['version']
+
+self._check_member('capabilities', abc.Sequence, "JSON array")
+self.capabilities = self._raw['capabilities']
+
+
+class ErrorResponse(Model):
+"""
+Defined in qmp-spec.txt, section 2.4.2, "error".
+
+:param raw: The raw ErrorResponse object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'error' member
+self.error: ErrorInfo
+#: 'id' member
+self.id: Optional[object] = None # pylint: disable=invalid-name
+
+self._check_member('error', abc.Mapping, "JSON object")
+self.error = ErrorInfo(self._raw['error'])
+
+if 'id' in raw:
+self.id = raw['id']
+
+
+class ErrorInfo(Model):
+"""
+Defined in qmp-spec.txt, section 2.4.2, "error".
+
+:param raw: The raw ErrorInfo object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'class' member, with an underscore to avoid conflicts in Python.
+self.class_: str
+#: 'desc' member
+self.desc: str
+
+self._check_member('class', str, "string")
+self.class_ = self._raw['class']
+
+self._check_member('desc', str, "string")
+self.desc = self._raw['desc']
--
2.31.1
[PATCH v2 10/24] python/aqmp: add configurable read buffer limit
QMP can transmit some pretty big messages, and the default limit of 64KB
isn't sufficient. Make sure that we can configure it.
Reported-by: G S Niteesh Babu
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 99b9614ba94..86002a52654 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -188,6 +188,9 @@ class AsyncProtocol(Generic[T]):
#: Logger object for debugging messages from this connection.
logger = logging.getLogger(__name__)
+# Maximum allowable size of read buffer
+_limit = (64 * 1024)
+
# -
# Section: Public interface
# -
@@ -453,6 +456,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
port=address[1],
ssl=ssl,
backlog=1,
+limit=self._limit,
)
else:
coro = asyncio.start_unix_server(
@@ -460,6 +464,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
path=address,
ssl=ssl,
backlog=1,
+limit=self._limit,
)
server = await coro # Starts listening
@@ -483,9 +488,18 @@ async def _do_connect(self, address: Union[str, Tuple[str,
int]],
self.logger.debug("Connecting to %s ...", address)
if isinstance(address, tuple):
-connect = asyncio.open_connection(address[0], address[1], ssl=ssl)
+connect = asyncio.open_connection(
+address[0],
+address[1],
+ssl=ssl,
+limit=self._limit,
+)
else:
-connect = asyncio.open_unix_connection(path=address, ssl=ssl)
+connect = asyncio.open_unix_connection(
+path=address,
+ssl=ssl,
+limit=self._limit,
+)
self._reader, self._writer = await connect
self.logger.debug("Connected.")
--
2.31.1
[PATCH v2 16/24] python/pylint: disable too-many-function-args
too-many-function-args seems prone to failure when considering things like Method Resolution Order, which mypy gets correct. When dealing with multiple inheritance, pylint doesn't seem to understand which method will actually get called, while mypy does. Remove the less powerful, redundant check. Signed-off-by: John Snow --- python/setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/setup.cfg b/python/setup.cfg index f87e32177ab..19d5e154630 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -88,7 +88,7 @@ ignore_missing_imports = True # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use "--disable=all --enable=classes # --disable=W". -disable= +disable=too-many-function-args, # mypy handles this with less false positives. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH v2 15/24] python/aqmp: add QMP event support
This class was designed as a "mix-in" primarily so that the feature
could be given its own treatment in its own python module.
It gets quite a bit too long otherwise.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 2 +
python/qemu/aqmp/events.py | 706 +++
2 files changed, 708 insertions(+)
create mode 100644 python/qemu/aqmp/events.py
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 035987c756c..084eb5ab3e8 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,6 +22,7 @@
# the COPYING file in the top-level directory.
from .error import AQMPError
+from .events import EventListener
from .message import Message
from .protocol import ConnectError, Runstate
@@ -30,6 +31,7 @@
__all__ = (
# Classes, most to least important
'Message',
+'EventListener',
'Runstate',
# Exceptions, most generic to most explicit
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
new file mode 100644
index 000..fb81d216102
--- /dev/null
+++ b/python/qemu/aqmp/events.py
@@ -0,0 +1,706 @@
+"""
+AQMP Events and EventListeners
+
+Asynchronous QMP uses `EventListener` objects to listen for events. An
+`EventListener` is a FIFO event queue that can be pre-filtered to listen
+for only specific events. Each `EventListener` instance receives its own
+copy of events that it hears, so events may be consumed without fear or
+worry for depriving other listeners of events they need to hear.
+
+
+EventListener Tutorial
+--
+
+In all of the following examples, we assume that we have a `QMPClient`
+instantiated named ``qmp`` that is already connected.
+
+
+`listener()` context blocks with one name
+~
+
+The most basic usage is by using the `listener()` context manager to
+construct them:
+
+.. code:: python
+
+ with qmp.listener('STOP') as listener:
+ await qmp.execute('stop')
+ await listener.get()
+
+The listener is active only for the duration of the ‘with’ block. This
+instance listens only for ‘STOP’ events.
+
+
+`listener()` context blocks with two or more names
+~~
+
+Multiple events can be selected for by providing any ``Iterable[str]``:
+
+.. code:: python
+
+ with qmp.listener(('STOP', 'RESUME')) as listener:
+ await qmp.execute('stop')
+ event = await listener.get()
+ assert event['event'] == 'STOP'
+
+ await qmp.execute('cont')
+ event = await listener.get()
+ assert event['event'] == 'RESUME'
+
+
+`listener()` context blocks with no names
+~
+
+By omitting names entirely, you can listen to ALL events.
+
+.. code:: python
+
+ with qmp.listener() as listener:
+ await qmp.execute('stop')
+ event = await listener.get()
+ assert event['event'] == 'STOP'
+
+This isn’t a very good use case for this feature: In a non-trivial
+running system, we may not know what event will arrive next. Grabbing
+the top of a FIFO queue returning multiple kinds of events may be prone
+to error.
+
+
+Using async iterators to retrieve events
+
+
+If you’d like to simply watch what events happen to arrive, you can use
+the listener as an async iterator:
+
+.. code:: python
+
+ with qmp.listener() as listener:
+ async for event in listener:
+ print(f"Event arrived: {event['event']}")
+
+This is analogous to the following code:
+
+.. code:: python
+
+ with qmp.listener() as listener:
+ while True:
+ event = listener.get()
+ print(f"Event arrived: {event['event']}")
+
+This event stream will never end, so these blocks will never terminate.
+
+
+Using asyncio.Task to concurrently retrieve events
+~~
+
+Since a listener’s event stream will never terminate, it is not likely
+useful to use that form in a script. For longer-running clients, we can
+create event handlers by using `asyncio.Task` to create concurrent
+coroutines:
+
+.. code:: python
+
+ async def print_events(listener):
+ try:
+ async for event in listener:
+ print(f"Event arrived: {event['event']}")
+ except asyncio.CancelledError:
+ return
+
+ with qmp.listener() as listener:
+ task = asyncio.Task(print_events(listener))
+ await qmp.execute('stop')
+ await qmp.execute('cont')
+ task.cancel()
+ await task
+
+However, there is no guarantee that these events will be received by the
+time we leave this context block. Once the context block is exited, the
+listener will cease to hear any new events, and becomes inert.
+
+Be mindful of the timing: the above example will *probably*– but does
+not *guarantee*– that both STOP/RESUMED events will be printed. The
+example below outlines how to use listeners outside of a
[PATCH v2 08/24] python/aqmp: add logging to AsyncProtocol
Give the connection and the reader/writer tasks nicknames, and add
logging statements throughout.
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 71
1 file changed, 64 insertions(+), 7 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 247b60c31a6..f9295546cda 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -14,6 +14,7 @@
from asyncio import StreamReader, StreamWriter
from enum import Enum
from functools import wraps
+import logging
from ssl import SSLContext
from typing import (
Any,
@@ -31,8 +32,10 @@
from .util import (
bottom_half,
create_task,
+exception_summary,
flush,
is_closing,
+pretty_traceback,
upper_half,
wait_closed,
)
@@ -173,14 +176,28 @@ class AsyncProtocol(Generic[T]):
can be written after the super() call.
- `_on_message`:
Actions to be performed when a message is received.
+
+:param name:
+Name used for logging messages, if any. By default, messages
+will log to 'qemu.aqmp.protocol', but each individual connection
+can be given its own logger by giving it a name; messages will
+then log to 'qemu.aqmp.protocol.${name}'.
"""
# pylint: disable=too-many-instance-attributes
+#: Logger object for debugging messages from this connection.
+logger = logging.getLogger(__name__)
+
# -
# Section: Public interface
# -
-def __init__(self) -> None:
+def __init__(self, name: Optional[str] = None) -> None:
+#: The nickname for this connection, if any.
+self.name: Optional[str] = name
+if self.name is not None:
+self.logger = self.logger.getChild(self.name)
+
# stream I/O
self._reader: Optional[StreamReader] = None
self._writer: Optional[StreamWriter] = None
@@ -207,6 +224,14 @@ def __init__(self) -> None:
self._runstate = Runstate.IDLE
self._runstate_changed: Optional[asyncio.Event] = None
+def __repr__(self) -> str:
+cls_name = type(self).__name__
+tokens = []
+if self.name is not None:
+tokens.append(f"name={self.name!r}")
+tokens.append(f"runstate={self.runstate.name}")
+return f"<{cls_name} {' '.join(tokens)}>"
+
@property # @upper_half
def runstate(self) -> Runstate:
"""The current `Runstate` of the connection."""
@@ -275,6 +300,8 @@ def _set_state(self, state: Runstate) -> None:
if state == self._runstate:
return
+self.logger.debug("Transitioning from '%s' to '%s'.",
+ str(self._runstate), str(state))
self._runstate = state
self._runstate_event.set()
self._runstate_event.clear()
@@ -314,8 +341,15 @@ async def _new_session(self,
except BaseException as err:
emsg = f"Failed to establish {phase}"
-# Reset from CONNECTING back to IDLE.
-await self.disconnect()
+self.logger.error("%s: %s", emsg, exception_summary(err))
+self.logger.debug("%s:\n%s\n", emsg, pretty_traceback())
+try:
+# Reset from CONNECTING back to IDLE.
+await self.disconnect()
+except:
+emsg = "Unexpected bottom half exception"
+self.logger.critical("%s:\n%s\n", emsg, pretty_traceback())
+raise
# NB: CancelledError is not a BaseException before Python 3.8
if isinstance(err, asyncio.CancelledError):
@@ -365,12 +399,16 @@ async def _do_connect(self, address: Union[str,
Tuple[str, int]],
:raise OSError: For stream-related errors.
"""
+self.logger.debug("Connecting to %s ...", address)
+
if isinstance(address, tuple):
connect = asyncio.open_connection(address[0], address[1], ssl=ssl)
else:
connect = asyncio.open_unix_connection(path=address, ssl=ssl)
self._reader, self._writer = await connect
+self.logger.debug("Connected.")
+
@upper_half
async def _establish_session(self) -> None:
"""
@@ -384,8 +422,8 @@ async def _establish_session(self) -> None:
self._outgoing = asyncio.Queue()
-reader_coro = self._bh_loop_forever(self._bh_recv_message)
-writer_coro = self._bh_loop_forever(self._bh_send_message)
+reader_coro = self._bh_loop_forever(self._bh_recv_message, 'Reader')
+writer_coro = self._bh_loop_forever(self._bh_send_message, 'Writer')
self._reader_task = create_task(reader_coro)
self._writer_task = create_task(writer_coro)
@@ -412,6 +450,7 @@ def _schedule_disconnect(self) -> None:
"""
if not self._dc_task:
self._set_state(Runstate.DISCONNECTING)
+
[PATCH v2 04/24] python/aqmp: add asyncio compatibility wrappers
Python 3.6 does not have all of the goodies that Python 3.7 does, and I
need to support both. Add some compatibility wrappers needed for this
purpose.
(Note: Python 3.6 is EOL December 2021.)
Signed-off-by: John Snow
---
python/qemu/aqmp/util.py | 106 +++
1 file changed, 106 insertions(+)
create mode 100644 python/qemu/aqmp/util.py
diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
new file mode 100644
index 000..7e7c2584d2b
--- /dev/null
+++ b/python/qemu/aqmp/util.py
@@ -0,0 +1,106 @@
+"""
+Miscellaneous Utilities
+
+This module provides asyncio utilities and compatibility wrappers for
+Python 3.6 to provide some features that otherwise become available in
+Python 3.7+.
+"""
+
+import asyncio
+import sys
+from typing import (
+Any,
+Coroutine,
+Optional,
+TypeVar,
+cast,
+)
+
+
+T = TypeVar('T')
+
+
+# --
+# Section: Utility Functions
+# --
+
+
+async def flush(writer: asyncio.StreamWriter) -> None:
+"""
+Utility function to ensure a StreamWriter is *fully* drained.
+
+`asyncio.StreamWriter.drain` only promises we will return to below
+the "high-water mark". This function ensures we flush the entire
+buffer -- by setting the high water mark to 0 and then calling
+drain. The flow control limits are restored after the call is
+completed.
+"""
+transport = cast(asyncio.WriteTransport, writer.transport)
+
+# https://github.com/python/typeshed/issues/5779
+low, high = transport.get_write_buffer_limits() # type: ignore
+transport.set_write_buffer_limits(0, 0)
+try:
+await writer.drain()
+finally:
+transport.set_write_buffer_limits(high, low)
+
+
+# ---
+# Section: Compatibility Wrappers
+# ---
+
+
+def create_task(coro: Coroutine[Any, Any, T],
+loop: Optional[asyncio.AbstractEventLoop] = None
+) -> 'asyncio.Future[T]':
+"""
+Python 3.6-compatible `asyncio.create_task` wrapper.
+
+:param coro: The coroutine to execute in a task.
+:param loop: Optionally, the loop to create the task in.
+
+:return: An `asyncio.Future` object.
+"""
+if sys.version_info >= (3, 7):
+if loop is not None:
+return loop.create_task(coro)
+return asyncio.create_task(coro) # pylint: disable=no-member
+
+# Python 3.6:
+return asyncio.ensure_future(coro, loop=loop)
+
+
+def is_closing(writer: asyncio.StreamWriter) -> bool:
+"""
+Python 3.6-compatible `asyncio.StreamWriter.is_closing` wrapper.
+
+:param writer: The `asyncio.StreamWriter` object.
+:return: `True` if the writer is closing, or closed.
+"""
+if sys.version_info >= (3, 7):
+return writer.is_closing()
+
+# Python 3.6:
+transport = writer.transport
+assert isinstance(transport, asyncio.WriteTransport)
+return transport.is_closing()
+
+
+async def wait_closed(writer: asyncio.StreamWriter) -> None:
+"""
+Python 3.6-compatible `asyncio.StreamWriter.wait_closed` wrapper.
+
+:param writer: The `asyncio.StreamWriter` to wait on.
+"""
+if sys.version_info >= (3, 7):
+await writer.wait_closed()
+return
+
+# Python 3.6
+transport = writer.transport
+assert isinstance(transport, asyncio.WriteTransport)
+
+while not transport.is_closing():
+await asyncio.sleep(0)
+await flush(writer)
--
2.31.1
[PATCH v2 13/24] python/aqmp: add QMP Message format
The Message class is here primarily to serve as a solid type to use for
mypy static typing for unambiguous annotation and documentation.
We can also stuff JSON serialization and deserialization into this class
itself so it can be re-used even outside this infrastructure.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 4 +-
python/qemu/aqmp/message.py | 209 +++
2 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 python/qemu/aqmp/message.py
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index ed65913c83e..035987c756c 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,12 +22,14 @@
# the COPYING file in the top-level directory.
from .error import AQMPError
+from .message import Message
from .protocol import ConnectError, Runstate
# The order of these fields impact the Sphinx documentation order.
__all__ = (
-# Classes
+# Classes, most to least important
+'Message',
'Runstate',
# Exceptions, most generic to most explicit
diff --git a/python/qemu/aqmp/message.py b/python/qemu/aqmp/message.py
new file mode 100644
index 000..f76ccc90746
--- /dev/null
+++ b/python/qemu/aqmp/message.py
@@ -0,0 +1,209 @@
+"""
+QMP Message Format
+
+This module provides the `Message` class, which represents a single QMP
+message sent to or from the server.
+"""
+
+import json
+from json import JSONDecodeError
+from typing import (
+Dict,
+Iterator,
+Mapping,
+MutableMapping,
+Optional,
+Union,
+)
+
+from .error import ProtocolError
+
+
+class Message(MutableMapping[str, object]):
+"""
+Represents a single QMP protocol message.
+
+QMP uses JSON objects as its basic communicative unit; so this
+Python object is a :py:obj:`~collections.abc.MutableMapping`. It may
+be instantiated from either another mapping (like a `dict`), or from
+raw `bytes` that still need to be deserialized.
+
+Once instantiated, it may be treated like any other MutableMapping::
+
+>>> msg = Message(b'{"hello": "world"}')
+>>> assert msg['hello'] == 'world'
+>>> msg['id'] = 'foobar'
+>>> print(msg)
+{
+ "hello": "world",
+ "id": "foobar"
+}
+
+It can be converted to `bytes`::
+
+>>> msg = Message({"hello": "world"})
+>>> print(bytes(msg))
+b'{"hello":"world","id":"foobar"}'
+
+Or back into a garden-variety `dict`::
+
+ >>> dict(msg)
+ {'hello': 'world'}
+
+
+:param value: Initial value, if any.
+:param eager:
+When `True`, attempt to serialize or deserialize the initial value
+immediately, so that conversion exceptions are raised during
+the call to ``__init__()``.
+"""
+# pylint: disable=too-many-ancestors
+
+def __init__(self,
+ value: Union[bytes, Mapping[str, object]] = b'{}', *,
+ eager: bool = True):
+self._data: Optional[bytes] = None
+self._obj: Optional[Dict[str, object]] = None
+
+if isinstance(value, bytes):
+self._data = value
+if eager:
+self._obj = self._deserialize(self._data)
+else:
+self._obj = dict(value)
+if eager:
+self._data = self._serialize(self._obj)
+
+# Methods necessary to implement the MutableMapping interface, see:
+#
https://docs.python.org/3/library/collections.abc.html#collections.abc.MutableMapping
+
+# We get pop, popitem, clear, update, setdefault, __contains__,
+# keys, items, values, get, __eq__ and __ne__ for free.
+
+def __getitem__(self, key: str) -> object:
+return self._object[key]
+
+def __setitem__(self, key: str, value: object) -> None:
+self._object[key] = value
+self._data = None
+
+def __delitem__(self, key: str) -> None:
+del self._object[key]
+self._data = None
+
+def __iter__(self) -> Iterator[str]:
+return iter(self._object)
+
+def __len__(self) -> int:
+return len(self._object)
+
+# Dunder methods not related to MutableMapping:
+
+def __repr__(self) -> str:
+if self._obj is not None:
+return f"Message({self._object!r})"
+return f"Message({bytes(self)!r})"
+
+def __str__(self) -> str:
+"""Pretty-printed representation of this QMP message."""
+return json.dumps(self._object, indent=2)
+
+def __bytes__(self) -> bytes:
+"""bytes representing this QMP message."""
+if self._data is None:
+self._data = self._serialize(self._obj or {})
+return self._data
+
+# Conversion Methods
+
+@property
+def _object(self) -> Dict[str, object]:
+"""
+A `dict` representing this QMP message.
+
+Generated on-demand, if required. This property is private
+because it returns an object that could
[PATCH v2 18/24] python/pylint: disable no-member check
mypy handles this better -- but we only need the workaround because pylint under Python 3.6 does not understand that a MutableMapping really does have a .get() method attached. We could remove this again once 3.7 is our minimum. Signed-off-by: John Snow --- python/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/python/setup.cfg b/python/setup.cfg index 19d5e154630..2573cd7bfb3 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -89,6 +89,7 @@ ignore_missing_imports = True # no Warning level messages displayed, use "--disable=all --enable=classes # --disable=W". disable=too-many-function-args, # mypy handles this with less false positives. +no-member, # mypy also handles this better. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH v2 05/24] python/aqmp: add generic async message-based protocol support
This is the bare minimum that you need to establish a full-duplex async
message-based protocol with Python's asyncio.
The features to be added in forthcoming commits are:
- Runstate tracking
- Logging
- Support for incoming connections via accept()
- _cb_outbound, _cb_inbound message hooks
- _readline() method
Some of the docstrings have dangling references, but they will resolve
themselves within the next few commits, and have been tested at the
conclusion of this series.
A note on the broad-except in _bh_disconnect:
I'm not sure if there's a more elegant solution here, but the problem is
that if an Exception occurred in the underlying
StreamReader/StreamWriter and causes one of the tasks to fail and
schedule a disconnect, the disconnect method itself will re-stumble
across the exact same Exception when attempting to close/flush the
stream.
Ignoring this Exception means we *might* miss a brand new Exception that
we didn't see already, but as we are in the process of tearing down the
connection anyway, I don't believe it matters. This stanza only really
cares that the writer is flushed and closed -- If it raises an
Exception, we know for sure it is not active and running.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 4 +-
python/qemu/aqmp/protocol.py | 508 +++
python/qemu/aqmp/util.py | 26 ++
3 files changed, 537 insertions(+), 1 deletion(-)
create mode 100644 python/qemu/aqmp/protocol.py
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index c97be950bf4..5c0de72672d 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,10 +22,12 @@
# the COPYING file in the top-level directory.
from .error import AQMPError
+from .protocol import ConnectError
# The order of these fields impact the Sphinx documentation order.
__all__ = (
-# Exceptions
+# Exceptions, most generic to most explicit
'AQMPError',
+'ConnectError',
)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
new file mode 100644
index 000..5413f4e50c1
--- /dev/null
+++ b/python/qemu/aqmp/protocol.py
@@ -0,0 +1,508 @@
+"""
+Generic Asynchronous Message-based Protocol Support
+
+This module provides a generic framework for sending and receiving
+messages over an asyncio stream. `AsyncProtocol` is an abstract class
+that implements the core mechanisms of a simple send/receive protocol,
+and is designed to be extended.
+
+In this package, it is used as the implementation for the `QMPClient`
+class.
+"""
+
+import asyncio
+from asyncio import StreamReader, StreamWriter
+from ssl import SSLContext
+# import exceptions will be removed in a forthcoming commit.
+# The problem stems from pylint/flake8 believing that 'Any'
+# is unused because of its only use in a string-quoted type.
+from typing import ( # pylint: disable=unused-import # noqa
+Any,
+Awaitable,
+Callable,
+Generic,
+Optional,
+Tuple,
+TypeVar,
+Union,
+)
+
+from .error import AQMPError
+from .util import (
+bottom_half,
+create_task,
+flush,
+is_closing,
+upper_half,
+wait_closed,
+)
+
+
+T = TypeVar('T')
+_TaskFN = Callable[[], Awaitable[None]] # aka ``async def func() -> None``
+_FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
+
+
+class ConnectError(AQMPError):
+"""
+Raised when the initial connection process has failed.
+
+This Exception always wraps a "root cause" exception that can be
+interrogated for additional information.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+def __init__(self, error_message: str, exc: Exception):
+super().__init__(error_message)
+#: Human-readable error string
+self.error_message: str = error_message
+#: Wrapped root cause exception
+self.exc: Exception = exc
+
+def __str__(self) -> str:
+return f"{self.error_message}: {self.exc!s}"
+
+
+class AsyncProtocol(Generic[T]):
+"""
+AsyncProtocol implements a generic async message-based protocol.
+
+This protocol assumes the basic unit of information transfer between
+client and server is a "message", the details of which are left up
+to the implementation. It assumes the sending and receiving of these
+messages is full-duplex and not necessarily correlated; i.e. it
+supports asynchronous inbound messages.
+
+It is designed to be extended by a specific protocol which provides
+the implementations for how to read and send messages. These must be
+defined in `_do_recv()` and `_do_send()`, respectively.
+
+Other callbacks have a default implementation, but are intended to be
+either extended or overridden:
+
+ - `_establish_session`:
+ The base implementation starts the reader/writer tasks.
+ A protocol implementation can override this call, inserting
+
[PATCH v2 03/24] python/pylint: Add exception for TypeVar names ('T')
'T' is a common TypeVar name, allow its use. See also https://github.com/PyCQA/pylint/issues/3401 -- In the future, we might be able to have a separate list of acceptable names for TypeVars exclusively. Signed-off-by: John Snow --- python/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/python/setup.cfg b/python/setup.cfg index ffb754fa9e5..f87e32177ab 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -101,6 +101,7 @@ good-names=i, fh, # fh = open(...) fd, # fd = os.open(...) c, # for c in string: ... + T, # for TypeVars. See pylint#3401 [pylint.similarities] # Ignore imports when computing similarities. -- 2.31.1
[PATCH v2 01/24] python/aqmp: add asynchronous QMP (AQMP) subpackage
For now, it's empty! Soon, it won't be. Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 27 +++ python/qemu/aqmp/py.typed| 0 python/setup.cfg | 1 + 3 files changed, 28 insertions(+) create mode 100644 python/qemu/aqmp/__init__.py create mode 100644 python/qemu/aqmp/py.typed diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py new file mode 100644 index 000..391141c9484 --- /dev/null +++ b/python/qemu/aqmp/__init__.py @@ -0,0 +1,27 @@ +""" +QEMU Monitor Protocol (QMP) development library & tooling. + +This package provides a fairly low-level class for communicating +asynchronously with QMP protocol servers, as implemented by QEMU, the +QEMU Guest Agent, and the QEMU Storage Daemon. + +`QMPClient` provides the main functionality of this package. All errors +raised by this library dervive from `AQMPError`, see `aqmp.error` for +additional detail. See `aqmp.events` for an in-depth tutorial on +managing QMP events. +""" + +# Copyright (C) 2020, 2021 John Snow for Red Hat, Inc. +# +# Authors: +# John Snow +# +# Based on earlier work by Luiz Capitulino . +# +# This work is licensed under the terms of the GNU GPL, version 2. See +# the COPYING file in the top-level directory. + + +# The order of these fields impact the Sphinx documentation order. +__all__ = ( +) diff --git a/python/qemu/aqmp/py.typed b/python/qemu/aqmp/py.typed new file mode 100644 index 000..e69de29bb2d diff --git a/python/setup.cfg b/python/setup.cfg index 14bab902883..ffb754fa9e5 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -27,6 +27,7 @@ packages = qemu.qmp qemu.machine qemu.utils +qemu.aqmp [options.package_data] * = py.typed -- 2.31.1
[PATCH v2 12/24] python/aqmp: add AsyncProtocol._readline() method
This is added as a courtesy: many protocols are line-based, including
QMP. Putting it in AsyncProtocol lets us keep the QMP class
implementation just a pinch more abstract.
(And, if we decide to add a QTEST implementation later, it will need
this, too. (Yes, I have a QTEST implementation.))
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 29 +
1 file changed, 29 insertions(+)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 6f83d3e3922..28cd5d9f4fd 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -774,6 +774,35 @@ def _cb_inbound(self, msg: T) -> T:
self.logger.debug("<-- %s", str(msg))
return msg
+@upper_half
+@bottom_half
+async def _readline(self) -> bytes:
+"""
+Wait for a newline from the incoming reader.
+
+This method is provided as a convenience for upper-layer
+protocols, as many are line-based.
+
+This method *may* return a sequence of bytes without a trailing
+newline if EOF occurs, but *some* bytes were received. In this
+case, the next call will raise `EOFError`. It is assumed that
+the layer 5 protocol will decide if there is anything meaningful
+to be done with a partial message.
+
+:raise OSError: For stream-related errors.
+:raise EOFError:
+If the reader stream is at EOF and there are no bytes to return.
+:return: bytes, including the newline.
+"""
+assert self._reader is not None
+msg_bytes = await self._reader.readline()
+
+if not msg_bytes:
+if self._reader.at_eof():
+raise EOFError
+
+return msg_bytes
+
@upper_half
@bottom_half
async def _do_recv(self) -> T:
--
2.31.1
[PATCH v2 07/24] python/aqmp: Add logging utility helpers
Signed-off-by: John Snow
---
python/qemu/aqmp/util.py | 56
1 file changed, 56 insertions(+)
diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
index 88abfc9eb22..70ef94ad600 100644
--- a/python/qemu/aqmp/util.py
+++ b/python/qemu/aqmp/util.py
@@ -4,10 +4,15 @@
This module provides asyncio utilities and compatibility wrappers for
Python 3.6 to provide some features that otherwise become available in
Python 3.7+.
+
+Various logging and debugging utilities are also provided, such as
+`exception_summary()` and `pretty_traceback()`, used primarily for
+adding information into the logging stream.
"""
import asyncio
import sys
+import traceback
from typing import (
Any,
Coroutine,
@@ -130,3 +135,54 @@ async def wait_closed(writer: asyncio.StreamWriter) ->
None:
while not transport.is_closing():
await asyncio.sleep(0)
await flush(writer)
+
+
+#
+# Section: Logging & Debugging
+#
+
+
+def exception_summary(exc: BaseException) -> str:
+"""
+Return a summary string of an arbitrary exception.
+
+It will be of the form "ExceptionType: Error Message", if the error
+string is non-empty, and just "ExceptionType" otherwise.
+"""
+name = type(exc).__qualname__
+smod = type(exc).__module__
+if smod not in ("__main__", "builtins"):
+name = smod + '.' + name
+
+error = str(exc)
+if error:
+return f"{name}: {error}"
+return name
+
+
+def pretty_traceback(prefix: str = " | ") -> str:
+"""
+Formats the current traceback, indented to provide visual distinction.
+
+This is useful for printing a traceback within a traceback for
+debugging purposes when encapsulating errors to deliver them up the
+stack; when those errors are printed, this helps provide a nice
+visual grouping to quickly identify the parts of the error that
+belong to the inner exception.
+
+:param prefix: The prefix to append to each line of the traceback.
+:return: A string, formatted something like the following::
+
+ | Traceback (most recent call last):
+ | File "foobar.py", line 42, in arbitrary_example
+ | foo.baz()
+ | ArbitraryError: [Errno 42] Something bad happened!
+"""
+output = "".join(traceback.format_exception(*sys.exc_info()))
+
+exc_lines = []
+for line in output.split('\n'):
+exc_lines.append(prefix + line)
+
+# The last line is always empty, omit it
+return "\n".join(exc_lines[:-1])
--
2.31.1
[PATCH v2 06/24] python/aqmp: add runstate state machine to AsyncProtocol
This serves a few purposes:
1. Protect interfaces when it's not safe to call them (via @require)
2. Add an interface by which an async client can determine if the state
has changed, for the purposes of connection management.
Signed-off-by: John Snow
---
python/qemu/aqmp/__init__.py | 5 +-
python/qemu/aqmp/protocol.py | 159 ++-
2 files changed, 159 insertions(+), 5 deletions(-)
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 5c0de72672d..ed65913c83e 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,11 +22,14 @@
# the COPYING file in the top-level directory.
from .error import AQMPError
-from .protocol import ConnectError
+from .protocol import ConnectError, Runstate
# The order of these fields impact the Sphinx documentation order.
__all__ = (
+# Classes
+'Runstate',
+
# Exceptions, most generic to most explicit
'AQMPError',
'ConnectError',
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 5413f4e50c1..247b60c31a6 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -12,11 +12,10 @@
import asyncio
from asyncio import StreamReader, StreamWriter
+from enum import Enum
+from functools import wraps
from ssl import SSLContext
-# import exceptions will be removed in a forthcoming commit.
-# The problem stems from pylint/flake8 believing that 'Any'
-# is unused because of its only use in a string-quoted type.
-from typing import ( # pylint: disable=unused-import # noqa
+from typing import (
Any,
Awaitable,
Callable,
@@ -25,6 +24,7 @@
Tuple,
TypeVar,
Union,
+cast,
)
from .error import AQMPError
@@ -43,6 +43,20 @@
_FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
+class Runstate(Enum):
+"""Protocol session runstate."""
+
+#: Fully quiesced and disconnected.
+IDLE = 0
+#: In the process of connecting or establishing a session.
+CONNECTING = 1
+#: Fully connected and active session.
+RUNNING = 2
+#: In the process of disconnecting.
+#: Runstate may be returned to `IDLE` by calling `disconnect()`.
+DISCONNECTING = 3
+
+
class ConnectError(AQMPError):
"""
Raised when the initial connection process has failed.
@@ -64,6 +78,76 @@ def __str__(self) -> str:
return f"{self.error_message}: {self.exc!s}"
+class StateError(AQMPError):
+"""
+An API command (connect, execute, etc) was issued at an inappropriate time.
+
+This error is raised when a command like
+:py:meth:`~AsyncProtocol.connect()` is issued at an inappropriate
+time.
+
+:param error_message: Human-readable string describing the state violation.
+:param state: The actual `Runstate` seen at the time of the violation.
+:param required: The `Runstate` required to process this command.
+"""
+def __init__(self, error_message: str,
+ state: Runstate, required: Runstate):
+super().__init__(error_message)
+self.error_message = error_message
+self.state = state
+self.required = required
+
+
+F = TypeVar('F', bound=Callable[..., Any]) # pylint: disable=invalid-name
+
+
+# Don't Panic.
+def require(required_state: Runstate) -> Callable[[F], F]:
+"""
+Decorator: protect a method so it can only be run in a certain `Runstate`.
+
+:param required_state: The `Runstate` required to invoke this method.
+:raise StateError: When the required `Runstate` is not met.
+"""
+def _decorator(func: F) -> F:
+# _decorator is the decorator that is built by calling the
+# require() decorator factory; e.g.:
+#
+# @require(Runstate.IDLE) def # foo(): ...
+# will replace 'foo' with the result of '_decorator(foo)'.
+
+@wraps(func)
+def _wrapper(proto: 'AsyncProtocol[Any]',
+ *args: Any, **kwargs: Any) -> Any:
+# _wrapper is the function that gets executed prior to the
+# decorated method.
+
+name = type(proto).__name__
+
+if proto.runstate != required_state:
+if proto.runstate == Runstate.CONNECTING:
+emsg = f"{name} is currently connecting."
+elif proto.runstate == Runstate.DISCONNECTING:
+emsg = (f"{name} is disconnecting."
+" Call disconnect() to return to IDLE state.")
+elif proto.runstate == Runstate.RUNNING:
+emsg = f"{name} is already connected and running."
+elif proto.runstate == Runstate.IDLE:
+emsg = f"{name} is disconnected and idle."
+else:
+assert False
+raise StateError(emsg, proto.runstate, required_state)
+# No StateError, so call the wrapped method.
+return func(proto, *args,
[PATCH v2 02/24] python/aqmp: add error classes
Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 4 +++ python/qemu/aqmp/error.py| 50 2 files changed, 54 insertions(+) create mode 100644 python/qemu/aqmp/error.py diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index 391141c9484..c97be950bf4 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -21,7 +21,11 @@ # This work is licensed under the terms of the GNU GPL, version 2. See # the COPYING file in the top-level directory. +from .error import AQMPError + # The order of these fields impact the Sphinx documentation order. __all__ = ( +# Exceptions +'AQMPError', ) diff --git a/python/qemu/aqmp/error.py b/python/qemu/aqmp/error.py new file mode 100644 index 000..5bdfdbfbda4 --- /dev/null +++ b/python/qemu/aqmp/error.py @@ -0,0 +1,50 @@ +""" +AQMP Error Classes + +This package seeks to provide semantic error classes that are intended +to be used directly by clients when they would like to handle particular +semantic failures (e.g. "failed to connect") without needing to know the +enumeration of possible reasons for that failure. + +AQMPError serves as the ancestor for all exceptions raised by this +package, and is suitable for use in handling semantic errors from this +library. In most cases, individual public methods will attempt to catch +and re-encapsulate various exceptions to provide a semantic +error-handling interface. + +.. admonition:: AQMP Exception Hierarchy Reference + + | `Exception` + |+-- `AQMPError` + | +-- `ConnectError` + | +-- `StateError` + | +-- `ExecInterruptedError` + | +-- `ExecuteError` + | +-- `ListenerError` + | +-- `ProtocolError` + | +-- `DeserializationError` + | +-- `UnexpectedTypeError` + | +-- `ServerParseError` + | +-- `BadReplyError` + | +-- `GreetingError` + | +-- `NegotiationError` +""" + + +class AQMPError(Exception): +"""Abstract error class for all errors originating from this package.""" + + +class ProtocolError(AQMPError): +""" +Abstract error class for protocol failures. + +Semantically, these errors are generally the fault of either the +protocol server or as a result of a bug in this this library. + +:param error_message: Human-readable string describing the error. +""" +def __init__(self, error_message: str): +super().__init__(error_message) +#: Human-readable error message, without any prefix. +self.error_message: str = error_message -- 2.31.1
[PATCH v2 09/24] python/aqmp: add AsyncProtocol.accept() method
It's a little messier than connect, because it wasn't designed to accept
*precisely one* connection. Such is life.
Signed-off-by: John Snow
---
python/qemu/aqmp/protocol.py | 89 ++--
1 file changed, 85 insertions(+), 4 deletions(-)
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index f9295546cda..99b9614ba94 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -245,6 +245,24 @@ async def runstate_changed(self) -> Runstate:
await self._runstate_event.wait()
return self.runstate
+@upper_half
+@require(Runstate.IDLE)
+async def accept(self, address: Union[str, Tuple[str, int]],
+ ssl: Optional[SSLContext] = None) -> None:
+"""
+Accept a connection and begin processing message queues.
+
+If this call fails, `runstate` is guaranteed to be set back to `IDLE`.
+
+:param address:
+Address to listen to; UNIX socket path or TCP address/port.
+:param ssl: SSL context to use, if any.
+
+:raise StateError: When the `Runstate` is not `IDLE`.
+:raise ConnectError: If a connection could not be accepted.
+"""
+await self._new_session(address, ssl, accept=True)
+
@upper_half
@require(Runstate.IDLE)
async def connect(self, address: Union[str, Tuple[str, int]],
@@ -309,7 +327,8 @@ def _set_state(self, state: Runstate) -> None:
@upper_half
async def _new_session(self,
address: Union[str, Tuple[str, int]],
- ssl: Optional[SSLContext] = None) -> None:
+ ssl: Optional[SSLContext] = None,
+ accept: bool = False) -> None:
"""
Establish a new connection and initialize the session.
@@ -318,9 +337,10 @@ async def _new_session(self,
to be set back to `IDLE`.
:param address:
-Address to connect to;
+Address to connect to/listen on;
UNIX socket path or TCP address/port.
:param ssl: SSL context to use, if any.
+:param accept: Accept a connection instead of connecting when `True`.
:raise ConnectError:
When a connection or session cannot be established.
@@ -334,7 +354,7 @@ async def _new_session(self,
try:
phase = "connection"
-await self._establish_connection(address, ssl)
+await self._establish_connection(address, ssl, accept)
phase = "session"
await self._establish_session()
@@ -368,6 +388,7 @@ async def _establish_connection(
self,
address: Union[str, Tuple[str, int]],
ssl: Optional[SSLContext] = None,
+accept: bool = False
) -> None:
"""
Establish a new connection.
@@ -376,6 +397,7 @@ async def _establish_connection(
Address to connect to/listen on;
UNIX socket path or TCP address/port.
:param ssl: SSL context to use, if any.
+:param accept: Accept a connection instead of connecting when `True`.
"""
assert self.runstate == Runstate.IDLE
self._set_state(Runstate.CONNECTING)
@@ -385,7 +407,66 @@ async def _establish_connection(
# otherwise yield.
await asyncio.sleep(0)
-await self._do_connect(address, ssl)
+if accept:
+await self._do_accept(address, ssl)
+else:
+await self._do_connect(address, ssl)
+
+@upper_half
+async def _do_accept(self, address: Union[str, Tuple[str, int]],
+ ssl: Optional[SSLContext] = None) -> None:
+"""
+Acting as the transport server, accept a single connection.
+
+:param address:
+Address to listen on; UNIX socket path or TCP address/port.
+:param ssl: SSL context to use, if any.
+
+:raise OSError: For stream-related errors.
+"""
+self.logger.debug("Awaiting connection on %s ...", address)
+connected = asyncio.Event()
+server: Optional[asyncio.AbstractServer] = None
+
+async def _client_connected_cb(reader: asyncio.StreamReader,
+ writer: asyncio.StreamWriter) -> None:
+"""Used to accept a single incoming connection, see below."""
+nonlocal server
+nonlocal connected
+
+# A connection has been accepted; stop listening for new ones.
+assert server is not None
+server.close()
+await server.wait_closed()
+server = None
+
+# Register this client as being connected
+self._reader, self._writer = (reader, writer)
+
+# Signal back: We've accepted a client!
+connected.set()
+
+if isinstance(address, tuple):
+coro = asyncio.start_server(
+
[PATCH v2 00/24] python: introduce Asynchronous QMP package
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-async-qmp-aqmp CI: https://gitlab.com/jsnow/qemu/-/pipelines/338508045 Docs: https://people.redhat.com/~jsnow/sphinx/html/qemu.aqmp.html Hi! This patch series adds an Asynchronous QMP package to the Python library. It offers a few improvements over the previous library: - out-of-band support - true asynchronous event support - avoids undocumented interfaces abusing non-blocking sockets - unit tests! - documentation! This library serves as the basis for a new qmp-shell program that will offer improved reconnection support, true asynchronous display of events, VM and job status update notifiers, and so on. My intent is to eventually publish this library directly to PyPI as a standalone package. I would like to phase out our usage of the old QMP library over time; eventually replacing it entirely with this one. This series looks big by line count, but it's *mostly* docstrings. Seriously! This package has *no* external dependencies whatsoever. Notes & Design == Here are some notes on the design of how the library works, to serve as a primer for review; however I also **highly recommend** browsing the generated Sphinx documentation for this series. Here's that link again: https://people.redhat.com/~jsnow/sphinx/html/qemu.aqmp.html The core machinery is split between the AsyncProtocol and QMP classes. AsyncProtocol provides the generic machinery, while QMP provides the QMP-specific details. The design uses two independent coroutines that act as the "bottom half", a writer task and a reader task. These tasks run for the duration of the connection and independently send and receive messages, respectively. A third task, disconnect, is scheduled asynchronously whenever an unrecoverable error occurs and facilitates coalescing of the other two tasks. This diagram for how execute() operates may be helpful for understanding how AsyncProtocol is laid out. The arrows indicate the direction of a QMP message; the long horizontal dash indicates the separation between the upper and lower half of the event loop. The queue mechanisms between both dashes serve as the intermediaries between the upper and lower half. +-+ | caller | +-+ ^ | | v +-+ +---> |execute()| ---+ | +-+| || [---] || |v ++--++++--+---+ | ExecQueue || EventListeners ||Outbound Queue| ++--+++---++--+---+ ^^ | || | [---] || | || v +--++---+ +---+---+ | Reader Task/Coroutine | | Writer Task/Coroutine | +---+---+ +---+---+ ^ | | v +-+--+ +-+--+ |StreamReader| |StreamWriter| ++ ++ The caller will invoke execute(), which in turn will deposit a message in the outbound send queue. This will wake up the writer task, which well send the message over the wire. The execute() method will then yield to wait for a reply delivered to an execution queue created solely for that execute statement. When a message arrives, the Reader task will unblock and route the message either to the EventListener subsystem, or place it in the appropriate pending execution queue. Once a message is placed in the pending execution queue, execute() will unblock and the execution will conclude, returning the result of the RPC call to the caller. Patch Layout Patches 1-4 add tiny pre-requisites, utilities, etc. Patches 5-12 add a generic async message-based protocol class, AsyncProtocol. They are split fairly small and should be reasonably self-contained. Patches 13-15 check in more QMP-centric components. Patches 16-21 add qmp_client.py, with a new 'QMPClient()' class. They're split into reasonably tiny pieces here. Patches 22-23 add a few finishing touches, they are small patches. Patch 24 adds unit tests. They're maybe a little messy yet, but they've been quite helpful to me so far. Coverage of protocol.py is at about 86%. Future Work === These items are in progress: - A synchronous QMP wrapper
[PATCH 1/2 v4] Configure script for Haiku
This refers to the email from a few weeks ago, regarding TPM & Haiku. It seems the assertion failure isn't really about the TPM, but about disabling PIE and adding -fPIC. There's discussion on the Haiku forum[1] about the incompatibility with PIE, and this fixes the assertion failure without altering the TPM configuration variable. [1] https://discuss.haiku-os.org/t/qemu-on-haiku-sdl-issue/10961/6?u=rjzak Previously, the TPM option was causing an assertion error at util/async.c:669 qemu_set_current_aio_context() !my_aiocontext. I suspect it was because the TPM option may have implied PIE. This patch ensures PIE doesn't get used, but -fPIC is used instead. diff --git a/configure b/configure index 650d9c0735..df834367b6 100755 --- a/configure +++ b/configure @@ -766,7 +766,8 @@ SunOS) ;; Haiku) haiku="yes" - QEMU_CFLAGS="-DB_USE_POSITIVE_POSIX_ERRORS -D_BSD_SOURCE $QEMU_CFLAGS" + pie="no" + QEMU_CFLAGS="-DB_USE_POSITIVE_POSIX_ERRORS -D_BSD_SOURCE -fPIC $QEMU_CFLAGS" ;; Linux) audio_drv_list="try-pa oss" -- Regards, Richard J. Zak Professional Genius PGP Key: https://keybase.io/rjzak/key.asc
Uninitialized variables err during dev
Hello all, I'm getting a strange error while doing some system target development. In the periphery of the qemu internals (things specifically other than the target), I'm getting uninitialized variable errors. I'm pretty sure I can't submit a patch to modify those internals, so I was wondering how or why such an error would crop up for me and what I should do about it? I don't see anywhere that configure or meson allow for build flags to be set. The specific error I get is: ../accel/tcg/cpu-exec.c: In function 'cpu_exec_step_atomic': tb-lookup..h:41:10: error: 'flags' may be used uninitialized in this function. In which case, I note that if I clear my changes that my build runs to completion just fine. I don't know what change I could have introduced that would cause this error. I also don't know why the compiler would get something so wrong either, because the address of the variable is used, not the variable itself.
RE: [PATCH 4/4] ui/gtk-egl: guest fb texture needs to be regenerated when reinitializing egl
>
> If guest fb is backed by dmabuf (blob-resource), the texture bound to the old
> context needs
> to be recreated in case the egl is re-initialized (e.g.
> new window for vc is created in case of detaching/reattaching of the tab)
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk-egl.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 32516b806c..5dd7c7e1f5 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -129,6 +129,10 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
> surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> surface_gl_create_texture(vc->gfx.gls, vc->gfx.ds);
> }
> +if (vc->gfx.guest_fb.dmabuf) {
> +vc->gfx.guest_fb.dmabuf->texture = 0;
[Kasireddy, Vivek] Shouldn't you call egl_dmabuf_release_texture() instead?
Thanks,
Vivek
> +gd_egl_scanout_dmabuf(dcl, vc->gfx.guest_fb.dmabuf);
> +}
> }
>
> graphic_hw_update(dcl->con);
> --
> 2.17.1
>
RE: [PATCH 2/3] ui/gtk-egl: make sure the right context is set as the current
Acknowledged-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon
> Subject: [PATCH 2/3] ui/gtk-egl: make sure the right context is set as the
> current
>
> Making the vc->gfx.ectx current before handling textures
> associated with it
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk-egl.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index 2a2e6d3a17..32516b806c 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -126,6 +126,7 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
> }
> vc->gfx.gls = qemu_gl_init_shader();
> if (vc->gfx.ds) {
> +surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> surface_gl_create_texture(vc->gfx.gls, vc->gfx.ds);
> }
> }
> @@ -152,6 +153,8 @@ void gd_egl_switch(DisplayChangeListener *dcl,
> surface_height(vc->gfx.ds) == surface_height(surface)) {
> resized = false;
> }
> +eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> + vc->gfx.esurface, vc->gfx.ectx);
>
> surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> vc->gfx.ds = surface;
> @@ -209,6 +212,11 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
> QemuDmaBuf *dmabuf)
> {
> #ifdef CONFIG_GBM
> +VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> +
> +eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> + vc->gfx.esurface, vc->gfx.ectx);
> +
> egl_dmabuf_import_texture(dmabuf);
> if (!dmabuf->texture) {
> return;
> --
> 2.17.1
>
Re: [PATCH for-6.2 19/34] target/arm: Move 'x' and 'a' bit definitions into vmlaldav formats
On 7/13/21 6:37 AM, Peter Maydell wrote: All the users of the vmlaldav formats have an 'x bit in bit 12 and an 'a' bit in bit 5; move these to the format rather than specifying them in each insn pattern. Signed-off-by: Peter Maydell --- Not sure why I didn't write it this way in the first place; when I came to implement VMLADAV I noticed the oddity and preferred to fix it rather than either copying it for VMLADAV or having VMLADAV different. --- target/arm/mve.decode | 16 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 18/34] target/arm: Implement MVE shift-by-scalar
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE instructions which perform shifts by a scalar. These are VSHL T2, VRSHL T2, VQSHL T1 and VQRSHL T2. They take the shift amount in a general purpose register and shift every element in the vector by that amount. Mostly we can reuse the helper functions for shift-by-immediate; we do need two new helpers for VQRSHL. Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 8 +++ target/arm/mve.decode | 23 --- target/arm/mve_helper.c| 2 ++ target/arm/translate-mve.c | 46 ++ 4 files changed, 76 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 17/34] target/arm: Implement MVE VMLAS
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE VMLAS insn, which multiplies a vector by a vector and adds a scalar. Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 8 target/arm/mve.decode | 3 +++ target/arm/mve_helper.c| 31 +++ target/arm/translate-mve.c | 2 ++ 4 files changed, 44 insertions(+) ... +/* Vector by vector plus scalar */ +#define DO_VMLAS(D, N, M) ((N) * (D) + (M)) + +DO_2OP_ACC_SCALAR_S(vmlass, DO_VMLAS) +DO_2OP_ACC_SCALAR_U(vmlasu, DO_VMLAS) This is confusing. The ARM says # Operations that do not perform # widening are always unsigned (encoded with U=1), This instruction does not perform widening, but it then codes on to enumerate the signed/unsigned encodings. I suppose you're matching what's written, so Reviewed-by: Richard Henderson r~
RE: [PATCH 1/3] ui/gtk-egl: un-tab and re-tab should destroy egl surface and context
Reviewed-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Romli, Khairul Anuar ; Kim, Dongwon
>
> Subject: [PATCH 1/3] ui/gtk-egl: un-tab and re-tab should destroy egl surface
> and context
>
> An old esurface should be destroyed and set to be NULL when doing
> un-tab and re-tab so that a new esurface an context can be created
> for the window widget that those will be bound to.
>
> Signed-off-by: Dongwon Kim
> Signed-off-by: Khairul Anuar Romli
> ---
> ui/gtk.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..bfb95f3b4b 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1232,6 +1232,14 @@ static gboolean gd_tab_window_close(GtkWidget *widget,
> GdkEvent *event,
> vc->tab_item, vc->label);
> gtk_widget_destroy(vc->window);
> vc->window = NULL;
> +if (vc->gfx.esurface) {
> +eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
> +vc->gfx.esurface = NULL;
> +}
> +if (vc->gfx.ectx) {
> +eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
> +vc->gfx.ectx = NULL;
> +}
> return TRUE;
> }
>
> @@ -1261,6 +1269,14 @@ static void gd_menu_untabify(GtkMenuItem *item, void
> *opaque)
> if (!vc->window) {
> gtk_widget_set_sensitive(vc->menu_item, false);
> vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +if (vc->gfx.esurface) {
> +eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
> +vc->gfx.esurface = NULL;
> +}
> +if (vc->gfx.esurface) {
> +eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
> +vc->gfx.ectx = NULL;
> +}
> gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
>
> g_signal_connect(vc->window, "delete-event",
> --
> 2.17.1
>
Re: [PATCH for-6.2 16/34] target/arm: Implement MVE VPSEL
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE VPSEL insn, which sets each byte of the destination vector Qd to the byte from either Qn or Qm depending on the value of the corresponding bit in VPR.P0. Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 2 ++ target/arm/mve.decode | 7 +-- target/arm/mve_helper.c| 19 +++ target/arm/translate-mve.c | 2 ++ 4 files changed, 28 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 15/34] target/arm: Implement MVE integer vector-vs-scalar comparisons
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE integer vector comparison instructions that compare each element against a scalar from a general purpose register. These are "VCMP (vector)" encodings T4, T5 and T6 and "VPT (vector)" encodings T4, T5 and T6. We have to move the decodetree pattern for VPST, because it overlaps with VCMP T4 with size = 0b11. Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 32 +++ target/arm/mve.decode | 18 +--- target/arm/mve_helper.c| 44 +++--- target/arm/translate-mve.c | 43 + 4 files changed, 126 insertions(+), 11 deletions(-) Reviewed-by: Richard Henderson r~
RE: [PATCH 3/3] ui/gtk: gd_draw_event returns FALSE when no cairo surface is bound
Reviewed-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon
> Subject: [PATCH 3/3] ui/gtk: gd_draw_event returns FALSE when no cairo
> surface is
> bound
>
> gd_draw_event shouldn't try to repaint if surface does not exist
> for the VC.
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index bfb95f3b4b..0a38deedc7 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -756,6 +756,9 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t
> *cr,
> void *opaque)
> if (!vc->gfx.ds) {
> return FALSE;
> }
> +if (!vc->gfx.surface) {
> +return FALSE;
> +}
>
> vc->gfx.dcl.update_interval =
> gd_monitor_update_interval(vc->window ? vc->window : s->window);
> --
> 2.17.1
>
Re: [PATCH for-6.2 14/34] target/arm: Implement MVE integer vector comparisons
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE integer vector comparison instructions. These are "VCMP (vector)" encodings T1, T2 and T3, and "VPT (vector)" encodings T1, T2 and T3. These insns compare corresponding elements in each vector, and update the VPR.P0 predicate bits with the results of the comparison. VPT also sets the VPR.MASK01 and VPR.MASK23 fields -- it is effectively "VCMP then VPST". Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 32 ++ target/arm/mve.decode | 18 +++- target/arm/mve_helper.c| 56 ++ target/arm/translate-mve.c | 47 4 files changed, 152 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v5 3/5] block/nbd: refactor nbd_recv_coroutines_wake_all()
On Wed, Jul 14, 2021 at 07:59:14PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Split out nbd_recv_coroutine_wake(), as it will be used in separate.
s/in separate/separately/
> Also add a possibility to wake only first found sleeping coroutine.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/nbd.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> +
> +static void nbd_recv_coroutines_wake_all(BDRVNBDState *s, bool only_one)
Without reading docs (including the parameter name), I would have guessed:
wake_all(s, true) - wakes all
wake_all(s, false) - wakes one
but your code does:
wake_all(s, true) - wakes one
wake_all(s, false) - wakes all
Maybe that means this function and/or its parameter is now misnamed.
Having the _all in the name with true to NOT be all is what threw me.
Would the following be any better:
nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)
where
wake(s, true) - wakes all
wake(s, false) - wakes one
and where your helper function needs to be renamed, and callers
updated to match those semantics?
> {
> int i;
>
> for (i = 0; i < MAX_NBD_REQUESTS; i++) {
> -NBDClientRequest *req = >requests[i];
> -
> -if (req->coroutine && req->receiving) {
> -req->receiving = false;
> -aio_co_wake(req->coroutine);
> +if (nbd_recv_coroutine_wake(>requests[i]) && only_one) {
> +return;
But while I'm not sold on the naming, the change in logic (to be able
to wake any one waiter but not the whole list) looks useful for future
patches.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH for-6.2 13/34] target/arm: Factor out gen_vpst()
On 7/13/21 6:37 AM, Peter Maydell wrote: Factor out the "generate code to update VPR.MASK01/MASK23" part of trans_VPST(); we are going to want to reuse it for the VPT insns. Signed-off-by: Peter Maydell --- target/arm/translate-mve.c | 31 +-- 1 file changed, 17 insertions(+), 14 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH V5 17/25] vfio-pci: cpr part 2
On Wed, 7 Jul 2021 10:20:26 -0700
Steve Sistare wrote:
> Finish cpr for vfio-pci by preserving eventfd's and vector state.
>
> Signed-off-by: Steve Sistare
> ---
> hw/vfio/pci.c | 118
> +-
> 1 file changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 0f5c542..07bd360 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
...
> @@ -3295,14 +3329,91 @@ static void vfio_merge_config(VFIOPCIDevice
*vdev)
> g_free(phys_config);
> }
>
> +static int vfio_pci_pre_save(void *opaque)
> +{
> +VFIOPCIDevice *vdev = opaque;
> +PCIDevice *pdev = >pdev;
> +int i;
> +
> +if (vfio_pci_read_config(pdev, PCI_INTERRUPT_PIN, 1)) {
> +error_report("%s: cpr does not support vfio-pci INTX",
> + vdev->vbasedev.name);
> +}
You're not only not supporting INTx, but devices that support INTx, so
this only works on VFs. Why? Is this just out of scope or is there
something fundamentally difficult about it?
This makes me suspect there's a gap in INTx routing setup if it's more
than just another eventfd to store and setup. If we hot-add a device
using INTx after cpr restart, are we going to find problems? Thanks,
Alex
Re: [PATCH v5 2/5] block/nbd: move nbd_recv_coroutines_wake_all() up
On Wed, Jul 14, 2021 at 07:59:13PM +0300, Vladimir Sementsov-Ogievskiy wrote: > We are going to use it in nbd_channel_error(), so move it up. Note, > that we are going also refactor and rename > nbd_recv_coroutines_wake_all() in future anyway, so keeping it where it > is and making forward declaration doesn't make real sense. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/nbd.c | 28 ++-- > 1 file changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2] docs: document file-posix locking protocol
16.07.2021 21:47, Vladimir Sementsov-Ogievskiy wrote: 16.07.2021 19:21, Vladimir Sementsov-Ogievskiy wrote: 15.07.2021 23:00, Vladimir Sementsov-Ogievskiy wrote: 03.07.2021 16:50, Vladimir Sementsov-Ogievskiy wrote: +Permission bytes. If permission byte is rd-locked, it means that some process +uses corresponding permission on that file. + +Byte Operation +100 read + Lock holder can read +101 write + Lock holder can write +102 write-unchanged + Lock holder can write same data if it sure, that this write doesn't + break concurrent readers. This is mostly used internally in Qemu + and it wouldn't be good idea to exploit it somehow. Let's make it more strict: New software should never lock this byte and interpret this byte locked by other process like write permission (same as 101). [*] +103 resize + Lock holder can resize the file. "write" permission is also required + for resizing, so lock byte 103 only if you also lock byte 101. +104 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Unshare bytes. If permission byte is rd-locked, it means that some process +does not allow the others use corresponding options on that file. + +Byte Operation +200 read + Lock holder don't allow read operation to other processes. +201 write + Lock holder don't allow write operation to other processes. This + still allows others to do write-uncahnged operations. Better not + exploit outside of Qemu. +202 write-unchanged + Lock holder don't allow write-unchanged operation to other processes. And here, correspondingly: New software should never lock this byte and interpret this byte locked by other process like write permission unshared (same as 201). Hmm, no. For [*] work correctly, new software should always lock 202 if it locks 201. +203 resize + Lock holder don't allow resizing the file by other processes. +204 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Handling the permissions works as follows: assume we want to open the file to do +some operations and in the same time want to disallow some operation to other +processes. So, we want to lock some of the bytes described above. We operate as +follows: + More on this. Hmm, we can just drop graph-mod. I don't think it is needed now, and anyway no sense in locking it in file-posix. write-unchanged is rather strange here. One process may read data and write it back and consider it WRITE_UNCHANGED, but other process may change the file intermediatly, and for it the WRITE_UNCHANGED of first process is not "unchanged".. It probably good to drop write-unchanged from the protocol at all. But what if we have old qemu that can lock write-unchanged (byte 102) and not write (byte 101)? The audit do we really have such a case now or in some previous version is rather hard to do.. So, if we want to support it in the most compatible way, all new software should lock both 201 and 202 to unshare write. Locking 102 in pair with 101 doesn't make real sense: if we has write access, we don't need additional write-unchanged anyway. Still, we may document that these bytes should be locked together just for consistency with "unshare" bytes. Any thoughts? Probably it worth also document that 203 is recommended to be locked if application want to lock 201, to avoid bad behaving process, that locks 103 and not 101 and thinks that it has permission to resize. And one more question to consider. What about read? Actually what's the sense of locking byte 200? What's wrong for us if some other process access the file in RO mode? In Qemu this permissin is called CONSISTENT_READ.. Who knows what it means? How much consistent may be read by process A, when process B has write permission and do write at the moment? Hmm, looking at BLK_PERM_CONSISTENT_READ description, I see that it mostly about intermediate nodes of commit block job. If we read from intermediate node of commite block job we may read old data (original view of the node) or new data (that was recently committed and actually unrelated to this intermediate node that we are going to remove). But I doubt that it make sense for interprocess syncrhonisation: if want to read consistantly, we must unshare write on the whole backing chain.. And no real sense in sharing READ.. Finally, it seems to me that only the following scenarios make sense: 1. read-only access, don't care about existing writing Qemu process: just open RO, don't care about locks (like qemu-img --force-share) 2. RO or RW access, want consistency: unshare all operations, lock bytes 20*. Than no real sense in locking any of 10*.. But for
Re: [PATCH v5 1/5] block/nbd: nbd_channel_error() shutdown channel unconditionally
On Wed, Jul 14, 2021 at 07:59:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Don't rely on connection being totally broken in case of -EIO. More
> safe and correct just shutdown the channel anyway, as we change the
> state and going to reconnect.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/nbd.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake
>
> diff --git a/block/nbd.c b/block/nbd.c
> index f6ff1c4fb4..d88f4b954c 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -129,15 +129,16 @@ static bool nbd_client_connected(BDRVNBDState *s)
>
> static void nbd_channel_error(BDRVNBDState *s, int ret)
> {
> +if (nbd_client_connected(s)) {
> +qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +
> if (ret == -EIO) {
> if (nbd_client_connected(s)) {
> s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
> NBD_CLIENT_CONNECTING_NOWAIT;
> }
> } else {
> -if (nbd_client_connected(s)) {
> -qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> -}
> s->state = NBD_CLIENT_QUIT;
> }
> }
> --
> 2.29.2
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Bug 1889621] Re: ARM Highbank Crashes Realted to GIC
Ok, thanks, then let's close this (and open new tickets on gitlab if it happens again) ** Changed in: qemu Status: Incomplete => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1889621 Title: ARM Highbank Crashes Realted to GIC Status in QEMU: Fix Released Bug description: Hello, Here are some QTest reproducers for crashes on ARM Highbank that all seem to be related to the gic device. Reproducer 1: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writel 0xfff11f00 0x8405f559 writel 0xfff117fd 0x5c057bd8 EOF ==10595==ERROR: AddressSanitizer: SEGV on unknown address 0x62b13e01 (pc 0x55b6ab85cc91 bp 0x7fff60bd4d70 sp 0x7fff60bd4ce0 T0) ==10595==The signal is caused by a READ memory access. #0 0x55b6ab85cc91 in gic_get_current_cpu /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:60:12 #1 0x55b6ab85e1bd in gic_dist_writeb /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1182:11 #2 0x55b6ab855a97 in gic_dist_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1514:9 #3 0x55b6aa1650d4 in memory_region_write_with_attrs_accessor /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12 #4 0x55b6aa163ac6 in access_with_adjusted_size /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18 #5 0x55b6aa161f35 in memory_region_dispatch_write /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13 #6 0x55b6a9313949 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23 #7 0x55b6a92fca11 in flatview_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14 #8 0x55b6a92fc54e in address_space_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18 = Reproducer 2: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writeq 0xfff11f00 0x613a650f0fda6555 EOF ==1375==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60801c80 at pc 0x5618928c486e bp 0x7ffe22c4ee10 sp 0x7ffe22c4ee08 READ of size 8 at 0x60801c80 thread T0 #0 0x5618928c486d in address_space_translate_iommu /home/alxndr/Development/qemu/general-fuzz/exec.c:451:23 #1 0x561892850acc in flatview_do_translate /home/alxndr/Development/qemu/general-fuzz/exec.c:524:16 #2 0x5618928514ad in flatview_translate /home/alxndr/Development/qemu/general-fuzz/exec.c:584:15 #3 0x5618928b1e14 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3199:14 #4 0x56189289aa11 in flatview_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14 #5 0x56189289a54e in address_space_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18 #6 0x5618937a5e13 in qtest_process_command /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:452:13 #7 0x56189379d89f in qtest_process_inbuf /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:710:9 #8 0x56189379c680 in qtest_read /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:722:5 = Reproducer 3: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writeq 0xfff11000 0x70b writeq 0xfff11f00 0x4f4f4fff54a7afaf writel 0xfff10100 0x61ff EOF ==23743==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b06a92 at pc 0x55d690d980e1 bp 0x7ffe606082d0 sp 0x7ffe606082c8 READ of size 1 at 0x62b06a92 thread T0 #0 0x55d690d980e0 in gic_get_best_irq /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:94:13 #1 0x55d690d9485b in gic_update_internal /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:185:13 #2 0x55d690d90376 in gic_update /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:226:5 #3 0x55d690dc0879 in gic_cpu_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1758:9 #4 0x55d690da41c0 in gic_thiscpu_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1777:12 #5 0x55d68f6b30d4 in memory_region_write_with_attrs_accessor /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12 #6 0x55d68f6b1ac6 in access_with_adjusted_size /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18 #7 0x55d68f6aff35 in memory_region_dispatch_write /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13 #8 0x55d68e861949 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23 #9 0x55d68e84aa11 in flatview_write
Re: QEMU System and User targets
After checking around, I don't see any _user_ss in any target directory. And I only see *_user_ss in the linux-user subdirectory. Were you talking about that meson.build in linux-user? On Fri, Jul 16, 2021 at 1:20 PM Kenneth Adam Miller < kennethadammil...@gmail.com> wrote: > Right, that's what I was thinking, that I shouldn't be building that for > the system target. That's why I started out with the question that I did, > because I was thinking that it probably hard codes it to user emulation. > Currently though, understanding qemu internals is not so clear to me as I'm > just becoming familiar with the code base. > > On Fri, Jul 16, 2021 at 1:05 PM Peter Maydell > wrote: > >> On Fri, 16 Jul 2021 at 18:50, Kenneth Adam Miller >> wrote: >> > There's a lot of files and I don't want to muddy up the discussion with >> too many details. >> >> If you don't provide details, you get vague answers. Your choice :-) >> >> > And for sure, this is not a problem with the upstream qemu. I'm working >> on adding a target, and this is just what I'm experiencing. As for my >> target, it has includes that correspond to finds within sub-directories of >> qemu components, and I just mean that the include directives are only the >> file name (no path prefix), but such file can be found only in folders >> other than the include directory. One example is qemu.h; it is in >> linux-user. You can get the compilation to find exactly just that file, but >> it includes other files, and it isn't reasonable to edit anything outside >> of my own architecture implementation. I'm wondering if perhaps anything >> that makes an include to linux-user would need to be moved into the user >> target source set, because currently it is in the shared. >> >> The broad-strokes answer is "your code in target/whatever should generally >> not be including files that are neither in include/ nor in >> target/whatever". >> If you find yourself doing that you've probably structured something wrong >> (otherwise other targets would also have run into this). >> >> For linux-user/qemu.h in particular, the top level meson.build does >> add linux-user/ to the include path, but only for when it is building >> files for the linux-user targets. (It makes no sense to include that >> header >> file into code built for system emulation.) >> >> Of the 4 targets that #include "qemu.h" in target/whatever code, 3 of them >> (m68k, nios2, arm) do it only for their semihosting .c file, and there >> the #include "qemu.h" is inside an #ifdef CONFIG_USER_ONLY. (Semihosting >> is a bit of an odd thing which works differently for usermode and >> system emulation mode, which is why it needs this linux-user header.) >> The 4th is hexagon, and that is a bug in the hexagon code which is only >> going unnoticed because hexagon currently supports only the linux-user >> target and not system emulation. >> >> thanks >> -- PMM >> >
Re: [PATCH for-6.2 12/34] target/arm: Implement MVE incrementing/decrementing dup insns
On 7/13/21 6:37 AM, Peter Maydell wrote:
+#define DO_VIDUP(OP, ESIZE, TYPE, FN) \
+uint32_t HELPER(mve_##OP)(CPUARMState *env, void *vd, \
+ uint32_t offset, uint32_t imm) \
+{ \
+TYPE *d = vd; \
+uint16_t mask = mve_element_mask(env); \
+unsigned e; \
+for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \
+mergemask([H##ESIZE(e)], offset, mask); \
+offset = FN(offset, imm); \
+} \
+mve_advance_vpt(env); \
+return offset; \
+}
+
+#define DO_VIWDUP(OP, ESIZE, TYPE, FN) \
+uint32_t HELPER(mve_##OP)(CPUARMState *env, void *vd, \
+ uint32_t offset, uint32_t wrap, \
+ uint32_t imm) \
+{ \
+TYPE *d = vd; \
+uint16_t mask = mve_element_mask(env); \
+unsigned e; \
+for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \
+mergemask([H##ESIZE(e)], offset, mask); \
+offset = FN(offset, wrap, imm); \
+} \
+mve_advance_vpt(env); \
+return offset; \
+}
+
+#define DO_VIDUP_ALL(OP, FN)\
+DO_VIDUP(OP##b, 1, int8_t, FN) \
+DO_VIDUP(OP##h, 2, int16_t, FN) \
+DO_VIDUP(OP##w, 4, int32_t, FN)
+
+#define DO_VIWDUP_ALL(OP, FN) \
+DO_VIWDUP(OP##b, 1, int8_t, FN) \
+DO_VIWDUP(OP##h, 2, int16_t, FN)\
+DO_VIWDUP(OP##w, 4, int32_t, FN)
Would it be useful to merge VIDUP and VIWDUP by passing wrap == 0?
Or merging VIDUP and VDDUP by negating imm?
Anyway,
Reviewed-by: Richard Henderson
r~
Re: [PATCH v6 2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation
On Fri, Jul 16, 2021 at 07:13:47PM +0800, huang...@chinatelecom.cn wrote:
> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> +{
> +int64_t msec = 0;
> +int64_t start_time;
> +DirtyPageRecord dirty_pages;
[1]
> +
> +dirtyrate_global_dirty_log_start();
> +
> +/*
> + * 1'round of log sync may return all 1 bits with
> + * KVM_DIRTY_LOG_INITIALLY_SET enable
> + * skip it unconditionally and start dirty tracking
> + * from 2'round of log sync
> + */
> +dirtyrate_global_dirty_log_sync();
> +
> +/*
> + * reset page protect manually and unconditionally.
> + * this make sure kvm dirty log be cleared if
> + * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
> + */
> +dirtyrate_manual_reset_protect();
> +
[2]
> +record_dirtypages_bitmap(_pages, true);
[3]
> +
> +start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +DirtyStat.start_time = start_time / 1000;
> +
> +msec = config.sample_period_seconds * 1000;
> +msec = set_sample_page_period(msec, start_time);
> +DirtyStat.calc_time = msec / 1000;
> +
> +/* fetch dirty bitmap from kvm and stop dirty tracking */
I don't think it really fetched anything.. So I think we need:
dirtyrate_global_dirty_log_sync();
It seems to be there in older versions but not in the latest two versions.
Please still remember to smoke the patches before posting, because without the
log sync we'll read nothing.
> +dirtyrate_global_dirty_log_stop();
> +
> +record_dirtypages_bitmap(_pages, false);
[4]
I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
taking it for every function. Then we can move the bql operations out of
dirtyrate_global_dirty_log_stop() in this patch.
Thanks,
> +
> +do_calculate_dirtyrate_bitmap(dirty_pages);
> +}
--
Peter Xu
Re: [PATCH v2] docs: document file-posix locking protocol
16.07.2021 19:21, Vladimir Sementsov-Ogievskiy wrote: 15.07.2021 23:00, Vladimir Sementsov-Ogievskiy wrote: 03.07.2021 16:50, Vladimir Sementsov-Ogievskiy wrote: +Permission bytes. If permission byte is rd-locked, it means that some process +uses corresponding permission on that file. + +Byte Operation +100 read + Lock holder can read +101 write + Lock holder can write +102 write-unchanged + Lock holder can write same data if it sure, that this write doesn't + break concurrent readers. This is mostly used internally in Qemu + and it wouldn't be good idea to exploit it somehow. Let's make it more strict: New software should never lock this byte and interpret this byte locked by other process like write permission (same as 101). [*] +103 resize + Lock holder can resize the file. "write" permission is also required + for resizing, so lock byte 103 only if you also lock byte 101. +104 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Unshare bytes. If permission byte is rd-locked, it means that some process +does not allow the others use corresponding options on that file. + +Byte Operation +200 read + Lock holder don't allow read operation to other processes. +201 write + Lock holder don't allow write operation to other processes. This + still allows others to do write-uncahnged operations. Better not + exploit outside of Qemu. +202 write-unchanged + Lock holder don't allow write-unchanged operation to other processes. And here, correspondingly: New software should never lock this byte and interpret this byte locked by other process like write permission unshared (same as 201). Hmm, no. For [*] work correctly, new software should always lock 202 if it locks 201. +203 resize + Lock holder don't allow resizing the file by other processes. +204 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Handling the permissions works as follows: assume we want to open the file to do +some operations and in the same time want to disallow some operation to other +processes. So, we want to lock some of the bytes described above. We operate as +follows: + More on this. Hmm, we can just drop graph-mod. I don't think it is needed now, and anyway no sense in locking it in file-posix. write-unchanged is rather strange here. One process may read data and write it back and consider it WRITE_UNCHANGED, but other process may change the file intermediatly, and for it the WRITE_UNCHANGED of first process is not "unchanged".. It probably good to drop write-unchanged from the protocol at all. But what if we have old qemu that can lock write-unchanged (byte 102) and not write (byte 101)? The audit do we really have such a case now or in some previous version is rather hard to do.. So, if we want to support it in the most compatible way, all new software should lock both 201 and 202 to unshare write. Locking 102 in pair with 101 doesn't make real sense: if we has write access, we don't need additional write-unchanged anyway. Still, we may document that these bytes should be locked together just for consistency with "unshare" bytes. Any thoughts? Probably it worth also document that 203 is recommended to be locked if application want to lock 201, to avoid bad behaving process, that locks 103 and not 101 and thinks that it has permission to resize. And one more question to consider. What about read? Actually what's the sense of locking byte 200? What's wrong for us if some other process access the file in RO mode? In Qemu this permissin is called CONSISTENT_READ.. Who knows what it means? How much consistent may be read by process A, when process B has write permission and do write at the moment? Hmm, looking at BLK_PERM_CONSISTENT_READ description, I see that it mostly about intermediate nodes of commit block job. If we read from intermediate node of commite block job we may read old data (original view of the node) or new data (that was recently committed and actually unrelated to this intermediate node that we are going to remove). But I doubt that it make sense for interprocess syncrhonisation: if want to read consistantly, we must unshare write on the whole backing chain.. And no real sense in sharing READ.. Finally, it seems to me that only the following scenarios make sense: 1. read-only access, don't care about existing writing Qemu process: just open RO, don't care about locks (like qemu-img --force-share) 2. RO or RW access, want consistency: unshare all operations, lock bytes 20*. Than no real sense in locking any of 10*.. But for consistency and safety better to lock them all. --
Re: QEMU System and User targets
Right, that's what I was thinking, that I shouldn't be building that for the system target. That's why I started out with the question that I did, because I was thinking that it probably hard codes it to user emulation. Currently though, understanding qemu internals is not so clear to me as I'm just becoming familiar with the code base. On Fri, Jul 16, 2021 at 1:05 PM Peter Maydell wrote: > On Fri, 16 Jul 2021 at 18:50, Kenneth Adam Miller > wrote: > > There's a lot of files and I don't want to muddy up the discussion with > too many details. > > If you don't provide details, you get vague answers. Your choice :-) > > > And for sure, this is not a problem with the upstream qemu. I'm working > on adding a target, and this is just what I'm experiencing. As for my > target, it has includes that correspond to finds within sub-directories of > qemu components, and I just mean that the include directives are only the > file name (no path prefix), but such file can be found only in folders > other than the include directory. One example is qemu.h; it is in > linux-user. You can get the compilation to find exactly just that file, but > it includes other files, and it isn't reasonable to edit anything outside > of my own architecture implementation. I'm wondering if perhaps anything > that makes an include to linux-user would need to be moved into the user > target source set, because currently it is in the shared. > > The broad-strokes answer is "your code in target/whatever should generally > not be including files that are neither in include/ nor in > target/whatever". > If you find yourself doing that you've probably structured something wrong > (otherwise other targets would also have run into this). > > For linux-user/qemu.h in particular, the top level meson.build does > add linux-user/ to the include path, but only for when it is building > files for the linux-user targets. (It makes no sense to include that header > file into code built for system emulation.) > > Of the 4 targets that #include "qemu.h" in target/whatever code, 3 of them > (m68k, nios2, arm) do it only for their semihosting .c file, and there > the #include "qemu.h" is inside an #ifdef CONFIG_USER_ONLY. (Semihosting > is a bit of an odd thing which works differently for usermode and > system emulation mode, which is why it needs this linux-user header.) > The 4th is hexagon, and that is a bug in the hexagon code which is only > going unnoticed because hexagon currently supports only the linux-user > target and not system emulation. > > thanks > -- PMM >
Re: QEMU System and User targets
On Fri, 16 Jul 2021 at 18:50, Kenneth Adam Miller wrote: > There's a lot of files and I don't want to muddy up the discussion with too > many details. If you don't provide details, you get vague answers. Your choice :-) > And for sure, this is not a problem with the upstream qemu. I'm working on > adding a target, and this is just what I'm experiencing. As for my target, it > has includes that correspond to finds within sub-directories of qemu > components, and I just mean that the include directives are only the file > name (no path prefix), but such file can be found only in folders other than > the include directory. One example is qemu.h; it is in linux-user. You can > get the compilation to find exactly just that file, but it includes other > files, and it isn't reasonable to edit anything outside of my own > architecture implementation. I'm wondering if perhaps anything that makes an > include to linux-user would need to be moved into the user target source set, > because currently it is in the shared. The broad-strokes answer is "your code in target/whatever should generally not be including files that are neither in include/ nor in target/whatever". If you find yourself doing that you've probably structured something wrong (otherwise other targets would also have run into this). For linux-user/qemu.h in particular, the top level meson.build does add linux-user/ to the include path, but only for when it is building files for the linux-user targets. (It makes no sense to include that header file into code built for system emulation.) Of the 4 targets that #include "qemu.h" in target/whatever code, 3 of them (m68k, nios2, arm) do it only for their semihosting .c file, and there the #include "qemu.h" is inside an #ifdef CONFIG_USER_ONLY. (Semihosting is a bit of an odd thing which works differently for usermode and system emulation mode, which is why it needs this linux-user header.) The 4th is hexagon, and that is a bug in the hexagon code which is only going unnoticed because hexagon currently supports only the linux-user target and not system emulation. thanks -- PMM
Re: QEMU System and User targets
There's a lot of files and I don't want to muddy up the discussion with too many details. And for sure, this is not a problem with the upstream qemu. I'm working on adding a target, and this is just what I'm experiencing. As for my target, it has includes that correspond to finds within sub-directories of qemu components, and I just mean that the include directives are only the file name (no path prefix), but such file can be found only in folders other than the include directory. One example is qemu.h; it is in linux-user. You can get the compilation to find exactly just that file, but it includes other files, and it isn't reasonable to edit anything outside of my own architecture implementation. I'm wondering if perhaps anything that makes an include to linux-user would need to be moved into the user target source set, because currently it is in the shared. On Fri, Jul 16, 2021 at 10:38 AM Peter Maydell wrote: > On Fri, 16 Jul 2021 at 16:16, Kenneth Adam Miller > wrote: > > > > When I go to build the qemu softmmu target the shared files - the > i386_ss of my arch - gives problems where the build system isn't specifying > the include headers for the compiler to find the surrounding headers that > belong to different parts of the qemu library. I was able to edit my own > source only so much before recursive header dependencies had included their > own respective qemu library components subdirectories, at which point the > build fails with cannot find header. I want to know, are these shared > source set files not supposed to include those qemu subcomponents? Or > resolve a different way for each target using #ifdef guards? I would think > that this would be modularized from within the qemu subcomponent library. > And I think that configure and meson are working together to generate the > correct build commands to these shared components. So I'm unsure what to do. > > Upstream QEMU builds fine for me, so you'll need to be more > specific about what you're trying to do and how it fails. > The top level meson.build always includes the current source > directory and the top level include/ directory on the compiler > include path, which should be all you need. > > thanks > -- PMM >
Re: [PULL v3 00/19] pc,pci,virtio: lots of new features
On Fri, 16 Jul 2021 at 16:15, Michael S. Tsirkin wrote: > > The following changes since commit bd306cfeeececee73ff2cdb3de1229ece72f3b28: > > Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20210714.0' > into staging (2021-07-15 21:39:04 +0100) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream3 > > for you to fetch changes up to 1e08fd0a465d70ad30d2928c66537c816f0af7f8: > > vhost-vsock: SOCK_SEQPACKET feature bit support (2021-07-16 11:10:45 -0400) > > > pc,pci,virtio: lots of new features > > Lots of last minute stuff. > > vhost-user-i2c. > vhost-vsock SOCK_SEQPACKET support. > IOMMU bypass. > ACPI based pci hotplug. > > Signed-off-by: Michael S. Tsirkin > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1 for any user-visible changes. -- PMM
Re: [PATCH V5 16/25] vfio-pci: cpr part 1
On Wed, 7 Jul 2021 10:20:25 -0700
Steve Sistare wrote:
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9220e64..40c882f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -31,6 +31,7 @@
> #include "exec/memory.h"
> #include "exec/ram_addr.h"
> #include "hw/hw.h"
> +#include "qemu/env.h"
> #include "qemu/error-report.h"
> #include "qemu/main-loop.h"
> #include "qemu/range.h"
> @@ -440,6 +441,10 @@ static int vfio_dma_unmap(VFIOContainer *container,
> return vfio_dma_unmap_bitmap(container, iova, size, iotlb);
> }
>
> +if (container->reused) {
> +return 0;
> +}
> +
> while (ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, )) {
> /*
> * The type1 backend has an off-by-one bug in the kernel
> (71a7d3d78e3c
> @@ -463,6 +468,11 @@ static int vfio_dma_unmap(VFIOContainer *container,
> return -errno;
> }
>
> +if (unmap.size != size) {
> +warn_report("VFIO_UNMAP_DMA(0x%lx, 0x%lx) only unmaps 0x%llx",
> + iova, size, unmap.size);
> +}
> +
I'm a tad nervous that we have paths that can trigger this, the ioctl
certainly supports that we can call it across multiple mappings and the
size returned is the sum of the previously mapped ranges that were
unmapped. See for instance vfio_listener_region_del()'s use of this
function.
> return 0;
> }
>
> @@ -477,6 +487,10 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr
> iova,
> .size = size,
> };
>
> +if (container->reused) {
> +return 0;
> +}
> +
> if (!readonly) {
> map.flags |= VFIO_DMA_MAP_FLAG_WRITE;
> }
> @@ -1603,6 +1617,10 @@ static int vfio_init_container(VFIOContainer
> *container, int group_fd,
> if (iommu_type < 0) {
> return iommu_type;
> }
> +if (container->reused) {
> +container->iommu_type = iommu_type;
> +return 0;
> +}
How would this handle the case where SPAPR_TCE_v2 falls back to
SPAPR_TCE (v1)?
>
> ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, >fd);
> if (ret) {
...
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 9fc12bc..0f5c542 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3264,6 +3272,61 @@ static Property vfio_pci_dev_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static void vfio_merge_config(VFIOPCIDevice *vdev)
> +{
> +PCIDevice *pdev = >pdev;
> +int size = MIN(pci_config_size(pdev), vdev->config_size);
> +uint8_t *phys_config = g_malloc(size);
> +uint32_t mask;
> +int ret, i;
> +
> +ret = pread(vdev->vbasedev.fd, phys_config, size, vdev->config_offset);
> +if (ret < size) {
> +ret = ret < 0 ? errno : EFAULT;
Leaks phys_config
> +error_report("failed to read device config space: %s",
> strerror(ret));
> +return;
> +}
> +
> +for (i = 0; i < size; i++) {
> +mask = vdev->emulated_config_bits[i];
> +pdev->config[i] = (pdev->config[i] & mask) | (phys_config[i] &
> ~mask);
> +}
> +
> +g_free(phys_config);
> +}
> +
> +static int vfio_pci_post_load(void *opaque, int version_id)
> +{
> +VFIOPCIDevice *vdev = opaque;
> +PCIDevice *pdev = >pdev;
> +bool enabled;
> +
> +vfio_merge_config(vdev);
> +
> +pdev->reused = false;
> +enabled = pci_get_word(pdev->config + PCI_COMMAND) & PCI_COMMAND_MASTER;
> +memory_region_set_enabled(>bus_master_enable_region, enabled);
This seems generic to any PCI device, I'm surprised we need to do it
explicitly. Thanks,
Alex
> +
> +return 0;
> +}
> +
[Bug 1892761] Re: Heap-use-after-free through double-fetch in ehci
Ok, let's close this one since it was not reproducible. If you find a reproducer, please open a new ticket in the gitlab tracker instead. ** Changed in: qemu Status: Incomplete => Won't Fix -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1892761 Title: Heap-use-after-free through double-fetch in ehci Status in QEMU: Won't Fix Bug description: Hello, I don't have a qtest reproducer for this crash because it involves a DMA double-fetch, and I don't think we can reproduce those with qtest. Instead, I attached the pseudo-qtest trace produced by the fuzzer, along with some trace events. The lines annotated with [DMA] are write commands that were triggered by a callback from a DMA read by the device. The lines annotated with [DOUBLE-FETCH] are DMA accesses that hit the same address more than once (possible double-fetches). I am still thinking of nicer ways of presenting this trace and providing a reproducer. -Alex To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1892761/+subscriptions
[Bug 1889621] Re: ARM Highbank Crashes Realted to GIC
I believe these were all taken care of by
edfe2eb436 ("hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register")
09bbdb89bc ("hw/intc/arm_gic: Allow to use QTest without crashing")
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1889621
Title:
ARM Highbank Crashes Realted to GIC
Status in QEMU:
Incomplete
Bug description:
Hello,
Here are some QTest reproducers for crashes on ARM Highbank that all seem to
be related to the gic device.
Reproducer 1:
cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \
-nographic -monitor none -serial none -qtest stdio
writel 0xfff11f00 0x8405f559
writel 0xfff117fd 0x5c057bd8
EOF
==10595==ERROR: AddressSanitizer: SEGV on unknown address 0x62b13e01 (pc
0x55b6ab85cc91 bp 0x7fff60bd4d70 sp 0x7fff60bd4ce0 T0)
==10595==The signal is caused by a READ memory access.
#0 0x55b6ab85cc91 in gic_get_current_cpu
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:60:12
#1 0x55b6ab85e1bd in gic_dist_writeb
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1182:11
#2 0x55b6ab855a97 in gic_dist_write
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1514:9
#3 0x55b6aa1650d4 in memory_region_write_with_attrs_accessor
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12
#4 0x55b6aa163ac6 in access_with_adjusted_size
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18
#5 0x55b6aa161f35 in memory_region_dispatch_write
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13
#6 0x55b6a9313949 in flatview_write_continue
/home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23
#7 0x55b6a92fca11 in flatview_write
/home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14
#8 0x55b6a92fc54e in address_space_write
/home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18
=
Reproducer 2:
cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \
-nographic -monitor none -serial none -qtest stdio
writeq 0xfff11f00 0x613a650f0fda6555
EOF
==1375==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60801c80 at pc 0x5618928c486e bp 0x7ffe22c4ee10 sp 0x7ffe22c4ee08
READ of size 8 at 0x60801c80 thread T0
#0 0x5618928c486d in address_space_translate_iommu
/home/alxndr/Development/qemu/general-fuzz/exec.c:451:23
#1 0x561892850acc in flatview_do_translate
/home/alxndr/Development/qemu/general-fuzz/exec.c:524:16
#2 0x5618928514ad in flatview_translate
/home/alxndr/Development/qemu/general-fuzz/exec.c:584:15
#3 0x5618928b1e14 in flatview_write_continue
/home/alxndr/Development/qemu/general-fuzz/exec.c:3199:14
#4 0x56189289aa11 in flatview_write
/home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14
#5 0x56189289a54e in address_space_write
/home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18
#6 0x5618937a5e13 in qtest_process_command
/home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:452:13
#7 0x56189379d89f in qtest_process_inbuf
/home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:710:9
#8 0x56189379c680 in qtest_read
/home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:722:5
=
Reproducer 3:
cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \
-nographic -monitor none -serial none -qtest stdio
writeq 0xfff11000 0x70b
writeq 0xfff11f00 0x4f4f4fff54a7afaf
writel 0xfff10100 0x61ff
EOF
==23743==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x62b06a92 at pc 0x55d690d980e1 bp 0x7ffe606082d0 sp 0x7ffe606082c8
READ of size 1 at 0x62b06a92 thread T0
#0 0x55d690d980e0 in gic_get_best_irq
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:94:13
#1 0x55d690d9485b in gic_update_internal
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:185:13
#2 0x55d690d90376 in gic_update
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:226:5
#3 0x55d690dc0879 in gic_cpu_write
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1758:9
#4 0x55d690da41c0 in gic_thiscpu_write
/home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1777:12
#5 0x55d68f6b30d4 in memory_region_write_with_attrs_accessor
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12
#6 0x55d68f6b1ac6 in access_with_adjusted_size
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18
#7 0x55d68f6aff35 in memory_region_dispatch_write
/home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13
#8 0x55d68e861949 in flatview_write_continue
/home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23
#9 0x55d68e84aa11 in flatview_write
[Bug 1918975] Re: [Feature request] Propagate interpreter to spawned processes
*** This bug is a duplicate of bug 1912107 *** https://bugs.launchpad.net/bugs/1912107 I think one ticket should be enough to track this problem, so let's continue the discussion here: https://gitlab.com/qemu-project/qemu/-/issues/306 ** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #306 https://gitlab.com/qemu-project/qemu/-/issues/306 ** This bug has been marked a duplicate of bug 1912107 Option to constrain linux-user exec() to emulated CPU only -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1918975 Title: [Feature request] Propagate interpreter to spawned processes Status in QEMU: Incomplete Bug description: I want QEMU user static to propagate interpreter to spawned processes, for instances by adding -R recursive. I.e. if my program is interpreted by QEMU static than everything what it launches should be interpreted by it, too. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1918975/+subscriptions
[Bug 1889621] Re: ARM Highbank Crashes Realted to GIC
Can you still reproduce one of these issues with the current master branch of QEMU? For me, all three reproduces do not seem to cause any trouble anymore... ** Changed in: qemu Status: Confirmed => Incomplete -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1889621 Title: ARM Highbank Crashes Realted to GIC Status in QEMU: Incomplete Bug description: Hello, Here are some QTest reproducers for crashes on ARM Highbank that all seem to be related to the gic device. Reproducer 1: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writel 0xfff11f00 0x8405f559 writel 0xfff117fd 0x5c057bd8 EOF ==10595==ERROR: AddressSanitizer: SEGV on unknown address 0x62b13e01 (pc 0x55b6ab85cc91 bp 0x7fff60bd4d70 sp 0x7fff60bd4ce0 T0) ==10595==The signal is caused by a READ memory access. #0 0x55b6ab85cc91 in gic_get_current_cpu /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:60:12 #1 0x55b6ab85e1bd in gic_dist_writeb /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1182:11 #2 0x55b6ab855a97 in gic_dist_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1514:9 #3 0x55b6aa1650d4 in memory_region_write_with_attrs_accessor /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12 #4 0x55b6aa163ac6 in access_with_adjusted_size /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18 #5 0x55b6aa161f35 in memory_region_dispatch_write /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13 #6 0x55b6a9313949 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23 #7 0x55b6a92fca11 in flatview_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14 #8 0x55b6a92fc54e in address_space_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18 = Reproducer 2: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writeq 0xfff11f00 0x613a650f0fda6555 EOF ==1375==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60801c80 at pc 0x5618928c486e bp 0x7ffe22c4ee10 sp 0x7ffe22c4ee08 READ of size 8 at 0x60801c80 thread T0 #0 0x5618928c486d in address_space_translate_iommu /home/alxndr/Development/qemu/general-fuzz/exec.c:451:23 #1 0x561892850acc in flatview_do_translate /home/alxndr/Development/qemu/general-fuzz/exec.c:524:16 #2 0x5618928514ad in flatview_translate /home/alxndr/Development/qemu/general-fuzz/exec.c:584:15 #3 0x5618928b1e14 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3199:14 #4 0x56189289aa11 in flatview_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3216:14 #5 0x56189289a54e in address_space_write /home/alxndr/Development/qemu/general-fuzz/exec.c:3308:18 #6 0x5618937a5e13 in qtest_process_command /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:452:13 #7 0x56189379d89f in qtest_process_inbuf /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:710:9 #8 0x56189379c680 in qtest_read /home/alxndr/Development/qemu/general-fuzz/softmmu/qtest.c:722:5 = Reproducer 3: cat << EOF | ./arm-softmmu/qemu-system-arm -machine highbank \ -nographic -monitor none -serial none -qtest stdio writeq 0xfff11000 0x70b writeq 0xfff11f00 0x4f4f4fff54a7afaf writel 0xfff10100 0x61ff EOF ==23743==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b06a92 at pc 0x55d690d980e1 bp 0x7ffe606082d0 sp 0x7ffe606082c8 READ of size 1 at 0x62b06a92 thread T0 #0 0x55d690d980e0 in gic_get_best_irq /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:94:13 #1 0x55d690d9485b in gic_update_internal /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:185:13 #2 0x55d690d90376 in gic_update /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:226:5 #3 0x55d690dc0879 in gic_cpu_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1758:9 #4 0x55d690da41c0 in gic_thiscpu_write /home/alxndr/Development/qemu/general-fuzz/hw/intc/arm_gic.c:1777:12 #5 0x55d68f6b30d4 in memory_region_write_with_attrs_accessor /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:503:12 #6 0x55d68f6b1ac6 in access_with_adjusted_size /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:544:18 #7 0x55d68f6aff35 in memory_region_dispatch_write /home/alxndr/Development/qemu/general-fuzz/softmmu/memory.c:1473:13 #8 0x55d68e861949 in flatview_write_continue /home/alxndr/Development/qemu/general-fuzz/exec.c:3176:23 #9 0x55d68e84aa11 in
Re: [PATCH for-6.2 11/34] target/arm: Implement MVE VMULL (polynomial)
On 7/13/21 6:37 AM, Peter Maydell wrote: Implement the MVE VMULL (polynomial) insn. Unlike Neon, this comes in two flavours: 8x8->16 and a 16x16->32. Also unlike Neon, the inputs are in either the low or the high half of each double-width element. The assembler for this insn indicates the size with "P8" or "P16", encoded into bit 28 as size = 0 or 1. We choose to follow the same encoding as VQDMULL and decode this into a->size as MO_16 or MO_32 indicating the size of the result elements. This then carries through to the helper function names where it then matches up with the existing pmull_h() which does an 8x8->16 operation and a new pmull_w() which does the 16x16->32. Signed-off-by: Peter Maydell --- target/arm/helper-mve.h| 5 + target/arm/vec_internal.h | 11 +++ target/arm/mve.decode | 14 ++ target/arm/mve_helper.c| 16 target/arm/translate-mve.c | 28 target/arm/vec_helper.c| 14 +- 6 files changed, 83 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 10/34] target/arm: Fix VLDRB/H/W for predicated elements
On 7/13/21 6:37 AM, Peter Maydell wrote: For vector loads, predicated elements are zeroed, instead of retaining their previous values (as happens for most data processing operations). This means we need to distinguish "beat not executed due to ECI" (don't touch destination element) from "beat executed but predicated out" (zero destination element). Signed-off-by: Peter Maydell --- target/arm/mve_helper.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 08/34] target/arm: Fix VPT advance when ECI is non-zero
On 7/13/21 6:37 AM, Peter Maydell wrote:
if (mask01 > 8) {
-/* high bit set, but not 0b1000: invert the relevant half of P0 */
-vpr ^= 0xff;
+if (eci == ECI_NONE) {
+/* high bit set, but not 0b1000: invert the relevant half of P0 */
+vpr ^= 0xff;
+} else if (eci == ECI_A0) {
+/* Invert only the beat 1 P0 bits, as we didn't execute beat 0 */
+vpr ^= 0xf0;
+} /* otherwise we didn't execute either beat 0 or beat 1 */
}
if (mask23 > 8) {
-/* high bit set, but not 0b1000: invert the relevant half of P0 */
-vpr ^= 0xff00;
+if (eci != ECI_A0A1A2 && eci != ECI_A0A1A2B0) {
+/* high bit set, but not 0b1000: invert the relevant half of P0 */
+vpr ^= 0xff00;
+} else {
+/* We didn't execute beat 2, only invert the beat 3 P0 bits */
+vpr ^= 0xf000;
+}
}
It might not be any cleaner, but I wondered if mve_eci_mask could help here.
inv_mask = mve_eci_mask(...);
if (mask01 <= 8) {
inv_mask &= ~0xff;
}
if (mask23 <= 8) {
inv_mask &= ~0xff00;
}
vpr ^= inv_mask;
r~
Re: [PATCH for-6.2 09/34] target/arm: Factor out mve_eci_mask()
On 7/13/21 6:37 AM, Peter Maydell wrote: In some situations we need a mask telling us which parts of the vector correspond to beats that are not being executed because of ECI, separately from the combined "which bytes are predicated away" mask. Factor this mask calculation out of mve_element_mask() into its own function. Signed-off-by: Peter Maydell --- target/arm/mve_helper.c | 58 - 1 file changed, 34 insertions(+), 24 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 08/34] target/arm: Fix VPT advance when ECI is non-zero
On 7/13/21 6:37 AM, Peter Maydell wrote: We were not paying attention to the ECI state when advancing the VPT state. Architecturally, VPT state advance happens for every beat (see the pseudocode VPTAdvance()), so on every beat the 4 bits of VPR.P0 corresponding to the current beat are inverted if required, and at the end of beats 1 and 3 the VPR MASK fields are updated. This means that if the ECI state says we should not be executing all 4 beats then we need to skip some of the updating of the VPR that we currently do in mve_advance_vpt(). Signed-off-by: Peter Maydell --- target/arm/mve_helper.c | 29 +++-- 1 file changed, 23 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 06/34] target/arm: Fix 48-bit saturating shifts
On Fri, 16 Jul 2021 at 17:34, Richard Henderson wrote: > > On 7/13/21 6:36 AM, Peter Maydell wrote: > > -return (1ULL << 47) - (src >= 0); > > +return sextract64((1ULL << 47) - (src >= 0), 0, 48); > > Clearer as > >return src >= 0 ? MAKE_64BIT_MASK(0, 47) : MAKE_64BIT_MASK(47, 17); Yeah, agreed. thanks -- PMM
Re: [PATCH for-6.2 07/34] target/arm: Fix calculation of LTP mask when LR is 0
On 7/13/21 6:36 AM, Peter Maydell wrote: In mve_element_mask(), we calculate a mask for tail predication which should have a number of 1 bits based on the value of LR. However, our MAKE_64BIT_MASK() macro has undefined behaviour when passed a zero length. Special case this to give the all-zeroes mask we require. Signed-off-by: Peter Maydell --- target/arm/mve_helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 06/34] target/arm: Fix 48-bit saturating shifts
On 7/13/21 6:36 AM, Peter Maydell wrote: -return (1ULL << 47) - (src >= 0); +return sextract64((1ULL << 47) - (src >= 0), 0, 48); Clearer as return src >= 0 ? MAKE_64BIT_MASK(0, 47) : MAKE_64BIT_MASK(47, 17); ? Otherwise, Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 05/34] target/arm: Fix mask handling for MVE narrowing operations
On 7/13/21 6:36 AM, Peter Maydell wrote: In the MVE helpers for the narrowing operations (DO_VSHRN and DO_VSHRN_SAT) we were using the wrong bits of the predicate mask for the 'top' versions of the insn. This is because the loop works over the double-sized input elements and shifts the predicate mask by that many bits each time, but when we write out the half-sized output we must look at the mask bits for whichever half of the element we are writing to. Correct this by shifting the whole mask right by ESIZE bits for the 'top' insns. This allows us also to simplify the saturation bit checking (where we had noticed that we needed to look at a different mask bit for the 'top' insn.) Signed-off-by: Peter Maydell --- target/arm/mve_helper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.2 03/34] target/arm: Fix MVE VSLI by 0 and VSRI by
On 7/13/21 6:36 AM, Peter Maydell wrote:
if (shift == 0 || shift == ESIZE * 8) { \
/* \
* Only VSLI can shift by 0; only VSRI can shift by . \
* The generic logic would give the right answer for 0 but \
- * fails for . \
+ * fails for . In both cases, we must not shift the \
+ * input but just copy it to the destination, honouring \
+ * the predicate mask. \
*/ \
+for (e = 0; e < 16 / 8; e++, mask >>= 8) { \
+mergemask([H8(e)], m[H8(e)], mask); \
+} \
goto done; \
} \
VSLI is d = op1 << shift | (d & ~(-1 << shift))
for shift = 0 does result in d = op1.
However,
VRSI is d = op1 >> shift | (d & ~(-1 >> shift))
for shift = 32 results in d = d.
r~
Re: [PATCH v2] docs: document file-posix locking protocol
15.07.2021 23:00, Vladimir Sementsov-Ogievskiy wrote: 03.07.2021 16:50, Vladimir Sementsov-Ogievskiy wrote: +Permission bytes. If permission byte is rd-locked, it means that some process +uses corresponding permission on that file. + +Byte Operation +100 read + Lock holder can read +101 write + Lock holder can write +102 write-unchanged + Lock holder can write same data if it sure, that this write doesn't + break concurrent readers. This is mostly used internally in Qemu + and it wouldn't be good idea to exploit it somehow. Let's make it more strict: New software should never lock this byte and interpret this byte locked by other process like write permission (same as 101). [*] +103 resize + Lock holder can resize the file. "write" permission is also required + for resizing, so lock byte 103 only if you also lock byte 101. +104 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Unshare bytes. If permission byte is rd-locked, it means that some process +does not allow the others use corresponding options on that file. + +Byte Operation +200 read + Lock holder don't allow read operation to other processes. +201 write + Lock holder don't allow write operation to other processes. This + still allows others to do write-uncahnged operations. Better not + exploit outside of Qemu. +202 write-unchanged + Lock holder don't allow write-unchanged operation to other processes. And here, correspondingly: New software should never lock this byte and interpret this byte locked by other process like write permission unshared (same as 201). Hmm, no. For [*] work correctly, new software should always lock 202 if it locks 201. +203 resize + Lock holder don't allow resizing the file by other processes. +204 graph-mod + Undefined. QEMU may sometimes locks this byte, but external programs + should not. QEMU will stop locking this byte in future + +Handling the permissions works as follows: assume we want to open the file to do +some operations and in the same time want to disallow some operation to other +processes. So, we want to lock some of the bytes described above. We operate as +follows: + More on this. Hmm, we can just drop graph-mod. I don't think it is needed now, and anyway no sense in locking it in file-posix. write-unchanged is rather strange here. One process may read data and write it back and consider it WRITE_UNCHANGED, but other process may change the file intermediatly, and for it the WRITE_UNCHANGED of first process is not "unchanged".. It probably good to drop write-unchanged from the protocol at all. But what if we have old qemu that can lock write-unchanged (byte 102) and not write (byte 101)? The audit do we really have such a case now or in some previous version is rather hard to do.. So, if we want to support it in the most compatible way, all new software should lock both 201 and 202 to unshare write. Locking 102 in pair with 101 doesn't make real sense: if we has write access, we don't need additional write-unchanged anyway. Still, we may document that these bytes should be locked together just for consistency with "unshare" bytes. Any thoughts? Probably it worth also document that 203 is recommended to be locked if application want to lock 201, to avoid bad behaving process, that locks 103 and not 101 and thinks that it has permission to resize. -- Best regards, Vladimir
Re: [PATCH v2 05/21] contrib/gitdm: add some new aliases to fix up commits
On 7/14/21 11:20 AM, Alex Bennée wrote: Signed-off-by: Alex Bennée Cc: Yuval Shaia Message-Id:<20210714093638.21077-6-alex.ben...@linaro.org> --- contrib/gitdm/aliases | 3 +++ 1 file changed, 3 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 04/21] configure: remove needless if leg
On 7/14/21 11:20 AM, Alex Bennée wrote:
It was pointed out in review of the previous patch that the if leg
isn't needed as the for loop will not enter on an empty $device_archs.
Fixes: d1d5e9eefd ("configure: allow the selection of alternate config in the
build")
Signed-off-by: Alex Bennée
Message-Id:<20210714093638.21077-5-alex.ben...@linaro.org>
---
configure | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH v3 1/2] Hexagon (target/hexagon) remove put_user_*/get_user_*
On 7/15/21 2:22 PM, Taylor Simpson wrote: Replace put_user_* with cpu_st*_data_ra Replace get_user_* with cpu_ld*_data_ra Suggested-by: Richard Henderson Signed-off-by: Taylor Simpson --- target/hexagon/op_helper.c | 39 ++- 1 file changed, 18 insertions(+), 21 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PULL 32/40] tcg/plugins: enable by default for most TCG builds
On 7/16/21 8:31 AM, Christian Borntraeger wrote: On 16.07.21 16:58, Christian Borntraeger wrote: On 16.07.21 16:51, Richard Henderson wrote: On 7/16/21 4:28 AM, Christian Borntraeger wrote: --extra-ldflags="-Wl,--build-id -pie -Wl,-z,relro -Wl,-z,now" Full configure line is ../configure --prefix=/usr --libdir=/usr/lib64 --sysconfdir=/etc --interp-prefix=/usr/qemu-%M --localstatedir=/var --libexecdir=/usr/libexec --extra-ldflags="-Wl,--build-id -pie -Wl,-z,relro -Wl,-z,now" --extra-cflags="-O2 -g -fPIE -DPIE" --enable-werror --disable-strip --disable-rbd --disable-fdt --disable-xen --enable-kvm --enable-trace-backend=log --iasl=false --target-list=s390x-softmmu,i386-softmmu,x86_64-softmmu,s390x-linux-user FWIW, -pie should not be buried in --extra-ldflags, but as --enable-pie on the normal configure line. I picked that from the configure script of an older fedora qemu src rpm some years ago and I use that to do build checks. using enable-pie instead of burying it in the ldflags seems to fix things. Question is do we still care about this regression? I don't think so. Your incorrect ldflags (appropriate for the main executable, perhaps) was leaking into the shared library plugins, where -shared and -pie conflict. r~
Re: [PATCH] qtest/hyperv: Introduce a simple hyper-v test
On Fri, Jul 16, 2021 at 02:55:28PM +0200, Vitaly Kuznetsov wrote:
> For the beginning, just test 'hv-passthrough' and a couple of custom
> Hyper-V enlightenments configurations through QMP. Later, it would
> be great to complement this by checking CPUID values from within the
> guest.
>
> Signed-off-by: Vitaly Kuznetsov
> ---
> - Changes since "[PATCH v8 0/9] i386: KVM: expand Hyper-V features early":
> make the test SKIP correctly when KVM is not present.
> ---
> MAINTAINERS | 1 +
> tests/qtest/hyperv-test.c | 228 ++
> tests/qtest/meson.build | 3 +-
> 3 files changed, 231 insertions(+), 1 deletion(-)
> create mode 100644 tests/qtest/hyperv-test.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 148153d74f5b..c1afd744edca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1576,6 +1576,7 @@ F: hw/isa/apm.c
> F: include/hw/isa/apm.h
> F: tests/unit/test-x86-cpuid.c
> F: tests/qtest/test-x86-cpuid-compat.c
> +F: tests/qtest/hyperv-test.c
>
> PC Chipset
> M: Michael S. Tsirkin
> diff --git a/tests/qtest/hyperv-test.c b/tests/qtest/hyperv-test.c
> new file mode 100644
> index ..2155e5d90970
> --- /dev/null
> +++ b/tests/qtest/hyperv-test.c
> @@ -0,0 +1,228 @@
> +/*
> + * Hyper-V emulation CPU feature test cases
> + *
> + * Copyright (c) 2021 Red Hat Inc.
> + * Authors:
> + * Vitaly Kuznetsov
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include
> +#include
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bitops.h"
> +#include "libqos/libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qjson.h"
> +
> +#define MACHINE_KVM "-machine pc-q35-5.2 -accel kvm "
> +#define QUERY_HEAD "{ 'execute': 'query-cpu-model-expansion', " \
> +" 'arguments': { 'type': 'full', "
> +#define QUERY_TAIL "}}"
> +
> +static bool kvm_enabled(QTestState *qts)
> +{
> +QDict *resp, *qdict;
> +bool enabled;
> +
> +resp = qtest_qmp(qts, "{ 'execute': 'query-kvm' }");
> +g_assert(qdict_haskey(resp, "return"));
> +qdict = qdict_get_qdict(resp, "return");
> +g_assert(qdict_haskey(qdict, "enabled"));
> +enabled = qdict_get_bool(qdict, "enabled");
> +qobject_unref(resp);
> +
> +return enabled;
> +}
> +
> +static bool kvm_has_cap(int cap)
> +{
> +int fd = open("/dev/kvm", O_RDWR);
> +int ret;
> +
> +if (fd < 0) {
> +return false;
> +}
> +
> +ret = ioctl(fd, KVM_CHECK_EXTENSION, cap);
> +
> +close(fd);
> +
> +return ret > 0;
> +}
> +
> +static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
> +{
> +return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
> + QUERY_TAIL, cpu_type);
> +}
> +
> +static bool resp_has_props(QDict *resp)
> +{
> +QDict *qdict;
> +
> +g_assert(resp);
> +
> +if (!qdict_haskey(resp, "return")) {
> +return false;
> +}
> +qdict = qdict_get_qdict(resp, "return");
> +
> +if (!qdict_haskey(qdict, "model")) {
> +return false;
> +}
> +qdict = qdict_get_qdict(qdict, "model");
> +
> +return qdict_haskey(qdict, "props");
> +}
> +
> +static QDict *resp_get_props(QDict *resp)
> +{
> +QDict *qdict;
> +
> +g_assert(resp);
> +g_assert(resp_has_props(resp));
> +
> +qdict = qdict_get_qdict(resp, "return");
> +qdict = qdict_get_qdict(qdict, "model");
> +qdict = qdict_get_qdict(qdict, "props");
> +
> +return qdict;
> +}
> +
> +static bool resp_get_feature(QDict *resp, const char *feature)
> +{
> +QDict *props;
> +
> +g_assert(resp);
> +g_assert(resp_has_props(resp));
> +props = resp_get_props(resp);
> +g_assert(qdict_get(props, feature));
> +return qdict_get_bool(props, feature);
> +}
> +
> +#define assert_has_feature(qts, cpu_type, feature) \
> +({ \
> +QDict *_resp = do_query_no_props(qts, cpu_type); \
> +g_assert(_resp); \
> +g_assert(resp_has_props(_resp)); \
> +g_assert(qdict_get(resp_get_props(_resp), feature)); \
> +qobject_unref(_resp); \
> +})
> +
> +#define resp_assert_feature(resp, feature, expected_value) \
> +({ \
> +QDict *_props; \
> + \
> +g_assert(_resp); \
> +g_assert(resp_has_props(_resp)); \
> +_props = resp_get_props(_resp);\
> +g_assert(qdict_get(_props, feature));
Re: QEMU System and User targets
On Fri, 16 Jul 2021 at 16:16, Kenneth Adam Miller wrote: > > When I go to build the qemu softmmu target the shared files - the i386_ss of > my arch - gives problems where the build system isn't specifying the include > headers for the compiler to find the surrounding headers that belong to > different parts of the qemu library. I was able to edit my own source only so > much before recursive header dependencies had included their own respective > qemu library components subdirectories, at which point the build fails with > cannot find header. I want to know, are these shared source set files not > supposed to include those qemu subcomponents? Or resolve a different way for > each target using #ifdef guards? I would think that this would be modularized > from within the qemu subcomponent library. And I think that configure and > meson are working together to generate the correct build commands to these > shared components. So I'm unsure what to do. Upstream QEMU builds fine for me, so you'll need to be more specific about what you're trying to do and how it fails. The top level meson.build always includes the current source directory and the top level include/ directory on the compiler include path, which should be all you need. thanks -- PMM
Re: [PULL 32/40] tcg/plugins: enable by default for most TCG builds
On 16.07.21 16:58, Christian Borntraeger wrote: On 16.07.21 16:51, Richard Henderson wrote: On 7/16/21 4:28 AM, Christian Borntraeger wrote: --extra-ldflags="-Wl,--build-id -pie -Wl,-z,relro -Wl,-z,now" Full configure line is ../configure --prefix=/usr --libdir=/usr/lib64 --sysconfdir=/etc --interp-prefix=/usr/qemu-%M --localstatedir=/var --libexecdir=/usr/libexec --extra-ldflags="-Wl,--build-id -pie -Wl,-z,relro -Wl,-z,now" --extra-cflags="-O2 -g -fPIE -DPIE" --enable-werror --disable-strip --disable-rbd --disable-fdt --disable-xen --enable-kvm --enable-trace-backend=log --iasl=false --target-list=s390x-softmmu,i386-softmmu,x86_64-softmmu,s390x-linux-user FWIW, -pie should not be buried in --extra-ldflags, but as --enable-pie on the normal configure line. I picked that from the configure script of an older fedora qemu src rpm some years ago and I use that to do build checks. using enable-pie instead of burying it in the ldflags seems to fix things. Question is do we still care about this regression?
Re: QEMU System and User targets
When I go to build the qemu softmmu target the shared files - the i386_ss of my arch - gives problems where the build system isn't specifying the include headers for the compiler to find the surrounding headers that belong to different parts of the qemu library. I was able to edit my own source only so much before recursive header dependencies had included their own respective qemu library components subdirectories, at which point the build fails with cannot find header. I want to know, are these shared source set files not supposed to include those qemu subcomponents? Or resolve a different way for each target using #ifdef guards? I would think that this would be modularized from within the qemu subcomponent library. And I think that configure and meson are working together to generate the correct build commands to these shared components. So I'm unsure what to do. On Thu, Jul 15, 2021 at 11:39 AM Kenneth Adam Miller < kennethadammil...@gmail.com> wrote: > Oh I didn't know that there was a i386_user_ss in order to see that it was > intended that they were shared that way, so I initially thought that > i386_ss was user only until I saw it in the build. > > On Thu, Jul 15, 2021 at 11:35 AM Peter Maydell > wrote: > >> On Thu, 15 Jul 2021 at 17:25, Kenneth Adam Miller >> wrote: >> > >> > Well certainly, I know they are different executables. I'm just trying >> to understand how the different targets work. >> > >> > By subsumes, I mean that just looking at the meson.build for i386, you >> can see that there are files added to the i386_ss, but not visibly added to >> the softmmu target. But the softmmu target has those files built whenever >> you configure and build it. >> >> In the meson.build files, i386_ss is files built for both softmmu and >> user; >> i386_user_ss is files built for usermode only; i386_softmmu_ss is files >> built for softmmu only. target/i386/meson.build sets target_arch, >> target_softmmu_arch and target_user_arch to these sourcesets. >> The top level meson.build adds the relevant target_* sourcesets to the >> set of sources required to build the various executables. >> >> Some source files also use #ifdefs: you can look for ifdefs on >> CONFIG_USER_ONLY and CONFIG_SOFTMMU to find code that's conditionally >> compiled in different ways for the two executables. >> >> -- PMM >> >
Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex
Am 13.07.2021 um 15:10 hat Stefan Hajnoczi geschrieben: > AIO_WAIT_WHILE() requires that AioContext is acquired according to its > documentation, but I'm not sure that's true anymore. Thread-safe/atomic > primitives are used by AIO_WAIT_WHILE(), so as long as the condition > being waited for is thread-safe too it should work without the > AioContext lock. Polling something in a different AioContext from the main thread still temporarily drops the lock, which crashes if it isn't locked. I'm not sure if the documentation claims that the lock is needed in more cases, I guess you could interpret it either way. Kevin signature.asc Description: PGP signature
[PULL v3 19/19] vhost-vsock: SOCK_SEQPACKET feature bit support
From: Arseny Krasnov
This adds processing of VIRTIO_VSOCK_F_SEQPACKET features bit. Guest
negotiates it with vhost, thus both will know that SOCK_SEQPACKET
supported by peer.
Signed-off-by: Arseny Krasnov
Message-Id: <20210622144747.2949134-1-arseny.kras...@kaspersky.com>
Reviewed-by: Stefano Garzarella
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
---
hw/virtio/vhost-vsock.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 777cafe70d..1b1a5c70ed 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -21,6 +21,11 @@
#include "hw/virtio/vhost-vsock.h"
#include "monitor/monitor.h"
+const int feature_bits[] = {
+VIRTIO_VSOCK_F_SEQPACKET,
+VHOST_INVALID_FEATURE_BIT
+};
+
static void vhost_vsock_get_config(VirtIODevice *vdev, uint8_t *config)
{
VHostVSock *vsock = VHOST_VSOCK(vdev);
@@ -108,8 +113,11 @@ static uint64_t vhost_vsock_get_features(VirtIODevice
*vdev,
uint64_t requested_features,
Error **errp)
{
-/* No feature bits used yet */
-return requested_features;
+VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
+
+virtio_add_feature(_features, VIRTIO_VSOCK_F_SEQPACKET);
+return vhost_get_features(>vhost_dev, feature_bits,
+requested_features);
}
static const VMStateDescription vmstate_virtio_vhost_vsock = {
--
MST
[PULL v3 16/19] hw/i386/acpi-build: Add DMAR support to bypass iommu
From: Xingang Wang
In DMAR table, the drhd is set to cover all PCI devices when intel_iommu
is on. To support bypass iommu feature, we need to walk the PCI bus with
bypass_iommu disabled and add explicit scope data in DMAR drhd structure.
/mnt/sdb/wxg/qemu-next/qemu/build/x86_64-softmmu/qemu-system-x86_64 \
-machine q35,accel=kvm,default_bus_bypass_iommu=true \
-cpu host \
-m 16G \
-smp 36,sockets=2,cores=18,threads=1 \
-device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3 \
-device pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x4,bypass_iommu=true \
-device pcie-root-port,port=0x1,chassis=1,id=pci.11,bus=pci.10,addr=0x0 \
-device pcie-root-port,port=0x2,chassis=2,id=pci.21,bus=pci.20,addr=0x0 \
-device virtio-scsi-pci,id=scsi0,bus=pci.11,addr=0x0 \
-device virtio-scsi-pci,id=scsi1,bus=pci.21,addr=0x0 \
-drive
file=/mnt/sdb/wxg/fedora-48g.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=none,aio=native
\
-device
scsi-hd,bus=scsi1.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
\
-device intel-iommu \
-nographic \
And we get the guest configuration:
~ lspci -vt
-+-[:20]---00.0-[21]00.0 Red Hat, Inc. Virtio SCSI
+-[:10]---00.0-[11]00.0 Red Hat, Inc. Virtio SCSI
\-[:00]-+-00.0 Intel Corporation 82G33/G31/P35/P31 Express DRAM Controller
+-01.0 Device 1234:
+-02.0 Intel Corporation 82574L Gigabit Network Connection
+-03.0 Red Hat, Inc. QEMU PCIe Expander bridge
+-04.0 Red Hat, Inc. QEMU PCIe Expander bridge
+-1f.0 Intel Corporation 82801IB (ICH9) LPC Interface Controller
+-1f.2 Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA
Controller [AHCI mode]
\-1f.3 Intel Corporation 82801I (ICH9 Family) SMBus Controller
With bypass_iommu enabled on root bus, the attached devices will bypass iommu:
/sys/class/iommu/dmar0
├── devices
│ ├── :10:00.0 -> ../../../../pci:10/:10:00.0
│ └── :11:00.0 -> ../../../../pci:10/:10:00.0/:11:00.0
Signed-off-by: Xingang Wang
Message-Id: <1625748919-52456-8-git-send-email-wangxinga...@huawei.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
---
hw/i386/acpi-build.c | 68 ++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index bc966a4110..7efc6285ac 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2022,6 +2022,56 @@ build_srat(GArray *table_data, BIOSLinker *linker,
MachineState *machine)
x86ms->oem_table_id);
}
+/*
+ * Insert DMAR scope for PCI bridges and endpoint devcie
+ */
+static void
+insert_scope(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+GArray *scope_blob = opaque;
+AcpiDmarDeviceScope *scope = NULL;
+
+if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
+/* Dmar Scope Type: 0x02 for PCI Bridge */
+build_append_int_noprefix(scope_blob, 0x02, 1);
+} else {
+/* Dmar Scope Type: 0x01 for PCI Endpoint Device */
+build_append_int_noprefix(scope_blob, 0x01, 1);
+}
+
+/* length */
+build_append_int_noprefix(scope_blob,
+ sizeof(*scope) + sizeof(scope->path[0]), 1);
+/* reserved */
+build_append_int_noprefix(scope_blob, 0, 2);
+/* enumeration_id */
+build_append_int_noprefix(scope_blob, 0, 1);
+/* bus */
+build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1);
+/* device */
+build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1);
+/* function */
+build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1);
+}
+
+/* For a given PCI host bridge, walk and insert DMAR scope */
+static int
+dmar_host_bridges(Object *obj, void *opaque)
+{
+GArray *scope_blob = opaque;
+
+if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+if (bus && !pci_bus_bypass_iommu(bus)) {
+pci_for_each_device(bus, pci_bus_num(bus), insert_scope,
+scope_blob);
+}
+}
+
+return 0;
+}
+
/*
* VT-d spec 8.1 DMA Remapping Reporting Structure
* (version Oct. 2014 or later)
@@ -2041,6 +2091,15 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
/* Root complex IOAPIC use one path[0] only */
size_t ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);
IntelIOMMUState *intel_iommu = INTEL_IOMMU_DEVICE(iommu);
+GArray *scope_blob = g_array_new(false, true, 1);
+
+/*
+ * A PCI bus walk, for each PCI host bridge.
+ * Insert scope for each PCI bridge and endpoint device which
+ * is attached to a bus with iommu enabled.
+ */
+object_child_foreach_recursive(object_get_root(),
+ dmar_host_bridges, scope_blob);
assert(iommu);
if
[PULL v3 18/19] docs: Add documentation for iommu bypass
From: Xingang Wang Signed-off-by: Xingang Wang Message-Id: <1625748919-52456-10-git-send-email-wangxinga...@huawei.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- docs/bypass-iommu.txt | 89 +++ 1 file changed, 89 insertions(+) create mode 100644 docs/bypass-iommu.txt diff --git a/docs/bypass-iommu.txt b/docs/bypass-iommu.txt new file mode 100644 index 00..e6677bddd3 --- /dev/null +++ b/docs/bypass-iommu.txt @@ -0,0 +1,89 @@ +BYPASS IOMMU PROPERTY += + +Description +=== +Traditionally, there is a global switch to enable/disable vIOMMU. All +devices in the system can only support go through vIOMMU or not, which +is not flexible. We introduce this bypass iommu property to support +coexist of devices go through vIOMMU and devices not. This is useful to +passthrough devices with no-iommu mode and devices go through vIOMMU in +the same virtual machine. + +PCI host bridges have a bypass_iommu property. This property is used to +determine whether the devices attached on the PCI host bridge will bypass +virtual iommu. The bypass_iommu property is valid only when there is a +virtual iommu in the system, it is implemented to allow some devices to +bypass vIOMMU. When bypass_iommu property is not set for a host bridge, +the attached devices will go through vIOMMU by default. + +Usage += +The bypass iommu feature support PXB host bridge and default main host +bridge, we add a bypass_iommu property for PXB and default_bus_bypass_iommu +for machine. Note that default_bus_bypass_iommu is available only when +the 'q35' machine type on x86 architecture and the 'virt' machine type +on AArch64. Other machine types do not support bypass iommu for default +root bus. + +1. The following is the bypass iommu options: + (1) PCI expander bridge + qemu -device pxb-pcie,bus_nr=0x10,addr=0x1,bypass_iommu=true + (2) Arm default host bridge + qemu -machine virt,iommu=smmuv3,default_bus_bypass_iommu=true + (3) X86 default root bus bypass iommu: + qemu -machine q35,default_bus_bypass_iommu=true + +2. Here is the detailed qemu command line for 'virt' machine with PXB on +AArch64: + +qemu-system-aarch64 \ + -machine virt,kernel_irqchip=on,iommu=smmuv3,default_bus_bypass_iommu=true \ + -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1 \ + -device pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x3.0x2,bypass_iommu=true \ + +And we got: + - a default host bridge which bypass SMMUv3 + - a pxb host bridge which go through SMMUv3 + - a pxb host bridge which bypass SMMUv3 + +3. Here is the detailed qemu command line for 'q35' machine with PXB on +x86 architecture: + +qemu-system-x86_64 \ + -machine q35,accel=kvm,default_bus_bypass_iommu=true \ + -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3 \ + -device pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x4,bypass_iommu=true \ + -device intel-iommu \ + +And we got: + - a default host bridge which bypass iommu + - a pxb host bridge which go through iommu + - a pxb host bridge which bypass iommu + +Limitations +=== +There might be potential security risk when devices bypass iommu, because +devices might send malicious dma request to virtual machine if there is no +iommu isolation. So it would be necessary to only bypass iommu for trusted +device. + +Implementation +== +The bypass iommu feature includes: + - Address space + Add bypass iommu property check of PCI Host and do not get iommu address + space for devices bypass iommu. + - Arm SMMUv3 support + We traverse all PCI root bus and get bus number ranges, then build explicit + RID mapping for devices which do not bypass iommu. + - X86 IOMMU support + To support Intel iommu, we traverse all PCI host bridge and get information + of devices which do not bypass iommu, then fill the DMAR drhd struct with + explicit device scope info. To support AMD iommu, add check of bypass iommu + when traverse the PCI hsot bridge. + - Machine and PXB options + We add bypass iommu options in machine option for default root bus, and add + option for PXB also. Note that the default value of bypass iommu is false, + so that the devices will by default go through iommu if there exist one. + -- MST
