[Qemu-devel] [PATCH v9 6/6] tests: qmp-test: add queue full test
We'll need to include "monitor/monitor.h" for the queue length macro, then we don't need to hard code it. Suggested-by: Markus Armbruster Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu --- tests/qmp-test.c | 26 ++ 1 file changed, 26 insertions(+) diff --git a/tests/qmp-test.c b/tests/qmp-test.c index cc9907b525..6989acbca4 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -18,6 +18,7 @@ #include "qapi/qmp/qlist.h" #include "qapi/qobject-input-visitor.h" #include "qapi/qmp/qstring.h" +#include "monitor/monitor.h" const char common_args[] = "-nodefaults -machine none"; @@ -218,6 +219,8 @@ static void test_qmp_oob(void) const QListEntry *entry; QList *capabilities; QString *qstr; +gchar *id; +int i; qts = qtest_init_without_qmp_handshake(common_args); @@ -272,6 +275,29 @@ static void test_qmp_oob(void) unblock_blocked_cmd(); recv_cmd_id(qts, "blocks-2"); recv_cmd_id(qts, "err-2"); + +/* + * Test queue full. When that happens, the out-of-band command + * will only be able to be handled after the queue is shrinked, so + * it'll be processed only after one existing in-band command + * finishes. + */ +for (i = 1; i <= QMP_REQ_QUEUE_LEN_MAX; i++) { +id = g_strdup_printf("queue-blocks-%d", i); +send_cmd_that_blocks(qts, id); +g_free(id); +} +send_oob_cmd_that_fails(qts, "oob-1"); +unblock_blocked_cmd(); +recv_cmd_id(qts, "queue-blocks-1"); +recv_cmd_id(qts, "oob-1"); +for (i = 2; i <= QMP_REQ_QUEUE_LEN_MAX; i++) { +unblock_blocked_cmd(); +id = g_strdup_printf("queue-blocks-%d", i); +recv_cmd_id(qts, id); +g_free(id); +} + cleanup_blocking_cmd(); qtest_quit(qts); -- 2.17.1
[Qemu-devel] [PATCH v9 5/6] tests: add oob functional test for test-qmp-cmds
Straightforward test just to let the test-qmp-cmds be complete. Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu --- tests/test-qmp-cmds.c | 16 1 file changed, 16 insertions(+) diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 4ab2b6e5ce..481cb069ca 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -126,6 +126,21 @@ static void test_dispatch_cmd(void) qobject_unref(req); } +static void test_dispatch_cmd_oob(void) +{ +QDict *req = qdict_new(); +QDict *resp; + +qdict_put_str(req, "exec-oob", "test-flags-command"); + +resp = qmp_dispatch(&qmp_commands, QOBJECT(req), true); +assert(resp != NULL); +assert(!qdict_haskey(resp, "error")); + +qobject_unref(resp); +qobject_unref(req); +} + /* test commands that return an error due to invalid parameters */ static void test_dispatch_cmd_failure(void) { @@ -302,6 +317,7 @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd); +g_test_add_func("/qmp/dispatch_cmd_oob", test_dispatch_cmd_oob); g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure); g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io); g_test_add_func("/qmp/dispatch_cmd_success_response", -- 2.17.1
[Qemu-devel] [PATCH v9 4/6] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a. Meanwhile, revert one line from fa198ad9bdef to make sure qtest_init_without_qmp_handshake() will only pass in one parameter. Reviewed-by: Markus Armbruster Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu --- tests/libqtest.c | 10 -- tests/libqtest.h | 4 +--- tests/qmp-test.c | 4 ++-- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/tests/libqtest.c b/tests/libqtest.c index 8bf9fd1ae3..e0bb032c5e 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -191,8 +191,7 @@ static const char *qtest_qemu_binary(void) return qemu_bin; } -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args) +QTestState *qtest_init_without_qmp_handshake(const char *extra_args) { QTestState *s; int sock, qmpsock, i; @@ -225,13 +224,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, command = g_strdup_printf("exec %s " "-qtest unix:%s,nowait " "-qtest-log %s " - "-chardev socket,path=%s,nowait,id=char0 " - "-mon chardev=char0,mode=control%s " + "-qmp unix:%s,nowait " "-machine accel=qtest " "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, "", + qmp_socket_path, extra_args ?: ""); execlp("/bin/sh", "sh", "-c", command, NULL); exit(1); @@ -266,7 +264,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, QTestState *qtest_init(const char *extra_args) { -QTestState *s = qtest_init_without_qmp_handshake(false, extra_args); +QTestState *s = qtest_init_without_qmp_handshake(extra_args); QDict *greeting; /* Read the QMP greeting and then do the handshake */ diff --git a/tests/libqtest.h b/tests/libqtest.h index ed88ff99d5..96a6c01352 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -55,14 +55,12 @@ QTestState *qtest_init(const char *extra_args); /** * qtest_init_without_qmp_handshake: - * @use_oob: true to have the server advertise OOB support * @extra_args: other arguments to pass to QEMU. CAUTION: these * arguments are subject to word splitting and shell evaluation. * * Returns: #QTestState instance. */ -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args); +QTestState *qtest_init_without_qmp_handshake(const char *extra_args); /** * qtest_quit: diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 030d813f77..cc9907b525 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -108,7 +108,7 @@ static void test_qmp_protocol(void) QList *capabilities; QTestState *qts; -qts = qtest_init_without_qmp_handshake(false, common_args); +qts = qtest_init_without_qmp_handshake(common_args); /* Test greeting */ resp = qtest_qmp_receive(qts); @@ -219,7 +219,7 @@ static void test_qmp_oob(void) QList *capabilities; QString *qstr; -qts = qtest_init_without_qmp_handshake(true, common_args); +qts = qtest_init_without_qmp_handshake(common_args); /* Check the greeting message. */ resp = qtest_qmp_receive(qts); -- 2.17.1
[Qemu-devel] [PATCH v9 3/6] monitor: remove "x-oob", turn oob on by default
OOB commands were introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to disable them by default for 2.12 (commit be933ffc23). http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). Time to re-enable OOB. This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), and turns it on again for non-MUX QMPs. Note that we can't enable Out-Of-Band for monitors with MUX-typed chardev backends, because not all the chardev frontends can run without main thread, or can run in multiple threads. Some trivial touch-up in the test code is required to make sure qmp-test won't break. Reviewed-by: Markus Armbruster Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu --- include/monitor/monitor.h | 1 - monitor.c | 22 ++ tests/libqtest.c | 2 +- tests/qmp-test.c | 2 +- vl.c | 5 - 5 files changed, 8 insertions(+), 24 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 0c0a37d8cb..c1b40a9cac 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -13,7 +13,6 @@ extern __thread Monitor *cur_mon; #define MONITOR_USE_READLINE 0x02 #define MONITOR_USE_CONTROL 0x04 #define MONITOR_USE_PRETTY0x08 -#define MONITOR_USE_OOB 0x10 #define QMP_REQ_QUEUE_LEN_MAX 8 diff --git a/monitor.c b/monitor.c index f5911399d8..65a5e5f41a 100644 --- a/monitor.c +++ b/monitor.c @@ -4545,19 +4545,12 @@ void monitor_init(Chardev *chr, int flags) { Monitor *mon = g_malloc(sizeof(*mon)); bool use_readline = flags & MONITOR_USE_READLINE; -bool use_oob = flags & MONITOR_USE_OOB; - -if (use_oob) { -if (CHARDEV_IS_MUX(chr)) { -error_report("Monitor out-of-band is not supported with " - "MUX typed chardev backend"); -exit(1); -} -if (use_readline) { -error_report("Monitor out-of-band is only supported by QMP"); -exit(1); -} -} +/* + * Note: we can't enable Out-Of-Band for monitors with MUX-typed + * chardev backends, because not all the chardev frontends can run + * without main thread, or can run in multiple threads. + */ +bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr); monitor_data_init(mon, false, use_oob); @@ -4647,9 +4640,6 @@ QemuOptsList qemu_mon_opts = { },{ .name = "pretty", .type = QEMU_OPT_BOOL, -},{ -.name = "x-oob", -.type = QEMU_OPT_BOOL, }, { /* end of list */ } }, diff --git a/tests/libqtest.c b/tests/libqtest.c index 2cd5736642..8bf9fd1ae3 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -231,7 +231,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, use_oob ? ",x-oob=on" : "", + qmp_socket_path, "", extra_args ?: ""); execlp("/bin/sh", "sh", "-c", command, NULL); exit(1); diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 6c419f6023..030d813f77 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -116,7 +116,7 @@ static void test_qmp_protocol(void) g_assert(q); test_version(qdict_get(q, "version")); capabilities = qdict_get_qlist(q, "capabilities"); -g_assert(capabilities && qlist_empty(capabilities)); +g_assert(capabilities); qobject_unref(resp); /* Test valid command before handshake */ diff --git a/vl.c b/vl.c index 4e25c78bff..db766f5295 100644 --- a/vl.c +++ b/vl.c @@ -2282,11 +2282,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) if (qemu_opt_get_bool(opts, "pretty", 0)) flags |= MONITOR_USE_PRETTY; -/* OOB is off by default */ -if (qemu_opt_get_bool(opts, "x-oob", 0)) { -flags |= MONITOR_USE_OOB; -} - chardev = qemu_opt_get(opts, "chardev"); chr = qemu_chr_find(chardev); if (chr == NULL) { -- 2.17.1
[Qemu-devel] [PATCH v9 1/6] monitor: Suspend monitor instead dropping commands
When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal: * QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea. * With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem. * With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands. We currently DROP. Switch to SUSPEND. Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again. Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever. And here since we need to make sure both: (1) popping request from the req queue, and (2) reading length of the req queue will be in the same critical section, we let the pop function take the corresponding queue lock when there is a request, then we release the lock from the caller. Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu --- docs/interop/qmp-spec.txt | 5 ++-- include/monitor/monitor.h | 2 ++ monitor.c | 52 +-- qapi/misc.json| 40 -- 4 files changed, 28 insertions(+), 71 deletions(-) diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt index 8f7da0245d..67e44a8120 100644 --- a/docs/interop/qmp-spec.txt +++ b/docs/interop/qmp-spec.txt @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands is recommended for clients that accept capability "oob". If the client sends in-band commands faster than the server can -execute them, the server will eventually drop commands to limit the -queue length. The sever sends event COMMAND_DROPPED then. +execute them, the server will stop reading the requests from the QMP +channel until the request queue length is reduced to an acceptable +range. Only a few commands support out-of-band execution. The ones that do have "allow-oob": true in output of query-qmp-schema. diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 6fd2c53b09..0c0a37d8cb 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -15,6 +15,8 @@ extern __thread Monitor *cur_mon; #define MONITOR_USE_PRETTY0x08 #define MONITOR_USE_OOB 0x10 +#define QMP_REQ_QUEUE_LEN_MAX 8 + bool monitor_cur_is_qmp(void); void monitor_init_globals(void); diff --git a/monitor.c b/monitor.c index b9258a7438..1f83775fff 100644 --- a/monitor.c +++ b/monitor.c @@ -4097,8 +4097,12 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) * processing commands only on a very busy monitor. To achieve that, * when we process one request on a specific monitor, we put that * monitor to the end of mon_list queue. + * + * Note: if the function returned with non-NULL, then the caller will + * be with mon->qmp.qmp_queue_lock held, and the caller is responsible + * to release it. */ -static QMPRequest *monitor_qmp_requests_pop_any(void) +static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void) { QMPRequest *req_obj = NULL; Monitor *mon; @@ -4108,10 +4112,11 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) QTAILQ_FOREACH(mon, &mon_list, entry) { qemu_mutex_lock(&mon->qmp.qmp_queue_lock); req_obj = g_queue_pop_head(mon->qmp.qmp_requests); -qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); if (req_obj) { +/* With the lock of corresponding queue held */ break; } +qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); } if (req_obj) { @@ -4130,30 +4135,34 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) static void monitor_qmp_bh_dispatcher(void *data) { -QMPRequest *req_obj = monitor_qmp_requests_pop_any(); +QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock(); QDict *rsp; bool need_resume; +Monitor *mon; if (!req_obj) { return; } +mon = req_obj->mon; /* qmp_oob_enabled() might change after "qmp_capabilities" */ -need_resume = !qmp_oob_enabled(req_obj->mon); +need_resume = !qmp_oob_enabled(mon) || +mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; +qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); if (req_obj->req) { trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj-
Re: [Qemu-devel] [RFC PATCH 00/21] Trace updates and plugin RFC
> From: Alex Bennée [mailto:alex.ben...@linaro.org] > Any serious analysis tool should allow for us to track all memory > accesses so I think the guest_mem_before trace point should probably > be split into guest_mem_before_store and guest_mem_after_load. We > could go the whole hog and add potential trace points for start/end of > all memory operations. I wanted to ask about memory tracing and found this one. Is it possible to use tracepoints for capturing all memory accesses? In our implementation we insert helpers before and after tcg read/write operations. Pavel Dovgalyuk
[Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed
Currently when QMP request queue full we won't resume the monitor until we have completely handled the current command. It's not necessary since even before it's handled the queue is already non-full. Moving the resume logic earlier before the command execution. Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and it's even possible (as pointed out by Marc-André) that the function itself may try to take the monitor lock again, so let's do the resume after the monitor lock is released to avoid possible dead lock. Signed-off-by: Peter Xu --- monitor.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/monitor.c b/monitor.c index 1f83775fff..f5911399d8 100644 --- a/monitor.c +++ b/monitor.c @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data) need_resume = !qmp_oob_enabled(mon) || mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); + +if (need_resume) { +/* Pairs with the monitor_suspend() in handle_qmp_command() */ +monitor_resume(mon); +} + if (req_obj->req) { trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); monitor_qmp_dispatch(mon, req_obj->req, req_obj->id); @@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data) qobject_unref(rsp); } -if (need_resume) { -/* Pairs with the monitor_suspend() in handle_qmp_command() */ -monitor_resume(mon); -} qmp_request_free(req_obj); /* Reschedule instead of looping so the main loop stays responsive */ -- 2.17.1
[Qemu-devel] [PATCH v9 0/6] monitor: enable OOB by default
Based-on: <20180828191048.29806-1-arm...@redhat.com> Based-on: <2018090716.1675-1-arm...@redhat.com> (this series is based on Markus's monitor-next tree) v9: - add r-bs - release the qmp queue lock before resume [Marc-Andre] v8: - remove patch 1 & 2 since already in the QAPI pull - squash patch 3 & 4, use Markus's version of commit message (with some of my additions), make sure popping and reading queue length is in the same critical section [Markus] - add one patch to cover test for queue full [Markus] - add one patch to resume the monitor earlier when queue not full [Markus] v7: - use Markus's commit message for patch "qapi: Drop qapi_event_send_FOO()'s Error ** argument" [Markus] - update commit message for "qapi: remove COMMAND_DROPPED event" since QEMU 3.0 is released [Eric/Dave] - rebase to Markus's monitor-next tree: http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next the patch "monitor: suspend monitor instead of send CMD_DROP" re-written since people prefer to drop need_resume flag so now I hand-made it. Dropped a few patches since not appliable any more. Please review. Thanks, Peter Xu (6): monitor: Suspend monitor instead dropping commands monitor: resume the monitor earlier if needed monitor: remove "x-oob", turn oob on by default Revert "tests: Add parameter to qtest_init_without_qmp_handshake" tests: add oob functional test for test-qmp-cmds tests: qmp-test: add queue full test docs/interop/qmp-spec.txt | 5 ++- include/monitor/monitor.h | 3 +- monitor.c | 82 --- qapi/misc.json| 40 --- tests/libqtest.c | 10 ++--- tests/libqtest.h | 4 +- tests/qmp-test.c | 32 +-- tests/test-qmp-cmds.c | 16 vl.c | 5 --- 9 files changed, 89 insertions(+), 108 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v2] ssi-sd: Make devices picking up backends unavailable with -device
On 2018-10-09 08:08, Markus Armbruster wrote: > Device models aren't supposed to go on fishing expeditions for > backends. They should expose suitable properties for the user to set. > For onboard devices, board code sets them. > > Device ssi-sd picks up its block backend in its init() method with > drive_get_next() instead. This mistake is already marked FIXME since > commit af9e40a. > > Unset user_creatable to remove the mistake from our external > interface. Since the SSI bus doesn't support hotplug, only -device > can be affected. Only certain ARM machines have ssi-sd and provide an > SSI bus for it; this patch breaks -device ssi-sd for these machines. > No actual use of -device ssi-sd is known. > > Signed-off-by: Markus Armbruster > Acked-by: Philippe Mathieu-Daudé > --- > v2: > * Rebase to master > * Improve commit message > > hw/sd/ssi-sd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c > index 95a143bfba..623d0333e8 100644 > --- a/hw/sd/ssi-sd.c > +++ b/hw/sd/ssi-sd.c > @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void > *data) > k->cs_polarity = SSI_CS_LOW; > dc->vmsd = &vmstate_ssi_sd; > dc->reset = ssi_sd_reset; > +/* Reason: init() method uses drive_get_next() */ > +dc->user_creatable = false; > } > > static const TypeInfo ssi_sd_info = { > Acked-by: Thomas Huth
Re: [Qemu-devel] [PATCH 06/31] char: Use error_printf() to print help and such
On 08/10/2018 19:31, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. Convert a few that are actually help and such to > error_printf(). > > Improves output of -chardev help from > > qemu-system-x86_64: -chardev help: Available chardev backend types: > serial > ... > > to > > Available chardev backend types: > serial > ... > > Cc: Paolo Bonzini > Cc: "Marc-André Lureau" > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > chardev/char-pty.c | 2 +- > chardev/char.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c > index e8d9a53476..f681d637c1 100644 > --- a/chardev/char-pty.c > +++ b/chardev/char-pty.c > @@ -259,7 +259,7 @@ static void char_pty_open(Chardev *chr, > qemu_set_nonblock(master_fd); > > chr->filename = g_strdup_printf("pty:%s", pty_name); > -error_report("char device redirected to %s (label %s)", > +error_printf("char device redirected to %s (label %s)\n", > pty_name, chr->label); > > s = PTY_CHARDEV(chr); > diff --git a/chardev/char.c b/chardev/char.c > index e115166995..7f07a1bfbd 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -634,7 +634,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error > **errp) > > chardev_name_foreach(help_string_append, str); > > -error_report("Available chardev backend types: %s", str->str); > +error_printf("Available chardev backend types: %s\n", str->str); > g_string_free(str, true); > return NULL; > } >
[Qemu-devel] [PATCH v2] ssi-sd: Make devices picking up backends unavailable with -device
Device models aren't supposed to go on fishing expeditions for backends. They should expose suitable properties for the user to set. For onboard devices, board code sets them. Device ssi-sd picks up its block backend in its init() method with drive_get_next() instead. This mistake is already marked FIXME since commit af9e40a. Unset user_creatable to remove the mistake from our external interface. Since the SSI bus doesn't support hotplug, only -device can be affected. Only certain ARM machines have ssi-sd and provide an SSI bus for it; this patch breaks -device ssi-sd for these machines. No actual use of -device ssi-sd is known. Signed-off-by: Markus Armbruster Acked-by: Philippe Mathieu-Daudé --- v2: * Rebase to master * Improve commit message hw/sd/ssi-sd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 95a143bfba..623d0333e8 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data) k->cs_polarity = SSI_CS_LOW; dc->vmsd = &vmstate_ssi_sd; dc->reset = ssi_sd_reset; +/* Reason: init() method uses drive_get_next() */ +dc->user_creatable = false; } static const TypeInfo ssi_sd_info = { -- 2.17.1
Re: [Qemu-devel] [PATCH 19/31] vl: Clean up error reporting in parse_add_fd()
On 08/10/2018 19:31, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. chardev_init_func() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > vl.c | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index b8576f8f10..ecb70f87d8 100644 > --- a/vl.c > +++ b/vl.c > @@ -2239,7 +2239,7 @@ static int chardev_init_func(void *opaque, QemuOpts > *opts, Error **errp) > > if (!qemu_chr_new_from_opts(opts, &local_err)) { > if (local_err) { > -error_report_err(local_err); > +error_propagate(errp, local_err); > return -1; > } > exit(0); > @@ -4185,10 +4185,8 @@ int main(int argc, char **argv, char **envp) >user_creatable_add_opts_foreach, >object_create_initial, &error_fatal); > > -if (qemu_opts_foreach(qemu_find_opts("chardev"), > - chardev_init_func, NULL, NULL)) { > -exit(1); > -} > +qemu_opts_foreach(qemu_find_opts("chardev"), > + chardev_init_func, NULL, &error_fatal); > > #ifdef CONFIG_VIRTFS > if (qemu_opts_foreach(qemu_find_opts("fsdev"), >
Re: [Qemu-devel] [PATCH 31/31] vl: Simplify call of parse_name()
On 08/10/2018 19:31, Markus Armbruster wrote: > main() checks for parse_name() failure even though it can't actually > fail. That's okay. Simplify it to check by passing &error_fatal, > like the other users of qemu_opts_foreach(). > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > vl.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/vl.c b/vl.c > index 101e0123d9..0a76c58943 100644 > --- a/vl.c > +++ b/vl.c > @@ -3926,10 +3926,8 @@ int main(int argc, char **argv, char **envp) > } > #endif > > -if (qemu_opts_foreach(qemu_find_opts("name"), > - parse_name, NULL, NULL)) { > -exit(1); > -} > +qemu_opts_foreach(qemu_find_opts("name"), > + parse_name, NULL, &error_fatal); > > #ifndef _WIN32 > qemu_opts_foreach(qemu_find_opts("add-fd"), >
Re: [Qemu-devel] [PATCH 23/31] vl: Clean up error reporting in device_init_func()
On 08/10/2018 19:31, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. device_init_func() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > vl.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index a3a39ec06b..86eee4c798 100644 > --- a/vl.c > +++ b/vl.c > @@ -,12 +,10 @@ static int device_help_func(void *opaque, QemuOpts > *opts, Error **errp) > > static int device_init_func(void *opaque, QemuOpts *opts, Error **errp) > { > -Error *err = NULL; > DeviceState *dev; > > -dev = qdev_device_add(opts, &err); > +dev = qdev_device_add(opts, errp); > if (!dev) { > -error_report_err(err); > return -1; > } > object_unref(OBJECT(dev)); > @@ -,10 +4442,8 @@ int main(int argc, char **argv, char **envp) > > /* init generic devices */ > rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); > -if (qemu_opts_foreach(qemu_find_opts("device"), > - device_init_func, NULL, NULL)) { > -exit(1); > -} > +qemu_opts_foreach(qemu_find_opts("device"), > + device_init_func, NULL, &error_fatal); > > cpu_synchronize_all_post_init(); > >
Re: [Qemu-devel] [PATCH 20/31] vl: Clean up error reporting in machine_set_property()
On 08/10/2018 19:31, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. machine_set_property() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > vl.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/vl.c b/vl.c > index ecb70f87d8..3adc9dbe4f 100644 > --- a/vl.c > +++ b/vl.c > @@ -2676,7 +2676,7 @@ static int machine_set_property(void *opaque, > g_free(qom_name); > > if (local_err) { > -error_report_err(local_err); > +error_propagate(errp, local_err); > return -1; > } > > @@ -4201,11 +4201,8 @@ int main(int argc, char **argv, char **envp) > } > > machine_opts = qemu_get_machine_opts(); > -if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine, > - NULL)) { > -object_unref(OBJECT(current_machine)); > -exit(1); > -} > +qemu_opt_foreach(machine_opts, machine_set_property, current_machine, > + &error_fatal); > > configure_accelerator(current_machine); > >
Re: [Qemu-devel] [PATCH 21/31] vl: Clean up error reporting in mon_init_func()
On 08/10/2018 19:31, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. mon_init_func() does that, and then fails without > setting an error. Its caller main(), via qemu_opts_foreach(), is fine > with it, but clean it up anyway. > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > vl.c | 14 ++ > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/vl.c b/vl.c > index 3adc9dbe4f..1009d708a0 100644 > --- a/vl.c > +++ b/vl.c > @@ -2270,8 +2270,8 @@ static int mon_init_func(void *opaque, QemuOpts *opts, > Error **errp) > } else if (strcmp(mode, "control") == 0) { > flags = MONITOR_USE_CONTROL; > } else { > -error_report("unknown monitor mode \"%s\"", mode); > -exit(1); > +error_setg(errp, "unknown monitor mode \"%s\"", mode); > +return -1; > } > > if (qemu_opt_get_bool(opts, "pretty", 0)) > @@ -2285,8 +2285,8 @@ static int mon_init_func(void *opaque, QemuOpts *opts, > Error **errp) > chardev = qemu_opt_get(opts, "chardev"); > chr = qemu_chr_find(chardev); > if (chr == NULL) { > -error_report("chardev \"%s\" not found", chardev); > -exit(1); > +error_setg(errp, "chardev \"%s\" not found", chardev); > +return -1; > } > > monitor_init(chr, flags); > @@ -4365,10 +4365,8 @@ int main(int argc, char **argv, char **envp) > default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); > default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS); > > -if (qemu_opts_foreach(qemu_find_opts("mon"), > - mon_init_func, NULL, NULL)) { > -exit(1); > -} > +qemu_opts_foreach(qemu_find_opts("mon"), > + mon_init_func, NULL, &error_fatal); > > if (foreach_device_config(DEV_SERIAL, serial_parse) < 0) > exit(1); >
Re: [Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
Hello Philippe, Philippe Mathieu-Daudé 于2018年10月9日周二 下午1:52写道: > Hi Li, > > On 09/10/2018 04:39, Li Qiang wrote: > > It makes sense to print the error message while reading > > file failed. > > OK > > > > > Change since v1: > > free error > > Changes are useful for reviewer, but not in the git history. > You can have them automatically stripped if you place them below the > next '---' separator. > > Thanks for your advice. > Hopefully the maintainer taking this patch can clean this up. > > > > > Signed-off-by: Li Qiang > > Reviewed-by: Philippe Mathieu-Daudé > > --- > > Here goes comment not worth to stay forever in git history: > > Since v1: ... > > > vl.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 4e25c78..69fc77c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts > *opts, Error **errp) > > size = strlen(str); /* NUL terminator NOT included in fw_cfg > blob */ > > buf = g_memdup(str, size); > > } else { > > -if (!g_file_get_contents(file, &buf, &size, NULL)) { > > -error_report("can't load %s", file); > > +GError *error = NULL; > > +if (!g_file_get_contents(file, &buf, &size, &error)) { > > +error_report("can't load %s, %s", file, error->message); > > If you ever respin, you can help Markus [1] effort using: > > error_setg(errp, "can't load %s, %s", file, error->message); > > Else your patch will clash with [2]. > OK, I will send out this patch based Markus' tree or later when his patch goes to upstream. Thanks, Li Qiang > > > +g_error_free(error); > > return -1; > > } > > } > > > > Regards, > > Phil. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01394.html > [2] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01406.html >
[Qemu-devel] [PATCH] i386: pc_sysfw: load bios using rom API
As the bios is a ROM, just using rom API. AFAICS no functionality changed. Signed-off-by: Li Qiang --- hw/i386/pc_sysfw.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 091e22dd60..7f469fd582 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -209,9 +209,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) goto bios_error; } bios = g_malloc(sizeof(*bios)); -memory_region_init_ram(bios, NULL, "pc.bios", bios_size, &error_fatal); -if (!isapc_ram_fw) { -memory_region_set_readonly(bios, true); +memory_region_init_rom(bios, NULL, "pc.bios", bios_size, &error_fatal); +if (isapc_ram_fw) { +memory_region_set_readonly(bios, false); } ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1); if (ret != 0) { @@ -230,8 +230,8 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) 0x10 - isa_bios_size, isa_bios, 1); -if (!isapc_ram_fw) { -memory_region_set_readonly(isa_bios, true); +if (isapc_ram_fw) { +memory_region_set_readonly(isa_bios, false); } /* map all the bios at the top of memory */ -- 2.11.0
Re: [Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
Hi Li, On 09/10/2018 04:39, Li Qiang wrote: > It makes sense to print the error message while reading > file failed. OK > > Change since v1: > free error Changes are useful for reviewer, but not in the git history. You can have them automatically stripped if you place them below the next '---' separator. Hopefully the maintainer taking this patch can clean this up. > > Signed-off-by: Li Qiang > Reviewed-by: Philippe Mathieu-Daudé > --- Here goes comment not worth to stay forever in git history: Since v1: ... > vl.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/vl.c b/vl.c > index 4e25c78..69fc77c 100644 > --- a/vl.c > +++ b/vl.c > @@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, > Error **errp) > size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ > buf = g_memdup(str, size); > } else { > -if (!g_file_get_contents(file, &buf, &size, NULL)) { > -error_report("can't load %s", file); > +GError *error = NULL; > +if (!g_file_get_contents(file, &buf, &size, &error)) { > +error_report("can't load %s, %s", file, error->message); If you ever respin, you can help Markus [1] effort using: error_setg(errp, "can't load %s, %s", file, error->message); Else your patch will clash with [2]. > +g_error_free(error); > return -1; > } > } > Regards, Phil. [1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01394.html [2] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01406.html
[Qemu-devel] [PATCH v2 3/3] Travis support for the acceptance tests
This enables the execution of the acceptance tests on Travis. Because the Travis environment is based on Ubuntu Trusty, it requires the python3-pip. Note: while another supposedely required component on newer versions (such as on Bionic) split the Python 3 installation further on the python3-venv package. Signed-off-by: Cleber Rosa --- .travis.yml | 6 ++ 1 file changed, 6 insertions(+) diff --git a/.travis.yml b/.travis.yml index 95be6ec59f..db1a31ea51 100644 --- a/.travis.yml +++ b/.travis.yml @@ -36,6 +36,7 @@ addons: - liburcu-dev - libusb-1.0-0-dev - libvte-2.90-dev + - python3-pip - sparse - uuid-dev - gcovr @@ -117,6 +118,11 @@ matrix: - env: CONFIG="--target-list=x86_64-softmmu" python: - "3.6" +# Acceptance (Functional) tests +- env: CONFIG="--python=/usr/bin/python3 --target-list=x86_64-softmmu" + TEST_CMD="make check-acceptance" + python: +- "3.6" # Using newer GCC with sanitizers - addons: apt: -- 2.17.1
[Qemu-devel] [PATCH v2 2/3] Acceptance tests: add make rule for running them
The acceptance (aka functional, aka Avocado-based) tests are Python files located in "tests/acceptance" that need to be run with the Avocado libs and test runner. Let's provide a convenient way for QEMU developers to run them, by making use of the tests-venv with the required setup. Also, while the Avocado test runner will take care of creating a location to save test results to, it was understood that it's better if the results are kept within the build tree. Signed-off-by: Cleber Rosa --- docs/devel/testing.rst | 35 ++- tests/Makefile.include | 17 +++-- tests/venv-requirements.txt | 1 + 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 727c4019b5..b992a2961d 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -545,10 +545,31 @@ Tests based on ``avocado_qemu.Test`` can easily: - http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test - http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html -Installation - +Running tests +- -To install Avocado and its dependencies, run: +You can run the acceptance tests simply by executing: + +.. code:: + + make check-acceptance + +This involves the automatic creation of Python virtual environment +within the build tree (at ``tests/venv``) which will have all the +right dependencies, and will save tests results also within the +build tree (at ``tests/results``). + +Note: the build environment must be using a Python 3 stack, and have +the ``venv`` and ``pip`` packages installed. If necessary, make sure +``configure`` is called with ``--python=`` and that those modules are +available. On Debian and Ubuntu based systems, depending on the +specific version, they may be on packages named ``python3-venv`` and +``python3-pip``. + +Manual Installation +--- + +To manually install Avocado and its dependencies, run: .. code:: @@ -689,11 +710,15 @@ The exact QEMU binary to be used on QEMUMachine. Uninstalling Avocado -If you've followed the installation instructions above, you can easily -uninstall Avocado. Start by listing the packages you have installed:: +If you've followed the manual installation instructions above, you can +easily uninstall Avocado. Start by listing the packages you have +installed:: pip list --user And remove any package you want with:: pip uninstall + +If you've used ``make check-acceptance``, the Python virtual environment where +Avocado is installed will be cleaned up as part of ``make check-clean``. diff --git a/tests/Makefile.include b/tests/Makefile.include index 68af79927d..00fdf9913e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -11,6 +11,7 @@ check-help: @echo " $(MAKE) check-qapi-schemaRun QAPI schema tests" @echo " $(MAKE) check-block Run block tests" @echo " $(MAKE) check-tcgRun TCG tests" + @echo " $(MAKE) check-acceptance Run all acceptance (functional) tests" @echo " $(MAKE) check-report.htmlGenerates an HTML test report" @echo " $(MAKE) check-venv Creates a Python venv for tests" @echo " $(MAKE) check-clean Clean the tests" @@ -1018,10 +1019,11 @@ check-decodetree: # Python venv for running tests -.PHONY: check-venv +.PHONY: check-venv check-acceptance TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv TESTS_VENV_REQ=$(SRC_PATH)/tests/venv-requirements.txt +TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results $(TESTS_VENV_DIR): $(call quiet-command, \ @@ -1031,8 +1033,19 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_DIR)/bin/pip -q install -r $(TESTS_VENV_REQ), \ PIP, $(TESTS_VENV_REQ)) +$(TESTS_RESULTS_DIR): + $(call quiet-command, mkdir -p $@, \ +MKDIR, $@) + check-venv: $(TESTS_VENV_DIR) +check-acceptance: check-venv $(TESTS_RESULTS_DIR) + $(call quiet-command, \ +$(TESTS_VENV_DIR)/bin/python -m avocado \ +--show=none run --job-results-dir=$(TESTS_RESULTS_DIR) --failfast=on \ +$(SRC_PATH)/tests/acceptance, \ +"AVOCADO", "tests/acceptance") + # Consolidated targets .PHONY: check-qapi-schema check-qtest check-unit check check-clean @@ -1046,7 +1059,7 @@ check-clean: rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y) rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y)) rm -f tests/test-qapi-gen-timestamp - rm -rf $(TESTS_VENV_DIR) + rm -rf $(TESTS_VENV_DIR) $(TESTS_RESULTS_DIR) clean: check-clean diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt index d39f9d1576..64c6e27a94 100644 --- a/tests/venv-requirements.txt +++ b/tests/venv-requirements.txt @@ -1,3 +1,4 @@ # Add Python module requirements, one pe
[Qemu-devel] [PATCH v2 0/3] Bootstrap Python venv and acceptance/functional tests
TL;DR = Allow acceptance tests to be run with `make check-acceptance`. Details === This introduces a Python virtual environment that will be setup within the QEMU build directory, that will contain the exact environment that tests may require. There's one current caveat: it requires Python 3, as it's based on the venv module. This was based on some discussions and perception about standardizing on Python 3, but can easily be made to accommodate Python 2 as well. Changes from v1: * TESTS_VENV_REQ (the path of "venv-requirements.txt") now points to the source path ($SRC_PATH instead of $BUILD_DIR) * Create the venv with "--system-site-packages", which allows the reuse of packages (and no additional downloads) in case there's a package installed system wide providing the same package and version. * Run Avocado with "python -m avocado". It may have been installed reusing the system wide packages, and then the script may not be available on the venv. * Improved documentation describing the Python 3, venv and pip requirements. * Updated avocado-framework requirement to latest released version (65.0) * (New commit) Added support for running the acceptance tests on Travis. Ideas discussed, but not implemented: * Install external packages such as python3-pip on Debian based systems, deemed too invasive on developer's systems. * Allow the use of Python 2, and consequently the "virtualenv" module. Cleber Rosa (3): Bootstrap Python venv for tests Acceptance tests: add make rule for running them Travis support for the acceptance tests .travis.yml | 6 ++ docs/devel/testing.rst | 35 ++- tests/Makefile.include | 32 tests/venv-requirements.txt | 4 4 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 tests/venv-requirements.txt -- 2.17.1
[Qemu-devel] [PATCH v2 1/3] Bootstrap Python venv for tests
A number of QEMU tests are written in Python, and may benefit from an untainted Python venv. By using make rules, tests that depend on specific Python libs can set that rule as a requiment, along with rules that require the presence or installation of specific libraries. The tests/venv-requirements.txt is supposed to contain the Python requirements that should be added to the venv created by check-venv. Signed-off-by: Cleber Rosa --- tests/Makefile.include | 19 +++ tests/venv-requirements.txt | 3 +++ 2 files changed, 22 insertions(+) create mode 100644 tests/venv-requirements.txt diff --git a/tests/Makefile.include b/tests/Makefile.include index 7a3059bf6c..68af79927d 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -12,6 +12,7 @@ check-help: @echo " $(MAKE) check-block Run block tests" @echo " $(MAKE) check-tcgRun TCG tests" @echo " $(MAKE) check-report.htmlGenerates an HTML test report" + @echo " $(MAKE) check-venv Creates a Python venv for tests" @echo " $(MAKE) check-clean Clean the tests" @echo @echo "Please note that HTML reports do not regenerate if the unit tests" @@ -1015,6 +1016,23 @@ check-decodetree: ./check.sh "$(PYTHON)" "$(SRC_PATH)/scripts/decodetree.py", \ TEST, decodetree.py) +# Python venv for running tests + +.PHONY: check-venv + +TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv +TESTS_VENV_REQ=$(SRC_PATH)/tests/venv-requirements.txt + +$(TESTS_VENV_DIR): + $(call quiet-command, \ +$(PYTHON) -m venv --system-site-packages $@, \ +VENV, $@) + $(call quiet-command, \ +$(TESTS_VENV_DIR)/bin/pip -q install -r $(TESTS_VENV_REQ), \ +PIP, $(TESTS_VENV_REQ)) + +check-venv: $(TESTS_VENV_DIR) + # Consolidated targets .PHONY: check-qapi-schema check-qtest check-unit check check-clean @@ -1028,6 +1046,7 @@ check-clean: rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y) rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y)) rm -f tests/test-qapi-gen-timestamp + rm -rf $(TESTS_VENV_DIR) clean: check-clean diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt new file mode 100644 index 00..d39f9d1576 --- /dev/null +++ b/tests/venv-requirements.txt @@ -0,0 +1,3 @@ +# Add Python module requirements, one per line, to be installed +# in the tests/venv Python virtual environment. For more info, +# refer to: https://pip.pypa.io/en/stable/user_guide/#id1 -- 2.17.1
Re: [Qemu-devel] [PATCH 09/31] ioapic: Fix error handling in realize()
On Mon, Oct 08, 2018 at 07:31:03PM +0200, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. ioapic_realize() does that, and then exit()s. > Currently mostly harmless, as the device cannot be hot-plugged. > > Fixes: 20fd4b7b6d9282fe0cb83601f1821f31bd257458 > Cc: Peter Xu > Signed-off-by: Markus Armbruster Reviewed-by: Peter Xu -- Peter Xu
[Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
It makes sense to print the error message while reading file failed. Change since v1: free error Signed-off-by: Li Qiang Reviewed-by: Philippe Mathieu-Daud?? --- vl.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vl.c b/vl.c index 4e25c78..69fc77c 100644 --- a/vl.c +++ b/vl.c @@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ buf = g_memdup(str, size); } else { -if (!g_file_get_contents(file, &buf, &size, NULL)) { -error_report("can't load %s", file); +GError *error = NULL; +if (!g_file_get_contents(file, &buf, &size, &error)) { +error_report("can't load %s, %s", file, error->message); +g_error_free(error); return -1; } } -- 1.8.3.1
Re: [Qemu-devel] [PATCH] util: aio-posix: fix a typo
On Sun, 10/07 19:16, Li Qiang wrote: > Cc: qemu-triv...@nongnu.org > Signed-off-by: Li Qiang Reviewed-by: Fam Zheng
Re: [Qemu-devel] [PATCH 03/31] cpus hw target: Use warn_report() & friends to report warnings
On Mon, Oct 08, 2018 at 07:30:57PM +0200, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. Convert a few that are actually warnings to > warn_report(). > > While there, split a warning consisting of multiple sentences to > conform to conventions spelled out in warn_report()'s contract. > > Cc: Alex Bennée > Cc: Mark Cave-Ayland > Cc: Alex Williamson > Cc: Fam Zheng > Cc: Wei Huang > Cc: David Gibson > Signed-off-by: Markus Armbruster ppc part Acked-by: David Gibson > --- > cpus.c | 8 > hw/display/cg3.c| 2 +- > hw/display/tcx.c| 2 +- > hw/misc/ivshmem.c | 4 ++-- > hw/net/virtio-net.c | 8 > hw/virtio/virtio-pci.c | 4 ++-- > target/i386/cpu.c | 17 + > target/ppc/translate_init.inc.c | 4 ++-- > 8 files changed, 25 insertions(+), 24 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 361678e459..7804071872 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -211,12 +211,12 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp) > error_setg(errp, "No MTTCG when icount is enabled"); > } else { > #ifndef TARGET_SUPPORTS_MTTCG > -error_report("Guest not yet converted to MTTCG - " > - "you may get unexpected results"); > +warn_report("Guest not yet converted to MTTCG - " > +"you may get unexpected results"); > #endif > if (!check_tcg_memory_orders_compatible()) { > -error_report("Guest expects a stronger memory ordering " > - "than the host provides"); > +warn_report("Guest expects a stronger memory ordering " > +"than the host provides"); > error_printf("This may cause strange/hard to debug > errors\n"); > } > mttcg_enabled = true; > diff --git a/hw/display/cg3.c b/hw/display/cg3.c > index 1c199ab369..e50d97e48c 100644 > --- a/hw/display/cg3.c > +++ b/hw/display/cg3.c > @@ -307,7 +307,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) > ret = load_image_mr(fcode_filename, &s->rom); > g_free(fcode_filename); > if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) { > -error_report("cg3: could not load prom '%s'", CG3_ROM_FILE); > +warn_report("cg3: could not load prom '%s'", CG3_ROM_FILE); > } > } > > diff --git a/hw/display/tcx.c b/hw/display/tcx.c > index b2786ee8d0..66f2459226 100644 > --- a/hw/display/tcx.c > +++ b/hw/display/tcx.c > @@ -823,7 +823,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp) > ret = load_image_mr(fcode_filename, &s->rom); > g_free(fcode_filename); > if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) { > -error_report("tcx: could not load prom '%s'", TCX_ROM_FILE); > +warn_report("tcx: could not load prom '%s'", TCX_ROM_FILE); > } > } > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 6febbabcaa..4794518b2c 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -1292,8 +1292,8 @@ static void ivshmem_realize(PCIDevice *dev, Error > **errp) > IVShmemState *s = IVSHMEM_COMMON(dev); > > if (!qtest_enabled()) { > -error_report("ivshmem is deprecated, please use ivshmem-plain" > - " or ivshmem-doorbell instead"); > +warn_report("ivshmem is deprecated, please use ivshmem-plain" > +" or ivshmem-doorbell instead"); > } > > if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) { > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 4bdd5b8532..385b1a03e9 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -2020,10 +2020,10 @@ static void virtio_net_device_realize(DeviceState > *dev, Error **errp) > > if (n->net_conf.tx && strcmp(n->net_conf.tx, "timer") > && strcmp(n->net_conf.tx, "bh")) { > -error_report("virtio-net: " > - "Unknown option tx=%s, valid options: \"timer\" \"bh\"", > - n->net_conf.tx); > -error_report("Defaulting to \"bh\""); > +warn_report("virtio-net: " > +"Unknown option tx=%s, valid options: \"timer\" \"bh\"", > +n->net_conf.tx); > +error_printf("Defaulting to \"bh\""); > } > > n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n), > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 3a01fe90f0..a954799267 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1683,8 +1683,8 @@ static void virtio_pci_device_plugged(DeviceState *d, > Error **errp) > if (err) { > /* Noti
Re: [Qemu-devel] [PATCH 01/31] Use error_fatal to simplify obvious fatal errors (again)
On Mon, Oct 08, 2018 at 07:30:55PM +0200, Markus Armbruster wrote: > Add a slight improvement of the Coccinelle semantic patch from commit > 07d04a0219b, and use it to clean up. It leaves dead Error * variables > behind, cleaned up manually. > > Cc: David Gibson > Cc: Alexander Graf > Cc: Eric Blake > Cc: Paolo Bonzini > Signed-off-by: Markus Armbruster ppc part Acked-by: David Gibson > --- > hw/intc/xics_kvm.c | 7 +-- > qemu-nbd.c | 6 +- > scripts/coccinelle/use-error_fatal.cocci | 20 > vl.c | 7 +-- > 4 files changed, 23 insertions(+), 17 deletions(-) > create mode 100644 scripts/coccinelle/use-error_fatal.cocci > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index 30c3769a20..e8fa9a53ae 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -198,17 +198,12 @@ static void ics_get_kvm_state(ICSState *ics) > { > uint64_t state; > int i; > -Error *local_err = NULL; > > for (i = 0; i < ics->nr_irqs; i++) { > ICSIRQState *irq = &ics->irqs[i]; > > kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES, > - i + ics->offset, &state, false, &local_err); > -if (local_err) { > -error_report_err(local_err); > -exit(1); > -} > + i + ics->offset, &state, false, &error_fatal); > > irq->server = state & KVM_XICS_DESTINATION_MASK; > irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT) > diff --git a/qemu-nbd.c b/qemu-nbd.c > index e76fe3082a..7874bc973c 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -1002,11 +1002,7 @@ int main(int argc, char **argv) > } > > exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, > nbd_export_closed, > - writethrough, NULL, &local_err); > -if (!exp) { > -error_report_err(local_err); > -exit(EXIT_FAILURE); > -} > + writethrough, NULL, &error_fatal); > nbd_export_set_name(exp, export_name); > nbd_export_set_description(exp, export_description); > > diff --git a/scripts/coccinelle/use-error_fatal.cocci > b/scripts/coccinelle/use-error_fatal.cocci > new file mode 100644 > index 00..10fff0aec4 > --- /dev/null > +++ b/scripts/coccinelle/use-error_fatal.cocci > @@ -0,0 +1,20 @@ > +@@ > +type T; > +identifier FUN, RET; > +expression list ARGS; > +expression ERR, EC, FAIL; > +@@ > +( > +-T RET = FUN(ARGS, &ERR); > ++T RET = FUN(ARGS, &error_fatal); > +| > +-RET = FUN(ARGS, &ERR); > ++RET = FUN(ARGS, &error_fatal); > +| > +-FUN(ARGS, &ERR); > ++FUN(ARGS, &error_fatal); > +) > +-if (FAIL) { > +-error_report_err(ERR); > +-exit(EC); > +-} > diff --git a/vl.c b/vl.c > index a867c9c4d9..9d2b38a31f 100644 > --- a/vl.c > +++ b/vl.c > @@ -2002,15 +2002,10 @@ static void select_vgahw(const char *p) > > static void parse_display_qapi(const char *optarg) > { > -Error *err = NULL; > DisplayOptions *opts; > Visitor *v; > > -v = qobject_input_visitor_new_str(optarg, "type", &err); > -if (!v) { > -error_report_err(err); > -exit(1); > -} > +v = qobject_input_visitor_new_str(optarg, "type", &error_fatal); > > visit_type_DisplayOptions(v, NULL, &opts, &error_fatal); > QAPI_CLONE_MEMBERS(DisplayOptions, &dpy, opts); -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[Qemu-devel] [Bug 1795527] Re: Malformed audio and video output stuttering after upgrade to QEMU 3.0
UPDATE 2 (STUTTERING ELIMINATED, AUDIO ISSUES _STILL_ PRESENT) I think I've tracked down the source of the stuttering that affected my machine, and it doesn't seem to be QEMU-related. I'm going to write something about it here anyway, waiting to report it to other, more appropriate channels, hoping that it could help someone else out there. QUICK FIX: toggle the `kvm=off` flag off, perform a boot/shutdown cycle, toggle it back on, boot again. POSSIBLE EXPLANATION: During the week of panicking trial&error troubleshooting between the discovery of the audio issue and this report's filling, I let Windows perform an update. It downloaded and installed KB4100347. Now, by the looks of things this update naively checks the MSR for hipervisor signatures and, not finding any because of the KVM hidden state flag, it carries on with its duties of putting in place whatever microcode-powered mitigations for Spectre V2 it is instructed to set up. Being the CPU really a virtual one, the process, unsurprisingly, fails, and the OS is left with some sort of software gimmick which tries to compensate for those unsupported functions. To my surprise, it seems that the MSR check is implemented as a persistent trigger and, as soon as it detects an hypervisor signature, it permanently removes the mitigation routines, leaving a smoothly running VM. I've performed 3 reboots since I toggled the flag off and on again, and the performance are consistent. Obviously, this explanation is just a wild guess, but I find it believable enough and in line with my experience. As I stated in the title, though, *the audio corruption is still well present* under QEMU 3.0, and eliminating the stuttering didn't help, leaving me with the original and most important problem. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1795527 Title: Malformed audio and video output stuttering after upgrade to QEMU 3.0 Status in QEMU: New Bug description: My host is an x86_64 Arch Linux OS with a recompiled 4.18.10 hardened kernel, running a few KVM guests with varying OSes and configurations managed through a Libvirt stack. Among these guests I have two Windows 10 VMs with VGA passthrough and PulseAudio-backed virtual audio devices. After upgrading to QEMU 3.0.0, both of the Win10 guests started showing corrupted audio output in the form of unnatural reproduction speed and occasional but consistently misplaced audio fragments originating from what seems to be a circular buffer wrapping over itself (misbehaviour detected by starting some games with known OSTs and dialogues: soundtracks sound accelerated and past dialogue lines start replaying middle-sentence until the next line starts playing). In addition, the video output of the malfunctioning VMs regularly stutters roughly twice a second for a fraction of a second (sync'ed with the suspected buffer wrapping and especially pronounced during not-pre-rendered cutscenes), toghether with mouse freezes that look like actual input misses more than simple lack of screen refreshes. The issue was succesfully reproduced without the managing stack, directly with the following command line, on the most capable Windows guest: QEMU_AUDIO_DRV=pa QEMU_PA_SERVER=127.0.0.1 /usr/bin/qemu-system-x86_64 -name guest=win10_gms,debug-threads=on \ -machine pc-i440fx-3.0,accel=kvm,usb=off,vmport=off,dump-guest-core=off \ -cpu host,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff,hv_vendor_id=123456789abc,kvm=off \ -drive file=/usr/share/ovmf/x64/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/var/lib/libvirt/qemu/nvram/win10_gms_VARS.fd,if=pflash,format=raw,unit=1 \ -m 5120 \ -realtime mlock=off \ -smp 3,sockets=1,cores=3,threads=1 \ -uuid 39b56ee2-6bae-4009-9108-7be26d5d63ac \ -display none \ -no-user-config \ -nodefaults \ -rtc base=localtime,driftfix=slew \ -global kvm-pit.lost_tick_policy=delay \ -no-hpet \ -no-shutdown \ -global PIIX4_PM.disable_s3=1 \ -global PIIX4_PM.disable_s4=1 \ -boot strict=on \ -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \ -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \
[Qemu-devel] [RFC v2 5/5] cputlb: dynamically resize TLBs based on use rate
Perform the resizing only on flushes, otherwise we'd have to take a perf hit by either rehashing the array or unnecessarily flushing it. We grow the array aggressively, and reduce the size more slowly. This accommodates mixed workloads, where some processes might be memory-heavy while others are not. As the following experiments show, this a net perf gain, particularly for memory-heavy workloads. Experiments are run on an Intel i7-6700K CPU @ 4.00GHz. 1. System boot + shudown, debian aarch64: - Before (tb-lock-v3): Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7469.363393 task-clock (msec) #0.998 CPUs utilized ( +- 0.07% ) 31,507,707,190 cycles#4.218 GHz ( +- 0.07% ) 57,101,577,452 instructions #1.81 insns per cycle ( +- 0.08% ) 10,265,531,804 branches # 1374.352 M/sec ( +- 0.07% ) 173,020,681 branch-misses #1.69% of all branches ( +- 0.10% ) 7.483359063 seconds time elapsed ( +- 0.08% ) - After: Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7185.036730 task-clock (msec) #0.999 CPUs utilized ( +- 0.11% ) 30,303,501,143 cycles#4.218 GHz ( +- 0.11% ) 54,198,386,487 instructions #1.79 insns per cycle ( +- 0.08% ) 9,726,518,945 branches # 1353.719 M/sec ( +- 0.08% ) 167,082,307 branch-misses #1.72% of all branches ( +- 0.08% ) 7.195597842 seconds time elapsed ( +- 0.11% ) That is, a 3.8% improvement. 2. System boot + shutdown, ubuntu 18.04 x86_64: - Before (tb-lock-v3): Performance counter stats for 'taskset -c 0 ../img/x86_64/ubuntu-die.sh -nographic' (2 runs): 49971.036482 task-clock (msec) #0.999 CPUs utilized ( +- 1.62% ) 210,766,077,140 cycles#4.218 GHz ( +- 1.63% ) 428,829,830,790 instructions #2.03 insns per cycle ( +- 0.75% ) 77,313,384,038 branches # 1547.164 M/sec ( +- 0.54% ) 835,610,706 branch-misses #1.08% of all branches ( +- 2.97% ) 50.003855102 seconds time elapsed ( +- 1.61% ) - After: Performance counter stats for 'taskset -c 0 ../img/x86_64/ubuntu-die.sh -nographic' (2 runs): 50118.124477 task-clock (msec) #0.999 CPUs utilized ( +- 4.30% ) 132,396 context-switches #0.003 M/sec ( +- 1.20% ) 0 cpu-migrations#0.000 K/sec ( +-100.00% ) 167,754 page-faults #0.003 M/sec ( +- 0.06% ) 211,414,701,601 cycles#4.218 GHz ( +- 4.30% ) stalled-cycles-frontend stalled-cycles-backend 431,618,818,597 instructions #2.04 insns per cycle ( +- 6.40% ) 80,197,256,524 branches # 1600.165 M/sec ( +- 8.59% ) 794,830,352 branch-misses #0.99% of all branches ( +- 2.05% ) 50.177077175 seconds time elapsed ( +- 4.23% ) No improvement (within noise range). 3. x86_64 SPEC06int: SPEC06int (test set) [ Y axis: speedup over master ] 8 +-+--+++-+++++++-+++--+-+ | | | tlb-lock-v3 | 7 +-+..$$$...+indirection +-+ |$ $ +resizing | |$ $| 6 +-+..$.$..+-+ |$ $| |$ $| 5 +-+..$.$..+-+ |$ $| |$ $| 4 +-+..$.$..+-+ |$ $
[Qemu-devel] [RFC v2 1/5] tcg: Add tlb_index and tlb_entry helpers
From: Richard Henderson Isolate the computation of an index from an address into a helper before we change that function. Signed-off-by: Richard Henderson [ cota: convert tlb_vaddr_to_host; use atomic_read on addr_write ] Signed-off-by: Emilio G. Cota --- accel/tcg/softmmu_template.h | 72 include/exec/cpu_ldst.h | 19 +++-- include/exec/cpu_ldst_template.h | 25 +-- accel/tcg/cputlb.c | 61 --- 4 files changed, 92 insertions(+), 85 deletions(-) diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h index 1e50263871..aa34c692ee 100644 --- a/accel/tcg/softmmu_template.h +++ b/accel/tcg/softmmu_template.h @@ -111,9 +111,10 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi, uintptr_t retaddr) { -unsigned mmu_idx = get_mmuidx(oi); -int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); -target_ulong tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ; +uintptr_t mmu_idx = get_mmuidx(oi); +uintptr_t index = tlb_index(env, mmu_idx, addr); +CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); +target_ulong tlb_addr = entry->ADDR_READ; unsigned a_bits = get_alignment_bits(get_memop(oi)); uintptr_t haddr; DATA_TYPE res; @@ -129,7 +130,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE, mmu_idx, retaddr); } -tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ; +tlb_addr = entry->ADDR_READ; } /* Handle an IO access. */ @@ -166,7 +167,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, return res; } -haddr = addr + env->tlb_table[mmu_idx][index].addend; +haddr = addr + entry->addend; #if DATA_SIZE == 1 res = glue(glue(ld, LSUFFIX), _p)((uint8_t *)haddr); #else @@ -179,9 +180,10 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi, uintptr_t retaddr) { -unsigned mmu_idx = get_mmuidx(oi); -int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); -target_ulong tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ; +uintptr_t mmu_idx = get_mmuidx(oi); +uintptr_t index = tlb_index(env, mmu_idx, addr); +CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); +target_ulong tlb_addr = entry->ADDR_READ; unsigned a_bits = get_alignment_bits(get_memop(oi)); uintptr_t haddr; DATA_TYPE res; @@ -197,7 +199,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE, mmu_idx, retaddr); } -tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ; +tlb_addr = entry->ADDR_READ; } /* Handle an IO access. */ @@ -234,7 +236,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, return res; } -haddr = addr + env->tlb_table[mmu_idx][index].addend; +haddr = addr + entry->addend; res = glue(glue(ld, LSUFFIX), _be_p)((uint8_t *)haddr); return res; } @@ -275,10 +277,10 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env, void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, TCGMemOpIdx oi, uintptr_t retaddr) { -unsigned mmu_idx = get_mmuidx(oi); -int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); -target_ulong tlb_addr = -atomic_read(&env->tlb_table[mmu_idx][index].addr_write); +uintptr_t mmu_idx = get_mmuidx(oi); +uintptr_t index = tlb_index(env, mmu_idx, addr); +CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); +target_ulong tlb_addr = atomic_read(&entry->addr_write); unsigned a_bits = get_alignment_bits(get_memop(oi)); uintptr_t haddr; @@ -293,8 +295,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE, mmu_idx, retaddr); } -tlb_addr = atomic_read(&env->tlb_table[mmu_idx][index].addr_write) & -~TLB_INVALID_MASK; +tlb_addr = atomic_read(&entry->addr_write) & ~TLB_INVALID_MASK; } /* Handle an IO access. */ @@ -315,16 +316,16 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, if (DATA_SIZE > 1 && unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1 >= TARGET_PAGE_SIZE)) { -int i, index2; -target_ulong page2, tlb_addr2; +int i; +target_ulong page2; +CPUTLBEntry *entry2; do_unaligned_access:
[Qemu-devel] [RFC v2 0/5] Dynamic TLB sizing
v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01146.html Changes since v1: - Add tlb_index and tlb_entry helpers from Richard - Introduce sizeof_tlb() and tlb_n_entries() - Extract tlb_mask as its own array in CPUArchState, as suggested by Richard. For the associated helpers (tlb_index etc) I tried several approaches, and performance-wise they're all the same, so went for the simplest implementation. - Use uintptr_t for tlb_mask, as done in Richard's patch + tcg/i386: use hrexw when reading tlb_mask + Define tlbtype and tlbrexw solely based on TARGET_PAGE_BITS - Rename tlb_is_invalid to tlb_entry_is_empty, comparing all fields (except .addend) against -1. - Rename CPUTLBDesc.used to .n_used_entries. - Drop the MIN/MAX CPU_TLB_BITS patches, defining instead some values for MIN/MAX as well as a default. - Use new_size and old_size consistently in the resizing function, as suggested by Richard. - Add an additional chart to the last patch, where softmmu performance is compared against user-mode: https://imgur.com/a/eXkjMCE You can fetch this series from: https://github.com/cota/qemu/tree/tlb-dyn-v2 Note that it applies on top of my tlb-lock-v4 series: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01421.html Thanks, Emilio
[Qemu-devel] [RFC v2 3/5] cputlb: do not evict empty entries to the vtlb
Currently we evict an entry to the victim TLB when it doesn't match the current address. But it could be that there's no match because the current entry is empty (i.e. all -1's, for instance via tlb_flush). Do not evict the entry to the vtlb in that case. This change will help us keep track of the TLB's use rate. Signed-off-by: Emilio G. Cota --- include/exec/cpu-all.h | 9 + accel/tcg/cputlb.c | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 117d2fbbca..e21140049b 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -362,6 +362,15 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr) return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK); } +/** + * tlb_entry_is_empty - return true if the entry is not in use + * @te: pointer to CPUTLBEntry + */ +static inline bool tlb_entry_is_empty(const CPUTLBEntry *te) +{ +return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1; +} + void dump_exec_info(FILE *f, fprintf_function cpu_fprintf); void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf); #endif /* !CONFIG_USER_ONLY */ diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 80406f1033..4dc47e603c 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -689,7 +689,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, * Only evict the old entry to the victim tlb if it's for a * different page; otherwise just overwrite the stale data. */ -if (!tlb_hit_page_anyprot(te, vaddr_page)) { +if (!tlb_hit_page_anyprot(te, vaddr_page) && !tlb_entry_is_empty(te)) { unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE; CPUTLBEntry *tv = &env->tlb_v_table[mmu_idx][vidx]; -- 2.17.1
[Qemu-devel] [RFC v2 4/5] cputlb: track TLB use rate
This paves the way for implementing a dynamically-sized softmmu. Signed-off-by: Emilio G. Cota --- include/exec/cpu-defs.h | 5 + accel/tcg/cputlb.c | 17 ++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 87cd015f60..56f1887c7f 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -141,10 +141,15 @@ typedef struct CPUIOTLBEntry { MemTxAttrs attrs; } CPUIOTLBEntry; +typedef struct CPUTLBDesc { +size_t n_used_entries; +} CPUTLBDesc; + #define CPU_COMMON_TLB \ /* The meaning of the MMU modes is defined in the target code. */ \ /* tlb_lock serializes updates to tlb_mask, tlb_table and tlb_v_table */ \ QemuSpin tlb_lock; \ +CPUTLBDesc tlb_desc[NB_MMU_MODES]; \ /* tlb_mask[i] contains (n_entries - 1) << CPU_TLB_ENTRY_BITS */\ uintptr_t tlb_mask[NB_MMU_MODES]; \ CPUTLBEntry *tlb_table[NB_MMU_MODES]; \ diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 4dc47e603c..11d6060eb0 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -82,6 +82,7 @@ void tlb_init(CPUState *cpu) for (i = 0; i < NB_MMU_MODES; i++) { size_t n_entries = CPU_TLB_SIZE; +env->tlb_desc[i].n_used_entries = 0; env->tlb_mask[i] = (n_entries - 1) << CPU_TLB_ENTRY_BITS; env->tlb_table[i] = g_new(CPUTLBEntry, n_entries); env->iotlb[i] = g_new0(CPUIOTLBEntry, n_entries); @@ -150,6 +151,7 @@ static void tlb_flush_nocheck(CPUState *cpu) qemu_spin_lock(&env->tlb_lock); for (i = 0; i < NB_MMU_MODES; i++) { memset(env->tlb_table[i], -1, sizeof_tlb(env, i)); +env->tlb_desc[i].n_used_entries = 0; } memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table)); qemu_spin_unlock(&env->tlb_lock); @@ -213,6 +215,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, run_on_cpu_data data) memset(env->tlb_table[mmu_idx], -1, sizeof_tlb(env, mmu_idx)); memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0])); +env->tlb_desc[mmu_idx].n_used_entries = 0; } } qemu_spin_unlock(&env->tlb_lock); @@ -273,12 +276,14 @@ static inline bool tlb_hit_page_anyprot(CPUTLBEntry *tlb_entry, } /* Called with tlb_lock held */ -static inline void tlb_flush_entry_locked(CPUTLBEntry *tlb_entry, +static inline bool tlb_flush_entry_locked(CPUTLBEntry *tlb_entry, target_ulong page) { if (tlb_hit_page_anyprot(tlb_entry, page)) { memset(tlb_entry, -1, sizeof(*tlb_entry)); +return true; } +return false; } /* Called with tlb_lock held */ @@ -316,7 +321,9 @@ static void tlb_flush_page_async_work(CPUState *cpu, run_on_cpu_data data) addr &= TARGET_PAGE_MASK; qemu_spin_lock(&env->tlb_lock); for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) { -tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr); +if (tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr)) { +env->tlb_desc[mmu_idx].n_used_entries--; +} tlb_flush_vtlb_page_locked(env, mmu_idx, addr); } qemu_spin_unlock(&env->tlb_lock); @@ -358,7 +365,9 @@ static void tlb_flush_page_by_mmuidx_async_work(CPUState *cpu, qemu_spin_lock(&env->tlb_lock); for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) { if (test_bit(mmu_idx, &mmu_idx_bitmap)) { -tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr); +if (tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr)) { +env->tlb_desc[mmu_idx].n_used_entries--; +} tlb_flush_vtlb_page_locked(env, mmu_idx, addr); } } @@ -696,6 +705,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, /* Evict the old entry into the victim tlb. */ copy_tlb_helper_locked(tv, te); env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index]; +env->tlb_desc[mmu_idx].n_used_entries--; } /* refill the tlb */ @@ -747,6 +757,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, } copy_tlb_helper_locked(te, &tn); +env->tlb_desc[mmu_idx].n_used_entries++; qemu_spin_unlock(&env->tlb_lock); } -- 2.17.1
[Qemu-devel] [RFC v2 2/5] (XXX) cputlb: introduce indirection for TLB size
This paves the way for implementing dynamic TLB resizing. XXX: convert other TCG backends Signed-off-by: Emilio G. Cota --- include/exec/cpu-defs.h | 10 ++ include/exec/cpu_ldst.h | 14 +- accel/tcg/cputlb.c| 18 +++--- tcg/i386/tcg-target.inc.c | 28 ++-- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 4ff62f32bf..87cd015f60 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -141,13 +141,15 @@ typedef struct CPUIOTLBEntry { MemTxAttrs attrs; } CPUIOTLBEntry; -#define CPU_COMMON_TLB \ +#define CPU_COMMON_TLB \ /* The meaning of the MMU modes is defined in the target code. */ \ -/* tlb_lock serializes updates to tlb_table and tlb_v_table */ \ +/* tlb_lock serializes updates to tlb_mask, tlb_table and tlb_v_table */ \ QemuSpin tlb_lock; \ -CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE]; \ +/* tlb_mask[i] contains (n_entries - 1) << CPU_TLB_ENTRY_BITS */\ +uintptr_t tlb_mask[NB_MMU_MODES]; \ +CPUTLBEntry *tlb_table[NB_MMU_MODES]; \ CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE]; \ -CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];\ +CPUIOTLBEntry *iotlb[NB_MMU_MODES]; \ CPUIOTLBEntry iotlb_v[NB_MMU_MODES][CPU_VTLB_SIZE]; \ size_t tlb_flush_count; \ target_ulong tlb_flush_addr;\ diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h index e3d8d738aa..3ded1df9b7 100644 --- a/include/exec/cpu_ldst.h +++ b/include/exec/cpu_ldst.h @@ -130,7 +130,9 @@ extern __thread uintptr_t helper_retaddr; static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx, target_ulong addr) { -return (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); +uintptr_t size_mask = env->tlb_mask[mmu_idx] >> CPU_TLB_ENTRY_BITS; + +return (addr >> TARGET_PAGE_BITS) & size_mask; } /* Find the TLB entry corresponding to the mmu_idx + address pair. */ @@ -140,6 +142,16 @@ static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, return &env->tlb_table[mmu_idx][tlb_index(env, mmu_idx, addr)]; } +static inline size_t sizeof_tlb(CPUArchState *env, uintptr_t mmu_idx) +{ +return env->tlb_mask[mmu_idx] + (1 << CPU_TLB_ENTRY_BITS); +} + +static inline size_t tlb_n_entries(CPUArchState *env, uintptr_t mmu_idx) +{ +return (env->tlb_mask[mmu_idx] >> CPU_TLB_ENTRY_BITS) + 1; +} + #ifdef MMU_MODE0_SUFFIX #define CPU_MMU_INDEX 0 #define MEMSUFFIX MMU_MODE0_SUFFIX diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index a5972773de..80406f1033 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -76,8 +76,16 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16); void tlb_init(CPUState *cpu) { CPUArchState *env = cpu->env_ptr; +int i; qemu_spin_init(&env->tlb_lock); +for (i = 0; i < NB_MMU_MODES; i++) { +size_t n_entries = CPU_TLB_SIZE; + +env->tlb_mask[i] = (n_entries - 1) << CPU_TLB_ENTRY_BITS; +env->tlb_table[i] = g_new(CPUTLBEntry, n_entries); +env->iotlb[i] = g_new0(CPUIOTLBEntry, n_entries); +} } /* flush_all_helper: run fn across all cpus @@ -120,6 +128,7 @@ size_t tlb_flush_count(void) static void tlb_flush_nocheck(CPUState *cpu) { CPUArchState *env = cpu->env_ptr; +int i; /* The QOM tests will trigger tlb_flushes without setting up TCG * so we bug out here in that case. @@ -139,7 +148,9 @@ static void tlb_flush_nocheck(CPUState *cpu) * that do not hold the lock are performed by the same owner thread. */ qemu_spin_lock(&env->tlb_lock); -memset(env->tlb_table, -1, sizeof(env->tlb_table)); +for (i = 0; i < NB_MMU_MODES; i++) { +memset(env->tlb_table[i], -1, sizeof_tlb(env, i)); +} memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table)); qemu_spin_unlock(&env->tlb_lock); @@ -200,7 +211,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, run_on_cpu_data data) if (test_bit(mmu_idx, &mmu_idx_bitmask)) { tlb_debug("%d\n", mmu_idx); -memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0])); +memset(env->tlb_table[mmu_idx], -1, sizeof_tlb(env, mmu_idx)); memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0])); } } @@ -523,8 +534,9 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length) qemu_spin_lock(&env->tlb_lock); for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) { unsigned int i; +unsigned int n = tlb_n_entries(env, mmu_idx); -
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/09/2018 01:21 AM, Max Reitz wrote: > On 09.10.18 00:14, Vladimir Sementsov-Ogievskiy wrote: >> >> >> On 10/09/2018 01:08 AM, Max Reitz wrote: >>> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote: On 10/08/2018 11:51 PM, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> Rewrite corrupted L2 table entry, which reference space out of >> underlying file. >> >> Make this L2 table entry read-as-all-zeros without any allocation. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-refcount.c | 32 >> 1 file changed, 32 insertions(+) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 3c004e5bfe..3de3768a3c 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState >> *bs, BdrvCheckResult *res, >> /* Mark cluster as used */ >> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) >> + 1) * >> BDRV_SECTOR_SIZE; >> +if (csize > s->cluster_size) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"compressed cluster larger than cluster: size >> 0x%" >> +PRIx64, csize); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + > > This seems recoverable, isn't it? Can we not try to just limit the > csize, or decompress the cluster with the given csize from the given > offset, disregarding the cluster limit? Hm, you want to assume that csize is corrupted but coffset may be correct? Unlikely, I think. >>> >>> Better to reconstruct probably garbage data than to definitely garbage >>> data (all zeroes) is what I think. >>> So, to carefully repair csize, we should decompress one cluster (or one cluster - 1 byte) of data, trying to get one cluster of decompressed data. If we succeed, we know csize, or we can safely set it to one cluster. >>> >>> Yes. >>> Or we can just set csize = 1 cluster, if it is larger. And leave problems to real execution which will lead to EIO in worst case. >>> >>> Or this, yes. >>> >> coffset = l2_entry & s->cluster_offset_mask & >> ~(BDRV_SECTOR_SIZE - 1); >> +if (coffset >= bdrv_getlength(bs->file->bs)) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"compressed cluster out of file: offset 0x%" >> PRIx64, >> +coffset); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + >> ret = qcow2_inc_refcounts_imrt(bs, res, >>refcount_table, >> refcount_table_size, >>coffset, csize); >> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState >> *bs, BdrvCheckResult *res, >> { >> uint64_t offset = l2_entry & L2E_OFFSET_MASK; >> >> +if (offset >= bdrv_getlength(bs->file->bs)) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"cluster out of file: offset 0x%" PRIx64, >> offset); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + > > These other two look OK, but they have another issue: If this is a v2 > image, you cannot create zero clusters; so you'll have to unallocate the > cluster in that case. Oho, it's a problem. It may be unsafe to discard clusters, making backing image available through the holes. What discard do on v2? Zeroing or holes? >>> >>> Oh, right! discard on v2 punches a hole. So I see three ways: >>> (1) You can do the same and point to that bit of code, or >>> (2) You allocate a data cluster full of zeroes in case of v2, or >>> (3) You just error out. >>> >>> (3) doesn't seem like the worst option. >> >>> Amending the image to be v3 is >>> always possible and trivial. >> >> how to do it for corrupted image? > > Oh, yeah, you can't open a corrupted image, can you... I suppose we > want a way to force-clear the flag anyway. :-) am, which flag? > > Max >
Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC
On Mon, Oct 08, 2018 at 02:19:09PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias > wrote: > > From: "Edgar E. Iglesias" > > > > Add a model of Xilinx Versal SoC. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > default-configs/aarch64-softmmu.mak | 1 + > > hw/arm/Makefile.objs| 1 + > > hw/arm/xlnx-versal.c| 339 > > > > include/hw/arm/xlnx-versal.h| 122 + > > 4 files changed, 463 insertions(+) > > create mode 100644 hw/arm/xlnx-versal.c > > create mode 100644 include/hw/arm/xlnx-versal.h > > > > > > +#define XLNX_VERSAL_ACPU_TYPE "cortex-a72" "-" TYPE_ARM_CPU > > ARM_CPU_TYPE_NAME("cortex-a72") is preferable to hand-assembling > the type name like this. Fixed for v2. > > > +#define GEM_REVISION0x40070106 > > + > > > +for (i = 0; i < nr_apu_cpus; i++) { > > +DeviceState *cpudev = DEVICE(s->fpd.apu.cpu[i]); > > +int ppibase = XLNX_VERSAL_NR_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS; > > +qemu_irq maint_irq; > > +int ti; > > +/* Mapping from the output timer irq lines from the CPU to the > > + * GIC PPI inputs we use for the virt board. > > + */ > > This isn't the virt board :-) -- cut-n-pasted comment ? Fixed for v2. > > > +const int timer_irq[] = { > > +[GTIMER_PHYS] = VERSAL_TIMER_NS_EL1_IRQ, > > +[GTIMER_VIRT] = VERSAL_TIMER_VIRT_IRQ, > > +[GTIMER_HYP] = VERSAL_TIMER_NS_EL2_IRQ, > > +[GTIMER_SEC] = VERSAL_TIMER_S_EL1_IRQ, > > +}; > > + > > +for (ti = 0; ti < ARRAY_SIZE(timer_irq); ti++) { > > +qdev_connect_gpio_out(cpudev, ti, > > + qdev_get_gpio_in(gicdev, > > + ppibase + > > timer_irq[ti])); > > +} > > +maint_irq = qdev_get_gpio_in(gicdev, > > +ppibase + VERSAL_GIC_MAINT_IRQ); > > +qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", > > +0, maint_irq); > > +sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, > > ARM_CPU_IRQ)); > > +sysbus_connect_irq(gicbusdev, i + nr_apu_cpus, > > + qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > > +sysbus_connect_irq(gicbusdev, i + 2 * nr_apu_cpus, > > + qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ)); > > +sysbus_connect_irq(gicbusdev, i + 3 * nr_apu_cpus, > > + qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); > > +} > > + > > +for (i = 0; i < XLNX_VERSAL_NR_IRQS; i++) { > > +pic[i] = qdev_get_gpio_in(gicdev, i); > > +} > > > +/* This takes the board allocated linear DDR memory and creates aliases > > + * for each split DDR range/apperture on the Versal address map. > > "aperture" Fixed > > > > > +static void versal_realize(DeviceState *dev, Error **errp) > > +{ > > +Versal *s = XLNX_VERSAL(dev); > > +qemu_irq pic[XLNX_VERSAL_NR_IRQS]; > > + > > +versal_create_apu_cpus(s, errp); > > +versal_create_apu_gic(s, pic, errp); > > +versal_create_uarts(s, pic); > > +versal_create_gems(s, pic); > > +versal_map_ddr(s); > > +versal_unimp(s); > > + > > +/* Create the OCM. */ > > +memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm", > > + MM_OCM_SIZE, &error_fatal); > > What's an OCM? Is it really memory, or is this a stub for something? I've changed the comment to spell out that it's an On Chip Memory. > > > + > > +memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, > > 0); > > +memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0); > > +} > > + > > +static void versal_init(Object *obj) > > +{ > > +Versal *s = XLNX_VERSAL(obj); > > + > > +memory_region_init(&s->fpd.apu.mr, obj, "mr-apu", UINT64_MAX); > > +memory_region_init(&s->mr_ps, obj, "mr-ps-switch", UINT64_MAX); > > +} > > + > > +static const VMStateDescription versal_vmstate = { > > +.name = "xlnx-ve", > > +.version_id = 1, > > +.minimum_version_id = 1, > > +.fields = (VMStateField[]) { > > +/* FIXME. */ > > Stray FIXME comment -- I think the answer may be "the > SoC object has no state of its own so needs neither a > vmsd nor a reset method" ? (If so and if you drop them, > do put a comment in the class init about why they're not > provided. Some day we may have a mechanism for a device > to explicitly say "I need no vmstate" so we can assert if > none is provided.) Thanks, I'll do that. > > > +VMSTATE_END_OF_LIST() > > +} > > +}; > > + > > +static Property versal_properties[] = { > > +DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION, > > + MemoryRegion *), > > +DEFINE_PROP_UINT32("psci-conduit", V
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 09.10.18 00:14, Vladimir Sementsov-Ogievskiy wrote: > > > On 10/09/2018 01:08 AM, Max Reitz wrote: >> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote: >>> >>> >>> On 10/08/2018 11:51 PM, Max Reitz wrote: On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Rewrite corrupted L2 table entry, which reference space out of > underlying file. > > Make this L2 table entry read-as-all-zeros without any allocation. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- >block/qcow2-refcount.c | 32 >1 file changed, 32 insertions(+) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 3c004e5bfe..3de3768a3c 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState > *bs, BdrvCheckResult *res, >/* Mark cluster as used */ >csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + > 1) * >BDRV_SECTOR_SIZE; > +if (csize > s->cluster_size) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"compressed cluster larger than cluster: size > 0x%" > +PRIx64, csize); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + This seems recoverable, isn't it? Can we not try to just limit the csize, or decompress the cluster with the given csize from the given offset, disregarding the cluster limit? >>> >>> Hm, you want to assume that csize is corrupted but coffset may be >>> correct? Unlikely, I think. >> >> Better to reconstruct probably garbage data than to definitely garbage >> data (all zeroes) is what I think. >> >>> So, to carefully repair csize, we should decompress one cluster (or one >>> cluster - 1 byte) of data, trying to get one cluster of decompressed >>> data. If we succeed, we know csize, or we can safely set it to one cluster. >> >> Yes. >> >>> Or we can just set csize = 1 cluster, if it is larger. And leave >>> problems to real execution which will lead to EIO in worst case. >> >> Or this, yes. >> >coffset = l2_entry & s->cluster_offset_mask & > ~(BDRV_SECTOR_SIZE - 1); > +if (coffset >= bdrv_getlength(bs->file->bs)) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"compressed cluster out of file: offset 0x%" > PRIx64, > +coffset); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + >ret = qcow2_inc_refcounts_imrt(bs, res, > refcount_table, > refcount_table_size, > coffset, csize); > @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState > *bs, BdrvCheckResult *res, >{ >uint64_t offset = l2_entry & L2E_OFFSET_MASK; > > +if (offset >= bdrv_getlength(bs->file->bs)) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"cluster out of file: offset 0x%" PRIx64, > offset); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + These other two look OK, but they have another issue: If this is a v2 image, you cannot create zero clusters; so you'll have to unallocate the cluster in that case. >>> >>> >>> Oho, it's a problem. It may be unsafe to discard clusters, making >>> backing image available through the holes. What discard do on v2? >>> Zeroing or holes? >> >> Oh, right! discard on v2 punches a hole. So I see three ways: >> (1) You can do the same and point to that bit of code, or >> (2) You allocate a data cluster full of zeroes in case of v2, or >> (3) You just error out. >> >> (3) doesn't seem like the worst option. > >> Amending the image to be v3 is >> always possible and trivial. > > how to do it for corrupted image? Oh, yeah, you can't open a corrupted image, can you... I suppose we want a way to force-clear the flag anyway. :-) Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] possible BUG, ATS12NSOPW fails
Hi Peter, I am chasing an address translation error, and it looks like it might be a QEMU bug, because I cannot reproduce the problem on a physical board. The issue is that a requested ATS12NSOPW translation in Xen is reported as failing by QEMU, but actually the address is correct. The workflow is as follows: 1) Linux as Dom0 registers a memory area with Xen at boot, see: drivers/xen/time.c:xen_setup_runstate_info in Linux 2) Xen tries to update such region with new data every so often, see: xen/arch/arm/domain.c:update_runstate_area As you can see from the code, Xen uses __raw_copy_to_guest and __copy_to_guest to copy the new values to the guest. Both of them eventually call xen/arch/arm/guestcopy.c:copy_guest: copy_guest -> translate_get_page -> get_page_from_gva -> gvirt_to_maddr to do the translation because Xen only memorizes the guest virtual address as passed by xen_setup_runstate_info in Linux. gvirt_to_maddr uses ATS12NSOPW for the translation. I double-checked all the addresses and they are correct. For some reason after starting the guest, ATS12NSOPW starts to fail for ffc006f91628, which is the same virtual address corresponding to the same runstate region allocated in Linux. I added a couple of printf's to QEMU and Xen: (XEN) DEBUG do_vcpu_op 1458 area.v=ffc006f91628 ### This is the virtual address as passed by Linux [...] (QEMU) DEBUG do_ats_write 2256 ret=1 phys_addr=200 value=ffc006f91628 (QEMU) DEBUG do_ats_write 2326 par64=80b ### This is the output from QEMU, I added the printks to do_ats_write, line numbers 2256 and 2326 (XEN) p2m.c:1426: d1v0: gvirt_to_maddr failed va=0xffc006f91628 flags=0x1 par=0x80b (XEN) DEBUG translate_get_page 42 addr=ffc006f91628 linear=1 write=1 (XEN) DEBUG raw_copy_to_guest 120 caller=domain.c#update_runstate_area+0x78/0x10c (XEN) DEBUG update_runstate_area 294 ### This is Xen failing the copy_to_guest in update_runstate_area. Do you have any ideas on what's wrong? I am happy to run more tests with QEMU to get more data. Cheers, Stefano
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/09/2018 01:08 AM, Max Reitz wrote: > On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote: >> >> >> On 10/08/2018 11:51 PM, Max Reitz wrote: >>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: Rewrite corrupted L2 table entry, which reference space out of underlying file. Make this L2 table entry read-as-all-zeros without any allocation. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2-refcount.c | 32 1 file changed, 32 insertions(+) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 3c004e5bfe..3de3768a3c 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, /* Mark cluster as used */ csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * BDRV_SECTOR_SIZE; +if (csize > s->cluster_size) { +ret = fix_l2_entry_to_zero( +bs, res, fix, l2_offset, i, active, +"compressed cluster larger than cluster: size 0x%" +PRIx64, csize); +if (ret < 0) { +goto fail; +} +continue; +} + >>> >>> This seems recoverable, isn't it? Can we not try to just limit the >>> csize, or decompress the cluster with the given csize from the given >>> offset, disregarding the cluster limit? >> >> Hm, you want to assume that csize is corrupted but coffset may be >> correct? Unlikely, I think. > > Better to reconstruct probably garbage data than to definitely garbage > data (all zeroes) is what I think. > >> So, to carefully repair csize, we should decompress one cluster (or one >> cluster - 1 byte) of data, trying to get one cluster of decompressed >> data. If we succeed, we know csize, or we can safely set it to one cluster. > > Yes. > >> Or we can just set csize = 1 cluster, if it is larger. And leave >> problems to real execution which will lead to EIO in worst case. > > Or this, yes. > coffset = l2_entry & s->cluster_offset_mask & ~(BDRV_SECTOR_SIZE - 1); +if (coffset >= bdrv_getlength(bs->file->bs)) { +ret = fix_l2_entry_to_zero( +bs, res, fix, l2_offset, i, active, +"compressed cluster out of file: offset 0x%" PRIx64, +coffset); +if (ret < 0) { +goto fail; +} +continue; +} + ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size, coffset, csize); @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, { uint64_t offset = l2_entry & L2E_OFFSET_MASK; +if (offset >= bdrv_getlength(bs->file->bs)) { +ret = fix_l2_entry_to_zero( +bs, res, fix, l2_offset, i, active, +"cluster out of file: offset 0x%" PRIx64, offset); +if (ret < 0) { +goto fail; +} +continue; +} + >>> >>> These other two look OK, but they have another issue: If this is a v2 >>> image, you cannot create zero clusters; so you'll have to unallocate the >>> cluster in that case. >> >> >> Oho, it's a problem. It may be unsafe to discard clusters, making >> backing image available through the holes. What discard do on v2? >> Zeroing or holes? > > Oh, right! discard on v2 punches a hole. So I see three ways: > (1) You can do the same and point to that bit of code, or > (2) You allocate a data cluster full of zeroes in case of v2, or > (3) You just error out. > > (3) doesn't seem like the worst option. > Amending the image to be v3 is > always possible and trivial. how to do it for corrupted image? > Maybe point the user to that option. > > Max >
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote: > > > On 10/08/2018 11:51 PM, Max Reitz wrote: >> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >>> Rewrite corrupted L2 table entry, which reference space out of >>> underlying file. >>> >>> Make this L2 table entry read-as-all-zeros without any allocation. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> block/qcow2-refcount.c | 32 >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>> index 3c004e5bfe..3de3768a3c 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs, >>> BdrvCheckResult *res, >>> /* Mark cluster as used */ >>> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * >>> BDRV_SECTOR_SIZE; >>> +if (csize > s->cluster_size) { >>> +ret = fix_l2_entry_to_zero( >>> +bs, res, fix, l2_offset, i, active, >>> +"compressed cluster larger than cluster: size 0x%" >>> +PRIx64, csize); >>> +if (ret < 0) { >>> +goto fail; >>> +} >>> +continue; >>> +} >>> + >> >> This seems recoverable, isn't it? Can we not try to just limit the >> csize, or decompress the cluster with the given csize from the given >> offset, disregarding the cluster limit? > > Hm, you want to assume that csize is corrupted but coffset may be > correct? Unlikely, I think. Better to reconstruct probably garbage data than to definitely garbage data (all zeroes) is what I think. > So, to carefully repair csize, we should decompress one cluster (or one > cluster - 1 byte) of data, trying to get one cluster of decompressed > data. If we succeed, we know csize, or we can safely set it to one cluster. Yes. > Or we can just set csize = 1 cluster, if it is larger. And leave > problems to real execution which will lead to EIO in worst case. Or this, yes. >>> coffset = l2_entry & s->cluster_offset_mask & >>> ~(BDRV_SECTOR_SIZE - 1); >>> +if (coffset >= bdrv_getlength(bs->file->bs)) { >>> +ret = fix_l2_entry_to_zero( >>> +bs, res, fix, l2_offset, i, active, >>> +"compressed cluster out of file: offset 0x%" >>> PRIx64, >>> +coffset); >>> +if (ret < 0) { >>> +goto fail; >>> +} >>> +continue; >>> +} >>> + >>> ret = qcow2_inc_refcounts_imrt(bs, res, >>> refcount_table, >>> refcount_table_size, >>> coffset, csize); >>> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs, >>> BdrvCheckResult *res, >>> { >>> uint64_t offset = l2_entry & L2E_OFFSET_MASK; >>> >>> +if (offset >= bdrv_getlength(bs->file->bs)) { >>> +ret = fix_l2_entry_to_zero( >>> +bs, res, fix, l2_offset, i, active, >>> +"cluster out of file: offset 0x%" PRIx64, offset); >>> +if (ret < 0) { >>> +goto fail; >>> +} >>> +continue; >>> +} >>> + >> >> These other two look OK, but they have another issue: If this is a v2 >> image, you cannot create zero clusters; so you'll have to unallocate the >> cluster in that case. > > > Oho, it's a problem. It may be unsafe to discard clusters, making > backing image available through the holes. What discard do on v2? > Zeroing or holes? Oh, right! discard on v2 punches a hole. So I see three ways: (1) You can do the same and point to that bit of code, or (2) You allocate a data cluster full of zeroes in case of v2, or (3) You just error out. (3) doesn't seem like the worst option. Amending the image to be v3 is always possible and trivial. Maybe point the user to that option. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 04/12] qemu-option: improve qemu_opts_print_help() output
First of all, this patch broke iotest 082. But then again, all that'd be needed is a correction of the reference output. However: On 07.09.18 09:59, Marc-André Lureau wrote: > Modify qemu_opts_print_help(): > - to print expected argument type > - skip description if not available > - sort lines > - prefix with the list name (like qdev, to avoid confusion) Is this really useful? My main issue right now is this: $ qemu-img amend -f qcow2 -o help Creation options for 'qcow2': qcow2-create-opts.backing_file=str - File name of a base image qcow2-create-opts.backing_fmt=str - Image format of the base image qcow2-create-opts.cluster_size=size - qcow2 cluster size [...] (The reason create is not affected is because it copies the options into a new list.) This is supposed to be a human-readable output. I don't see how the rather internal list name[1] really helps here. ([1] I suppose if you write a configuration file, these identifiers become visible. But you don't use "help" in a configuration file, so I find that point becomes rather moot.) So this is why I don't think it is a good idea to print this name. Now if it helped to prevent confusion, OK, but without further explanation I don't see how. Is this because I can use "help" in places where it's unclear to the user what exactly the "help" refers to? Also, this is bikeshedding, but still, I don't like the dot notation here. It doesn't mean anything (I can't use "qcow2-create-opts.backing_file", nor can I use "virtio-blk-pci.iothread"), so I'd prefer a nice "human" prefix like "(qcow2-create-opts) backing_file" or "qcow2-create-opts: ". Actually, I don't like the whole notation. It looks like code. Here is an excerpt from -device virtio-blk-pci,help: virtio-blk-pci.iothread=link virtio-blk-pci.request-merging=bool (on/off) virtio-blk-pci.logical_block_size=uint16 (A power of two between 512 and 32768) I can imagine some ways this would be more readable to human users (and I don't think ease of parsing is a high priority here), for instance: virtio-blk-pci options: iothread: link to object of type iothread request-merging: bool (on/off) logical_block_size: uint16 (A power of two between 512 and 32768) (But it appears that transforming "link" to something readable wouldn't be easy. Though then again, I really think it's unreadable unless you know what it's supposed to mean.) So my concrete requests are this: 1. If the ID is really useful, I would put it above the whole list in an own line. It's not useful to human readers to re-print it on every single line. It would be nice to have an option to disable this line, however, if the caller has already printed something along the lines (which qemu-img amend does). 2. Put some more whitespace into the whole thing, and make it less robot-y overall. I much prefer ": " when reading over "=" (even programming languages do when it's about types, in fact). I'm not writing a patch yet because maybe there is some reason to print the ID on every single line. So I'm just proposing first. Max > - drop 16-chars alignment, use a '-' as seperator for option name and > description > > For ex, "-spice help" output is changed from: > > port No description available > tls-port No description available > addr No description available > [...] > gl No description available > rendernode No description available > > to: > > spice.addr=str > spice.agent-mouse=bool (on/off) > spice.disable-agent-file-xfer=bool (on/off) > [...] > spice.x509-key-password=str > spice.zlib-glz-wan-compression=str > > "qemu-img create -f qcow2 -o help", changed from: > > size Virtual disk size > compat Compatibility level (0.10 or 1.1) > backing_file File name of a base image > [...] > lazy_refcounts Postpone refcount updates > refcount_bitsWidth of a reference count entry in bits > > to: > > backing_file=str - File name of a base image > backing_fmt=str - Image format of the base image > cluster_size=size - qcow2 cluster size > [...] > refcount_bits=num - Width of a reference count entry in bits > size=size - Virtual disk size > > Signed-off-by: Marc-André Lureau > Reviewed-by: Eric Blake > --- > util/qemu-option.c | 38 -- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 557b6c6626..069de36d2c 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -208,17 +208,51 @@ out: > return result; > } > > +static const char *opt_type_to_string(enum QemuOptType type) > +{ > +switch (type) { > +case QEMU_OPT_STRING: > +return "str"; > +case QEMU_OPT_BOOL: > +return "bool (on/off)"; > +case QEMU_OPT_NUMBER: > +return "num"; > +case QEMU_OPT_SIZE: > +return "size"; > +} > + > +g_assert_not_reached(); > +} > + > void qemu_opts_print_help(QemuOptsList *list) > { >
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/08/2018 11:51 PM, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> Rewrite corrupted L2 table entry, which reference space out of >> underlying file. >> >> Make this L2 table entry read-as-all-zeros without any allocation. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-refcount.c | 32 >> 1 file changed, 32 insertions(+) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 3c004e5bfe..3de3768a3c 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs, >> BdrvCheckResult *res, >> /* Mark cluster as used */ >> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * >> BDRV_SECTOR_SIZE; >> +if (csize > s->cluster_size) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"compressed cluster larger than cluster: size 0x%" >> +PRIx64, csize); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + > > This seems recoverable, isn't it? Can we not try to just limit the > csize, or decompress the cluster with the given csize from the given > offset, disregarding the cluster limit? Hm, you want to assume that csize is corrupted but coffset may be correct? Unlikely, I think. So, to carefully repair csize, we should decompress one cluster (or one cluster - 1 byte) of data, trying to get one cluster of decompressed data. If we succeed, we know csize, or we can safely set it to one cluster. Or we can just set csize = 1 cluster, if it is larger. And leave problems to real execution which will lead to EIO in worst case. > >> coffset = l2_entry & s->cluster_offset_mask & >> ~(BDRV_SECTOR_SIZE - 1); >> +if (coffset >= bdrv_getlength(bs->file->bs)) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"compressed cluster out of file: offset 0x%" PRIx64, >> +coffset); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + >> ret = qcow2_inc_refcounts_imrt(bs, res, >> refcount_table, >> refcount_table_size, >> coffset, csize); >> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs, >> BdrvCheckResult *res, >> { >> uint64_t offset = l2_entry & L2E_OFFSET_MASK; >> >> +if (offset >= bdrv_getlength(bs->file->bs)) { >> +ret = fix_l2_entry_to_zero( >> +bs, res, fix, l2_offset, i, active, >> +"cluster out of file: offset 0x%" PRIx64, offset); >> +if (ret < 0) { >> +goto fail; >> +} >> +continue; >> +} >> + > > These other two look OK, but they have another issue: If this is a v2 > image, you cannot create zero clusters; so you'll have to unallocate the > cluster in that case. Oho, it's a problem. It may be unsafe to discard clusters, making backing image available through the holes. What discard do on v2? Zeroing or holes? > > Max >
[Qemu-devel] [Bug 1796754] Re: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context"
I was hit by this issue when I tried to run some Java program. And it turns out jdk sets the buf to NULL: http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/887e525597f8/src/solaris/native/java/net/NetworkInterface.c#l1042 Setting to NULL is valid according to http://man7.org/linux/man- pages/man7/netdevice.7.html But qemu doesn’t handle the case: https://github.com/qemu/qemu/blob/aa8e26de9617756febcbf794dda965df307fdaaa /linux-user/syscall.c#L4105 I guess qemu developers didn’t handle the case because the Linux kernel changed and they were based on behavior of old version: https://linux.die.net/man/7/netdevice Please add the support for it otherwise a wide range of network related Java programs won’t run. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1796754 Title: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context" Status in QEMU: New Bug description: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1796754/+subscriptions
Re: [Qemu-devel] [PATCH] qemu-system-hppa: Raise exception 26 on emulated hardware
On 10/7/18 1:51 PM, Helge Deller wrote: > On PCXS chips (PA7000, pa 1.1a), trap #18 is raised on memory faults, > while all later chips (>= PA7100) generate either trap #26, #27 or #28 > (depending on the fault type). > > Since the current qemu emulation emulates a B160L machine (with a > PA7300LC PCX-L2 chip, we should raise trap #26 (EXCP_DMAR) instead of > #18 (EXCP_DMP) on access faults by the Linux kernel to page zero. > > With the patch we now get the correct output (I tested against real > hardware): > Kernel Fault: Code=26 (Data memory access rights trap) (Addr=0004) > instead of: > Kernel Fault: Code=18 (Data memory protection/unaligned access trap) > (Addr=0004) > > Signed-off-by: Helge Deller Queued, thanks. r~
[Qemu-devel] [PATCH v3 03/10] target/arm: Convert v8 extensions from feature bits to isar tests
Most of the v8 extensions are self-contained within the ISAR registers and are not implied by other feature bits, which makes them the easiest to convert. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 123 + target/arm/translate-a64.h | 20 ++ target/arm/translate.h | 16 + linux-user/elfload.c | 46 -- target/arm/cpu.c | 27 +--- target/arm/cpu64.c | 52 ++-- target/arm/translate-a64.c | 101 +++--- target/arm/translate.c | 36 +-- 8 files changed, 294 insertions(+), 127 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index f00c0444c4..66eaa40ed9 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1573,30 +1573,18 @@ enum arm_features { ARM_FEATURE_LPAE, /* has Large Physical Address Extension */ ARM_FEATURE_V8, ARM_FEATURE_AARCH64, /* supports 64 bit mode */ -ARM_FEATURE_V8_AES, /* implements AES part of v8 Crypto Extensions */ ARM_FEATURE_CBAR, /* has cp15 CBAR */ ARM_FEATURE_CRC, /* ARMv8 CRC instructions */ ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */ ARM_FEATURE_EL2, /* has EL2 Virtualization support */ ARM_FEATURE_EL3, /* has EL3 Secure monitor support */ -ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */ ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */ ARM_FEATURE_PMU, /* has PMU support */ ARM_FEATURE_VBAR, /* has cp15 VBAR */ ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */ ARM_FEATURE_SVE, /* has Scalable Vector Extension */ -ARM_FEATURE_V8_SHA512, /* implements SHA512 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_SM3, /* implements SM3 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_SM4, /* implements SM4 part of v8 Crypto Extensions */ -ARM_FEATURE_V8_ATOMICS, /* ARMv8.1-Atomics feature */ -ARM_FEATURE_V8_RDM, /* implements v8.1 simd round multiply */ -ARM_FEATURE_V8_DOTPROD, /* implements v8.2 simd dot product */ ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */ -ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions. */ ARM_FEATURE_M_MAIN, /* M profile Main Extension */ }; @@ -3148,4 +3136,115 @@ static inline uint64_t *aa64_vfp_qreg(CPUARMState *env, unsigned regno) /* Shared between translate-sve.c and sve_helper.c. */ extern const uint64_t pred_esz_masks[4]; +/* + * 32-bit feature tests via id registers. + */ +static inline bool aa32_feature_aes(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0; +} + +static inline bool aa32_feature_pmull(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) > 1; +} + +static inline bool aa32_feature_sha1(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, SHA1) != 0; +} + +static inline bool aa32_feature_sha2(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, SHA2) != 0; +} + +static inline bool aa32_feature_crc32(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, CRC32) != 0; +} + +static inline bool aa32_feature_rdm(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, RDM) != 0; +} + +static inline bool aa32_feature_vcma(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar5, ID_ISAR5, VCMA) != 0; +} + +static inline bool aa32_feature_dp(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar6, ID_ISAR6, DP) != 0; +} + +/* + * 64-bit feature tests via id registers. + */ +static inline bool aa64_feature_aes(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, AES) != 0; +} + +static inline bool aa64_feature_pmull(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, AES) > 1; +} + +static inline bool aa64_feature_sha1(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA1) != 0; +} + +static inline bool aa64_feature_sha256(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA2) != 0; +} + +static inline bool aa64_feature_sha512(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA2) > 1; +} + +static inline bool aa64_feature_crc32(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, CRC32) != 0; +} + +static inline bool aa64_feature_atomics(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, ATOMIC) != 0; +} + +static inline bool aa64_feature_rdm(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, RDM) != 0; +} + +static inline bool aa64_feature_sha3(ARMCPU *cpu) +{ +return FI
[Qemu-devel] [PATCH v3 00/10] target/arm: Rely on id regs instead of features
This edition fixes a number of conflicts with master, and adds a few field definitions from ARMv8.5, courtesy of Philippe. It also fixes a big think-o in a last-minute change to the sve system mode patch set that was applied to master today. That would be patch 1. Sorry for not testing the original more thoroughly. r~ Richard Henderson (10): target/arm: Fix aarch64_sve_change_el wrt EL0 target/arm: Define fields of ISAR registers target/arm: Convert v8 extensions from feature bits to isar tests target/arm: Align cortex-r5 id_isar0 target/arm: Fix cortex-a7 id_isar0 target/arm: Convert division from feature bits to isar0 tests target/arm: Convert jazelle from feature bit to isar1 test target/arm: Convert t32ee from feature bit to isar3 test target/arm: Convert sve from feature bit to aa64pfr0 test target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test target/arm/cpu.h| 275 +--- target/arm/translate-a64.h | 22 +++ target/arm/translate.h | 20 +++ linux-user/aarch64/signal.c | 4 +- linux-user/elfload.c| 60 linux-user/syscall.c| 10 +- target/arm/cpu.c| 65 + target/arm/cpu64.c | 66 + target/arm/helper.c | 29 ++-- target/arm/machine.c| 6 +- target/arm/op_helper.c | 6 +- target/arm/translate-a64.c | 145 ++- target/arm/translate.c | 48 +++ 13 files changed, 538 insertions(+), 218 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH v3 01/10] target/arm: Fix aarch64_sve_change_el wrt EL0
At present we assert: arm_el_is_aa64: Assertion `el >= 1 && el <= 3' failed. The comment in arm_el_is_aa64 explains why asking about EL0 without extra information is impossible. Add an extra argument to provide it from the surrounding context. Fixes: 0ab5953b00b3 Signed-off-by: Richard Henderson --- target/arm/cpu.h | 7 +-- target/arm/helper.c| 16 target/arm/op_helper.c | 6 +- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 3a2aff1192..54362ddce8 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -911,10 +911,13 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg); int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg); void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq); -void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el); +void aarch64_sve_change_el(CPUARMState *env, int old_el, + int new_el, bool el0_a64); #else static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { } -static inline void aarch64_sve_change_el(CPUARMState *env, int o, int n) { } +static inline void aarch64_sve_change_el(CPUARMState *env, int o, + int n, bool a) +{ } #endif target_ulong do_arm_semihosting(CPUARMState *env); diff --git a/target/arm/helper.c b/target/arm/helper.c index c83f7c1109..0efbb5c76c 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -8374,7 +8374,11 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) unsigned int new_mode = aarch64_pstate_mode(new_el, true); unsigned int cur_el = arm_current_el(env); -aarch64_sve_change_el(env, cur_el, new_el); +/* + * Note that new_el can never be 0. If cur_el is 0, then + * el0_a64 is is_a64(), else el0_a64 is ignored. + */ +aarch64_sve_change_el(env, cur_el, new_el, is_a64(env)); if (cur_el < new_el) { /* Entry vector offset depends on whether the implemented EL @@ -12791,9 +12795,11 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) /* * Notice a change in SVE vector size when changing EL. */ -void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el) +void aarch64_sve_change_el(CPUARMState *env, int old_el, + int new_el, bool el0_a64) { int old_len, new_len; +bool old_a64, new_a64; /* Nothing to do if no SVE. */ if (!arm_feature(env, ARM_FEATURE_SVE)) { @@ -12817,9 +12823,11 @@ void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el) * we already have the correct register contents when encountering the * vq0->vq0 transition between EL0->EL1. */ -old_len = (arm_el_is_aa64(env, old_el) && !sve_exception_el(env, old_el) +old_a64 = old_el ? arm_el_is_aa64(env, old_el) : el0_a64; +old_len = (old_a64 && !sve_exception_el(env, old_el) ? sve_zcr_len_for_el(env, old_el) : 0); -new_len = (arm_el_is_aa64(env, new_el) && !sve_exception_el(env, new_el) +new_a64 = new_el ? arm_el_is_aa64(env, new_el) : el0_a64; +new_len = (new_a64 && !sve_exception_el(env, new_el) ? sve_zcr_len_for_el(env, new_el) : 0); /* When changing vector length, clear inaccessible state. */ diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index fb15a13e6c..d915579712 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -1101,7 +1101,11 @@ void HELPER(exception_return)(CPUARMState *env) "AArch64 EL%d PC 0x%" PRIx64 "\n", cur_el, new_el, env->pc); } -aarch64_sve_change_el(env, cur_el, new_el); +/* + * Note that cur_el can never be 0. If new_el is 0, then + * el0_a64 is return_to_aa64, else el0_a64 is ignored. + */ +aarch64_sve_change_el(env, cur_el, new_el, return_to_aa64); qemu_mutex_lock_iothread(); arm_call_el_change_hook(arm_env_get_cpu(env)); -- 2.17.1
[Qemu-devel] [PATCH v3 02/10] target/arm: Define fields of ISAR registers
Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 88 1 file changed, 88 insertions(+) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 54362ddce8..f00c0444c4 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1443,6 +1443,94 @@ FIELD(V7M_CSSELR, LEVEL, 1, 3) */ FIELD(V7M_CSSELR, INDEX, 0, 4) +/* + * System register ID fields. + */ +FIELD(ID_ISAR0, SWAP, 0, 4) +FIELD(ID_ISAR0, BITCOUNT, 4, 4) +FIELD(ID_ISAR0, BITFIELD, 8, 4) +FIELD(ID_ISAR0, CMPBRANCH, 12, 4) +FIELD(ID_ISAR0, COPROC, 16, 4) +FIELD(ID_ISAR0, DEBUG, 20, 4) +FIELD(ID_ISAR0, DIVIDE, 24, 4) + +FIELD(ID_ISAR1, ENDIAN, 0, 4) +FIELD(ID_ISAR1, EXCEPT, 4, 4) +FIELD(ID_ISAR1, EXCEPT_AR, 8, 4) +FIELD(ID_ISAR1, EXTEND, 12, 4) +FIELD(ID_ISAR1, IFTHEN, 16, 4) +FIELD(ID_ISAR1, IMMEDIATE, 20, 4) +FIELD(ID_ISAR1, INTERWORK, 24, 4) +FIELD(ID_ISAR1, JAZELLE, 28, 4) + +FIELD(ID_ISAR2, LOADSTORE, 0, 4) +FIELD(ID_ISAR2, MEMHINT, 4, 4) +FIELD(ID_ISAR2, MULTIACCESSINT, 8, 4) +FIELD(ID_ISAR2, MULT, 12, 4) +FIELD(ID_ISAR2, MULTS, 16, 4) +FIELD(ID_ISAR2, MULTU, 20, 4) +FIELD(ID_ISAR2, PSR_AR, 24, 4) +FIELD(ID_ISAR2, REVERSAL, 28, 4) + +FIELD(ID_ISAR3, SATURATE, 0, 4) +FIELD(ID_ISAR3, SIMD, 4, 4) +FIELD(ID_ISAR3, SVC, 8, 4) +FIELD(ID_ISAR3, SYNCHPRIM, 12, 4) +FIELD(ID_ISAR3, TABBRANCH, 16, 4) +FIELD(ID_ISAR3, T32COPY, 20, 4) +FIELD(ID_ISAR3, TRUENOP, 24, 4) +FIELD(ID_ISAR3, T32EE, 28, 4) + +FIELD(ID_ISAR4, UNPRIV, 0, 4) +FIELD(ID_ISAR4, WITHSHIFTS, 4, 4) +FIELD(ID_ISAR4, WRITEBACK, 8, 4) +FIELD(ID_ISAR4, SMC, 12, 4) +FIELD(ID_ISAR4, BARRIER, 16, 4) +FIELD(ID_ISAR4, SYNCHPRIM_FRAC, 20, 4) +FIELD(ID_ISAR4, PSR_M, 24, 4) +FIELD(ID_ISAR4, SWP_FRAC, 28, 4) + +FIELD(ID_ISAR5, SEVL, 0, 4) +FIELD(ID_ISAR5, AES, 4, 4) +FIELD(ID_ISAR5, SHA1, 8, 4) +FIELD(ID_ISAR5, SHA2, 12, 4) +FIELD(ID_ISAR5, CRC32, 16, 4) +FIELD(ID_ISAR5, RDM, 24, 4) +FIELD(ID_ISAR5, VCMA, 28, 4) + +FIELD(ID_ISAR6, JSCVT, 0, 4) +FIELD(ID_ISAR6, DP, 4, 4) +FIELD(ID_ISAR6, FHM, 8, 4) +FIELD(ID_ISAR6, SB, 12, 4) +FIELD(ID_ISAR6, SPECRES, 16, 4) + +FIELD(ID_AA64ISAR0, AES, 4, 4) +FIELD(ID_AA64ISAR0, SHA1, 8, 4) +FIELD(ID_AA64ISAR0, SHA2, 12, 4) +FIELD(ID_AA64ISAR0, CRC32, 16, 4) +FIELD(ID_AA64ISAR0, ATOMIC, 20, 4) +FIELD(ID_AA64ISAR0, RDM, 28, 4) +FIELD(ID_AA64ISAR0, SHA3, 32, 4) +FIELD(ID_AA64ISAR0, SM3, 36, 4) +FIELD(ID_AA64ISAR0, SM4, 40, 4) +FIELD(ID_AA64ISAR0, DP, 44, 4) +FIELD(ID_AA64ISAR0, FHM, 48, 4) +FIELD(ID_AA64ISAR0, TS, 52, 4) +FIELD(ID_AA64ISAR0, TLB, 56, 4) +FIELD(ID_AA64ISAR0, RNDR, 60, 4) + +FIELD(ID_AA64ISAR1, DPB, 0, 4) +FIELD(ID_AA64ISAR1, APA, 4, 4) +FIELD(ID_AA64ISAR1, API, 8, 4) +FIELD(ID_AA64ISAR1, JSCVT, 12, 4) +FIELD(ID_AA64ISAR1, FCMA, 16, 4) +FIELD(ID_AA64ISAR1, LRCPC, 20, 4) +FIELD(ID_AA64ISAR1, GPA, 24, 4) +FIELD(ID_AA64ISAR1, GPI, 28, 4) +FIELD(ID_AA64ISAR1, FRINTTS, 32, 4) +FIELD(ID_AA64ISAR1, SB, 36, 4) +FIELD(ID_AA64ISAR1, SPECRES, 40, 4) + QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK); /* If adding a feature bit which corresponds to a Linux ELF -- 2.17.1
Re: [Qemu-devel] [PATCH v1 10/12] target/arm: Add the Cortex-A72
On Mon, Oct 08, 2018 at 02:10:29PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias > wrote: > > From: "Edgar E. Iglesias" > > > > Add the ARM Cortex-A72. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > target/arm/cpu64.c | 59 > > ++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > > index 800bff7..02e500b 100644 > > --- a/target/arm/cpu64.c > > +++ b/target/arm/cpu64.c > > @@ -218,6 +218,64 @@ static void aarch64_a53_initfn(Object *obj) > > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > } > > > > +static void aarch64_a72_initfn(Object *obj) > > +{ > > +ARMCPU *cpu = ARM_CPU(obj); > > + > > +cpu->dtb_compatible = "arm,cortex-a72"; > > +set_feature(&cpu->env, ARM_FEATURE_V8); > > +set_feature(&cpu->env, ARM_FEATURE_VFP4); > > +set_feature(&cpu->env, ARM_FEATURE_NEON); > > +set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); > > +set_feature(&cpu->env, ARM_FEATURE_AARCH64); > > +set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); > > +set_feature(&cpu->env, ARM_FEATURE_V8_AES); > > +set_feature(&cpu->env, ARM_FEATURE_V8_SHA1); > > +set_feature(&cpu->env, ARM_FEATURE_V8_SHA256); > > +set_feature(&cpu->env, ARM_FEATURE_V8_PMULL); > > +set_feature(&cpu->env, ARM_FEATURE_CRC); > > +set_feature(&cpu->env, ARM_FEATURE_EL2); > > +set_feature(&cpu->env, ARM_FEATURE_EL3); > > No ARM_FEATURE_PMU ? Will add in v2. > > > +cpu->midr = 0x410fd083; > > +cpu->revidr = 0x; > > +cpu->reset_fpsid = 0x41034080; > > +cpu->mvfr0 = 0x10110222; > > +cpu->mvfr1 = 0x1211; > > +cpu->mvfr2 = 0x0043; > > +cpu->ctr = 0x8444c004; > > +cpu->reset_sctlr = 0x00c50838; > > Do you happen to have the hardware to hand to check what the > top 4 bits of the reset value of SCTLR_ELx are? I think they > should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as > does the A72 TRM, though its top level summary table lists > 0x00c50838 as the reset value for some of the SCTLR_ELx.) > > QEMU may have the wrong value for A53/A57 here too, I suspect. I don't have access to the A72s at the moment but looking at logs it seems to be 0x00c50838 for both the A53 and A72. Looking at "Table 4-118 SCTLR bit assignments" in the A72 TRM, bits [30:28] seem to have been allocated. Bit 30 depends on configuration inputs to the core and [29:28] seem to be hard-coded to zero. On ZynqMP and Versal, bit 30 (CFGTE) seems to be runtime configurable prior to releasing the A53s/A72s from reset. I haven't tried it though. I ran a test on the ZynqMP a53s and got the folloing right after reset, for EL1, EL2 and EL3: sctlr c52838 30c50838 c52838 The 0x2000 mask is due to V-hivecs being set. SCLTR_EL2 seems to get the RES1 bits set to 1... Perhaps we should just leave it as is for now? > > > +cpu->id_pfr0 = 0x0131; > > +cpu->id_pfr1 = 0x00011011; > > +cpu->id_dfr0 = 0x03010066; > > +cpu->id_afr0 = 0x; > > +cpu->id_mmfr0 = 0x10201105; > > +cpu->id_mmfr1 = 0x4000; > > +cpu->id_mmfr2 = 0x0126; > > +cpu->id_mmfr3 = 0x02102211; > > +cpu->id_isar0 = 0x02101110; > > +cpu->id_isar1 = 0x13112111; > > +cpu->id_isar2 = 0x21232042; > > +cpu->id_isar3 = 0x01112131; > > +cpu->id_isar4 = 0x00011142; > > +cpu->id_isar5 = 0x00011121; > > +cpu->id_aa64pfr0 = 0x; > > +cpu->id_aa64dfr0 = 0x10305106; > > +cpu->pmceid0 = 0x; > > +cpu->pmceid1 = 0x; > > +cpu->id_aa64isar0 = 0x00011120; > > +cpu->id_aa64mmfr0 = 0x1124; > > +cpu->dbgdidr = 0x3516d000; > > +cpu->clidr = 0x0a200023; > > +cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */ > > +cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */ > > +cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */ > > +cpu->dcz_blocksize = 4; /* 64 bytes */ > > +cpu->gic_num_lrs = 4; > > +cpu->gic_vpribits = 5; > > +cpu->gic_vprebits = 5; > > +define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo); > > The impdef registers do seem to be basically the same as > the A53/A57. The function name is now a bit inaccurate, > though, but I don't have a good idea for a better name. > > Otherwise > Reviewed-by: Peter Maydell > > thanks > -- PMM
[Qemu-devel] [PATCH v3 05/10] target/arm: Fix cortex-a7 id_isar0
The incorrect value advertised only thumb2 div without arm div. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index ac46641541..83a6cb535f 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1587,7 +1587,10 @@ static void cortex_a7_initfn(Object *obj) cpu->id_mmfr1 = 0x4000; cpu->id_mmfr2 = 0x0124; cpu->id_mmfr3 = 0x02102211; -cpu->id_isar0 = 0x01101110; +/* a7_mpcore_r0p5_trm, page 4-4 gives 0x01101110; but + * table 4-41 gives 0x02101110, which includes the arm div insns. + */ +cpu->id_isar0 = 0x02101110; cpu->id_isar1 = 0x13112111; cpu->id_isar2 = 0x21232041; cpu->id_isar3 = 0x2131; -- 2.17.1
[Qemu-devel] [PATCH v3 07/10] target/arm: Convert jazelle from feature bit to isar1 test
Having V6 alone imply jazelle was wrong for cortex-m0. Change to an assertion for V6 & !M. This was harmless, because the only place we tested ARM_FEATURE_JAZELLE was for 'bxj' in disas_arm(), which is unreachable for M-profile cores. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 6 +- target/arm/translate.h | 1 + target/arm/cpu.c | 17 ++--- target/arm/translate.c | 2 +- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 2af971e823..557ef8daf9 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1580,7 +1580,6 @@ enum arm_features { ARM_FEATURE_PMU, /* has PMU support */ ARM_FEATURE_VBAR, /* has cp15 VBAR */ ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ -ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */ ARM_FEATURE_SVE, /* has Scalable Vector Extension */ ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */ ARM_FEATURE_M_MAIN, /* M profile Main Extension */ @@ -3147,6 +3146,11 @@ static inline bool aa32_feature_arm_div(ARMCPU *cpu) return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) > 1; } +static inline bool aa32_feature_jazelle(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar1, ID_ISAR1, JAZELLE) != 0; +} + static inline bool aa32_feature_aes(ARMCPU *cpu) { return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0; diff --git a/target/arm/translate.h b/target/arm/translate.h index 8279465d1d..bd394bdf69 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -197,6 +197,7 @@ static inline TCGv_i32 get_ahp_flag(void) FORWARD_FEATURE(thumb_div) FORWARD_FEATURE(arm_div) +FORWARD_FEATURE(jazelle) FORWARD_FEATURE(aes) FORWARD_FEATURE(pmull) FORWARD_FEATURE(sha1) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index f068b4c476..4e2609aa7e 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -850,8 +850,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } if (arm_feature(env, ARM_FEATURE_V6)) { set_feature(env, ARM_FEATURE_V5); -set_feature(env, ARM_FEATURE_JAZELLE); if (!arm_feature(env, ARM_FEATURE_M)) { +assert(aa32_feature_jazelle(cpu)); set_feature(env, ARM_FEATURE_AUXCR); } } @@ -1078,11 +1078,16 @@ static void arm926_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_VFP); set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); set_feature(&cpu->env, ARM_FEATURE_CACHE_TEST_CLEAN); -set_feature(&cpu->env, ARM_FEATURE_JAZELLE); cpu->midr = 0x41069265; cpu->reset_fpsid = 0x41011090; cpu->ctr = 0x1dd20d2; cpu->reset_sctlr = 0x00090078; + +/* + * ARMv5 does not have the ID_ISAR registers, but we can still + * set the field to indicate Jazelle support within QEMU. + */ +cpu->id_isar1 = FIELD_DP32(cpu->id_isar1, ID_ISAR1, JAZELLE, 1); } static void arm946_initfn(Object *obj) @@ -1108,12 +1113,18 @@ static void arm1026_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_AUXCR); set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); set_feature(&cpu->env, ARM_FEATURE_CACHE_TEST_CLEAN); -set_feature(&cpu->env, ARM_FEATURE_JAZELLE); cpu->midr = 0x4106a262; cpu->reset_fpsid = 0x410110a0; cpu->ctr = 0x1dd20d2; cpu->reset_sctlr = 0x00090078; cpu->reset_auxcr = 1; + +/* + * ARMv5 does not have the ID_ISAR registers, but we can still + * set the field to indicate Jazelle support within QEMU. + */ +cpu->id_isar1 = FIELD_DP32(cpu->id_isar1, ID_ISAR1, JAZELLE, 1); + { /* The 1026 had an IFAR at c6,c0,0,1 rather than the ARMv6 c6,c0,0,2 */ ARMCPRegInfo ifar = { diff --git a/target/arm/translate.c b/target/arm/translate.c index b1ee6533cc..54ecf369cb 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -42,7 +42,7 @@ #define ENABLE_ARCH_5 arm_dc_feature(s, ARM_FEATURE_V5) /* currently all emulated v5 cores are also v5TE, so don't bother */ #define ENABLE_ARCH_5TE arm_dc_feature(s, ARM_FEATURE_V5) -#define ENABLE_ARCH_5Jarm_dc_feature(s, ARM_FEATURE_JAZELLE) +#define ENABLE_ARCH_5Jaa32_dc_feature_jazelle(s) #define ENABLE_ARCH_6 arm_dc_feature(s, ARM_FEATURE_V6) #define ENABLE_ARCH_6Karm_dc_feature(s, ARM_FEATURE_V6K) #define ENABLE_ARCH_6T2 arm_dc_feature(s, ARM_FEATURE_THUMB2) -- 2.17.1
[Qemu-devel] [PATCH v3 04/10] target/arm: Align cortex-r5 id_isar0
The missing nibble made it more difficult to read. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index b7d9942aa3..ac46641541 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1397,7 +1397,7 @@ static void cortex_r5_initfn(Object *obj) cpu->id_mmfr1 = 0x; cpu->id_mmfr2 = 0x0120; cpu->id_mmfr3 = 0x0211; -cpu->id_isar0 = 0x210; +cpu->id_isar0 = 0x0210; cpu->id_isar1 = 0x13112111; cpu->id_isar2 = 0x21232141; cpu->id_isar3 = 0x01112131; -- 2.17.1
[Qemu-devel] [PATCH v3 06/10] target/arm: Convert division from feature bits to isar0 tests
Both arm and thumb2 division are controlled by the same ISAR field, which takes care of the arm implies thumb case. Having M imply thumb2 division was wrong for cortex-m0, which is v6m and does not have thumb2 at all, much less thumb2 division. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 12 ++-- target/arm/translate.h | 2 ++ linux-user/elfload.c | 4 ++-- target/arm/cpu.c | 10 +- target/arm/translate.c | 4 ++-- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 66eaa40ed9..2af971e823 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1550,7 +1550,6 @@ enum arm_features { ARM_FEATURE_VFP3, ARM_FEATURE_VFP_FP16, ARM_FEATURE_NEON, -ARM_FEATURE_THUMB_DIV, /* divide supported in Thumb encoding */ ARM_FEATURE_M, /* Microcontroller profile. */ ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ ARM_FEATURE_THUMB2EE, @@ -1560,7 +1559,6 @@ enum arm_features { ARM_FEATURE_V5, ARM_FEATURE_STRONGARM, ARM_FEATURE_VAPA, /* cp15 VA to PA lookups */ -ARM_FEATURE_ARM_DIV, /* divide supported in ARM encoding */ ARM_FEATURE_VFP4, /* VFPv4 (implies that NEON is v2) */ ARM_FEATURE_GENERIC_TIMER, ARM_FEATURE_MVFR, /* Media and VFP Feature Registers 0 and 1 */ @@ -3139,6 +3137,16 @@ extern const uint64_t pred_esz_masks[4]; /* * 32-bit feature tests via id registers. */ +static inline bool aa32_feature_thumb_div(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) != 0; +} + +static inline bool aa32_feature_arm_div(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) > 1; +} + static inline bool aa32_feature_aes(ARMCPU *cpu) { return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0; diff --git a/target/arm/translate.h b/target/arm/translate.h index 1d60569583..8279465d1d 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -195,6 +195,8 @@ static inline TCGv_i32 get_ahp_flag(void) static inline bool aa32_dc_feature_##NAME(DisasContext *dc) \ { return aa32_feature_##NAME(dc->cpu); } +FORWARD_FEATURE(thumb_div) +FORWARD_FEATURE(arm_div) FORWARD_FEATURE(aes) FORWARD_FEATURE(pmull) FORWARD_FEATURE(sha1) diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 571beca8fd..9b00e977d8 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -471,8 +471,8 @@ static uint32_t get_elf_hwcap(void) GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3); GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS); GET_FEATURE(ARM_FEATURE_VFP4, ARM_HWCAP_ARM_VFPv4); -GET_FEATURE(ARM_FEATURE_ARM_DIV, ARM_HWCAP_ARM_IDIVA); -GET_FEATURE(ARM_FEATURE_THUMB_DIV, ARM_HWCAP_ARM_IDIVT); +GET_FEATURE_ID(arm_div, ARM_HWCAP_ARM_IDIVA); +GET_FEATURE_ID(thumb_div, ARM_HWCAP_ARM_IDIVT); /* All QEMU's VFPv3 CPUs have 32 registers, see VFP_DREG in translate.c. * Note that the ARM_HWCAP_ARM_VFPv3D16 bit is always the inverse of * ARM_HWCAP_ARM_VFPD32 (and so always clear for QEMU); it is unrelated diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 83a6cb535f..f068b4c476 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -825,7 +825,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) * Presence of EL2 itself is ARM_FEATURE_EL2, and of the * Security Extensions is ARM_FEATURE_EL3. */ -set_feature(env, ARM_FEATURE_ARM_DIV); +assert(aa32_feature_arm_div(cpu)); set_feature(env, ARM_FEATURE_LPAE); set_feature(env, ARM_FEATURE_V7); } @@ -858,12 +858,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) if (arm_feature(env, ARM_FEATURE_V5)) { set_feature(env, ARM_FEATURE_V4T); } -if (arm_feature(env, ARM_FEATURE_M)) { -set_feature(env, ARM_FEATURE_THUMB_DIV); -} -if (arm_feature(env, ARM_FEATURE_ARM_DIV)) { -set_feature(env, ARM_FEATURE_THUMB_DIV); -} if (arm_feature(env, ARM_FEATURE_VFP4)) { set_feature(env, ARM_FEATURE_VFP3); set_feature(env, ARM_FEATURE_VFP_FP16); @@ -1384,8 +1378,6 @@ static void cortex_r5_initfn(Object *obj) ARMCPU *cpu = ARM_CPU(obj); set_feature(&cpu->env, ARM_FEATURE_V7); -set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV); -set_feature(&cpu->env, ARM_FEATURE_ARM_DIV); set_feature(&cpu->env, ARM_FEATURE_V7MP); set_feature(&cpu->env, ARM_FEATURE_PMSA); cpu->midr = 0x411fc153; /* r1p3 */ diff --git a/target/arm/translate.c b/target/arm/translate.c index ea8545c43b..b1ee6533cc 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -9755,7 +9755,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) case 1: case 3: /* SDIV, UDIV */ -if (!arm_dc_feature(s, ARM_FEATURE_ARM_DIV)) { +
[Qemu-devel] [PATCH v3 09/10] target/arm: Convert sve from feature bit to aa64pfr0 test
Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h| 16 +++- target/arm/translate-a64.h | 1 + linux-user/aarch64/signal.c | 4 ++-- linux-user/elfload.c| 2 +- linux-user/syscall.c| 10 ++ target/arm/cpu64.c | 5 - target/arm/helper.c | 9 ++--- target/arm/machine.c| 3 +-- target/arm/translate-a64.c | 4 ++-- 9 files changed, 38 insertions(+), 16 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index d8cb9633d2..a97b471fff 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1531,6 +1531,16 @@ FIELD(ID_AA64ISAR1, FRINTTS, 32, 4) FIELD(ID_AA64ISAR1, SB, 36, 4) FIELD(ID_AA64ISAR1, SPECRES, 40, 4) +FIELD(ID_AA64PFR0, EL0, 0, 4) +FIELD(ID_AA64PFR0, EL1, 4, 4) +FIELD(ID_AA64PFR0, EL2, 8, 4) +FIELD(ID_AA64PFR0, EL3, 12, 4) +FIELD(ID_AA64PFR0, FP, 16, 4) +FIELD(ID_AA64PFR0, ADVSIMD, 20, 4) +FIELD(ID_AA64PFR0, GIC, 24, 4) +FIELD(ID_AA64PFR0, RAS, 28, 4) +FIELD(ID_AA64PFR0, SVE, 32, 4) + QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK); /* If adding a feature bit which corresponds to a Linux ELF @@ -1579,7 +1589,6 @@ enum arm_features { ARM_FEATURE_PMU, /* has PMU support */ ARM_FEATURE_VBAR, /* has cp15 VBAR */ ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ -ARM_FEATURE_SVE, /* has Scalable Vector Extension */ ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */ ARM_FEATURE_M_MAIN, /* M profile Main Extension */ }; @@ -3263,4 +3272,9 @@ static inline bool aa64_feature_fcma(ARMCPU *cpu) return FIELD_EX64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0; } +static inline bool aa64_feature_sve(ARMCPU *cpu) +{ +return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE) != 0; +} + #endif diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h index b4ef9eb024..636f3fded3 100644 --- a/target/arm/translate-a64.h +++ b/target/arm/translate-a64.h @@ -140,6 +140,7 @@ FORWARD_FEATURE(sm3) FORWARD_FEATURE(sm4) FORWARD_FEATURE(dp) FORWARD_FEATURE(fcma) +FORWARD_FEATURE(sve) #undef FORWARD_FEATURE diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c index 07fedfc33c..65272fb7a9 100644 --- a/linux-user/aarch64/signal.c +++ b/linux-user/aarch64/signal.c @@ -314,7 +314,7 @@ static int target_restore_sigframe(CPUARMState *env, break; case TARGET_SVE_MAGIC: -if (arm_feature(env, ARM_FEATURE_SVE)) { +if (aa64_feature_sve(arm_env_get_cpu(env))) { vq = (env->vfp.zcr_el[1] & 0xf) + 1; sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16); if (!sve && size == sve_size) { @@ -433,7 +433,7 @@ static void target_setup_frame(int usig, struct target_sigaction *ka, &layout); /* SVE state needs saving only if it exists. */ -if (arm_feature(env, ARM_FEATURE_SVE)) { +if (aa64_feature_sve(arm_env_get_cpu(env))) { vq = (env->vfp.zcr_el[1] & 0xf) + 1; sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16); sve_ofs = alloc_sigframe_space(sve_size, &layout); diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 3061d703b2..e3585f4cb6 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -593,7 +593,7 @@ static uint32_t get_elf_hwcap(void) GET_FEATURE_ID(rdm, ARM_HWCAP_A64_ASIMDRDM); GET_FEATURE_ID(dp, ARM_HWCAP_A64_ASIMDDP); GET_FEATURE_ID(fcma, ARM_HWCAP_A64_FCMA); -GET_FEATURE(ARM_FEATURE_SVE, ARM_HWCAP_A64_SVE); +GET_FEATURE_ID(sve, ARM_HWCAP_A64_SVE); #undef GET_FEATURE #undef GET_FEATURE_ID diff --git a/linux-user/syscall.c b/linux-user/syscall.c index ae3c0dfef7..37f315ad4a 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -9356,7 +9356,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, * even though the current architectural maximum is VQ=16. */ ret = -TARGET_EINVAL; -if (arm_feature(cpu_env, ARM_FEATURE_SVE) +if (aa64_feature_sve(arm_env_get_cpu(cpu_env)) && arg2 >= 0 && arg2 <= 512 * 16 && !(arg2 & 15)) { CPUARMState *env = cpu_env; ARMCPU *cpu = arm_env_get_cpu(env); @@ -9375,9 +9375,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, return ret; case TARGET_PR_SVE_GET_VL: ret = -TARGET_EINVAL; -if (arm_feature(cpu_env, ARM_FEATURE_SVE)) { -CPUARMState *env = cpu_env; -ret = ((env->vfp.zcr_el[1] & 0xf) + 1) * 16; +{ +ARMCPU *cpu = arm_env_get_cpu(cpu_env); +if (aa64_feature_sve(cpu)) { +ret = ((cpu->env.vfp.zcr_el[1] & 0xf) + 1) * 16; +} } return ret; #endif /* AARCH64
[Qemu-devel] [PATCH v3 08/10] target/arm: Convert t32ee from feature bit to isar3 test
Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 6 +- linux-user/elfload.c | 2 +- target/arm/cpu.c | 4 target/arm/helper.c | 2 +- target/arm/machine.c | 3 +-- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 557ef8daf9..d8cb9633d2 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1552,7 +1552,6 @@ enum arm_features { ARM_FEATURE_NEON, ARM_FEATURE_M, /* Microcontroller profile. */ ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ -ARM_FEATURE_THUMB2EE, ARM_FEATURE_V7MP,/* v7 Multiprocessing Extensions */ ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */ ARM_FEATURE_V4T, @@ -3151,6 +3150,11 @@ static inline bool aa32_feature_jazelle(ARMCPU *cpu) return FIELD_EX32(cpu->id_isar1, ID_ISAR1, JAZELLE) != 0; } +static inline bool aa32_feature_t32ee(ARMCPU *cpu) +{ +return FIELD_EX32(cpu->id_isar3, ID_ISAR3, T32EE) != 0; +} + static inline bool aa32_feature_aes(ARMCPU *cpu) { return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0; diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 9b00e977d8..3061d703b2 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -466,7 +466,7 @@ static uint32_t get_elf_hwcap(void) GET_FEATURE(ARM_FEATURE_V5, ARM_HWCAP_ARM_EDSP); GET_FEATURE(ARM_FEATURE_VFP, ARM_HWCAP_ARM_VFP); GET_FEATURE(ARM_FEATURE_IWMMXT, ARM_HWCAP_ARM_IWMMXT); -GET_FEATURE(ARM_FEATURE_THUMB2EE, ARM_HWCAP_ARM_THUMBEE); +GET_FEATURE_ID(t32ee, ARM_HWCAP_ARM_THUMBEE); GET_FEATURE(ARM_FEATURE_NEON, ARM_HWCAP_ARM_NEON); GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3); GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 4e2609aa7e..f8faea7933 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1436,7 +1436,6 @@ static void cortex_a8_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_V7); set_feature(&cpu->env, ARM_FEATURE_VFP3); set_feature(&cpu->env, ARM_FEATURE_NEON); -set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); set_feature(&cpu->env, ARM_FEATURE_EL3); cpu->midr = 0x410fc080; @@ -1505,7 +1504,6 @@ static void cortex_a9_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_VFP3); set_feature(&cpu->env, ARM_FEATURE_VFP_FP16); set_feature(&cpu->env, ARM_FEATURE_NEON); -set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); set_feature(&cpu->env, ARM_FEATURE_EL3); /* Note that A9 supports the MP extensions even for * A9UP and single-core A9MP (which are both different @@ -1568,7 +1566,6 @@ static void cortex_a7_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_V7VE); set_feature(&cpu->env, ARM_FEATURE_VFP4); set_feature(&cpu->env, ARM_FEATURE_NEON); -set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); @@ -1614,7 +1611,6 @@ static void cortex_a15_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_V7VE); set_feature(&cpu->env, ARM_FEATURE_VFP4); set_feature(&cpu->env, ARM_FEATURE_NEON); -set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); diff --git a/target/arm/helper.c b/target/arm/helper.c index 0efbb5c76c..0da13175be 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5356,7 +5356,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); define_arm_cp_regs(cpu, vmsa_cp_reginfo); } -if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { +if (aa32_feature_t32ee(cpu)) { define_arm_cp_regs(cpu, t2ee_cp_reginfo); } if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { diff --git a/target/arm/machine.c b/target/arm/machine.c index ff4ec22bf7..d44e891533 100644 --- a/target/arm/machine.c +++ b/target/arm/machine.c @@ -301,9 +301,8 @@ static const VMStateDescription vmstate_m = { static bool thumb2ee_needed(void *opaque) { ARMCPU *cpu = opaque; -CPUARMState *env = &cpu->env; -return arm_feature(env, ARM_FEATURE_THUMB2EE); +return aa32_feature_t32ee(cpu); } static const VMStateDescription vmstate_thumb2ee = { -- 2.17.1
[Qemu-devel] [PATCH v3 10/10] target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test
Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 17 +++- target/arm/translate-a64.h | 1 + target/arm/translate.h | 1 + linux-user/elfload.c | 6 +- target/arm/cpu64.c | 9 ++--- target/arm/helper.c| 2 +- target/arm/translate-a64.c | 40 +++--- target/arm/translate.c | 6 +++--- 8 files changed, 45 insertions(+), 37 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index a97b471fff..1c880b0c29 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1589,7 +1589,6 @@ enum arm_features { ARM_FEATURE_PMU, /* has PMU support */ ARM_FEATURE_VBAR, /* has cp15 VBAR */ ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ -ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */ ARM_FEATURE_M_MAIN, /* M profile Main Extension */ }; @@ -3204,6 +3203,16 @@ static inline bool aa32_feature_dp(ARMCPU *cpu) return FIELD_EX32(cpu->id_isar6, ID_ISAR6, DP) != 0; } +static inline bool aa32_feature_fp16_arith(ARMCPU *cpu) +{ +/* + * This is a placeholder for use by VCMA until the rest of + * the ARMv8.2-FP16 extension is implemented for aa32 mode. + * At which point we can properly set and check MVFR1.FPHP. + */ +return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1; +} + /* * 64-bit feature tests via id registers. */ @@ -3272,6 +3281,12 @@ static inline bool aa64_feature_fcma(ARMCPU *cpu) return FIELD_EX64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0; } +static inline bool aa64_feature_fp16(ARMCPU *cpu) +{ +/* We always set the AdvSIMD and FP fields identically wrt FP16. */ +return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1; +} + static inline bool aa64_feature_sve(ARMCPU *cpu) { return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE) != 0; diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h index 636f3fded3..e122cef242 100644 --- a/target/arm/translate-a64.h +++ b/target/arm/translate-a64.h @@ -140,6 +140,7 @@ FORWARD_FEATURE(sm3) FORWARD_FEATURE(sm4) FORWARD_FEATURE(dp) FORWARD_FEATURE(fcma) +FORWARD_FEATURE(fp16) FORWARD_FEATURE(sve) #undef FORWARD_FEATURE diff --git a/target/arm/translate.h b/target/arm/translate.h index bd394bdf69..ad911de98c 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -206,6 +206,7 @@ FORWARD_FEATURE(crc32) FORWARD_FEATURE(rdm) FORWARD_FEATURE(vcma) FORWARD_FEATURE(dp) +FORWARD_FEATURE(fp16_arith) #undef FORWARD_FEATURE diff --git a/linux-user/elfload.c b/linux-user/elfload.c index e3585f4cb6..d041ef9d49 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -573,8 +573,6 @@ static uint32_t get_elf_hwcap(void) hwcaps |= ARM_HWCAP_A64_ASIMD; /* probe for the extra features */ -#define GET_FEATURE(feat, hwcap) \ -do { if (arm_feature(&cpu->env, feat)) { hwcaps |= hwcap; } } while (0) #define GET_FEATURE_ID(feat, hwcap) \ do { if (aa64_feature_##feat(cpu)) { hwcaps |= hwcap; } } while (0) @@ -587,15 +585,13 @@ static uint32_t get_elf_hwcap(void) GET_FEATURE_ID(sha3, ARM_HWCAP_A64_SHA3); GET_FEATURE_ID(sm3, ARM_HWCAP_A64_SM3); GET_FEATURE_ID(sm4, ARM_HWCAP_A64_SM4); -GET_FEATURE(ARM_FEATURE_V8_FP16, -ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP); +GET_FEATURE_ID(fp16, ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP); GET_FEATURE_ID(atomics, ARM_HWCAP_A64_ATOMICS); GET_FEATURE_ID(rdm, ARM_HWCAP_A64_ASIMDRDM); GET_FEATURE_ID(dp, ARM_HWCAP_A64_ASIMDDP); GET_FEATURE_ID(fcma, ARM_HWCAP_A64_FCMA); GET_FEATURE_ID(sve, ARM_HWCAP_A64_SVE); -#undef GET_FEATURE #undef GET_FEATURE_ID return hwcaps; diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index ee2c04a627..38e9afef3b 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -266,6 +266,8 @@ static void aarch64_max_initfn(Object *obj) t = cpu->id_aa64pfr0; t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); +t = FIELD_DP64(t, ID_AA64PFR0, FP, 1); +t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1); cpu->id_aa64pfr0 = t; /* Replicate the same data to the 32-bit id registers. */ @@ -283,13 +285,6 @@ static void aarch64_max_initfn(Object *obj) cpu->id_isar6 = u; #ifdef CONFIG_USER_ONLY -/* We don't set these in system emulation mode for the moment, - * since we don't correctly set the ID registers to advertise them, - * and in some cases they're only available in AArch64 and not AArch32, - * whereas the architecture requires them to be present in both if - * present in either. - */ -set_feature(&cpu->env, ARM_FEATURE_V8_FP16); /* For usermode -cpu max we can use a larger and more efficient DCZ * blocksize since we don't have to follow what the hardware does. */ diff --git a/target/arm/helper.c b/target/arm/h
Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
On 08.10.18 19:31, Markus Armbruster wrote: > Calling error_report() from within a a function that takes an Error ** > argument is suspicious. drive_new() does that, and its caller > drive_init_func() then exit()s. I'm afraid I don't quite follow you here. There is no function here that takes an Error ** already and then calls error_report(). There is however drive_new() that does not take an Error **, consequentially calls error_report(), and there is its caller drive_init_func() which does take an Error ** but does not set it. So while I fully agree with you to make drive_new() take an Error ** (and thus effectively fix drive_init_func()), I don't quite understand this explanation. (Furthermore, drive_init_func() does not exit(). It's main() that exit()s after calling drive_init_func().) > Its caller main(), via > qemu_opts_foreach(), is fine with it, but clean it up anyway: > > * Convert drive_new() to Error > > * Update add_init_drive() to report the error received from > drive_new(). > > * Make main() pass &error_fatal through qemu_opts_foreach(), > drive_init_func() to drive_new() > > * Make default_drive() pass &error_abort through qemu_opts_foreach(), > drive_init_func() to drive_new() > > Cc: Kevin Wolf > Cc: Max Reitz > Signed-off-by: Markus Armbruster > --- > blockdev.c| 27 ++- > device-hotplug.c | 5 - > include/sysemu/blockdev.h | 3 ++- > vl.c | 11 --- > 4 files changed, 24 insertions(+), 22 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index a8755bd908..574adbcb7f 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = { [...] > @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, > BlockInterfaceType block_default_type) > bs_opts = NULL; > if (!blk) { > if (local_err) { > -error_report_err(local_err); > +error_propagate(errp, local_err); > } Wait, what would be the case where blockdev_init() returns NULL but *errp remains unse——— oh no. There is only one case which is someone specified "format=help". Hm. I suppose you are as unhappy as me about the fact that because of this drive_new() cannot guarantee that *errp is set in case of an error. I think it's ""fine"" (*gnashing teeth*) to keep it this way, but it means that callers need to continue to check the return value and not *errp alone. > goto fail; > } else { > diff --git a/device-hotplug.c b/device-hotplug.c > index cd427e2c76..6090d5f1e9 100644 > --- a/device-hotplug.c > +++ b/device-hotplug.c > @@ -28,6 +28,7 @@ > #include "sysemu/block-backend.h" > #include "sysemu/blockdev.h" > #include "qapi/qmp/qdict.h" > +#include "qapi/error.h" > #include "qemu/config-file.h" > #include "qemu/option.h" > #include "sysemu/sysemu.h" > @@ -36,6 +37,7 @@ > > static DriveInfo *add_init_drive(const char *optstr) > { > +Error *err = NULL; > DriveInfo *dinfo; > QemuOpts *opts; > MachineClass *mc; > @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr) > return NULL; > > mc = MACHINE_GET_CLASS(current_machine); > -dinfo = drive_new(opts, mc->block_default_type); > +dinfo = drive_new(opts, mc->block_default_type, &err); > if (!dinfo) { > +error_report_err(err); > qemu_opts_del(opts); > return NULL; > } > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h > index 24954b94e0..d34c4920dc 100644 > --- a/include/sysemu/blockdev.h > +++ b/include/sysemu/blockdev.h > @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type); > QemuOpts *drive_def(const char *optstr); > QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, > const char *optstr); > -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); > +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type, > + Error **errp); > > /* device-hotplug */ > > diff --git a/vl.c b/vl.c > index 0d25956b2f..101e0123d9 100644 > --- a/vl.c > +++ b/vl.c > @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts > *opts, Error **errp) > { > BlockInterfaceType *block_default_type = opaque; > > -return drive_new(opts, *block_default_type) == NULL; > +return drive_new(opts, *block_default_type, errp) == NULL; > } > > static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp) > @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, > BlockInterfaceType type, > drive_enable_snapshot(NULL, opts, NULL); > } > > -dinfo = drive_new(opts, type); > -assert(dinfo); > +dinfo = drive_new(opts, type, &error_abort); Which means the assertion is still necessary here. > dinfo->is_default = true; > > } > @@ -4348,10 +4347,8 @@ int main(int argc, c
Re: [Qemu-devel] [PATCH 29/31] vl: Assert drive_new() does not fail in default_drive()
On 08.10.18 19:31, Markus Armbruster wrote: > If creating (empty) default drives fails, it's a bug. Therefore, > assert() is more appropriate than exit(1). > > Cc: Kevin Wolf > Cc: Max Reitz > Signed-off-by: Markus Armbruster > --- > vl.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Rewrite corrupted L2 table entry, which reference space out of > underlying file. > > Make this L2 table entry read-as-all-zeros without any allocation. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 32 > 1 file changed, 32 insertions(+) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 3c004e5bfe..3de3768a3c 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs, > BdrvCheckResult *res, > /* Mark cluster as used */ > csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) * > BDRV_SECTOR_SIZE; > +if (csize > s->cluster_size) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"compressed cluster larger than cluster: size 0x%" > +PRIx64, csize); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + This seems recoverable, isn't it? Can we not try to just limit the csize, or decompress the cluster with the given csize from the given offset, disregarding the cluster limit? > coffset = l2_entry & s->cluster_offset_mask & >~(BDRV_SECTOR_SIZE - 1); > +if (coffset >= bdrv_getlength(bs->file->bs)) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"compressed cluster out of file: offset 0x%" PRIx64, > +coffset); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + > ret = qcow2_inc_refcounts_imrt(bs, res, > refcount_table, > refcount_table_size, > coffset, csize); > @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs, > BdrvCheckResult *res, > { > uint64_t offset = l2_entry & L2E_OFFSET_MASK; > > +if (offset >= bdrv_getlength(bs->file->bs)) { > +ret = fix_l2_entry_to_zero( > +bs, res, fix, l2_offset, i, active, > +"cluster out of file: offset 0x%" PRIx64, offset); > +if (ret < 0) { > +goto fail; > +} > +continue; > +} > + These other two look OK, but they have another issue: If this is a v2 image, you cannot create zero clusters; so you'll have to unallocate the cluster in that case. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() from within a a function that takes an Error ** s/a a/a/ argument is suspicious. drive_new() does that, and its caller drive_init_func() then exit()s. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway: * Convert drive_new() to Error * Update add_init_drive() to report the error received from drive_new(). * Make main() pass &error_fatal through qemu_opts_foreach(), drive_init_func() to drive_new() * Make default_drive() pass &error_abort through qemu_opts_foreach(), drive_init_func() to drive_new() Cc: Kevin Wolf Cc: Max Reitz Signed-off-by: Markus Armbruster --- blockdev.c| 27 ++- device-hotplug.c | 5 - include/sysemu/blockdev.h | 3 ++- vl.c | 11 --- 4 files changed, 24 insertions(+), 22 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 29/31] vl: Assert drive_new() does not fail in default_drive()
On 10/8/18 12:31 PM, Markus Armbruster wrote: If creating (empty) default drives fails, it's a bug. Therefore, assert() is more appropriate than exit(1). Cc: Kevin Wolf Cc: Max Reitz Signed-off-by: Markus Armbruster --- vl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) The assert disappears in the next patch (replaced by an &error_fatal), but the separation of patches makes sense. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 28/31] fsdev: Clean up error reporting in qemu_fsdev_add()
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() from within a a function that takes an Error ** s/a a/a/ argument is suspicious. qemu_fsdev_add() does that, and its caller fsdev_init_func() then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 08.10.18 22:22, Vladimir Sementsov-Ogievskiy wrote: > > > On 10/08/2018 06:31 PM, Max Reitz wrote: >> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat >>> an unpredictable amount of memory on corrupted table entries, which are >>> referencing regions far beyond the end of file. >>> >>> Prevent this, by skipping such regions from further processing. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> block/qcow2-refcount.c | 14 ++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>> index 615847eb09..566c19fbfa 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, >>> BdrvCheckResult *res, >>> { >>> BDRVQcow2State *s = bs->opaque; >>> uint64_t start, last, cluster_offset, k, refcount; >>> +int64_t file_len; >>> int ret; >>> >>> if (size <= 0) { >>> return 0; >>> } >>> >>> +file_len = bdrv_getlength(bs->file->bs); >>> +if (file_len < 0) { >>> +return file_len; >>> +} >> >> Doesn't this slow things down? Can we not cache the length somewhere >> and update it whenever the image is modified? > > > hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is > good idea to improve it locally for these series. If we can improve it > somehow with a cache or something like this, it should be done for all > users and therefore it is outside of these series.. I wanted to write: Sure it's used everywhere, but usually that is before someone performs some I/O, so it isn't too bad. But this is a function that's suppose to just increment a couple of values in memory, which is different. However, I put the "wanted to write" prefix there, because: I knew that we already have a central cache for bdrv_getlength(), but it isn't used when the block driver reports has_variable_length as true. I thought file-posix did that. But it only does so for CD-ROM devices. So I think it should be OK to call the function here, yes. >>> + >>> +if (offset + size - file_len > s->cluster_size) { >>> +fprintf(stderr, "ERROR: counting reference for region exceeding >>> the " >>> +"end of the file by more than one cluster: offset 0x%" >>> PRIx64 >>> +" size 0x%" PRIx64 "\n", offset, size); >> >> Why is one cluster OK? Is there a specific case you're trying to catch >> here? > > raw file under qcow2 may be not aligned in real size to qcow2 cluster, > as I understand, it's normal for the last cluster to be semi-allocated Ah, that's true, thanks. I'd appreciate a comment here, though, and in that case I think we don't need to check whether the reference is off by more than a cluster, but whether it's off by a cluster or more (so >=). Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 18/31] qom: Clean up error reporting in user_creatable_add_opts_foreach()
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() in a function that takes an Error ** argument is suspicious. user_creatable_add_opts_foreach() does that, and then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. Cc: Daniel P. Berrangé Signed-off-by: Markus Armbruster --- qemu-io.c | 8 +++- qemu-nbd.c | 8 +++- qom/object_interfaces.c | 4 +--- vl.c| 16 ++-- 4 files changed, 13 insertions(+), 23 deletions(-) While I could take this through my NBD tree, I think it makes more sense to go through your error tree with the rest of the series. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 02/31] block: Use warn_report() & friends to report warnings
On 10/8/18 12:30 PM, Markus Armbruster wrote: Calling error_report() in a function that takes an Error ** argument is suspicious. Convert a few that are actually warnings to warn_report(). While there, split warnings consisting of multiple sentences to conform to conventions spelled out in warn_report()'s contract, and improve a rather useless warning in sheepdog.c. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 01/31] Use error_fatal to simplify obvious fatal errors (again)
On 10/8/18 12:30 PM, Markus Armbruster wrote: Add a slight improvement of the Coccinelle semantic patch from commit 07d04a0219b, which shares the same commit title, but does not actually have a semantic patch, but rather defers to the even older 007b065. But I'm not too worried about either the duplicated commit title nor the chain of references to follow - as this is a no-semantic-change patch rather than a bugfix, it's less likely to cause confusion to any downstream backport efforts. and use it to clean up. It leaves dead Error * variables behind, cleaned up manually. Coccinelle can handle that too, if we want to make the .cocci file longer (but off-hand, I don't remember the exact semantic patch formula to express a variable that is initialized but then never used, as a result of applying earlier semantic patches). So the manual cleanup for now still seems tractable. Cc: David Gibson Cc: Alexander Graf Cc: Eric Blake Cc: Paolo Bonzini Signed-off-by: Markus Armbruster --- hw/intc/xics_kvm.c | 7 +-- qemu-nbd.c | 6 +- scripts/coccinelle/use-error_fatal.cocci | 20 vl.c | 7 +-- 4 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 scripts/coccinelle/use-error_fatal.cocci +++ b/scripts/coccinelle/use-error_fatal.cocci @@ -0,0 +1,20 @@ +@@ +type T; +identifier FUN, RET; +expression list ARGS; +expression ERR, EC, FAIL; The slight improvement from the original git commit log script is the addition of FAIL, +@@ +( +-T RET = FUN(ARGS, &ERR); ++T RET = FUN(ARGS, &error_fatal); +| +-RET = FUN(ARGS, &ERR); ++RET = FUN(ARGS, &error_fatal); +| +-FUN(ARGS, &ERR); ++FUN(ARGS, &error_fatal); +) +-if (FAIL) { and a check for arbitrary condition FAIL rather than a more specific ERR != NULL. Makes sense. +-error_report_err(ERR); +-exit(EC); +-} Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 08.10.18 21:59, John Snow wrote: > > > On 10/08/2018 03:57 PM, Max Reitz wrote: >> On 08.10.18 21:34, John Snow wrote: >>> >>> >>> On 10/05/2018 07:39 PM, Max Reitz wrote: This function queries a node; since we cannot do that right now, it executes query-named-block-nodes and returns the matching node's object. Signed-off-by: Max Reitz --- tests/qemu-iotests/iotests.py | 7 +++ 1 file changed, 7 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 5c45788dac..604f200600 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine): else: iotests.log(ev) +def node_info(self, node_name): +nodes = self.qmp('query-named-block-nodes') +for x in nodes['return']: +if x['node-name'] == node_name: +return x +return None + index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') >>> >>> Reviewed-by: John Snow >>> >>> Do we ever want to revisit the idea that our API should not do any >>> pre-filtering and that it's always up to the client to do so? >> >> I mean we certainly want to revisit the idea that we should have a >> proper query API. >> >> But what exactly do you mean by pre-filtering? >> >> Max >> > > i.e. applying selection criteria before responding to the query. e.g., > by looking for matches on node-name. > > (Did I use i.e. and e.g. right?) So you mean our API should do pre-filtering at the request of the client and the client shouldn't be fed all nodes and then have to do post-filtering? Well, I certainly think we want a query command that doesn't return all nodes. I personally would prefer something that only returns a single node in the graph and its relationships, although I'm not sure whether that is very efficient. Maybe something like inverse blockdev-add, were the user can specify a depth on how deep references should be expanded. Overall, I'm of the impression we want to have blockdev-reopen first so that we have a good idea of the type of information we want to report for a node; that being its node options, first and foremost. Probably some stats as well, though. Max (The i.e. looks weird because you're not expanding a thought you've just given, but you're actually responding to my question. It's like saying "I want feature A." -- "We can do that, do you have any other remarks?" -- "And I want B, too." So I would have started either with "e.g." (if you think there are other things that could be meant by pre-filtering), or by e.g. "I thought of". Probably the latter, since that is how I interpreted your "i.e.".) ((Did I use the semicolon correctly?)) signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 10/08/2018 06:31 PM, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat >> an unpredictable amount of memory on corrupted table entries, which are >> referencing regions far beyond the end of file. >> >> Prevent this, by skipping such regions from further processing. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-refcount.c | 14 ++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 615847eb09..566c19fbfa 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, >> BdrvCheckResult *res, >> { >> BDRVQcow2State *s = bs->opaque; >> uint64_t start, last, cluster_offset, k, refcount; >> +int64_t file_len; >> int ret; >> >> if (size <= 0) { >> return 0; >> } >> >> +file_len = bdrv_getlength(bs->file->bs); >> +if (file_len < 0) { >> +return file_len; >> +} > > Doesn't this slow things down? Can we not cache the length somewhere > and update it whenever the image is modified? hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is good idea to improve it locally for these series. If we can improve it somehow with a cache or something like this, it should be done for all users and therefore it is outside of these series.. > >> + >> +if (offset + size - file_len > s->cluster_size) { >> +fprintf(stderr, "ERROR: counting reference for region exceeding the >> " >> +"end of the file by more than one cluster: offset 0x%" >> PRIx64 >> +" size 0x%" PRIx64 "\n", offset, size); > > Why is one cluster OK? Is there a specific case you're trying to catch > here? raw file under qcow2 may be not aligned in real size to qcow2 cluster, as I understand, it's normal for the last cluster to be semi-allocated > > Max > >> +res->corruptions++; >> +return 0; >> +} >> + >> start = start_of_cluster(s, offset); >> last = start_of_cluster(s, offset + size - 1); >> for(cluster_offset = start; cluster_offset <= last; >> > >
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 10/08/2018 06:31 PM, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat >> an unpredictable amount of memory on corrupted table entries, which are >> referencing regions far beyond the end of file. >> >> Prevent this, by skipping such regions from further processing. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-refcount.c | 14 ++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 615847eb09..566c19fbfa 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, >> BdrvCheckResult *res, >> { >> BDRVQcow2State *s = bs->opaque; >> uint64_t start, last, cluster_offset, k, refcount; >> +int64_t file_len; >> int ret; >> >> if (size <= 0) { >> return 0; >> } >> >> +file_len = bdrv_getlength(bs->file->bs); >> +if (file_len < 0) { >> +return file_len; >> +} > > Doesn't this slow things down? Can we not cache the length somewhere > and update it whenever the image is modified? hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is good idea to improve it locally for these series. If we can improve it somehow with a cache or something like this, it should be done for all users and therefore it is outside of these series.. > >> + >> +if (offset + size - file_len > s->cluster_size) { >> +fprintf(stderr, "ERROR: counting reference for region exceeding the >> " >> +"end of the file by more than one cluster: offset 0x%" >> PRIx64 >> +" size 0x%" PRIx64 "\n", offset, size); > > Why is one cluster OK? Is there a specific case you're trying to catch > here? raw file under qcow2 may be not aligned in real size to qcow2 cluster, as I understand, it's normal for the last cluster to be semi-allocated > > Max > >> +res->corruptions++; >> +return 0; >> +} >> + >> start = start_of_cluster(s, offset); >> last = start_of_cluster(s, offset + size - 1); >> for(cluster_offset = start; cluster_offset <= last; >> > >
Re: [Qemu-devel] [PATCH] migration: invalidate cache before source start
On 10/08/2018 11:36 AM, Vladimir Sementsov-Ogievskiy wrote: > 26.06.2018 11:44, Vladimir Sementsov-Ogievskiy wrote: >> 25.06.2018 20:50, Dr. David Alan Gilbert wrote: >>> * Dr. David Alan Gilbert (dgilb...@redhat.com) wrote: * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: > 15.06.2018 15:06, Dr. David Alan Gilbert wrote: >> * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: >>> Invalidate cache before source start in case of failed migration. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> >> Why doesn't the code at the bottom of migration_completion, >> fail_invalidate: and the code in migrate_fd_cancel handle this? >> >> What case did you see it in that those didn't handle? >> (Also I guess it probably should set s->block_inactive = false) > on source I see: > > 81392@1529065750.766289:migrate_set_state new state 7 > 81392@1529065750.766330:migration_thread_file_err > 81392@1529065750.766332:migration_thread_after_loop > > so, we are leaving loop on > if (qemu_file_get_error(s->to_dst_file)) { > migrate_set_state(&s->state, current_active_state, > MIGRATION_STATUS_FAILED); > trace_migration_thread_file_err(); > break; > } > > and skip migration_completion() >> >> >> John is right, this ls an unrelated log, here we fail before >> inactivation and there are no problems. >> >> Actual problem is when we fail in postcopy_start, at the end. And >> source log looks like: >> >> 84297@1530001796.287344:migrate_set_state new state 1 >> 84297@1530001796.287374:migration_fd_outgoing fd=101 >> 84297@1530001796.287383:migration_set_outgoing_channel >> ioc=0x56363454d630 ioctype=qio-channel-socket hostname=(null) >> 84297@1530001796.294032:migration_bitmap_sync_start >> 84297@1530001796.300483:migration_bitmap_sync_end dirty_pages 932 >> 84297@1530001796.300561:migrate_set_state new state 4 >> 84297@1530001796.300588:migration_thread_setup_complete >> 84297@1530001796.300593:migrate_pending pending size 1107976192 max 0 >> (pre = 0 compat=1107976192 post=0) >> 84297@1530001796.300595:migrate_set_state new state 5 >> Tap fd 33 disable, ret 0 >> 84297@1530001796.426477:migration_bitmap_sync_start >> 84297@1530001796.433227:migration_bitmap_sync_end dirty_pages 1091 >> 84297@1530001796.439077:migrate_global_state_pre_save saved state: >> running >> 2018-06-26T08:29:56.439134Z qemu-kvm: postcopy_start: Migration stream >> errored -5 >> 84297@1530001796.439141:migrate_set_state new state 7 >> 84297@1530001796.439181:migration_thread_after_loop >> Tap fd 33 enable, ret 0 >> 84297@1530001796.453639:migrate_fd_cleanup >> qemu-kvm: block/io.c:1655: bdrv_co_pwritev: Assertion >> `!(bs->open_flags & 0x0800)' failed. >> 2018-06-26 08:29:56.605+: shutting down, reason=crashed >> >> Yeh, OK; I'd seen soemthing else a few days ago, where a cancellation test that had previously ended with a 'cancelled' state has now ended up in 'failed' (which is the state 7 you have above). I suspect there's something else going on as well; I think what is supposed to happen in the case of 'cancel' is that it spins in 'cancelling' for a while in migrate_fd_cancel and then at the bottom of migrate_fd_cancel it does the recovery, but because it's going to failed instead, then it's jumping over that recovery. >>> Going back and actually looking at the patch again; >>> can I ask for 1 small change; >>> Can you set s->block_inactive = false in the case where you >>> don't get the local_err (Like we do at the bottom of migrate_fd_cancel) >>> >>> >>> Does that make sense? >> >> Ok, I'll resend. >> >> Hm, looks like I'm fixing an outdated version (based on v2.9.0) And my >> reproduce isn't appropriate for upstream. >> But looks like current code have a possibility of the same fail: >> >> postcopy_start() >> >> ret = qemu_file_get_error(ms->to_dst_file); >> if (ret) { >> error_report("postcopy_start: Migration stream errored"); >> >> leads to "return MIG_ITERATE_SKIP;" in migration_iteration_run >> >> then the loop should finish, as state should be >> MIGRATION_STATUS_FAILED, so we will not call migration_completion. >> >> Hm, I have questions now: >> >> 1. should we check s->block_inactive, and if it is false, don't >> invalidate? it is done in migrate_fd_cancel(), but not don in >> migration_completion(). >> 2. should we call qemu_mutex_lock_iothread() like in >> migration_completion()? Why is it needed in migration_completion(), >> when vm is not running? > > > Hm, forgotten thread, I should resend, but what do you think about these > questions? > Personally, I can't remember where we stand on this thread at all. If you have changes that you think are still important (because you ran into the issue again), probably best to send a respin (with a new cover letter and
Re: [Qemu-devel] [RFC 2/6] cputlb: do not evict invalid entries to the vtlb
On Mon, Oct 08, 2018 at 12:46:26 -0700, Richard Henderson wrote: > On 10/8/18 7:42 AM, Emilio G. Cota wrote: > > On Sun, Oct 07, 2018 at 19:09:01 -0700, Richard Henderson wrote: > >> On 10/6/18 2:45 PM, Emilio G. Cota wrote: > >>> Currently we evict an entry to the victim TLB when it doesn't match > >>> the current address. But it could be that there's no match because > >>> the current entry is invalid. Do not evict the entry to the vtlb > >>> in that case. > >>> > >>> This change will help us keep track of the TLB's use rate. > >>> > >>> Signed-off-by: Emilio G. Cota > >>> --- > >>> include/exec/cpu-all.h | 14 ++ > >>> accel/tcg/cputlb.c | 2 +- > >>> 2 files changed, 15 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > >>> index 117d2fbbca..d938dedafc 100644 > >>> --- a/include/exec/cpu-all.h > >>> +++ b/include/exec/cpu-all.h > >>> @@ -362,6 +362,20 @@ static inline bool tlb_hit(target_ulong tlb_addr, > >>> target_ulong addr) > >>> return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK); > >>> } > >>> > >>> +/** > >>> + * tlb_is_valid - return true if at least one of the addresses is valid > >>> + * @te: pointer to CPUTLBEntry > >>> + * > >>> + * This is useful when we don't have a particular address to compare > >>> against, > >>> + * and we just want to know whether any entry holds valid data. > >>> + */ > >>> +static inline bool tlb_is_valid(const CPUTLBEntry *te) > >>> +{ > >>> +return !(te->addr_read & TLB_INVALID_MASK) || > >>> + !(te->addr_write & TLB_INVALID_MASK) || > >>> + !(te->addr_code & TLB_INVALID_MASK); > >>> +} > >> > >> No, I think you misunderstand. > >> > >> First, TLB_INVALID_MASK is only ever set for addr_write, in response to > >> PAGE_WRITE_INV. Second, an entry that is invalid for write is still valid > >> for > >> read+exec. So there is benefit to swapping it out to the victim cache. > >> > >> This is used by the s390x target to make the "lowpage" read-only, which is > >> a > >> special architected 512 byte range within pages 0 and 1. This is done by > >> forcing writes, but not reads, back through tlb_fill. > > > > Aah I see. The point is to avoid pushing to the victim cache > > an entry that is all invalid, not just partially invalid. > > I've slightly misspoken here. It is (ab)used for the s390 thing, but that bit > is also set by memset -1. Perhaps you might check > > static inline bool tlb_is_invalid(const CPUTLBEntry *te) > { > return te->addr_read & te->addr_write & te->addr_code & TLB_INVALID_MASK; > } > > That is, all forms of access have the INVALID bit set. I see. I just ran a few tests and the below gives the best performance, so I'll go with it: static inline bool tlb_is_empty(const CPUTLBEntry *te) { return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1; } I renamed it from "invalid" to "empty" to avoid even thinking about the invalid flag. Thanks, Emilio
Re: [Qemu-devel] [PATCH v2 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
On 09/26/2018 02:05 PM, Jeff Cody wrote: > I'll not be involved with day-to-day qemu development, and John > Snow is a block jobs wizard. Have him take over block job > maintainership duties. > ... I didn't realize he wrote it like this, and I am told I should accept compliments, ... Well, factually it's Kevin and I who have worked on them the most lately, and Kevin's already got enough stuff, so I think it's okay for me to take this unless someone disagrees. > Signed-off-by: Jeff Cody > --- > MAINTAINERS | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ce7c351afa..0e22b795e6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1455,7 +1455,7 @@ F: include/scsi/* > F: scsi/* > > Block Jobs > -M: Jeff Cody > +M: John Snow Acked-by: John Snow `*~ pour one out for your homies ~*' > L: qemu-bl...@nongnu.org > S: Supported > F: blockjob.c > @@ -1468,7 +1468,7 @@ F: block/commit.c > F: block/stream.c > F: block/mirror.c > F: qapi/job.json > -T: git git://github.com/codyprime/qemu-kvm-jtc.git block > +T: git git://github.com/jnsnow/qemu.git jobs > > Block QAPI, monitor, command line > M: Markus Armbruster >
Re: [Qemu-devel] [PATCH v2 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Zero out corrupted L1 table entry, which reference L2 table out of > underlying file. > Zero L1 table entry means that "the L2 table and all clusters described > by this L2 table are unallocated." > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 44 > 1 file changed, 44 insertions(+) Hm. The specification actually says nothing about offsets being allowed past the end of the file, and I don't think we ever use them (outside of a very short period during image creation, where we point to refcount structures beyond the EOF). So the patch looks OK to me, although I'd still prefer a separate fprintf() and think it would be fine for check_refcounts_l1() to call fix_table_entry() directly. Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [Bug 1796754] [NEW] ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context"
Public bug reported: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. ** Affects: qemu Importance: Undecided Status: New ** Tags: aarch64 arm linux-user qemu ** Attachment added: "crash.c" https://bugs.launchpad.net/bugs/1796754/+attachment/5198861/+files/crash.c ** Tags added: aarch64 ** Tags added: linux-user qemu ** Tags added: arm -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1796754 Title: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context" Status in QEMU: New Bug description: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1796754/+subscriptions
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 08.10.18 21:34, John Snow wrote: > > > On 10/05/2018 07:39 PM, Max Reitz wrote: >> This function queries a node; since we cannot do that right now, it >> executes query-named-block-nodes and returns the matching node's object. >> >> Signed-off-by: Max Reitz >> --- >> tests/qemu-iotests/iotests.py | 7 +++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index 5c45788dac..604f200600 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine): >> else: >> iotests.log(ev) >> >> +def node_info(self, node_name): >> +nodes = self.qmp('query-named-block-nodes') >> +for x in nodes['return']: >> +if x['node-name'] == node_name: >> +return x >> +return None >> + >> >> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') >> >> > > Reviewed-by: John Snow > > Do we ever want to revisit the idea that our API should not do any > pre-filtering and that it's always up to the client to do so? I mean we certainly want to revisit the idea that we should have a proper query API. But what exactly do you mean by pre-filtering? Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 10/08/2018 03:57 PM, Max Reitz wrote: > On 08.10.18 21:34, John Snow wrote: >> >> >> On 10/05/2018 07:39 PM, Max Reitz wrote: >>> This function queries a node; since we cannot do that right now, it >>> executes query-named-block-nodes and returns the matching node's object. >>> >>> Signed-off-by: Max Reitz >>> --- >>> tests/qemu-iotests/iotests.py | 7 +++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>> index 5c45788dac..604f200600 100644 >>> --- a/tests/qemu-iotests/iotests.py >>> +++ b/tests/qemu-iotests/iotests.py >>> @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine): >>> else: >>> iotests.log(ev) >>> >>> +def node_info(self, node_name): >>> +nodes = self.qmp('query-named-block-nodes') >>> +for x in nodes['return']: >>> +if x['node-name'] == node_name: >>> +return x >>> +return None >>> + >>> >>> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') >>> >>> >> >> Reviewed-by: John Snow >> >> Do we ever want to revisit the idea that our API should not do any >> pre-filtering and that it's always up to the client to do so? > > I mean we certainly want to revisit the idea that we should have a > proper query API. > > But what exactly do you mean by pre-filtering? > > Max > i.e. applying selection criteria before responding to the query. e.g., by looking for matches on node-name. (Did I use i.e. and e.g. right?) --js
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:30:20PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias > wrote: > > From: "Edgar E. Iglesias" > > > > Add support for selecting the Memory Region that the GEM > > will do DMA to. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > hw/net/cadence_gem.c | 63 > > > > include/hw/net/cadence_gem.h | 2 ++ > > 2 files changed, 43 insertions(+), 22 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > > index 759c1d7..ab02515 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -28,6 +28,7 @@ > > #include "hw/net/cadence_gem.h" > > #include "qapi/error.h" > > #include "qemu/log.h" > > +#include "sysemu/dma.h" > > #include "net/checksum.h" > > > > #ifdef CADENCE_GEM_ERR_DEBUG > > @@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q) > > { > > DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]); > > /* read current descriptor */ > > -cpu_physical_memory_read(s->rx_desc_addr[q], > > - (uint8_t *)s->rx_desc[q], > > - sizeof(uint32_t) * gem_get_desc_len(s, true)); > > +address_space_read(s->dma_as, s->rx_desc_addr[q], > > MEMTXATTRS_UNSPECIFIED, > > + (uint8_t *)s->rx_desc[q], > > + sizeof(uint32_t) * gem_get_desc_len(s, true)); > > At some point you might want to add support for handling "descriptor > read/write failed", incidentally: address_space_read/write return a > MemTxResult which you can check for != MEMTX_OK. Yes, the GEM can report those errors to SW so that's a nice feature to follow up with. Thanks, Edgar
Re: [Qemu-devel] [PATCH v1 09/12] target-arm: powerctl: Enable HVC when starting CPUs to EL2
On Mon, Oct 08, 2018 at 01:41:36PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias > wrote: > > From: "Edgar E. Iglesias" > > > > When QEMU provides the equivalent of the EL3 firmware, we > > need to enable HVCs in scr_el3 when turning on CPUs that > > target EL2. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > target/arm/arm-powerctl.c | 11 +++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c > > index ce55eeb..54f2974 100644 > > --- a/target/arm/arm-powerctl.c > > +++ b/target/arm/arm-powerctl.c > > @@ -63,6 +63,7 @@ static void arm_set_cpu_on_async_work(CPUState > > *target_cpu_state, > > struct CpuOnInfo *info = (struct CpuOnInfo *) data.host_ptr; > > > > /* Initialize the cpu we are turning on */ > > +qemu_log("CPU%d reset\n", target_cpu_state->cpu_index); > > qemu_log logging should always be masked to a particular > log kind (either via qemu_log_mask() or by explicit check on > the log level). If this was accidentally left-in debug, you could > just drop it :-) Hah, yes this is an accidental leftover... > > > cpu_reset(target_cpu_state); > > target_cpu_state->halted = 0; > > > > @@ -103,6 +104,16 @@ static void arm_set_cpu_on_async_work(CPUState > > *target_cpu_state, > > } else { > > /* Processor is not in secure mode */ > > target_cpu->env.cp15.scr_el3 |= SCR_NS; > > + > > +/* > > + * If QEMU is providing the equivalent of EL3 firmware, then we > > need > > + * to make sure a CPU targeting EL2 comes out of reset with a > > + * functional HVC insn. > > + */ > > +if (arm_feature(&target_cpu->env, ARM_FEATURE_EL3) > > +&& info->target_el == 2) { > > +target_cpu->env.cp15.scr_el3 |= SCR_HCE; > > +} > > } > > Otherwise > Reviewed-by: Peter Maydell > This is definitely required for PSCI and I think it makes sense > in other cases that end up in this powerctl code with EL3 set. Thanks, Edgar
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:24:51PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias > wrote: > > From: "Edgar E. Iglesias" > > > > Add support for selecting the Memory Region that the GEM > > will do DMA to. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > > > @@ -1500,6 +1506,13 @@ static void gem_realize(DeviceState *dev, Error > > **errp) > > CadenceGEMState *s = CADENCE_GEM(dev); > > int i; > > > > +if (s->dma_mr) { > > +s->dma_as = g_malloc0(sizeof(AddressSpace)); > > +address_space_init(s->dma_as, s->dma_mr, NULL); > > Why not just have the CadenceGEMState embed the AddressSpace > > AddressSpace dma_as; > > rather than doing a separate memory allocation here? No reason not to, I copied this from a pattern in our code and didn't reflect too much about the allocation. I'll change it for next version. Cheers, Edgar > > > +} else { > > +s->dma_as = &address_space_memory; > > +} > > thanks > -- PMM
Re: [Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Split entry repairing to separate function, to be reused later. > > Note: entry in in-memory l2 table (local variable in > check_refcounts_l2) is not updated after this patch. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 147 > - > 1 file changed, 109 insertions(+), 38 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 3b057b635d..d9c8cd666b 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1554,6 +1554,99 @@ enum { > CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */ > }; > > +/* Update entry in L1 or L2 table > + * > + * Returns: -errno if overlap check failed > + * 0 if write failed > + * 1 on success > + */ > +static int write_table_entry(BlockDriverState *bs, const char *table_name, > + uint64_t table_offset, int entry_index, > + uint64_t new_val, int ign) > +{ > +int ret; > +uint64_t entry_offset = > +table_offset + (uint64_t)entry_index * sizeof(new_val); > + > +cpu_to_be64s(&new_val); > +ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset, > sizeof(new_val)); > +if (ret < 0) { > +fprintf(stderr, > +"ERROR: Can't write %s table entry: overlap check failed: > %s\n", I recently complained to Berto that I don't like elisions ("can't") in user interfaces, so I suppose I'll have to complain here, too, that I'd prefer a "Cannot". > +table_name, strerror(-ret)); > +return ret; > +} > + > +ret = bdrv_pwrite_sync(bs->file, entry_offset, &new_val, > sizeof(new_val)); > +if (ret < 0) { > +fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n", > +table_name, strerror(-ret)); > +return 0; > +} > + > +return 1; > +} > + > +/* Try to fix (if allowed) entry in L1 or L2 table. Update @res > correspondingly. > + * > + * Returns: -errno if overlap check failed > + * 0 if entry was not updated for other reason > + *(fixing disabled or write failed) > + * 1 on success > + */ > +static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, const char *table_name, > + uint64_t table_offset, int entry_index, > + uint64_t new_val, int ign, > + const char *fmt, va_list args) > +{ > +int ret; > + > +fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: "); > +vfprintf(stderr, fmt, args); > +fprintf(stderr, "\n"); If you're just going to print this here, right at the start, I think it would be better to just do it in the caller. Sure, with this solution the caller safes an fprintf() call, but I find it a bit over the top to start with vararg handling here when the caller can just do an additional fprintf(). (Also, I actually find it clearer if you have to call two separate functions.) > + > +if (!(fix & BDRV_FIX_ERRORS)) { > +res->corruptions++; > +return 0; > +} > + > +ret = write_table_entry(bs, table_name, table_offset, entry_index, > new_val, > +ign); > + > +if (ret == 1) { > +res->corruptions_fixed++; > +} else { > +res->check_errors++; > +} > + > +return ret; > +} > + > +/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN > + * > + * Returns: -errno if overlap check failed > + * 0 if write failed > + * 1 on success > + */ > +static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res, > +BdrvCheckMode fix, int64_t l2_offset, > +int l2_index, bool active, > +const char *fmt, ...) > +{ > +int ret; > +int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2; > +uint64_t l2_entry = QCOW_OFLAG_ZERO; > +va_list args; > + > +va_start(args, fmt); > +ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry, > + ign, fmt, args); > +va_end(args); > + > +return ret; > +} If you drop the fprintf() from fix_table_entry(), this function will make less sense as well. Just calling fix_table_entry() directly will be just as easy. (Yes, I see that you use the function in patch 7 again, and yes, you'd have to use a full line for the "active ?:" ternary, but still.) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain
On 10/8/18 11:40 AM, Kevin Wolf wrote: Am 08.10.2018 um 17:43 hat Peter Maydell geschrieben: Looking at the backtraces I'm wondering if this is the result of an implicit reliance on the order in which per-thread destructors are called (which is left unspecified by POSIX) -- the destructor function qemu_thread_atexit_run() is called after some other destructor, but accesses its memory. Specifically, the memory it's trying to read looks like the __thread local variable pollfds_cleanup_notifier in util/aio-posix.c. So I think what is happening is: * util/aio-posix.c calls qemu_thread_atexit_add(), passing it a pointer to a thread-local variable pollfds_cleanup_notifier * qemu_thread_atexit_add() works by arranging to run the notifiers when its 'exit_key' variable's destructor is called * the destructor for pollfds_cleanup_notifier runs before that for exit_key, and so the qemu_thread_atexit_run() function ends up touching freed memory I'm pretty confident this analysis of the problem is correct: unfortunately I have no idea what the right way to fix it is... Yes, I agree with your analysis. If __thread variables can be destructed before pthread_key_create() destructors are called (and in particular if the former are implemented in terms of the latter), this implies at least two rules: 1. The Notfier itself can't be a TLS variable 2. The notifier callback can't access any TLS variables Of course, with these restrictions, qemu_thread_atexit_*() with its existing API is as useless as it could be. The best I can think of at the moment would be to use a separate pthread_key_create() (and therefore a separate destructor) for registering each TLS variable, so that the destructor always gets a valid pointer. Maybe move all __thread variables of a file into a single malloced struct to make it more managable (we could then keep a __thread pointer to it for convenience, but only free the struct with the pointer passed by the pthread_key destructor so that we don't have to access __thread variables in the destructor). pthread_key_create() says that a when a destructor is triggered, it sets the value of the key to NULL; but that you can once again set the key back to a non-NULL value, and that the implementation will loop at least PTHREAD_DESTRUCTOR_ITERATIONS over all destructors or until it has convinced the destructors to leave values at NULL. Thus, while you cannot guarantee ordering between destructors within a single iteration of the cleanup loop, you CAN do some sort of witness locking or down-counter where a destructor purposefully calls pthread_setspecific() to revive the value to survive into the next iteration of destructor calls, for variables which are known to be referenced by other destructors while the witness count is still high enough, as a way of imposing order between loops. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 05/10] docs/devel/testing.rst: add missing newlines after code block
On 10/04/2018 12:18 PM, Cleber Rosa wrote: > The line immediate following a ".. code::" block is considered > to contains arguments to the "code directive". The lack of a > new line gives me during at parse time: > >testing.rst:63: (ERROR/3) Error in "code" directive: >maximum 1 argument(s) allowed, 3 supplied. > >.. code:: > make check-unit V=1 > >testing.rst:120: (ERROR/3) Error in "code" directive: >maximum 1 argument(s) allowed, 3 supplied. > >.. code:: > make check-qtest V=1 > pandoc doesn't complain, but rst.ninjs.org does. What tool did you use to find these? we should formalize a formatting checker for rst files. Actually, we should formalize building our RST docs at all... --js > Let's add the missing newlines, both for consistency and to > avoid the parsing errors. > > Signed-off-by: Cleber Rosa Reviewed-by: John Snow
Re: [Qemu-devel] [RFC 2/6] cputlb: do not evict invalid entries to the vtlb
On 10/8/18 7:42 AM, Emilio G. Cota wrote: > On Sun, Oct 07, 2018 at 19:09:01 -0700, Richard Henderson wrote: >> On 10/6/18 2:45 PM, Emilio G. Cota wrote: >>> Currently we evict an entry to the victim TLB when it doesn't match >>> the current address. But it could be that there's no match because >>> the current entry is invalid. Do not evict the entry to the vtlb >>> in that case. >>> >>> This change will help us keep track of the TLB's use rate. >>> >>> Signed-off-by: Emilio G. Cota >>> --- >>> include/exec/cpu-all.h | 14 ++ >>> accel/tcg/cputlb.c | 2 +- >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h >>> index 117d2fbbca..d938dedafc 100644 >>> --- a/include/exec/cpu-all.h >>> +++ b/include/exec/cpu-all.h >>> @@ -362,6 +362,20 @@ static inline bool tlb_hit(target_ulong tlb_addr, >>> target_ulong addr) >>> return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK); >>> } >>> >>> +/** >>> + * tlb_is_valid - return true if at least one of the addresses is valid >>> + * @te: pointer to CPUTLBEntry >>> + * >>> + * This is useful when we don't have a particular address to compare >>> against, >>> + * and we just want to know whether any entry holds valid data. >>> + */ >>> +static inline bool tlb_is_valid(const CPUTLBEntry *te) >>> +{ >>> +return !(te->addr_read & TLB_INVALID_MASK) || >>> + !(te->addr_write & TLB_INVALID_MASK) || >>> + !(te->addr_code & TLB_INVALID_MASK); >>> +} >> >> No, I think you misunderstand. >> >> First, TLB_INVALID_MASK is only ever set for addr_write, in response to >> PAGE_WRITE_INV. Second, an entry that is invalid for write is still valid >> for >> read+exec. So there is benefit to swapping it out to the victim cache. >> >> This is used by the s390x target to make the "lowpage" read-only, which is a >> special architected 512 byte range within pages 0 and 1. This is done by >> forcing writes, but not reads, back through tlb_fill. > > Aah I see. The point is to avoid pushing to the victim cache > an entry that is all invalid, not just partially invalid. I've slightly misspoken here. It is (ab)used for the s390 thing, but that bit is also set by memset -1. Perhaps you might check static inline bool tlb_is_invalid(const CPUTLBEntry *te) { return te->addr_read & te->addr_write & te->addr_code & TLB_INVALID_MASK; } That is, all forms of access have the INVALID bit set. r~
Re: [Qemu-devel] [Qemu-block] [PATCH 09/10] scripts/qemu.py: use a more consistent docstring style
On 10/04/2018 12:18 PM, Cleber Rosa wrote: > Signed-off-by: Cleber Rosa > --- > dtc | 2 +- > scripts/qemu.py | 65 +++-- > 2 files changed, 42 insertions(+), 25 deletions(-) > > diff --git a/dtc b/dtc > index 88f18909db..e54388015a 16 > --- a/dtc > +++ b/dtc > @@ -1 +1 @@ > -Subproject commit 88f18909db731a627456f26d779445f84e449536 > +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42 > diff --git a/scripts/qemu.py b/scripts/qemu.py > index f099ce7278..7abe26de69 100644 > --- a/scripts/qemu.py > +++ b/scripts/qemu.py > @@ -53,9 +53,9 @@ class QEMUMachineAddDeviceError(QEMUMachineError): > """ > > class MonitorResponseError(qmp.qmp.QMPError): > -''' > +""" > Represents erroneous QMP monitor reply > -''' > +""" This seems obviously correct, as per the Python Dogma Handbook ... > def __init__(self, reply): > try: > desc = reply["error"]["desc"] > @@ -66,14 +66,15 @@ class MonitorResponseError(qmp.qmp.QMPError): > > > class QEMUMachine(object): > -'''A QEMU VM > +""" > +A QEMU VM > > Use this object as a context manager to ensure the QEMU process > terminates:: > > with VM(binary) as vm: > ... > # vm is guaranteed to be shut down here > -''' > +""" As does this,, > > def __init__(self, binary, args=None, wrapper=None, name=None, > test_dir="/var/tmp", monitor_address=None, > @@ -135,7 +136,9 @@ class QEMUMachine(object): > self._args.append(args) > > def add_fd(self, fd, fdset, opaque, opts=''): > -'''Pass a file descriptor to the VM''' > +""" > +Pass a file descriptor to the VM > +""" However, is it established practice among ne'er-do-wells to format one-line docstrings as three-liners? (And without punctuation to boot -- for shame!) PEP257 suggests that one-liners are allowed, but doesn't seem to necessitate their usage. Does this kind of change have any kind of benefit? > options = ['fd=%d' % fd, > 'set=%d' % fdset, > 'opaque=%s' % opaque] > @@ -168,7 +171,9 @@ class QEMUMachine(object): > > @staticmethod > def _remove_if_exists(path): > -'''Remove file object at path if it exists''' > +""" > +Remove file object at path if it exists > +""" > try: > os.remove(path) > except OSError as exception: > @@ -271,7 +276,9 @@ class QEMUMachine(object): > raise > > def _launch(self): > -'''Launch the VM and establish a QMP connection''' > +""" > +Launch the VM and establish a QMP connection > +""" > devnull = open(os.path.devnull, 'rb') > self._pre_launch() > self._qemu_full_args = (self._wrapper + [self._binary] + > @@ -284,14 +291,18 @@ class QEMUMachine(object): > self._post_launch() > > def wait(self): > -'''Wait for the VM to power off''' > +""" > +Wait for the VM to power off > +""" > self._popen.wait() > self._qmp.close() > self._load_io_log() > self._post_shutdown() > > def shutdown(self): > -'''Terminate the VM and clean up''' > +""" > +Terminate the VM and clean up > +""" > if self.is_running(): > try: > self._qmp.cmd('quit') > @@ -315,7 +326,9 @@ class QEMUMachine(object): > self._launched = False > > def qmp(self, cmd, conv_keys=True, **args): > -'''Invoke a QMP command and return the response dict''' > +""" > +Invoke a QMP command and return the response dict > +""" > qmp_args = dict() > for key, value in args.items(): > if conv_keys: > @@ -326,11 +339,11 @@ class QEMUMachine(object): > return self._qmp.cmd(cmd, args=qmp_args) > > def command(self, cmd, conv_keys=True, **args): > -''' > +""" > Invoke a QMP command. > On success return the response dict. > On failure raise an exception. > -''' > +""" > reply = self.qmp(cmd, conv_keys, **args) > if reply is None: > raise qmp.qmp.QMPError("Monitor is closed") > @@ -339,13 +352,17 @@ class QEMUMachine(object): > return reply["return"] > > def get_qmp_event(self, wait=False): > -'''Poll for one queued QMP events and return it''' > +""" > +Poll for one queued QMP events and return it > +""" > if len(self._events) > 0: > return self._events.pop(0) > return self._qmp.pull_event(wait=wait) > > def get_qmp_events(self, wait=False): > -'''Poll for queued QMP events and return a list of dicts''' > +""" > +Poll for queued QMP events and return a l
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 10/05/2018 07:39 PM, Max Reitz wrote: > This function queries a node; since we cannot do that right now, it > executes query-named-block-nodes and returns the matching node's object. > > Signed-off-by: Max Reitz > --- > tests/qemu-iotests/iotests.py | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 5c45788dac..604f200600 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine): > else: > iotests.log(ev) > > +def node_info(self, node_name): > +nodes = self.qmp('query-named-block-nodes') > +for x in nodes['return']: > +if x['node-name'] == node_name: > +return x > +return None > + > > index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') > > Reviewed-by: John Snow Do we ever want to revisit the idea that our API should not do any pre-filtering and that it's always up to the client to do so? --js
Re: [Qemu-devel] insecure git submodule URLs
On 7/15/18 7:56 PM, Jann Horn via Qemu-devel wrote: On Sun, Jul 15, 2018 at 11:18 PM Peter Maydell wrote: On 15 July 2018 at 20:50, Jann Horn via Qemu-devel wrote: I noticed that when I build QEMU from git for the first time, it pulls in submodules over the insecure git:// protocol - in other words, as far as I can tell, if I'm e.g. on an open wifi network while building QEMU for the first time, even if I cloned the main repository over https, anyone could smuggle in malicious code as part of e.g. a submodule's makefile. Yes, this came up the other week. I'm not sure what your preferred fix for this is, so I'm not sending a patch yet. As far as I can tell, the two options are: - change .gitmodules to use https for everything We should probably do this... As far as I can tell, the QEMU git server only supports the "dumb" git protocol when accessed over HTTPS, not the "smart" protocol. I'm not sure whether that might be why QEMU is currently still using the insecure git protocol instead of git over HTTPS? This is why we haven't switched over the submodules yet, yes. It's on Jeff's todo list for the server, though. Did we ever get this done? (And updating this thread to pull in Jeff's new email). (Reminded of this now that there is yet another submodule being proposed for mirroring) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PULL 0/8] softfloat queue
Adding Jeff and Stefan On 10/8/18 8:47 AM, Peter Maydell wrote: On 5 October 2018 at 19:01, Richard Henderson wrote: The following changes since commit ae7a4c0a4604bcfed40170db6cca576c44d872a2: Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20181004' into staging (2018-10-05 16:05:06 +0100) are available in the Git repository at: https://github.com/rth7680/qemu.git tags/pull-fpu-20181005 for you to fetch changes up to 27ae5109a2ba8b6b679cce3e03e16570a34390a0: softfloat: Specialize udiv_qrnnd for ppc64 (2018-10-05 12:57:41 -0500) Testing infrastructure for softfpu (not run by default). Drop countLeadingZeros. Fix div_floats. Add udiv_qrnnd specializations for x86_64, s390x, ppc64 hosts. Applied, thanks. I guess we should mirror the new submodules on git.qemu.org. Yes, that's been our general practice. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure
On 10/1/18 3:59 AM, Igor Mammedov wrote: Anyway, what about this: The command returns an object with a "qom-path" member for each present CPU. In this case, it shows an IvyBridge-IBRS-x86_64-cpu in socket 0. It returns an object without a "qom-path" for every possibly CPU hot-plug. In this case, it shows you can plug an IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional properties you need to pass to device_add for that. not really sure my English (CCed Eric) but to match 'an object' with the rest of sentence: It returns an object without a "qom-path" for a possible to hot-plug CPU. + In this case, it shows you can plug an IvyBridge-IBRS-x86_64-cpu into socket 1/core = 0/thread 0, where 'props' list describes additional properties you need to pass to device_add for hot-pluging that CPU. Maybe: The command returns an object for CPUs that are present (containing a "qom-path" member) or which may be hot-plugged (no "qom-path" member). In this example, an IvyBridge-IBRS-x86_64-cpu is present in socket 0, while hot-plugging a CPU into socket 1 requires passing the listed properties to device_add. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats
On 08.10.18 20:50, Max Reitz wrote: > There are some 2D resource formats that can be used through virtio-gpu, > but which are not supported by SDL2 when used for a scanout; these are > all alpha-channel formats and also XBGR (RGBX in non-BE pixman). Oops, it's the other way round. The virtio-gpu format is RGBX, and the non-BE pixman (and SDL) constant is XBGR. Max > Add these formats in the switch converting pixman to SDL format > constants so a guest cannot crash the VM by triggering the > g_assert_not_reached() with an unsupported format. > > Signed-off-by: Max Reitz > --- > ui/sdl2-2d.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c > index 85484407be..c9df72636a 100644 > --- a/ui/sdl2-2d.c > +++ b/ui/sdl2-2d.c > @@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl, > case PIXMAN_r5g6b5: > format = SDL_PIXELFORMAT_RGB565; > break; > +case PIXMAN_a8r8g8b8: > case PIXMAN_x8r8g8b8: > format = SDL_PIXELFORMAT_ARGB; > break; > +case PIXMAN_a8b8g8r8: > +case PIXMAN_x8b8g8r8: > +format = SDL_PIXELFORMAT_ABGR; > +break; > +case PIXMAN_r8g8b8a8: > case PIXMAN_r8g8b8x8: > format = SDL_PIXELFORMAT_RGBA; > break; > case PIXMAN_b8g8r8x8: > format = SDL_PIXELFORMAT_BGRX; > break; > +case PIXMAN_b8g8r8a8: > +format = SDL_PIXELFORMAT_BGRA; > +break; > default: > g_assert_not_reached(); > } > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 05/31] vfio: Clean up error reporting after previous commit
On Mon, 8 Oct 2018 19:30:59 +0200 Markus Armbruster wrote: > The previous commit changed vfio's warning messages from > > vfio warning: DEV-NAME: Could not frobnicate > > to > > warning: vfio DEV-NAME: Could not frobnicate > > To match this change, change error messages from > > vfio error: DEV-NAME: On fire > > to > > vfio DEV-NAME: On fire > > Note the loss of "error". If we think marking error messages that way > is a good idea, we should mark *all* error messages, i.e. make > error_report() print it. > > Cc: Alex Williamson > Signed-off-by: Markus Armbruster > --- > hw/vfio/pci-quirks.c | 4 ++-- > hw/vfio/pci.c | 8 > hw/vfio/platform.c| 2 +- > include/hw/vfio/vfio-common.h | 1 - > 4 files changed, 7 insertions(+), 8 deletions(-) Acked-by: Alex Williamson
Re: [Qemu-devel] [PATCH 04/31] vfio: Use warn_report() & friends to report warnings
On Mon, 8 Oct 2018 19:30:58 +0200 Markus Armbruster wrote: > The vfio code reports warnings like > > error_report(WARN_PREFIX "Could not frobnicate", DEV-NAME); > > where WARN_PREFIX is defined so the message comes out as > > vfio warning: DEV-NAME: Could not frobnicate > > This usage predates the introduction of warn_report() & friends in > commit 97f40301f1d. It's time to convert to that interface. Since > these functions already prefix the message with "warning: ", replace > WARN_PREFIX by VFIO_MSG_PREFIX, so the messages come out like > > warning: vfio DEV-NAME: Could not frobnicate > > The next commit will replace ERR_PREFIX. > > Cc: Alex Williamson > Signed-off-by: Markus Armbruster > --- > hw/vfio/pci.c | 14 +++--- > hw/vfio/platform.c| 4 ++-- > include/hw/vfio/vfio-common.h | 2 +- > 3 files changed, 10 insertions(+), 10 deletions(-) Acked-by: Alex Williamson
[Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats
There are some 2D resource formats that can be used through virtio-gpu, but which are not supported by SDL2 when used for a scanout; these are all alpha-channel formats and also XBGR (RGBX in non-BE pixman). Add these formats in the switch converting pixman to SDL format constants so a guest cannot crash the VM by triggering the g_assert_not_reached() with an unsupported format. Signed-off-by: Max Reitz --- ui/sdl2-2d.c | 9 + 1 file changed, 9 insertions(+) diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c index 85484407be..c9df72636a 100644 --- a/ui/sdl2-2d.c +++ b/ui/sdl2-2d.c @@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl, case PIXMAN_r5g6b5: format = SDL_PIXELFORMAT_RGB565; break; +case PIXMAN_a8r8g8b8: case PIXMAN_x8r8g8b8: format = SDL_PIXELFORMAT_ARGB; break; +case PIXMAN_a8b8g8r8: +case PIXMAN_x8b8g8r8: +format = SDL_PIXELFORMAT_ABGR; +break; +case PIXMAN_r8g8b8a8: case PIXMAN_r8g8b8x8: format = SDL_PIXELFORMAT_RGBA; break; case PIXMAN_b8g8r8x8: format = SDL_PIXELFORMAT_BGRX; break; +case PIXMAN_b8g8r8a8: +format = SDL_PIXELFORMAT_BGRA; +break; default: g_assert_not_reached(); } -- 2.17.1
Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()
On 08.10.18 20:13, Alberto Garcia wrote: > On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote: > >>> +/* Old values are used for options that aren't set yet */ >>> +old_options = qdict_clone_shallow(bs->options); >>> +bdrv_join_options(bs, options, old_options); >>> +qobject_unref(old_options); >>> + >>> +/* We have the final set of options so let's update the flags */ >>> +{ >>> +Error *local_err = NULL; >>> +QemuOpts *opts; >>> +QDict *options_copy = qdict_clone_shallow(options); >> >> I-I'm not sure this conforms to our coding style. >> >> While I applaud your effort to keep the patch size small, I know for >> sure I don't want to read code like this. (Unless it's necessary >> because of some variables' lifetimes.) > > I actually think it makes the code more readable if there are variables > with a very limited scope (like in this case). But I can also rewrite it > using a more traditional style. I think it should be a function call then. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] tcg: Add tlb_index and tlb_entry helpers
On Sun, Oct 07, 2018 at 18:05:22 -0700, Richard Henderson wrote: > Isolate the computation of an index from an address into a > helper before we change that function. > > Signed-off-by: Richard Henderson > --- > > Emilio, this should make your dynamic tlb sizing patch 1/6 > significantly smaller. Nice! I'm adding this as patch 1 for v2. Had to fix the conflicts with the .addr_write conversion to atomic_write, and also fixed a few typos like the following: > void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > TCGMemOpIdx oi, uintptr_t retaddr) > { > -unsigned mmu_idx = get_mmuidx(oi); > -int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > -target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write; > +uintptr_t mmu_idx = get_mmuidx(oi); > +uintptr_t index = tlb_index(env, mmu_idx, addr); > +CPUTLBEntry *entry = tlb_entry(env, mmu_idx, index); Should be tlb_entry(env, mmu_idx, addr) Thanks, Emilio
[Qemu-devel] [PATCH v1 4/5] RISC-V: Add missing free for plic_hart_config
From: Michael Clark Cc: Palmer Dabbelt Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Alistair Francis Signed-off-by: Michael Clark Reviewed-by: Alistair Francis --- hw/riscv/virt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 005169eabc..6bd723dc3a 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -385,6 +385,8 @@ static void riscv_virt_board_init(MachineState *machine) serial_mm_init(system_memory, memmap[VIRT_UART0].base, 0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193, serial_hd(0), DEVICE_LITTLE_ENDIAN); + +g_free(plic_hart_config); } static void riscv_virt_board_machine_init(MachineClass *mc) -- 2.17.1
[Qemu-devel] [PATCH v1 2/5] RISC-V: Move non-ops from op_helper to cpu_helper
From: Michael Clark This patch makes op_helper.c contain only instruction operation helpers used by translate.c and moves any unrelated cpu helpers into cpu_helper.c. No logic is changed by this patch. Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Palmer Dabbelt Cc: Alistair Francis Signed-off-by: Michael Clark Reviewed-by: Alistair Francis --- target/riscv/Makefile.objs | 2 +- target/riscv/{helper.c => cpu_helper.c} | 35 - target/riscv/op_helper.c| 34 3 files changed, 35 insertions(+), 36 deletions(-) rename target/riscv/{helper.c => cpu_helper.c} (95%) diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs index abd0a7cde3..fcc5d34c1f 100644 --- a/target/riscv/Makefile.objs +++ b/target/riscv/Makefile.objs @@ -1 +1 @@ -obj-y += translate.o op_helper.o helper.o cpu.o fpu_helper.o gdbstub.o pmp.o +obj-y += translate.o op_helper.o cpu_helper.o cpu.o fpu_helper.o gdbstub.o pmp.o diff --git a/target/riscv/helper.c b/target/riscv/cpu_helper.c similarity index 95% rename from target/riscv/helper.c rename to target/riscv/cpu_helper.c index 63b3386b76..86f9f4730c 100644 --- a/target/riscv/helper.c +++ b/target/riscv/cpu_helper.c @@ -1,5 +1,5 @@ /* - * RISC-V emulation helpers for qemu. + * RISC-V CPU helpers for qemu. * * Copyright (c) 2016-2017 Sagar Karandikar, sag...@eecs.berkeley.edu * Copyright (c) 2017-2018 SiFive, Inc. @@ -72,6 +72,39 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request) #if !defined(CONFIG_USER_ONLY) +/* iothread_mutex must be held */ +uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value) +{ +CPURISCVState *env = &cpu->env; +uint32_t old, new, cmp = atomic_read(&env->mip); + +do { +old = cmp; +new = (old & ~mask) | (value & mask); +cmp = atomic_cmpxchg(&env->mip, old, new); +} while (old != cmp); + +if (new && !old) { +cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); +} else if (!new && old) { +cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); +} + +return old; +} + +void riscv_set_mode(CPURISCVState *env, target_ulong newpriv) +{ +if (newpriv > PRV_M) { +g_assert_not_reached(); +} +if (newpriv == PRV_H) { +newpriv = PRV_U; +} +/* tlb_flush is unnecessary as mode is contained in mmu_idx */ +env->priv = newpriv; +} + /* get_physical_address - get the physical address for this virtual address * * Do a page table walk to obtain the physical address corresponding to a diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index d0883d329b..495390ab1c 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -654,39 +654,6 @@ target_ulong helper_csrrc(CPURISCVState *env, target_ulong src, #ifndef CONFIG_USER_ONLY -/* iothread_mutex must be held */ -uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value) -{ -CPURISCVState *env = &cpu->env; -uint32_t old, new, cmp = atomic_read(&env->mip); - -do { -old = cmp; -new = (old & ~mask) | (value & mask); -cmp = atomic_cmpxchg(&env->mip, old, new); -} while (old != cmp); - -if (new && !old) { -cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); -} else if (!new && old) { -cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); -} - -return old; -} - -void riscv_set_mode(CPURISCVState *env, target_ulong newpriv) -{ -if (newpriv > PRV_M) { -g_assert_not_reached(); -} -if (newpriv == PRV_H) { -newpriv = PRV_U; -} -/* tlb_flush is unnecessary as mode is contained in mmu_idx */ -env->priv = newpriv; -} - target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb) { if (!(env->priv >= PRV_S)) { @@ -737,7 +704,6 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb) return retpc; } - void helper_wfi(CPURISCVState *env) { CPUState *cs = CPU(riscv_env_get_cpu(env)); -- 2.17.1
[Qemu-devel] [PATCH v1 5/5] RISC-V: Don't add NULL bootargs to device-tree
From: Michael Clark Cc: Palmer Dabbelt Cc: Alistair Francis Signed-off-by: Michael Clark Reviewed-by: Alistair Francis --- hw/riscv/sifive_u.c | 4 +++- hw/riscv/spike.c| 6 -- hw/riscv/virt.c | 4 +++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 862f8ff5f7..ef07df2442 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -230,7 +230,9 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap, qemu_fdt_add_subnode(fdt, "/chosen"); qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename); -qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +if (cmdline) { +qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +} g_free(nodename); } diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index be5ef85e81..8a712ed490 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -156,8 +156,10 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap, g_free(cells); g_free(nodename); -qemu_fdt_add_subnode(fdt, "/chosen"); -qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +if (cmdline) { +qemu_fdt_add_subnode(fdt, "/chosen"); +qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +} } static void spike_v1_10_0_board_init(MachineState *machine) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 6bd723dc3a..4a137a503c 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -254,7 +254,9 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, qemu_fdt_add_subnode(fdt, "/chosen"); qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename); -qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +if (cmdline) { +qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); +} g_free(nodename); return fdt; -- 2.17.1
[Qemu-devel] [PATCH v1 3/5] RISC-V: Update CSR and interrupt definitions
From: Michael Clark * Add user-mode CSR defininitions. * Reorder CSR definitions to match the specification. * Change H mode interrupt comment to 'reserved'. * Remove unused X_COP interrupt. * Add user-mode interrupts. * Remove erroneous until comments on machine mode interrupts. * Move together paging mode and page table bit definitions. * Move together interrupt and exception cause definitions. Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Palmer Dabbelt Cc: Alistair Francis Signed-off-by: Michael Clark Reviewed-by: Alistair Francis --- target/riscv/cpu.c | 6 +- target/riscv/cpu_bits.h | 683 +-- target/riscv/op_helper.c | 2 +- 3 files changed, 370 insertions(+), 321 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index d630e8fd6c..a025a0a3ba 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -74,8 +74,10 @@ const char * const riscv_intr_names[] = { "s_external", "h_external", "m_external", -"coprocessor", -"host" +"reserved", +"reserved", +"reserved", +"reserved" }; typedef struct RISCVCPUInfo { diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index 12b4757088..5439f4719e 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -6,242 +6,283 @@ (((target_ulong)(val) * ((mask) & ~((mask) << 1))) & \ (target_ulong)(mask))) -#define PGSHIFT 12 - -#define FSR_RD_SHIFT 5 -#define FSR_RD (0x7 << FSR_RD_SHIFT) - -#define FPEXC_NX 0x01 -#define FPEXC_UF 0x02 -#define FPEXC_OF 0x04 -#define FPEXC_DZ 0x08 -#define FPEXC_NV 0x10 - -#define FSR_AEXC_SHIFT 0 -#define FSR_NVA (FPEXC_NV << FSR_AEXC_SHIFT) -#define FSR_OFA (FPEXC_OF << FSR_AEXC_SHIFT) -#define FSR_UFA (FPEXC_UF << FSR_AEXC_SHIFT) -#define FSR_DZA (FPEXC_DZ << FSR_AEXC_SHIFT) -#define FSR_NXA (FPEXC_NX << FSR_AEXC_SHIFT) -#define FSR_AEXC (FSR_NVA | FSR_OFA | FSR_UFA | FSR_DZA | FSR_NXA) - -/* CSR numbers */ -#define CSR_FFLAGS 0x1 -#define CSR_FRM 0x2 -#define CSR_FCSR 0x3 -#define CSR_CYCLE 0xc00 -#define CSR_TIME 0xc01 -#define CSR_INSTRET 0xc02 -#define CSR_HPMCOUNTER3 0xc03 -#define CSR_HPMCOUNTER4 0xc04 -#define CSR_HPMCOUNTER5 0xc05 -#define CSR_HPMCOUNTER6 0xc06 -#define CSR_HPMCOUNTER7 0xc07 -#define CSR_HPMCOUNTER8 0xc08 -#define CSR_HPMCOUNTER9 0xc09 -#define CSR_HPMCOUNTER10 0xc0a -#define CSR_HPMCOUNTER11 0xc0b -#define CSR_HPMCOUNTER12 0xc0c -#define CSR_HPMCOUNTER13 0xc0d -#define CSR_HPMCOUNTER14 0xc0e -#define CSR_HPMCOUNTER15 0xc0f -#define CSR_HPMCOUNTER16 0xc10 -#define CSR_HPMCOUNTER17 0xc11 -#define CSR_HPMCOUNTER18 0xc12 -#define CSR_HPMCOUNTER19 0xc13 -#define CSR_HPMCOUNTER20 0xc14 -#define CSR_HPMCOUNTER21 0xc15 -#define CSR_HPMCOUNTER22 0xc16 -#define CSR_HPMCOUNTER23 0xc17 -#define CSR_HPMCOUNTER24 0xc18 -#define CSR_HPMCOUNTER25 0xc19 -#define CSR_HPMCOUNTER26 0xc1a -#define CSR_HPMCOUNTER27 0xc1b -#define CSR_HPMCOUNTER28 0xc1c -#define CSR_HPMCOUNTER29 0xc1d -#define CSR_HPMCOUNTER30 0xc1e -#define CSR_HPMCOUNTER31 0xc1f -#define CSR_SSTATUS 0x100 -#define CSR_SIE 0x104 -#define CSR_STVEC 0x105 -#define CSR_SCOUNTEREN 0x106 -#define CSR_SSCRATCH 0x140 -#define CSR_SEPC 0x141 -#define CSR_SCAUSE 0x142 -#define CSR_SBADADDR 0x143 -#define CSR_SIP 0x144 -#define CSR_SPTBR 0x180 -#define CSR_SATP 0x180 -#define CSR_MSTATUS 0x300 -#define CSR_MISA 0x301 -#define CSR_MEDELEG 0x302 -#define CSR_MIDELEG 0x303 -#define CSR_MIE 0x304 -#define CSR_MTVEC 0x305 -#define CSR_MCOUNTEREN 0x306 -#define CSR_MSCRATCH 0x340 -#define CSR_MEPC 0x341 -#define CSR_MCAUSE 0x342 -#define CSR_MBADADDR 0x343 -#define CSR_MIP 0x344 -#define CSR_PMPCFG0 0x3a0 -#define CSR_PMPCFG1 0x3a1 -#define CSR_PMPCFG2 0x3a2 -#define CSR_PMPCFG3 0x3a3 -#define CSR_PMPADDR0 0x3b0 -#define CSR_PMPADDR1 0x3b1 -#define CSR_PMPADDR2 0x3b2 -#define CSR_PMPADDR3 0x3b3 -#define CSR_PMPADDR4 0x3b4 -#define CSR_PMPADDR5 0x3b5 -#define CSR_PMPADDR6 0x3b6 -#define CSR_PMPADDR7 0x3b7 -#define CSR_PMPADDR8 0x3b8 -#define CSR_PMPADDR9 0x3b9 -#define CSR_PMPADDR10 0x3ba -#define CSR_PMPADDR11 0x3bb -#define CSR_PMPADDR12 0x3bc -#define CSR_PMPADDR13 0x3bd -#define CSR_PMPADDR14 0x3be -#define CSR_PMPADDR15 0x3bf -#define CSR_TSELECT 0x7a0 -#define CSR_TDATA1 0x7a1 -#define CSR_TDATA2 0x7a2 -#define CSR_TDATA3 0x7a3 -#define CSR_DCSR 0x7b0 -#define CSR_DPC 0x7b1 -#define CSR_DSCRATCH 0x7b2 -#define CSR_MCYCLE 0xb00 -#define CSR_MINSTRET 0xb02 -#define CSR_MHPMCOUNTER3 0xb03 -#define CSR_MHPMCOUNTER4 0xb04 -#define CSR_MHPMCOUNTER5 0xb05 -#define CSR_MHPMCOUNTER6 0xb06 -#define CSR_MHPMCOUNTER7 0xb07 -#define CSR_MHPMCOUNTER8 0xb08 -#define CSR_MHPMCOUNTER9 0xb09 -#define CSR_MHPMCOUNTER10 0xb0a -#define CSR_MHPMCOUNTER11 0xb0b -#define CSR_MHPMCOUNTER12 0xb0c -#define CSR_MHPMCOUNTER13 0xb0d -#define CSR_MHPMCOUNTER14 0xb0e -#define CSR_MHPMCOUNTER15 0xb0f -#define CSR_MHPMCOUNTER16 0xb10 -#define CSR_MHPMCOUNTER17 0xb11 -#define CSR_MHPMCOUNTER18 0xb12 -#define CSR_MHPMCOUNTER19
[Qemu-devel] [PATCH v1 0/5] Misc RISC-V patches
These are some patches that I have cherry picked from Michael's RISC-V tree that are ready to be applied. Unless anyone has any comments against these I'll send a PR later this week. Michael Clark (5): RISC-V: Allow setting and clearing multiple irqs RISC-V: Move non-ops from op_helper to cpu_helper RISC-V: Update CSR and interrupt definitions RISC-V: Add missing free for plic_hart_config RISC-V: Don't add NULL bootargs to device-tree hw/riscv/sifive_clint.c | 8 +- hw/riscv/sifive_plic.c | 4 +- hw/riscv/sifive_u.c | 4 +- hw/riscv/spike.c| 6 +- hw/riscv/virt.c | 6 +- target/riscv/Makefile.objs | 2 +- target/riscv/cpu.c | 6 +- target/riscv/cpu.h | 22 +- target/riscv/cpu_bits.h | 683 +--- target/riscv/{helper.c => cpu_helper.c} | 35 +- target/riscv/op_helper.c| 34 +- 11 files changed, 438 insertions(+), 372 deletions(-) rename target/riscv/{helper.c => cpu_helper.c} (95%) -- 2.17.1