[Qemu-devel] [PATCH 25/31] numa: Clean up error reporting in parse_numa()
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_numa() 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.
While there, give parse_numa() internal linkage.
Cc: Eduardo Habkost
Signed-off-by: Markus Armbruster
---
include/sysemu/numa.h | 1 -
numa.c| 8 +++-
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 7a0ae751aa..21713b7e2f 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -22,7 +22,6 @@ struct NumaNodeMem {
};
extern NodeInfo numa_info[MAX_NODES];
-int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
void parse_numa_opts(MachineState *ms);
void numa_complete_configuration(MachineState *ms);
void query_numa_node_mem(NumaNodeMem node_mem[]);
diff --git a/numa.c b/numa.c
index 1d7c49ad43..50ec016013 100644
--- a/numa.c
+++ b/numa.c
@@ -215,7 +215,7 @@ end:
error_propagate(errp, err);
}
-int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
{
NumaOptions *object = NULL;
MachineState *ms = MACHINE(opaque);
@@ -239,7 +239,7 @@ int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
end:
qapi_free_NumaOptions(object);
if (err) {
-error_report_err(err);
+error_propagate(errp, err);
return -1;
}
@@ -444,9 +444,7 @@ void numa_complete_configuration(MachineState *ms)
void parse_numa_opts(MachineState *ms)
{
-if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, _fatal);
}
void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
--
2.17.1
[Qemu-devel] [PATCH 12/31] l2tpv3: Improve -netdev/netdev_add/-net/... error reporting
When -netdev l2tpv3 fails, it first reports a specific error, then a
generic one, like this:
$ qemu-system-x86_64 -netdev l2tpv3,id=foo,src=,dst=,txsession=1
qemu-system-x86_64: -netdev l2tpv3,id=foo,src=,dst=,txsession=1:
l2tpv3_open : could not resolve src, errno = Name or service not known
qemu-system-x86_64: Device 'l2tpv3' could not be initialized
With the command line, the messages go to stderr. In HMP, they go to
the monitor. In QMP, the second one becomes the error reply, and the
first one goes to stderr.
Convert net_init_tap() to Error. This suppresses the unwanted second
message, and makes the specific error the QMP error reply.
Cc: Jason Wang
Signed-off-by: Markus Armbruster
---
net/l2tpv3.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 6745b78990..0c5dd22ef7 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -28,6 +28,7 @@
#include
#include "net/net.h"
#include "clients.h"
+#include "qapi/error.h"
#include "qemu-common.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
@@ -528,7 +529,6 @@ int net_init_l2tpv3(const Netdev *netdev,
const char *name,
NetClientState *peer, Error **errp)
{
-/* FIXME error_setg(errp, ...) on failure */
const NetdevL2TPv3Options *l2tpv3;
NetL2TPV3State *s;
NetClientState *nc;
@@ -555,7 +555,7 @@ int net_init_l2tpv3(const Netdev *netdev,
}
if ((l2tpv3->has_offset) && (l2tpv3->offset > 256)) {
-error_report("l2tpv3_open : offset must be less than 256 bytes");
+error_setg(errp, "l2tpv3_open : offset must be less than 256 bytes");
goto outerr;
}
@@ -563,6 +563,8 @@ int net_init_l2tpv3(const Netdev *netdev,
if (l2tpv3->has_rxcookie && l2tpv3->has_txcookie) {
s->cookie = true;
} else {
+error_setg(errp,
+ "require both 'rxcookie' and 'txcookie' or neither");
goto outerr;
}
} else {
@@ -578,7 +580,8 @@ int net_init_l2tpv3(const Netdev *netdev,
if (l2tpv3->has_udp && l2tpv3->udp) {
s->udp = true;
if (!(l2tpv3->has_srcport && l2tpv3->has_dstport)) {
-error_report("l2tpv3_open : need both src and dst port for udp");
+error_setg(errp,
+ "l2tpv3_open : need both src and dst port for udp");
goto outerr;
} else {
srcport = l2tpv3->srcport;
@@ -639,20 +642,19 @@ int net_init_l2tpv3(const Netdev *netdev,
gairet = getaddrinfo(l2tpv3->src, srcport, , );
if ((gairet != 0) || (result == NULL)) {
-error_report(
-"l2tpv3_open : could not resolve src, errno = %s",
-gai_strerror(gairet)
-);
+error_setg(errp, "l2tpv3_open : could not resolve src, errno = %s",
+ gai_strerror(gairet));
goto outerr;
}
fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
if (fd == -1) {
fd = -errno;
-error_report("l2tpv3_open : socket creation failed, errno = %d", -fd);
+error_setg(errp, "l2tpv3_open : socket creation failed, errno = %d",
+ -fd);
goto outerr;
}
if (bind(fd, (struct sockaddr *) result->ai_addr, result->ai_addrlen)) {
-error_report("l2tpv3_open : could not bind socket err=%i", errno);
+error_setg(errp, "l2tpv3_open : could not bind socket err=%i", errno);
goto outerr;
}
if (result) {
@@ -677,10 +679,8 @@ int net_init_l2tpv3(const Netdev *netdev,
result = NULL;
gairet = getaddrinfo(l2tpv3->dst, dstport, , );
if ((gairet != 0) || (result == NULL)) {
-error_report(
-"l2tpv3_open : could not resolve dst, error = %s",
-gai_strerror(gairet)
-);
+error_setg(errp, "l2tpv3_open : could not resolve dst, error = %s",
+ gai_strerror(gairet));
goto outerr;
}
--
2.17.1
[Qemu-devel] [PATCH 31/31] vl: Simplify call of parse_name()
main() checks for parse_name() failure even though it can't actually
fail. That's okay. Simplify it to check by passing _fatal,
like the other users of qemu_opts_foreach().
Signed-off-by: Markus Armbruster
---
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, _fatal);
#ifndef _WIN32
qemu_opts_foreach(qemu_find_opts("add-fd"),
--
2.17.1
[Qemu-devel] [PATCH 19/31] vl: Clean up error reporting in parse_add_fd()
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
---
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, _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, _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, _fatal);
#ifdef CONFIG_VIRTFS
if (qemu_opts_foreach(qemu_find_opts("fsdev"),
--
2.17.1
[Qemu-devel] [PATCH 16/31] seccomp: Clean up error reporting in parse_sandbox()
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_sandbox() 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: Eduardo Otubo
Signed-off-by: Markus Armbruster
---
qemu-seccomp.c | 18 +-
vl.c | 4 ++--
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 1baa5c69ed..6d27699409 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -12,11 +12,12 @@
* Contributions after 2012-01-13 are licensed under the terms of the
* GNU GPL, version 2 or (at your option) any later version.
*/
+
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "qemu/module.h"
-#include "qemu/error-report.h"
#include
#include
#include "sysemu/seccomp.h"
@@ -190,7 +191,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error
**errp)
* to provide a little bit of consistency for
* the command line */
} else {
-error_report("invalid argument for obsolete");
+error_setg(errp, "invalid argument for obsolete");
return -1;
}
}
@@ -205,14 +206,13 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error
**errp)
/* calling prctl directly because we're
* not sure if host has CAP_SYS_ADMIN set*/
if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
-error_report("failed to set no_new_privs "
- "aborting");
+error_setg(errp, "failed to set no_new_privs " "aborting");
return -1;
}
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
-error_report("invalid argument for elevateprivileges");
+error_setg(errp, "invalid argument for elevateprivileges");
return -1;
}
}
@@ -224,7 +224,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error
**errp)
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
-error_report("invalid argument for spawn");
+error_setg(errp, "invalid argument for spawn");
return -1;
}
}
@@ -236,14 +236,14 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error
**errp)
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
-error_report("invalid argument for resourcecontrol");
+error_setg(errp, "invalid argument for resourcecontrol");
return -1;
}
}
if (seccomp_start(seccomp_opts) < 0) {
-error_report("failed to install seccomp syscall filter "
- "in the kernel");
+error_setg(errp, "failed to install seccomp syscall filter "
+ "in the kernel");
return -1;
}
}
diff --git a/vl.c b/vl.c
index 9d2b38a31f..485c3fc008 100644
--- a/vl.c
+++ b/vl.c
@@ -3925,8 +3925,8 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_SECCOMP
olist = qemu_find_opts_err("sandbox", NULL);
-if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
-exit(1);
+if (olist) {
+qemu_opts_foreach(olist, parse_sandbox, NULL, _fatal);
}
#endif
--
2.17.1
[Qemu-devel] [PATCH 28/31] fsdev: Clean up error reporting in qemu_fsdev_add()
Calling error_report() from within a a function that takes an Error **
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.
Cc: Greg Kurz
Signed-off-by: Markus Armbruster
---
fsdev/qemu-fsdev-dummy.c | 2 +-
fsdev/qemu-fsdev.c | 12 +---
fsdev/qemu-fsdev.h | 2 +-
hw/9pfs/xen-9p-backend.c | 7 ++-
vl.c | 8 +++-
5 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c
index 6dc0fbc4c4..489cd29081 100644
--- a/fsdev/qemu-fsdev-dummy.c
+++ b/fsdev/qemu-fsdev-dummy.c
@@ -15,7 +15,7 @@
#include "qemu/config-file.h"
#include "qemu/module.h"
-int qemu_fsdev_add(QemuOpts *opts)
+int qemu_fsdev_add(QemuOpts *opts, Error **errp)
{
return 0;
}
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 8a4afbffbd..7a3b87cc9e 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -30,7 +30,7 @@ static FsDriverTable FsDrivers[] = {
{ .name = "proxy", .ops = _ops},
};
-int qemu_fsdev_add(QemuOpts *opts)
+int qemu_fsdev_add(QemuOpts *opts, Error **errp)
{
int i;
struct FsDriverListEntry *fsle;
@@ -38,10 +38,9 @@ int qemu_fsdev_add(QemuOpts *opts)
const char *fsdriver = qemu_opt_get(opts, "fsdriver");
const char *writeout = qemu_opt_get(opts, "writeout");
bool ro = qemu_opt_get_bool(opts, "readonly", 0);
-Error *local_err = NULL;
if (!fsdev_id) {
-error_report("fsdev: No id specified");
+error_setg(errp, "fsdev: No id specified");
return -1;
}
@@ -53,11 +52,11 @@ int qemu_fsdev_add(QemuOpts *opts)
}
if (i == ARRAY_SIZE(FsDrivers)) {
-error_report("fsdev: fsdriver %s not found", fsdriver);
+error_setg(errp, "fsdev: fsdriver %s not found", fsdriver);
return -1;
}
} else {
-error_report("fsdev: No fsdriver specified");
+error_setg(errp, "fsdev: No fsdriver specified");
return -1;
}
@@ -76,8 +75,7 @@ int qemu_fsdev_add(QemuOpts *opts)
}
if (fsle->fse.ops->parse_opts) {
-if (fsle->fse.ops->parse_opts(opts, >fse, _err)) {
-error_report_err(local_err);
+if (fsle->fse.ops->parse_opts(opts, >fse, errp)) {
g_free(fsle->fse.fsdev_id);
g_free(fsle);
return -1;
diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index 65e4b1cfab..d9716b4144 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -38,7 +38,7 @@ typedef struct FsDriverListEntry {
QTAILQ_ENTRY(FsDriverListEntry) next;
} FsDriverListEntry;
-int qemu_fsdev_add(QemuOpts *opts);
+int qemu_fsdev_add(QemuOpts *opts, Error **errp);
FsDriverEntry *get_fsdev_fsentry(char *id);
extern FileOperations local_ops;
extern FileOperations handle_ops;
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 6026780f95..3f54a21c76 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -14,6 +14,7 @@
#include "hw/9pfs/9p.h"
#include "hw/xen/xen_backend.h"
#include "hw/9pfs/xen-9pfs.h"
+#include "qapi/error.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "fsdev/qemu-fsdev.h"
@@ -355,6 +356,7 @@ static int xen_9pfs_free(struct XenDevice *xendev)
static int xen_9pfs_connect(struct XenDevice *xendev)
{
+Error *err = NULL;
int i;
Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
V9fsState *s = _9pdev->state;
@@ -452,7 +454,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
qemu_opt_set(fsdev, "path", xen_9pdev->path, NULL);
qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL);
qemu_opts_set_id(fsdev, s->fsconf.fsdev_id);
-qemu_fsdev_add(fsdev);
+qemu_fsdev_add(fsdev, );
+if (err) {
+error_report_err(err);
+}
v9fs_device_realize_common(s, _9p_transport, NULL);
return 0;
diff --git a/vl.c b/vl.c
index b6f9212f09..4aa976c4cc 100644
--- a/vl.c
+++ b/vl.c
@@ -2249,7 +2249,7 @@ static int chardev_init_func(void *opaque, QemuOpts
*opts, Error **errp)
#ifdef CONFIG_VIRTFS
static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
-return qemu_fsdev_add(opts);
+return qemu_fsdev_add(opts, errp);
}
#endif
@@ -4188,10 +4188,8 @@ int main(int argc, char **argv, char **envp)
chardev_init_func, NULL, _fatal);
#ifdef CONFIG_VIRTFS
-if (qemu_opts_foreach(qemu_find_opts("fsdev"),
- fsdev_init_func, NULL, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("fsdev"),
+ fsdev_init_func, NULL, _fatal);
#endif
if (qemu_opts_foreach(qemu_find_opts("device"),
--
2.17.1
[Qemu-devel] [PATCH 15/31] xen/pt: Fix incomplete conversion to realize()
The conversion of "xen-pci-passthrough" to realize() (commit
5a11d0f7549, v2.6.0) neglected to convert the xen_pt_config_init()
error path. If xen_pt_config_init() fails, xen_pt_realize() reports
the error, then returns success without completing its job. I don't
know the exact impact, but it can't be good.
Belatedly convert the error path.
Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
Cc: Stefano Stabellini
Cc: Anthony Perard
Signed-off-by: Markus Armbruster
---
hw/xen/xen_pt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index e5a6eff44f..f1f3a3727c 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -830,7 +830,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
xen_pt_config_init(s, );
if (err) {
error_append_hint(, "PCI Config space initialisation failed");
-error_report_err(err);
+error_propagate(errp, err);
rc = -1;
goto err_out;
}
--
2.17.1
[Qemu-devel] [PATCH 20/31] vl: Clean up error reporting in machine_set_property()
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
---
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,
+ _fatal);
configure_accelerator(current_machine);
--
2.17.1
[Qemu-devel] [PATCH 10/31] smbios: Clean up error handling in smbios_add()
Calling error_report() in a function that takes an Error ** argument
is suspicious. smbios_entry_add() does that, and then exit()s. It
also passes _fatal to qemu_opts_validate(). Both wrong, but
currently harmless, as its only caller passes _fatal. Messed up
in commit 1007a37e208. Clean it up.
Cc: Paolo Bonzini
Signed-off-by: Markus Armbruster
---
hw/smbios/smbios.c | 90 +++---
1 file changed, 62 insertions(+), 28 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index a27e54b2fa..920939454e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
void smbios_entry_add(QemuOpts *opts, Error **errp)
{
+Error *err = NULL;
const char *val;
assert(!smbios_immutable);
@@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
int size;
struct smbios_table *table; /* legacy mode only */
-qemu_opts_validate(opts, qemu_smbios_file_opts, _fatal);
+qemu_opts_validate(opts, qemu_smbios_file_opts, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
size = get_image_size(val);
if (size == -1 || size < sizeof(struct smbios_structure_header)) {
-error_report("Cannot read SMBIOS file %s", val);
-exit(1);
+error_setg(errp, "Cannot read SMBIOS file %s", val);
+return;
}
/*
@@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
smbios_tables_len);
if (load_image(val, (uint8_t *)header) != size) {
-error_report("Failed to load SMBIOS file %s", val);
-exit(1);
+error_setg(errp, "Failed to load SMBIOS file %s", val);
+return;
}
if (test_bit(header->type, have_fields_bitmap)) {
-error_report("can't load type %d struct, fields already
specified!",
- header->type);
-exit(1);
+error_setg(errp,
+ "can't load type %d struct, fields already specified!",
+ header->type);
+return;
}
set_bit(header->type, have_binfile_bitmap);
@@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
unsigned long type = strtoul(val, NULL, 0);
if (type > SMBIOS_MAX_TYPE) {
-error_report("out of range!");
-exit(1);
+error_setg(errp, "out of range!");
+return;
}
if (test_bit(type, have_binfile_bitmap)) {
-error_report("can't add fields, binary file already loaded!");
-exit(1);
+error_setg(errp, "can't add fields, binary file already loaded!");
+return;
}
set_bit(type, have_fields_bitmap);
switch (type) {
case 0:
-qemu_opts_validate(opts, qemu_smbios_type0_opts, _fatal);
+qemu_opts_validate(opts, qemu_smbios_type0_opts, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
save_opt(, opts, "vendor");
save_opt(, opts, "version");
save_opt(, opts, "date");
@@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "release");
if (val) {
if (sscanf(val, "%hhu.%hhu", , ) != 2)
{
-error_report("Invalid release");
-exit(1);
+error_setg(errp, "Invalid release");
+return;
}
type0.have_major_minor = true;
}
return;
case 1:
-qemu_opts_validate(opts, qemu_smbios_type1_opts, _fatal);
+qemu_opts_validate(opts, qemu_smbios_type1_opts, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
save_opt(, opts, "manufacturer");
save_opt(, opts, "product");
save_opt(, opts, "version");
@@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "uuid");
if (val) {
if (qemu_uuid_parse(val, _uuid) != 0) {
-error_report("Invalid UUID");
-exit(1);
+error_setg(errp, "Invalid UUID");
+return;
}
qemu_uuid_set = true;
}
return;
case 2:
-qemu_opts_validate(opts, qemu_smbios_type2_opts, _fatal);
+qemu_opts_validate(opts, qemu_smbios_type2_opts, );
+if (err) {
+error_propagate(errp, err);
+return;
+
[Qemu-devel] [PATCH 13/31] net/socket: Fix invalid socket type error handling
Calling error_report() in a function that takes an Error ** argument
is suspicious. net_socket_fd_init() does that, and then fails without
setting an error. Wrong. I didn't analyze how exactly this can
break. A caller that reports the error on failure would crash.
Broken when commit c37f0bb1d0d (v2.11.0) converted the function to
Error. Fix by calling error_setg() instead of error_report().
Fixes: c37f0bb1d0d24e3a6b5f4659bb305913dcb798a6
Cc: Jason Wang
Signed-off-by: Markus Armbruster
---
net/socket.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 6917fbcbf5..90ef3517be 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -453,8 +453,8 @@ static NetSocketState *net_socket_fd_init(NetClientState
*peer,
case SOCK_STREAM:
return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
default:
-error_report("socket type=%d for fd=%d must be either"
- " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+error_setg(errp, "socket type=%d for fd=%d must be either"
+ " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
closesocket(fd);
}
return NULL;
--
2.17.1
[Qemu-devel] [PATCH 17/31] vl: Clean up error reporting in parse_add_fd()
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_add_fd() 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.
Also change call of cleanup_add_fd(), which can't fail, for symmetry.
Signed-off-by: Markus Armbruster
---
vl.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/vl.c b/vl.c
index 485c3fc008..7ce8299d9d 100644
--- a/vl.c
+++ b/vl.c
@@ -1059,12 +1059,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts,
Error **errp)
fd_opaque = qemu_opt_get(opts, "opaque");
if (fd < 0) {
-error_report("fd option is required and must be non-negative");
+error_setg(errp, "fd option is required and must be non-negative");
return -1;
}
if (fd <= STDERR_FILENO) {
-error_report("fd cannot be a standard I/O stream");
+error_setg(errp, "fd cannot be a standard I/O stream");
return -1;
}
@@ -1074,12 +1074,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts,
Error **errp)
*/
flags = fcntl(fd, F_GETFD);
if (flags == -1 || (flags & FD_CLOEXEC)) {
-error_report("fd is not valid or already in use");
+error_setg(errp, "fd is not valid or already in use");
return -1;
}
if (fdset_id < 0) {
-error_report("set option is required and must be non-negative");
+error_setg(errp, "set option is required and must be non-negative");
return -1;
}
@@ -1092,7 +1092,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts,
Error **errp)
}
#endif
if (dupfd == -1) {
-error_report("error duplicating fd: %s", strerror(errno));
+error_setg(errp, "error duplicating fd: %s", strerror(errno));
return -1;
}
@@ -3936,15 +3936,11 @@ int main(int argc, char **argv, char **envp)
}
#ifndef _WIN32
-if (qemu_opts_foreach(qemu_find_opts("add-fd"),
- parse_add_fd, NULL, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("add-fd"),
+ parse_add_fd, NULL, _fatal);
-if (qemu_opts_foreach(qemu_find_opts("add-fd"),
- cleanup_add_fd, NULL, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("add-fd"),
+ cleanup_add_fd, NULL, _fatal);
#endif
current_machine = MACHINE(object_new(object_class_get_name(
--
2.17.1
[Qemu-devel] [PATCH 06/31] char: Use error_printf() to print help and such
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
---
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;
}
--
2.17.1
[Qemu-devel] [PATCH 03/31] cpus hw target: Use warn_report() & friends to report warnings
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
---
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, >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, >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(>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) {
/* Notice when a system that supports MSIx can't initialize it */
if (err != -ENOTSUP) {
-error_report("unable to init msix vectors to %" PRIu32,
- proxy->nvectors);
+warn_report("unable to init msix vectors to %" PRIu32,
+proxy->nvectors);
[Qemu-devel] [PATCH 27/31] spice: Clean up error reporting in add_channel()
Calling error_report() in a function that takes an Error ** argument
is suspicious. add_channel() does that, and then exit()s. Its caller
main(), via qemu_opts_foreach(), is fine with it, but clean it up
anyway.
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
---
ui/spice-core.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a4fbbc3898..ebaae24643 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -597,9 +597,9 @@ static int add_channel(void *opaque, const char *name,
const char *value,
if (strcmp(name, "tls-channel") == 0) {
int *tls_port = opaque;
if (!*tls_port) {
-error_report("spice: tried to setup tls-channel"
- " without specifying a TLS port");
-exit(1);
+error_setg(errp, "spice: tried to setup tls-channel"
+ " without specifying a TLS port");
+return -1;
}
security = SPICE_CHANNEL_SECURITY_SSL;
}
@@ -615,8 +615,9 @@ static int add_channel(void *opaque, const char *name,
const char *value,
rc = spice_server_set_channel_security(spice_server, value, security);
}
if (rc != 0) {
-error_report("spice: failed to set channel security for %s", value);
-exit(1);
+error_setg(errp, "spice: failed to set channel security for %s",
+ value);
+return -1;
}
return 0;
}
@@ -787,7 +788,7 @@ void qemu_spice_init(void)
spice_server_set_playback_compression
(spice_server, qemu_opt_get_bool(opts, "playback-compression", 1));
-qemu_opt_foreach(opts, add_channel, _port, NULL);
+qemu_opt_foreach(opts, add_channel, _port, _fatal);
spice_server_set_name(spice_server, qemu_name);
spice_server_set_uuid(spice_server, (unsigned char *)_uuid);
--
2.17.1
[Qemu-devel] [PATCH 14/31] numa: Fix QMP command set-numa-node error handling
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_numa_node() does that, and then exit()s. It
also passes _fatal to machine_set_cpu_numa_node(). Both wrong.
Attempting to configure numa when the machine doesn't support it kills
the VM:
$ qemu-system-x86_64 -nodefaults -S -display none -M none -preconfig -qmp
stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3},
"package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}}
{"execute": "qmp_capabilities"}
{"return": {}}
{"execute": "set-numa-node", "arguments": {"type": "node"}}
NUMA is not supported by this machine-type
$ echo $?
1
Messed up when commit 64c2a8f6d3f and 7c88e65d9e9 (v2.10.0) added
incorrect error handling right next to correct examples. Latent bug
until commit f3be67812c2 (v3.0.0) made it accessible via QMP. Fairly
harmless in practice, because it's limited to RUN_STATE_PRECONFIG.
The fix is obvious: replace error_report(); exit() by error_setg();
return.
This affects parse_numa_node()'s other caller
numa_complete_configuration(): since it ignores errors, the "NUMA is
not supported by this machine-type" is now ignored, too. But that
error is as unexpected there as any other. Change it to abort on
error instead.
Fixes: f3be67812c226162f86ce92634bd913714445420
Cc: Igor Mammedov
Signed-off-by: Markus Armbruster
fixup! numa: Fix QMP command set-numa-node error handling
---
numa.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/numa.c b/numa.c
index 81542d4ebb..1d7c49ad43 100644
--- a/numa.c
+++ b/numa.c
@@ -60,6 +60,7 @@ NodeInfo numa_info[MAX_NODES];
static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
Error **errp)
{
+Error *err = NULL;
uint16_t nodenr;
uint16List *cpus = NULL;
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -82,8 +83,8 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions
*node,
}
if (!mc->cpu_index_to_instance_props || !mc->get_default_cpu_node_id) {
-error_report("NUMA is not supported by this machine-type");
-exit(1);
+error_setg(errp, "NUMA is not supported by this machine-type");
+return;
}
for (cpus = node->cpus; cpus; cpus = cpus->next) {
CpuInstanceProperties props;
@@ -97,7 +98,11 @@ static void parse_numa_node(MachineState *ms,
NumaNodeOptions *node,
props = mc->cpu_index_to_instance_props(ms, cpus->value);
props.node_id = nodenr;
props.has_node_id = true;
-machine_set_cpu_numa_node(ms, , _fatal);
+machine_set_cpu_numa_node(ms, , );
+if (err) {
+error_propagate(errp, err);
+return;
+}
}
if (node->has_mem && node->has_memdev) {
@@ -367,7 +372,7 @@ void numa_complete_configuration(MachineState *ms)
if (ms->ram_slots > 0 && nb_numa_nodes == 0 &&
mc->auto_enable_numa_with_memhp) {
NumaNodeOptions node = { };
-parse_numa_node(ms, , NULL);
+parse_numa_node(ms, , _abort);
}
assert(max_numa_nodeid <= MAX_NODES);
--
2.17.1
[Qemu-devel] [PATCH 05/31] vfio: Clean up error reporting after previous commit
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(-)
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 481fd08df7..eae31c74d6 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1670,7 +1670,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice
*vdev, int nr)
* but also no point in us enabling VGA if disabled in hardware.
*/
if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, )) {
-error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
error_report("IGD device %s failed to enable VGA access, "
"legacy mode disabled", vdev->vbasedev.name);
goto out;
@@ -1696,7 +1696,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice
*vdev, int nr)
ret = vfio_pci_igd_opregion_init(vdev, opregion, );
if (ret) {
error_append_hint(, "IGD legacy mode disabled\n");
-error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
goto out;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 36f1182249..fedf81e386 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -742,7 +742,7 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
vfio_intx_enable(vdev, );
if (err) {
-error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
@@ -2194,7 +2194,7 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
vfio_intx_enable(vdev, );
if (err) {
-error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) {
@@ -2828,7 +2828,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (stat(vdev->vbasedev.sysfsdev, ) < 0) {
error_setg_errno(errp, errno, "no such host device");
-error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
+error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev);
return;
}
@@ -3079,7 +3079,7 @@ out_teardown:
vfio_teardown_msi(vdev);
vfio_bars_exit(vdev);
error:
-error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
+error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
static void vfio_instance_finalize(Object *obj)
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index c1aecac43c..61852f711b 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -668,7 +668,7 @@ out:
}
if (vdev->vbasedev.name) {
-error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
+error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
} else {
error_prepend(errp, "vfio error: ");
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ad747aab7f..89ed6a5743 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -30,7 +30,6 @@
#include
#endif
-#define ERR_PREFIX "vfio error: %s: "
#define VFIO_MSG_PREFIX "vfio %s: "
enum {
--
2.17.1
[Qemu-devel] [PATCH 00/31] Replace some unwise uses of error_report() & friends
Calling error_report() or similar in a function that takes an Error ** argument is suspicious. Fux a number of instances that are actually wrong. Clean up a few more that are merely fragile / bad examples. Markus Armbruster (31): Use error_fatal to simplify obvious fatal errors (again) block: Use warn_report() & friends to report warnings cpus hw target: Use warn_report() & friends to report warnings vfio: Use warn_report() & friends to report warnings vfio: Clean up error reporting after previous commit char: Use error_printf() to print help and such 9pfs: Fix CLI parsing crash on error pc: Fix machine property nvdimm-persistence error handling ioapic: Fix error handling in realize() smbios: Clean up error handling in smbios_add() migration: Fix !replay_can_snapshot() error handling l2tpv3: Improve -netdev/netdev_add/-net/... error reporting net/socket: Fix invalid socket type error handling numa: Fix QMP command set-numa-node error handling xen/pt: Fix incomplete conversion to realize() seccomp: Clean up error reporting in parse_sandbox() vl: Clean up error reporting in parse_add_fd() qom: Clean up error reporting in user_creatable_add_opts_foreach() vl: Clean up error reporting in parse_add_fd() vl: Clean up error reporting in machine_set_property() vl: Clean up error reporting in mon_init_func() vl: Clean up error reporting in parse_fw_cfg() vl: Clean up error reporting in device_init_func() vl: Clean up error reporting in vnc_init_func() numa: Clean up error reporting in parse_numa() tpm: Clean up error reporting in tpm_init_tpmdev() spice: Clean up error reporting in add_channel() fsdev: Clean up error reporting in qemu_fsdev_add() vl: Assert drive_new() does not fail in default_drive() blockdev: Convert drive_new() to Error vl: Simplify call of parse_name() block/bochs.c| 8 +- block/cloop.c| 8 +- block/dmg.c | 8 +- block/iscsi.c| 2 +- block/rbd.c | 12 +- block/sheepdog.c | 2 +- block/vvfat.c| 8 +- blockdev.c | 27 ++--- chardev/char-pty.c | 2 +- chardev/char.c | 2 +- cpus.c | 8 +- device-hotplug.c | 5 +- fsdev/qemu-fsdev-dummy.c | 2 +- fsdev/qemu-fsdev.c | 12 +- fsdev/qemu-fsdev.h | 2 +- hw/9pfs/9p-handle.c | 6 +- hw/9pfs/xen-9p-backend.c | 7 +- hw/display/cg3.c | 2 +- hw/display/tcx.c | 2 +- hw/i386/pc.c | 5 +- hw/intc/ioapic.c | 8 +- hw/intc/xics_kvm.c | 7 +- hw/misc/ivshmem.c| 4 +- hw/net/virtio-net.c | 8 +- hw/smbios/smbios.c | 90 ++- hw/vfio/pci-quirks.c | 4 +- hw/vfio/pci.c| 22 ++-- hw/vfio/platform.c | 6 +- hw/virtio/virtio-pci.c | 4 +- hw/xen/xen_pt.c | 2 +- include/hw/vfio/vfio-common.h| 3 +- include/sysemu/blockdev.h| 3 +- include/sysemu/numa.h| 1 - include/sysemu/tpm.h | 2 +- migration/savevm.c | 8 +- net/l2tpv3.c | 26 ++--- net/socket.c | 4 +- numa.c | 21 ++-- qemu-io.c| 8 +- qemu-nbd.c | 14 +-- qemu-seccomp.c | 18 +-- qom/object_interfaces.c | 4 +- scripts/coccinelle/use-error_fatal.cocci | 20 stubs/tpm.c | 3 +- target/i386/cpu.c| 17 +-- target/ppc/translate_init.inc.c | 4 +- tpm.c| 22 ++-- ui/spice-core.c | 13 ++- ui/vnc.c | 5 +- vl.c | 140 +-- 50 files changed, 317 insertions(+), 304 deletions(-) create mode 100644 scripts/coccinelle/use-error_fatal.cocci -- 2.17.1
[Qemu-devel] [PATCH 08/31] pc: Fix machine property nvdimm-persistence error handling
Calling error_report() in a function that takes an Error ** argument
is suspicious. pc.c's pc_machine_set_nvdimm_persistence() does that,
and then exit()s. Wrong. Attempting to set machine property
nvdimm-persistence to a bad value instantly kills the VM:
$ qemu-system-x86_64 -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3},
"package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}}
{"execute": "qmp_capabilities"}
{"return": {}}
{"execute": "qom-set", "arguments": {"path": "/machine", "property":
"nvdimm-persistence", "value": "instadeath"}}
-machine nvdimm-persistence=instadeath: unsupported option
$ echo $?
1
Broken when commit 11c39b5cd96 (v3.0.0) replaced error_propagate();
return by error_report(); exit() instead of error_setg(); return. Fix
that.
Fixes: 11c39b5cd966ddc067a1ca0c5392ec9b666c45b7
Cc: "Michael S. Tsirkin"
Signed-off-by: Markus Armbruster
---
hw/i386/pc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cd5029c149..eab8572f2a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2209,8 +2209,9 @@ static void pc_machine_set_nvdimm_persistence(Object
*obj, const char *value,
else if (strcmp(value, "mem-ctrl") == 0)
nvdimm_state->persistence = 2;
else {
-error_report("-machine nvdimm-persistence=%s: unsupported option",
value);
-exit(EXIT_FAILURE);
+error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
+ value);
+return;
}
g_free(nvdimm_state->persistence_string);
--
2.17.1
[Qemu-devel] [PATCH 04/31] vfio: Use warn_report() & friends to report warnings
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(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 866f0deeb7..36f1182249 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -249,7 +249,7 @@ static void vfio_intx_update(PCIDevice *pdev)
vfio_intx_enable_kvm(vdev, );
if (err) {
-error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
/* Re-enable the interrupt in cased we missed an EOI */
@@ -314,7 +314,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error
**errp)
vfio_intx_enable_kvm(vdev, );
if (err) {
-error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
vdev->interrupt = VFIO_INT_INTx;
@@ -1555,7 +1555,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos,
Error **errp)
);
if (ret < 0) {
if (ret == -ENOTSUP) {
-error_report_err(err);
+warn_report_err(err);
return 0;
}
@@ -2588,9 +2588,9 @@ static void vfio_populate_device(VFIOPCIDevice *vdev,
Error **errp)
} else if (irq_info.count == 1) {
vdev->pci_aer = true;
} else {
-error_report(WARN_PREFIX
- "Could not enable error recovery for the device",
- vbasedev->name);
+warn_report(VFIO_MSG_PREFIX
+"Could not enable error recovery for the device",
+vbasedev->name);
}
}
@@ -2715,7 +2715,7 @@ static void vfio_req_notifier_handler(void *opaque)
qdev_unplug(>pdev.qdev, );
if (err) {
-error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 57c4a0ee2b..c1aecac43c 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -657,8 +657,8 @@ static void vfio_platform_realize(DeviceState *dev, Error
**errp)
for (i = 0; i < vbasedev->num_regions; i++) {
if (vfio_region_mmap(vdev->regions[i])) {
-error_report("%s mmap unsupported. Performance may be slow",
- memory_region_name(vdev->regions[i]->mem));
+warn_report("%s mmap unsupported, performance may be slow",
+memory_region_name(vdev->regions[i]->mem));
}
sysbus_init_mmio(sbdev, vdev->regions[i]->mem);
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 821def0565..ad747aab7f 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -31,7 +31,7 @@
#endif
#define ERR_PREFIX "vfio error: %s: "
-#define WARN_PREFIX "vfio warning: %s: "
+#define VFIO_MSG_PREFIX "vfio %s: "
enum {
VFIO_DEVICE_TYPE_PCI = 0,
--
2.17.1
[Qemu-devel] [PATCH 18/31] qom: Clean up error reporting in user_creatable_add_opts_foreach()
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(-)
diff --git a/qemu-io.c b/qemu-io.c
index 13829f5e21..6df7731af4 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -620,11 +620,9 @@ int main(int argc, char **argv)
exit(1);
}
-if (qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- NULL, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, _fatal);
if (!trace_init_backends()) {
exit(1);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7874bc973c..ca7109652e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -766,11 +766,9 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
-if (qemu_opts_foreach(_object_opts,
- user_creatable_add_opts_foreach,
- NULL, NULL)) {
-exit(EXIT_FAILURE);
-}
+qemu_opts_foreach(_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, _fatal);
if (!trace_init_backends()) {
exit(1);
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 72b97a8bed..4052d6c4a7 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -143,7 +143,6 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts
*opts, Error **errp)
{
bool (*type_predicate)(const char *) = opaque;
Object *obj = NULL;
-Error *err = NULL;
const char *type;
type = qemu_opt_get(opts, "qom-type");
@@ -152,9 +151,8 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts
*opts, Error **errp)
return 0;
}
-obj = user_creatable_add_opts(opts, );
+obj = user_creatable_add_opts(opts, errp);
if (!obj) {
-error_report_err(err);
return -1;
}
object_unref(obj);
diff --git a/vl.c b/vl.c
index 7ce8299d9d..b8576f8f10 100644
--- a/vl.c
+++ b/vl.c
@@ -4181,11 +4181,9 @@ int main(int argc, char **argv, char **envp)
page_size_init();
socket_init();
-if (qemu_opts_foreach(qemu_find_opts("object"),
- user_creatable_add_opts_foreach,
- object_create_initial, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("object"),
+ user_creatable_add_opts_foreach,
+ object_create_initial, _fatal);
if (qemu_opts_foreach(qemu_find_opts("chardev"),
chardev_init_func, NULL, NULL)) {
@@ -4316,11 +4314,9 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
-if (qemu_opts_foreach(qemu_find_opts("object"),
- user_creatable_add_opts_foreach,
- object_create_delayed, NULL)) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("object"),
+ user_creatable_add_opts_foreach,
+ object_create_delayed, _fatal);
if (tpm_init() < 0) {
exit(1);
--
2.17.1
[Qemu-devel] [PATCH 24/31] vl: Clean up error reporting in vnc_init_func()
Calling error_report() in a function that takes an Error ** argument
is suspicious. vnc_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.
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
---
ui/vnc.c | 5 +++--
vl.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index cf221c83cc..df6c84b802 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -4082,8 +4082,9 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error
**errp)
vnc_display_init(id);
vnc_display_open(id, _err);
if (local_err != NULL) {
-error_reportf_err(local_err, "Failed to start VNC server: ");
-exit(1);
+error_propagate(errp, local_err);
+error_prepend(errp, "Failed to start VNC server: ");
+return -1;
}
return 0;
}
diff --git a/vl.c b/vl.c
index 86eee4c798..5c159ca45c 100644
--- a/vl.c
+++ b/vl.c
@@ -4479,7 +4479,7 @@ int main(int argc, char **argv, char **envp)
/* init remote displays */
#ifdef CONFIG_VNC
qemu_opts_foreach(qemu_find_opts("vnc"),
- vnc_init_func, NULL, NULL);
+ vnc_init_func, NULL, _fatal);
#endif
if (using_spice) {
--
2.17.1
[Qemu-devel] [PATCH 02/31] block: Use warn_report() & friends to report warnings
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.
Cc: Kevin Wolf
Cc: Ronnie Sahlberg
Cc: Paolo Bonzini
Cc: Peter Lieven
Cc: Hitoshi Mitake
Cc: Liu Yuan
Signed-off-by: Markus Armbruster
---
block/bochs.c| 8
block/cloop.c| 8
block/dmg.c | 8
block/iscsi.c| 2 +-
block/rbd.c | 12 ++--
block/sheepdog.c | 2 +-
block/vvfat.c| 8
7 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..36c1b45bd2 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -112,10 +112,10 @@ static int bochs_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (!bdrv_is_read_only(bs)) {
-error_report("Opening bochs images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+warn_report("Opening bochs images without an explicit read-only=on "
+"option is deprecated");
+error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
if (ret < 0) {
return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..a558e67cb0 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -74,10 +74,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options,
int flags,
}
if (!bdrv_is_read_only(bs)) {
-error_report("Opening cloop images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+warn_report("Opening cloop images without an explicit read-only=on "
+"option is deprecated");
+error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp);
if (ret < 0) {
return ret;
diff --git a/block/dmg.c b/block/dmg.c
index c9b3c519c4..9fb814460d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -420,10 +420,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
}
if (!bdrv_is_read_only(bs)) {
-error_report("Opening dmg images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+warn_report("Opening dmg images without an explicit read-only=on "
+"option is deprecated");
+error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp);
if (ret < 0) {
return ret;
diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..73998c2860 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1844,7 +1844,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
iscsi_set_timeout(iscsi, timeout);
#else
if (timeout) {
-error_report("iSCSI: ignoring timeout value for libiscsi <1.15.0");
+warn_report("iSCSI: ignoring timeout value for libiscsi <1.15.0");
}
#endif
diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..6e26bac170 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -750,8 +750,8 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
/* Take care whenever deciding to actually deprecate; once this ability
* is removed, we will not be able to open any images with
legacy-styled
* backing image strings. */
-error_report("RBD options encoded in the filename as keyvalue pairs "
- "is deprecated");
+warn_report("RBD options encoded in the filename as keyvalue pairs "
+"is deprecated");
}
/* Remove the processed options from the QDict (the visitor processes
@@ -781,10 +781,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
* leave as-is */
if (s->snap != NULL) {
if (!bdrv_is_read_only(bs)) {
-error_report("Opening rbd snapshots without an explicit "
- "read-only=on option is deprecated. Future versions "
-
[Qemu-devel] [PATCH 29/31] vl: Assert drive_new() does not fail in default_drive()
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(-)
diff --git a/vl.c b/vl.c
index 4aa976c4cc..0d25956b2f 100644
--- a/vl.c
+++ b/vl.c
@@ -1156,9 +1156,7 @@ static void default_drive(int enable, int snapshot,
BlockInterfaceType type,
}
dinfo = drive_new(opts, type);
-if (!dinfo) {
-exit(1);
-}
+assert(dinfo);
dinfo->is_default = true;
}
--
2.17.1
[Qemu-devel] [PATCH 21/31] vl: Clean up error reporting in mon_init_func()
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
---
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, _fatal);
if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
exit(1);
--
2.17.1
[Qemu-devel] [PATCH 09/31] ioapic: Fix error handling in realize()
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
---
hw/intc/ioapic.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index b6896ac4ce..4e529729b4 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -21,7 +21,7 @@
*/
#include "qemu/osdep.h"
-#include "qemu/error-report.h"
+#include "qapi/error.h"
#include "monitor/monitor.h"
#include "hw/hw.h"
#include "hw/i386/pc.h"
@@ -393,9 +393,9 @@ static void ioapic_realize(DeviceState *dev, Error **errp)
IOAPICCommonState *s = IOAPIC_COMMON(dev);
if (s->version != 0x11 && s->version != 0x20) {
-error_report("IOAPIC only supports version 0x11 or 0x20 "
- "(default: 0x%x).", IOAPIC_VER_DEF);
-exit(1);
+error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 "
+ "(default: 0x%x).", IOAPIC_VER_DEF);
+return;
}
memory_region_init_io(>io_memory, OBJECT(s), _io_ops, s,
--
2.17.1
[Qemu-devel] [PATCH 01/31] Use error_fatal to simplify obvious fatal errors (again)
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
---
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 = >irqs[i];
kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
- i + ics->offset, , false, _err);
-if (local_err) {
-error_report_err(local_err);
-exit(1);
-}
+ i + ics->offset, , false, _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, _err);
-if (!exp) {
-error_report_err(local_err);
-exit(EXIT_FAILURE);
-}
+ writethrough, NULL, _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, );
++T RET = FUN(ARGS, _fatal);
+|
+-RET = FUN(ARGS, );
++RET = FUN(ARGS, _fatal);
+|
+-FUN(ARGS, );
++FUN(ARGS, _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", );
-if (!v) {
-error_report_err(err);
-exit(1);
-}
+v = qobject_input_visitor_new_str(optarg, "type", _fatal);
visit_type_DisplayOptions(v, NULL, , _fatal);
QAPI_CLONE_MEMBERS(DisplayOptions, , opts);
--
2.17.1
[Qemu-devel] [PATCH 11/31] migration: Fix !replay_can_snapshot() error handling
Calling error_report() in a function that takes an Error ** argument
is suspicious. save_snapshot() and load_snapshot() do that, and then
fail without setting an error. Wrong. The HMP commands survive this
unscathed, since hmp_handle_error() does nothing when no error has
been set. Callers main() (on behalf of -loadvm) and
replay_vmstate_init() crash, but I'm not sure the error is possible
there.
Screwed up when commit 377b21ccea1 (v2.12.0) added incorrect error
handling right next to correct examples. Fix by calling error_setg()
instead of error_report().
Fixes: 377b21ccea1755a8b0dae822c29567c58dda6939
Cc: Paolo Bonzini
Signed-off-by: Markus Armbruster
---
migration/savevm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 2d10e45582..5f8eb38676 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2414,8 +2414,8 @@ int save_snapshot(const char *name, Error **errp)
AioContext *aio_context;
if (!replay_can_snapshot()) {
-error_report("Record/replay does not allow making snapshot "
- "right now. Try once more later.");
+error_setg(errp, "Record/replay does not allow making snapshot "
+ "right now. Try once more later.");
return ret;
}
@@ -2611,8 +2611,8 @@ int load_snapshot(const char *name, Error **errp)
MigrationIncomingState *mis = migration_incoming_get_current();
if (!replay_can_snapshot()) {
-error_report("Record/replay does not allow loading snapshot "
- "right now. Try once more later.");
+error_setg(errp, "Record/replay does not allow loading snapshot "
+ "right now. Try once more later.");
return -EINVAL;
}
--
2.17.1
[Qemu-devel] [PATCH 23/31] vl: Clean up error reporting in device_init_func()
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
---
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, );
+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, _fatal);
cpu_synchronize_all_post_init();
--
2.17.1
[Qemu-devel] [PATCH 26/31] tpm: Clean up error reporting in tpm_init_tpmdev()
Calling error_report() in a function that takes an Error ** argument
is suspicious. tpm_init_tpmdev() does that, and then fails without
setting an error. Its caller main(), via tpm_init() and
qemu_opts_foreach(), is fine with it, but clean it up anyway.
Cc: Stefan Berger
Signed-off-by: Markus Armbruster
---
include/sysemu/tpm.h | 2 +-
stubs/tpm.c | 3 +--
tpm.c| 22 +-
vl.c | 4 +---
4 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 9ae1ab6da3..17a97ed77a 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -16,7 +16,7 @@
#include "qom/object.h"
int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
-int tpm_init(void);
+void tpm_init(void);
void tpm_cleanup(void);
typedef enum TPMVersion {
diff --git a/stubs/tpm.c b/stubs/tpm.c
index 6729bc8517..80939cd3db 100644
--- a/stubs/tpm.c
+++ b/stubs/tpm.c
@@ -9,9 +9,8 @@
#include "qapi/qapi-commands-tpm.h"
#include "sysemu/tpm.h"
-int tpm_init(void)
+void tpm_init(void)
{
-return 0;
}
void tpm_cleanup(void)
diff --git a/tpm.c b/tpm.c
index 93031723ad..9c9e20bbb7 100644
--- a/tpm.c
+++ b/tpm.c
@@ -89,19 +89,19 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts,
Error **errp)
int i;
if (!QLIST_EMPTY(_backends)) {
-error_report("Only one TPM is allowed.");
+error_setg(errp, "Only one TPM is allowed.");
return 1;
}
id = qemu_opts_id(opts);
if (id == NULL) {
-error_report(QERR_MISSING_PARAMETER, "id");
+error_setg(errp, QERR_MISSING_PARAMETER, "id");
return 1;
}
value = qemu_opt_get(opts, "type");
if (!value) {
-error_report(QERR_MISSING_PARAMETER, "type");
+error_setg(errp, QERR_MISSING_PARAMETER, "type");
tpm_display_backend_drivers();
return 1;
}
@@ -109,8 +109,8 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts,
Error **errp)
i = qapi_enum_parse(_lookup, value, -1, NULL);
be = i >= 0 ? tpm_be_find_by_type(i) : NULL;
if (be == NULL) {
-error_report(QERR_INVALID_PARAMETER_VALUE,
- "type", "a TPM backend type");
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
+ "a TPM backend type");
tpm_display_backend_drivers();
return 1;
}
@@ -118,7 +118,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts,
Error **errp)
/* validate backend specific opts */
qemu_opts_validate(opts, be->opts, _err);
if (local_err) {
-error_report_err(local_err);
+error_propagate(errp, local_err);
return 1;
}
@@ -151,14 +151,10 @@ void tpm_cleanup(void)
* Initialize the TPM. Process the tpmdev command line options describing the
* TPM backend.
*/
-int tpm_init(void)
+void tpm_init(void)
{
-if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
- tpm_init_tpmdev, NULL, NULL)) {
-return -1;
-}
-
-return 0;
+qemu_opts_foreach(qemu_find_opts("tpmdev"),
+ tpm_init_tpmdev, NULL, _fatal);
}
/*
diff --git a/vl.c b/vl.c
index 5c159ca45c..b6f9212f09 100644
--- a/vl.c
+++ b/vl.c
@@ -4312,9 +4312,7 @@ int main(int argc, char **argv, char **envp)
user_creatable_add_opts_foreach,
object_create_delayed, _fatal);
-if (tpm_init() < 0) {
-exit(1);
-}
+tpm_init();
/* init the bluetooth world */
if (foreach_device_config(DEV_BT, bt_parse))
--
2.17.1
Re: [Qemu-devel] [QEMU PATCH v2 0/2]: KVM: i386: Add support for save and restore nested state
Gentle ping on v2 of this series.
(I noticed 1st patch of series was already applied)
Thanks,
-Liran
> On 16 Sep 2018, at 15:46, Liran Alon wrote:
>
> Hi,
>
> This series aims to add support for QEMU to be able to migrate VMs that
> are running nested hypervisors. In order to do so, it utilizes the new
> IOCTLs introduced in KVM commit 8fcc4b5923af ("kvm: nVMX: Introduce
> KVM_CAP_NESTED_STATE") which were created for this purpose.
>
> 1st patch is not really related to the goal of the patch series. It just
> makes CPUX86State->xsave_buf to be compiled only when needed (When
> compiling with KVM or HVF CPU accelerator).
>
> 2nd patch adds the support to migrate VMs that are running nested
> hypervisors.
>
> Regards,
> -Liran
>
> v1->v2 Changes:
> * Renamed kvm_nested_state_length() to kvm_max_nested_state_length()
> to better indicate it represents the max nested state size that can
> be returned from kernel.
> * Added error_report() calls to nested_state_post_load() to make
> failures in migration easier to diagnose.
> * Fixed support of migrating with various nested_state buffer sizes.
> The following scenarios were tested:
> (a) src and dest have same nested state size.
> ==> Migration succeeds.
> (b) src don't have nested state while dest do.
> ==> Migration succeed and src don't send it's nested state.
> (c) src have nested state while dest don't.
> ==> Migration fails as it cannot restore nested state.
> (d) dest have bigger max nested state size than src
> ==> Migration succeeds.
> (e) dest have smaller max nested state size than src but enough to store it's
> saved nested state
> ==> Migration succeeds
> (f) dest have smaller max nested state size than src but not enough to store
> it's saved nested state
> ==> Migration fails
>
Re: [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain
On 8 October 2018 at 17:40, Kevin Wolf wrote: > By the way, can you reproduce this with virtio-blk/scsi and an iothread > in a real QEMU or is it only the test case that fails? In theory, I > don't see what would prevent QEMU from hanging at shutdown. I haven't tested, but I suspect this is less likely to cause a problem, because in a real QEMU run we'll only kill these threads and cause the memory corruption at the end of the run when QEMU is exiting anyway. The problem in the test case is that we kill threads and corrupt memory, and then continue to use the same process for the next test in the set, which then crashes later as a result of the memory corruption. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 2/2] MAINTAINERS: Remove myself as block maintainer
On 10/07/2018 07:51 PM, Max Reitz wrote: > On 26.09.18 20:05, Jeff Cody wrote: >> I'll not be involved in day-to-day qemu development. Remove myself as >> maintainer from the remainder of the network block drivers, and revert >> them to the general block layer maintainership. >> >> Move 'sheepdog' to the 'Odd Fixes' support level. >> >> For VHDX, added my personal email address as a maintainer, as I can >> answer questions or send the occassional bug fix. Leaving it as >> 'Supported', instead of 'Odd Fixes', because I think the rest of the >> block layer maintainers and developers will upkeep it as well, if >> needed. >> >> Signed-off-by: Jeff Cody >> --- >> MAINTAINERS | 17 ++--- >> 1 file changed, 2 insertions(+), 15 deletions(-) > > Not sure who's going to merge this (maybe me, but then I'd like an ACK > from John on patch 1), but: > > Acked-by: Max Reitz > I thought I did, but maybe I thought it would be rude to confirm myself before someone else did. signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
Am 08.10.2018 um 18:04 hat Anton Nefedov geschrieben:
>
>
> On 8/10/2018 6:46 PM, Kevin Wolf wrote:
> > Am 08.10.2018 um 17:25 hat Anton Nefedov geschrieben:
> >>
> >>
> >> On 8/10/2018 6:03 PM, Kevin Wolf wrote:
> >>> Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
> On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> > Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
> >> Signed-off-by: Anton Nefedov
> >> Reviewed-by: Alberto Garcia
> >> ---
> >> hw/ide/core.c | 12
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >> index 2c62efc..352429b 100644
> >> --- a/hw/ide/core.c
> >> +++ b/hw/ide/core.c
> >> @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int
> >> ret)
> >> TrimAIOCB *iocb = opaque;
> >> IDEState *s = iocb->s;
> >>
> >> +if (iocb->i >= 0) {
> >> +if (ret >= 0) {
> >> +block_acct_done(blk_get_stats(s->blk), >acct);
> >> +} else {
> >> +block_acct_failed(blk_get_stats(s->blk), >acct);
> >> +}
> >> +}
> >> +
> >> if (ret >= 0) {
> >> while (iocb->j < iocb->qiov->niov) {
> >> int j = iocb->j;
> >> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int
> >> ret)
> >> goto done;
> >> }
> >>
> >> +block_acct_start(blk_get_stats(s->blk), >acct,
> >> + count << BDRV_SECTOR_BITS,
> >> BLOCK_ACCT_UNMAP);
> >> +
> >> /* Got an entry! Submit and exit. */
> >> iocb->aiocb = blk_aio_pdiscard(s->blk,
> >>sector <<
> >> BDRV_SECTOR_BITS,
> >> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
> >> }
> >>
> >> if (ret == -EINVAL) {
> >> +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
> >
> > This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> > also for reads and writes, and each of them could return -EINVAL.
> >
>
> Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
>
> > Also, -EINVAL doesn't necessarily mean that the guest driver did
> > something wrong, it could also be the result of a host problem.
> > Therefore, it isn't right to call block_acct_invalid() here - especially
> > since the request may already have been accounted for as either done or
> > failed in ide_issue_trim_cb().
> >
>
> Couldn't be accounted done with such retcode;
> and it seems I shouldnt do block_acct_failed() there anyway - or it's
> accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
>
> But if EINVAL (from further layers) should not be accounted as an
> invalid op, then it should be accounted failed instead, the thing that
> current code does not do.
> (and which brings us back to possible double-accounting if we account
> invalid in ide_issue_trim_cb() )
> >>>
> >>> Yes, commit caeadbc8ba4 was already wrong in assuming that there is
> >>> only one possible source for -EINVAL.
> >>>
> > Instead, I think it would be better to immediately account for invalid
> > requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> > know for sure that indeed !ide_sect_range_ok() is the cause for the
> > -EINVAL return code.
> >
> So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
> acct_failed there, and filter off TRIM commands in the common
> accounting.
> >>>
> >>> blk_aio_discard() can fail with -EINVAL, too, so getting this error code
> >>> from a TRIM command doesn't mean anything. It can still have multiple
> >>> possible sources.
> >>>
> >>
> >> I meant that common ide_dma_cb() should account EINVAL (along with other
> >> errors) as failed, unless it's TRIM, which means it's already
> >> accounted (either invalid or failed)
> >
> > Oh, you would already account for failure in ide_issue_trim_cb(), too,
> > but only if it's EINVAL? That feels like it would complicate the code
> > quite a bit.
> >
>
> No, no :) ide_issue_trim_cb does the proper accounting (failed/invalid)
> for TRIM.
> Then common path (ide_dma_cb()) does not account TRIM operations at all
> regardless of the error code. No need to check for TRIM specifically if
> we have BLOCK_ACCT_NONE.
>
> > And actually, didn't commit caeadbc8ba4 break werror=stop for requests
> > returning -EINVAL because we don't call ide_handle_rw_error() any more?
> >
>
> Yes.
>
> Read/write ignore werror=stop for invalid range case (not for EINVAL).
> I wonder if it's crucial to ignore it for TRIM too, otherwise we could
> just remove this chunk
Re: [Qemu-devel] [PATCH] .travis.yml: split MacOSX builds and reduce target list
Hi Alex, On Mon, Oct 8, 2018 at 5:21 PM Alex Bennée wrote: > We have reached the point where the MacOSX build was regularly timing > out. So as before I've reduced the target list to "major" > architectures to try and bring the build time down. I've added an > additional MacOSX build with the latest XCode with a minimal list of > "most likely" targets on MacOS. > > Signed-off-by: Alex Bennée > --- > .travis.yml | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/.travis.yml b/.travis.yml > index 95be6ec59f..13a09facd3 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -107,8 +107,14 @@ matrix: > - env: CONFIG="--disable-tcg" > TEST_CMD="" >compiler: gcc > -- env: CONFIG="" > +# MacOSX builds > +- env: > CONFIG="--target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu" >os: osx > + osx_image: xcode9.4 > + compiler: clang > +- env: > CONFIG="--target-list=i386-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,x86_64-softmmu" > + os: osx > + osx_image: xcode10 I'd also add another entry with --enable-user. >compiler: clang > # Python builds > - env: CONFIG="--target-list=x86_64-softmmu" > -- > 2.17.1 >
Re: [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain
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). By the way, can you reproduce this with virtio-blk/scsi and an iothread in a real QEMU or is it only the test case that fails? In theory, I don't see what would prevent QEMU from hanging at shutdown. Kevin
Re: [Qemu-devel] [PATCH] vl.c: print error message if load fw_cfg file failed
Hi Li,
On Sun, Oct 7, 2018 at 7:48 PM Philippe Mathieu-Daudé wrote:
> On 10/7/18 6:33 AM, Li Qiang wrote:
> > It makes sense to print the error message while reading
> > file failed.
> >
> > Signed-off-by: Li Qiang
> > ---
> > vl.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index cc55fe04a2..3db410e771 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2207,8 +2207,9 @@ 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, , , NULL)) {
> > -error_report("can't load %s", file);
> > +GError *error = NULL;
> > +if (!g_file_get_contents(file, , , )) {
> > +error_report("can't load %s, %s", file, error->message);
You forgot:
g_error_free(error);
With the free:
Reviewed-by: Philippe Mathieu-Daudé
>
> > return -1;
> > }
> > }
> >
[Qemu-devel] [PATCH v3 3/3] linux-user: Implement special usbfs ioctls.
Userspace submits a USB Request Buffer to the kernel, optionally
discards it, and finally reaps the URB. Thunk buffers from target
to host and back.
Tested by running an i386 scanner driver on ARMv7 and by running
the PowerPC lsusb utility on x86_64. The discardurb ioctl is
not exercised in these tests.
Signed-off-by: Cortland Tölva
---
There are two alternatives for the strategy of holding lock_user on
memory from submit until reap. v3 of this series tries to determine
the access permissions for user memory from endpoint direction, but
the logic for this is complex. The first alternative is to request
write access. If that fails, request read access. If that fails, try
to submit the ioctl with no buffer - perhaps the user code filled in
fields the kernel will ignore. The second alternative is to read user
memory into an allocated buffer, pass it to the kernel, and write back
to target memory only if the kernel indicates that writes occurred.
Changes from v1:
improve pointer cast to int compatibility
remove unimplemented types for usb streams
struct definitions moved to this patch where possible
Changes from v2:
organize urb thunk metadata in a struct
hold lock_user from submit until discard
fixes for 64-bit hosts
linux-user/ioctls.h| 8 ++
linux-user/syscall.c | 177 +
linux-user/syscall_defs.h | 4 +
linux-user/syscall_types.h | 20 +
4 files changed, 209 insertions(+)
diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 92f6177f1d..ae8951625f 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -143,6 +143,14 @@
IOCTL(USBDEVFS_SETCONFIGURATION, IOC_W, MK_PTR(TYPE_INT))
IOCTL(USBDEVFS_GETDRIVER, IOC_R,
MK_PTR(MK_STRUCT(STRUCT_usbdevfs_getdriver)))
+ IOCTL_SPECIAL(USBDEVFS_SUBMITURB, IOC_W, do_ioctl_usbdevfs_submiturb,
+ MK_PTR(MK_STRUCT(STRUCT_usbdevfs_urb)))
+ IOCTL_SPECIAL(USBDEVFS_DISCARDURB, IOC_RW, do_ioctl_usbdevfs_discardurb,
+ MK_PTR(MK_STRUCT(STRUCT_usbdevfs_urb)))
+ IOCTL_SPECIAL(USBDEVFS_REAPURB, IOC_R, do_ioctl_usbdevfs_reapurb,
+ MK_PTR(TYPE_PTRVOID))
+ IOCTL_SPECIAL(USBDEVFS_REAPURBNDELAY, IOC_R, do_ioctl_usbdevfs_reapurb,
+ MK_PTR(TYPE_PTRVOID))
IOCTL(USBDEVFS_DISCSIGNAL, IOC_W,
MK_PTR(MK_STRUCT(STRUCT_usbdevfs_disconnectsignal)))
IOCTL(USBDEVFS_CLAIMINTERFACE, IOC_W, MK_PTR(TYPE_INT))
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2641260186..9b7ea96cfb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -96,6 +96,7 @@
#include
#if defined(CONFIG_USBFS)
#include
+#include
#endif
#include
#include
@@ -4199,6 +4200,182 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie,
uint8_t *buf_temp,
return ret;
}
+#if defined(CONFIG_USBFS)
+#if HOST_LONG_BITS > 64
+#error USBDEVFS thunks do not support >64 bit hosts yet.
+#endif
+struct live_urb {
+uint64_t target_urb_adr;
+uint64_t target_buf_adr;
+char *target_buf_ptr;
+struct usbdevfs_urb host_urb;
+};
+
+static GHashTable *usbdevfs_urb_hashtable(void)
+{
+static GHashTable *urb_hashtable;
+
+if (!urb_hashtable) {
+urb_hashtable = g_hash_table_new(g_int64_hash, g_int64_equal);
+}
+return urb_hashtable;
+}
+
+static void urb_hashtable_insert(struct live_urb *urb)
+{
+GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
+g_hash_table_insert(urb_hashtable, urb, urb);
+}
+
+static struct live_urb *urb_hashtable_lookup(uint64_t target_urb_adr)
+{
+GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
+return g_hash_table_lookup(urb_hashtable, _urb_adr);
+}
+
+static void urb_hashtable_remove(struct live_urb *urb)
+{
+GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
+g_hash_table_remove(urb_hashtable, urb);
+}
+
+static abi_long
+do_ioctl_usbdevfs_reapurb(const IOCTLEntry *ie, uint8_t *buf_temp,
+ int fd, int cmd, abi_long arg)
+{
+const argtype usbfsurb_arg_type[] = { MK_STRUCT(STRUCT_usbdevfs_urb) };
+const argtype ptrvoid_arg_type[] = { TYPE_PTRVOID, 0, 0 };
+struct live_urb *lurb;
+void *argptr;
+uint64_t hurb;
+int target_size;
+uintptr_t target_urb_adr;
+abi_long ret;
+
+target_size = thunk_type_size(usbfsurb_arg_type, THUNK_TARGET);
+
+memset(buf_temp, 0, sizeof(uint64_t));
+ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
+if (is_error(ret)) {
+return ret;
+}
+
+memcpy(, buf_temp, sizeof(uint64_t));
+lurb = (void *)((uintptr_t)hurb - offsetof(struct live_urb, host_urb));
+if (!lurb->target_urb_adr) {
+return -TARGET_EFAULT;
+}
+urb_hashtable_remove(lurb);
+unlock_user(lurb->target_buf_ptr, lurb->target_buf_adr,
+lurb->host_urb.buffer_length);
+lurb->target_buf_ptr = NULL;
+
+/* restore the guest buffer pointer */
+lurb->host_urb.buffer = (void *)(uintptr_t)lurb->target_buf_adr;
+
+/* update the guest urb struct */
+
[Qemu-devel] [PATCH v3 1/3] linux-user: Check for Linux USBFS in configure
In preparation for adding user mode emulation support for the Linux usbfs interface, check for its kernel header. Signed-off-by: Cortland Tölva Reviewed-by: Laurent Vivier Message-Id: <20180925071228.32040-2-...@tolva.net> Signed-off-by: Laurent Vivier --- configure | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/configure b/configure index f89d293585..19d66bbdeb 100755 --- a/configure +++ b/configure @@ -4236,7 +4236,14 @@ if compile_prog "" "" ; then memfd=yes fi - +# check for usbfs +have_usbfs=no +if test "$linux_user" = "yes"; then + if check_include linux/usbdevice_fs.h; then +have_usbfs=yes + fi + have_usbfs=yes +fi # check for fallocate fallocate=no @@ -6350,6 +6357,9 @@ fi if test "$memfd" = "yes" ; then echo "CONFIG_MEMFD=y" >> $config_host_mak fi +if test "$have_usbfs" = "yes" ; then + echo "CONFIG_USBFS=y" >> $config_host_mak +fi if test "$fallocate" = "yes" ; then echo "CONFIG_FALLOCATE=y" >> $config_host_mak fi -- 2.11.0
[Qemu-devel] [PATCH v3 2/3] linux-user: Define ordinary usbfs ioctls.
Provide ioctl definitions for the generic thunk mechanism to
convert most usbfs calls. Calculate arg size at runtime.
Signed-off-by: Cortland Tölva
---
Changes from v1:
move some type definitions to patch 3/3
Changes from v2:
calculate ioctl arg size at runtime
linux-user/ioctls.h| 38
linux-user/syscall.c | 3 +++
linux-user/syscall_defs.h | 24 +++
linux-user/syscall_types.h | 48 ++
4 files changed, 113 insertions(+)
diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 586c794639..92f6177f1d 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -131,6 +131,44 @@
IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
IOCTL(FS_IOC_SETFLAGS, IOC_W, MK_PTR(TYPE_INT))
+#ifdef CONFIG_USBFS
+ /* USB ioctls */
+ IOCTL(USBDEVFS_CONTROL, IOC_RW,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_ctrltransfer)))
+ IOCTL(USBDEVFS_BULK, IOC_RW,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_bulktransfer)))
+ IOCTL(USBDEVFS_RESETEP, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_SETINTERFACE, IOC_W,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_setinterface)))
+ IOCTL(USBDEVFS_SETCONFIGURATION, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_GETDRIVER, IOC_R,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_getdriver)))
+ IOCTL(USBDEVFS_DISCSIGNAL, IOC_W,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_disconnectsignal)))
+ IOCTL(USBDEVFS_CLAIMINTERFACE, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_RELEASEINTERFACE, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_CONNECTINFO, IOC_R,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_connectinfo)))
+ IOCTL(USBDEVFS_IOCTL, IOC_RW, MK_PTR(MK_STRUCT(STRUCT_usbdevfs_ioctl)))
+ IOCTL(USBDEVFS_HUB_PORTINFO, IOC_R,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_hub_portinfo)))
+ IOCTL(USBDEVFS_RESET, 0, TYPE_NULL)
+ IOCTL(USBDEVFS_CLEAR_HALT, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_DISCONNECT, 0, TYPE_NULL)
+ IOCTL(USBDEVFS_CONNECT, 0, TYPE_NULL)
+ IOCTL(USBDEVFS_CLAIM_PORT, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_RELEASE_PORT, IOC_W, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_GET_CAPABILITIES, IOC_R, MK_PTR(TYPE_INT))
+ IOCTL(USBDEVFS_DISCONNECT_CLAIM, IOC_W,
+MK_PTR(MK_STRUCT(STRUCT_usbdevfs_disconnect_claim)))
+#ifdef USBDEVFS_DROP_PRIVILEGES
+ IOCTL(USBDEVFS_DROP_PRIVILEGES, IOC_W, MK_PTR(TYPE_INT))
+#endif
+#ifdef USBDEVFS_GET_SPEED
+ IOCTL(USBDEVFS_GET_SPEED, 0, TYPE_NULL)
+#endif
+#endif /* CONFIG_USBFS */
+
IOCTL(SIOCATMARK, IOC_R, MK_PTR(TYPE_INT))
IOCTL(SIOCGIFNAME, IOC_RW, MK_PTR(TYPE_INT))
IOCTL(SIOCGIFFLAGS, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ae3c0dfef7..2641260186 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -94,6 +94,9 @@
#include
#endif
#include
+#if defined(CONFIG_USBFS)
+#include
+#endif
#include
#include
#include
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 18d434d6dc..2daa5ebdcc 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -863,6 +863,30 @@ struct target_pollfd {
#define TARGET_FS_IOC_FIEMAP TARGET_IOWR('f',11,struct fiemap)
+/* usb ioctls */
+#define TARGET_USBDEVFS_CONTROL TARGET_IOWRU('U', 0)
+#define TARGET_USBDEVFS_BULK TARGET_IOWRU('U', 2)
+#define TARGET_USBDEVFS_RESETEP TARGET_IORU('U', 3)
+#define TARGET_USBDEVFS_SETINTERFACE TARGET_IORU('U', 4)
+#define TARGET_USBDEVFS_SETCONFIGURATION TARGET_IORU('U', 5)
+#define TARGET_USBDEVFS_GETDRIVER TARGET_IOWU('U', 8)
+#define TARGET_USBDEVFS_DISCSIGNAL TARGET_IORU('U', 14)
+#define TARGET_USBDEVFS_CLAIMINTERFACE TARGET_IORU('U', 15)
+#define TARGET_USBDEVFS_RELEASEINTERFACE TARGET_IORU('U', 16)
+#define TARGET_USBDEVFS_CONNECTINFO TARGET_IOWU('U', 17)
+#define TARGET_USBDEVFS_IOCTL TARGET_IOWRU('U', 18)
+#define TARGET_USBDEVFS_HUB_PORTINFO TARGET_IORU('U', 19)
+#define TARGET_USBDEVFS_RESET TARGET_IO('U', 20)
+#define TARGET_USBDEVFS_CLEAR_HALT TARGET_IORU('U', 21)
+#define TARGET_USBDEVFS_DISCONNECT TARGET_IO('U', 22)
+#define TARGET_USBDEVFS_CONNECT TARGET_IO('U', 23)
+#define TARGET_USBDEVFS_CLAIM_PORT TARGET_IORU('U', 24)
+#define TARGET_USBDEVFS_RELEASE_PORT TARGET_IORU('U', 25)
+#define TARGET_USBDEVFS_GET_CAPABILITIES TARGET_IORU('U', 26)
+#define TARGET_USBDEVFS_DISCONNECT_CLAIM TARGET_IORU('U', 27)
+#define TARGET_USBDEVFS_DROP_PRIVILEGES TARGET_IOWU('U', 30)
+#define TARGET_USBDEVFS_GET_SPEED TARGET_IO('U', 31)
+
/* cdrom commands */
#define TARGET_CDROMPAUSE 0x5301 /* Pause Audio Operation */
#define TARGET_CDROMRESUME 0x5302 /* Resume paused Audio Operation
*/
diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
index 24631b09be..6f64a8bdf7 100644
--- a/linux-user/syscall_types.h
+++ b/linux-user/syscall_types.h
@@ -266,3 +266,51 @@ STRUCT(blkpg_ioctl_arg,
TYPE_INT, /* flags */
TYPE_INT, /* datalen */
TYPE_PTRVOID) /* data */
+
[Qemu-devel] [PATCH v3 0/3] Linux usermode emulation user mode USB driver support.
This patch series enables programs running under QEMU Linux user mode emulation to implement user-space USB drivers via the USBFS ioctl()s. Support is limited to control, bulk, and possibly interrupt transfers. The series compiles for i386, ppc64, ppc64le, mips, mipsel, xtensa, and xtensaeb with an armv7l host and an x86_64 host. The i386-linux-user target is tested working with a USB scanner driver on an armv7l host. Additionally, a patched copy of strace was used to verify the conversion for reaping. Additionally, a MIPS binary of lsusb was run on armv7l host to check reaping and other functionality across endianness. Changes from v1: use check_include in configure move struct definitions to later patch where possible improve pointer cast to int compatibility remove unimplemented types for usb streams Changes from v2: calculate ioctl arg size at runtime organize urb metadata with struct hold lock_user memory from submit until reap supersedes patch series 'linux-user: usbfs improvements' Cortland Tölva (3): linux-user: Check for Linux USBFS in configure linux-user: Define ordinary usbfs ioctls. linux-user: Implement special usbfs ioctls. configure | 12 ++- linux-user/ioctls.h| 46 linux-user/syscall.c | 180 + linux-user/syscall_defs.h | 28 +++ linux-user/syscall_types.h | 68 + 5 files changed, 333 insertions(+), 1 deletion(-) -- 2.11.0
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On 10/08/2018 10:44 AM, Thomas Huth wrote:
On 2018-10-08 16:20, Tony Krowiak wrote:
On 09/27/2018 08:52 AM, Cornelia Huck wrote:
On Thu, 27 Sep 2018 14:29:01 +0200
Thomas Huth wrote:
On 2018-09-27 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Introduces the base object model for virtualizing AP devices.
Signed-off-by: Tony Krowiak
---
+typedef struct APBridge {
+SysBusDevice sysbus_dev;
+bool css_dev_path;
What is this css_dev_path variable good for? I don't see it used in any
of the other patches?
If you don't need it, I think you could get rid of this struct
completely?
Huh, now I remember complaining about it before. Looks like a
copy-and-paste from the css bridge; that variable is used for compat
handling there (and should be ditched here).
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct APBus {
+BusState parent_obj;
+} APBus;
+
+#define TYPE_AP_BUS "ap-bus"
+#define AP_BUS(obj) \
+ OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
struct APBus.
If there's nothing interesting to put in these inherited structures,
probably yes.
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h
b/include/hw/s390x/ap-device.h
new file mode 100644
index ..693df90cc041
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
or (at
+ * your option) any later version. See the COPYING file in the
top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE "ap-device"
+
+typedef struct APDevice {
+DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
Do you really need any of these definitions except AP_DEVICE_TYPE ?
Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
patch 5/6.
Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE().
We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
AP_DEVICE_CLASS(klass), but aren't those typically included in all
QOM definitions?
As long as you don't really need them, I'd simply remove them. They can
be added back when some code really needs them.
That is the plan
Thomas
Re: [Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
On Mon, 8 Oct 2018 17:32:04 +0200
David Hildenbrand wrote:
> linux-user should always enable AFP, otherwise our emulated binary
> might crash once it tries to make use of additional floating-point
> registers or instructions.
>
> Cc: Peter Maydell
> Cc: Alex Bennée
> Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
> exceptions")
> Reported-by: Alex Bennée
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8ed4823d6e..18ba7f85a5 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -145,6 +145,11 @@ static void s390_cpu_full_reset(CPUState *s)
> env->cregs[0] = CR0_RESET;
> env->cregs[14] = CR14_RESET;
>
> +#if defined(CONFIG_USER_ONLY)
> +/* user mode should always be allowed to use the full FPU */
> +env->cregs[0] |= CR0_AFP;
> +#endif
> +
> /* architectured initial value for Breaking-Event-Address register */
> env->gbea = 1;
>
In case this is applied directly:
Reviewed-by: Cornelia Huck
Tested-by: Cornelia Huck
Re: [Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
David Hildenbrand writes:
> linux-user should always enable AFP, otherwise our emulated binary
> might crash once it tries to make use of additional floating-point
> registers or instructions.
>
> Cc: Peter Maydell
> Cc: Alex Bennée
> Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
> exceptions")
> Reported-by: Alex Bennée
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8ed4823d6e..18ba7f85a5 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -145,6 +145,11 @@ static void s390_cpu_full_reset(CPUState *s)
> env->cregs[0] = CR0_RESET;
> env->cregs[14] = CR14_RESET;
>
> +#if defined(CONFIG_USER_ONLY)
> +/* user mode should always be allowed to use the full FPU */
> +env->cregs[0] |= CR0_AFP;
> +#endif
> +
Reviewed-by: Alex Bennée
Tested-by: Alex Bennée
> /* architectured initial value for Breaking-Event-Address register */
> env->gbea = 1;
--
Alex Bennée
Re: [Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
On Mon, 8 Oct 2018 17:32:04 +0200
David Hildenbrand wrote:
> linux-user should always enable AFP, otherwise our emulated binary
> might crash once it tries to make use of additional floating-point
> registers or instructions.
>
> Cc: Peter Maydell
> Cc: Alex Bennée
> Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
> exceptions")
> Reported-by: Alex Bennée
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8ed4823d6e..18ba7f85a5 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -145,6 +145,11 @@ static void s390_cpu_full_reset(CPUState *s)
> env->cregs[0] = CR0_RESET;
> env->cregs[14] = CR14_RESET;
>
> +#if defined(CONFIG_USER_ONLY)
> +/* user mode should always be allowed to use the full FPU */
> +env->cregs[0] |= CR0_AFP;
> +#endif
> +
> /* architectured initial value for Breaking-Event-Address register */
> env->gbea = 1;
>
I've reproduced the crash via make check-tcg, and this patch fixes it.
Thanks, applied.
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
On 8/10/2018 6:46 PM, Kevin Wolf wrote:
> Am 08.10.2018 um 17:25 hat Anton Nefedov geschrieben:
>>
>>
>> On 8/10/2018 6:03 PM, Kevin Wolf wrote:
>>> Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
>> Signed-off-by: Anton Nefedov
>> Reviewed-by: Alberto Garcia
>> ---
>> hw/ide/core.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 2c62efc..352429b 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>> TrimAIOCB *iocb = opaque;
>> IDEState *s = iocb->s;
>>
>> +if (iocb->i >= 0) {
>> +if (ret >= 0) {
>> +block_acct_done(blk_get_stats(s->blk), >acct);
>> +} else {
>> +block_acct_failed(blk_get_stats(s->blk), >acct);
>> +}
>> +}
>> +
>> if (ret >= 0) {
>> while (iocb->j < iocb->qiov->niov) {
>> int j = iocb->j;
>> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>> goto done;
>> }
>>
>> +block_acct_start(blk_get_stats(s->blk), >acct,
>> + count << BDRV_SECTOR_BITS,
>> BLOCK_ACCT_UNMAP);
>> +
>> /* Got an entry! Submit and exit. */
>> iocb->aiocb = blk_aio_pdiscard(s->blk,
>>sector <<
>> BDRV_SECTOR_BITS,
>> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
>> }
>>
>> if (ret == -EINVAL) {
>> +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
>
> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> also for reads and writes, and each of them could return -EINVAL.
>
Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
> Also, -EINVAL doesn't necessarily mean that the guest driver did
> something wrong, it could also be the result of a host problem.
> Therefore, it isn't right to call block_acct_invalid() here - especially
> since the request may already have been accounted for as either done or
> failed in ide_issue_trim_cb().
>
Couldn't be accounted done with such retcode;
and it seems I shouldnt do block_acct_failed() there anyway - or it's
accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
But if EINVAL (from further layers) should not be accounted as an
invalid op, then it should be accounted failed instead, the thing that
current code does not do.
(and which brings us back to possible double-accounting if we account
invalid in ide_issue_trim_cb() )
>>>
>>> Yes, commit caeadbc8ba4 was already wrong in assuming that there is
>>> only one possible source for -EINVAL.
>>>
> Instead, I think it would be better to immediately account for invalid
> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> know for sure that indeed !ide_sect_range_ok() is the cause for the
> -EINVAL return code.
>
So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
acct_failed there, and filter off TRIM commands in the common
accounting.
>>>
>>> blk_aio_discard() can fail with -EINVAL, too, so getting this error code
>>> from a TRIM command doesn't mean anything. It can still have multiple
>>> possible sources.
>>>
>>
>> I meant that common ide_dma_cb() should account EINVAL (along with other
>> errors) as failed, unless it's TRIM, which means it's already
>> accounted (either invalid or failed)
>
> Oh, you would already account for failure in ide_issue_trim_cb(), too,
> but only if it's EINVAL? That feels like it would complicate the code
> quite a bit.
>
No, no :) ide_issue_trim_cb does the proper accounting (failed/invalid)
for TRIM.
Then common path (ide_dma_cb()) does not account TRIM operations at all
regardless of the error code. No need to check for TRIM specifically if
we have BLOCK_ACCT_NONE.
> And actually, didn't commit caeadbc8ba4 break werror=stop for requests
> returning -EINVAL because we don't call ide_handle_rw_error() any more?
>
Yes.
Read/write ignore werror=stop for invalid range case (not for EINVAL).
I wonder if it's crucial to ignore it for TRIM too, otherwise we could
just remove this chunk
if (ret == -EINVAL) {
ide_dma_error(s);
return;
}
Re: [Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
On 10/8/18 8:32 AM, David Hildenbrand wrote:
> linux-user should always enable AFP, otherwise our emulated binary
> might crash once it tries to make use of additional floating-point
> registers or instructions.
>
> Cc: Peter Maydell
> Cc: Alex Bennée
> Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
> exceptions")
> Reported-by: Alex Bennée
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
Reviewed-by: Richard Henderson
r~
[Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying
POSTCOPY_NOTIFY_INBOUND_END handlers will remove userfault fds
from the postcopy_remote_fds array which could be still in
use by the fault thread. Let's stop the thread before
notification to avoid possible accessing wrong memory.
Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Ilya Maximets
---
migration/postcopy-ram.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 853d8b32ca..e5c02a32c5 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -533,6 +533,12 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState
*mis)
if (mis->have_fault_thread) {
Error *local_err = NULL;
+/* Let the fault thread quit */
+atomic_set(>fault_thread_quit, 1);
+postcopy_fault_thread_notify(mis);
+trace_postcopy_ram_incoming_cleanup_join();
+qemu_thread_join(>fault_thread);
+
if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_END, _err)) {
error_report_err(local_err);
return -1;
@@ -541,11 +547,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState
*mis)
if (qemu_ram_foreach_migratable_block(cleanup_range, mis)) {
return -1;
}
-/* Let the fault thread quit */
-atomic_set(>fault_thread_quit, 1);
-postcopy_fault_thread_notify(mis);
-trace_postcopy_ram_incoming_cleanup_join();
-qemu_thread_join(>fault_thread);
trace_postcopy_ram_incoming_cleanup_closeuf();
close(mis->userfault_fd);
--
2.17.1
[Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak
'fd' received from the vhost side is never freed.
Also, everything (including 'postcopy_listen' state) should be
cleaned up on vhost cleanup.
Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
Fixes: f82c11165ffa ("vhost+postcopy: Register shared ufd with postcopy")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Ilya Maximets
---
hw/virtio/vhost-user.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index c442daa562..e09bed0e4a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1280,6 +1280,7 @@ static int vhost_user_postcopy_end(struct vhost_dev *dev,
Error **errp)
return ret;
}
postcopy_unregister_shared_ufd(>postcopy_fd);
+close(u->postcopy_fd.fd);
u->postcopy_fd.handler = NULL;
trace_vhost_user_postcopy_end_exit();
@@ -1419,6 +1420,12 @@ static int vhost_user_backend_cleanup(struct vhost_dev
*dev)
postcopy_remove_notifier(>postcopy_notifier);
u->postcopy_notifier.notify = NULL;
}
+u->postcopy_listen = false;
+if (u->postcopy_fd.handler) {
+postcopy_unregister_shared_ufd(>postcopy_fd);
+close(u->postcopy_fd.fd);
+u->postcopy_fd.handler = NULL;
+}
if (u->slave_fd >= 0) {
qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
close(u->slave_fd);
--
2.17.1
[Qemu-devel] [RFC 0/2] vhost+postcopy fixes
Sending as RFC because it's not fully tested yet. Ilya Maximets (2): migration: Stop postcopy fault thread before notifying vhost-user: Fix userfaultfd leak hw/virtio/vhost-user.c | 7 +++ migration/postcopy-ram.c | 11 ++- 2 files changed, 13 insertions(+), 5 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
Am 08.10.2018 um 17:25 hat Anton Nefedov geschrieben:
>
>
> On 8/10/2018 6:03 PM, Kevin Wolf wrote:
> > Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
> >> On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> >>> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
> Signed-off-by: Anton Nefedov
> Reviewed-by: Alberto Garcia
> ---
> hw/ide/core.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 2c62efc..352429b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> TrimAIOCB *iocb = opaque;
> IDEState *s = iocb->s;
>
> +if (iocb->i >= 0) {
> +if (ret >= 0) {
> +block_acct_done(blk_get_stats(s->blk), >acct);
> +} else {
> +block_acct_failed(blk_get_stats(s->blk), >acct);
> +}
> +}
> +
> if (ret >= 0) {
> while (iocb->j < iocb->qiov->niov) {
> int j = iocb->j;
> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> goto done;
> }
>
> +block_acct_start(blk_get_stats(s->blk), >acct,
> + count << BDRV_SECTOR_BITS,
> BLOCK_ACCT_UNMAP);
> +
> /* Got an entry! Submit and exit. */
> iocb->aiocb = blk_aio_pdiscard(s->blk,
> sector <<
> BDRV_SECTOR_BITS,
> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
> }
>
> if (ret == -EINVAL) {
> +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
> >>>
> >>> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> >>> also for reads and writes, and each of them could return -EINVAL.
> >>>
> >>
> >> Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
> >>
> >>> Also, -EINVAL doesn't necessarily mean that the guest driver did
> >>> something wrong, it could also be the result of a host problem.
> >>> Therefore, it isn't right to call block_acct_invalid() here - especially
> >>> since the request may already have been accounted for as either done or
> >>> failed in ide_issue_trim_cb().
> >>>
> >>
> >> Couldn't be accounted done with such retcode;
> >> and it seems I shouldnt do block_acct_failed() there anyway - or it's
> >> accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
> >>
> >> But if EINVAL (from further layers) should not be accounted as an
> >> invalid op, then it should be accounted failed instead, the thing that
> >> current code does not do.
> >> (and which brings us back to possible double-accounting if we account
> >> invalid in ide_issue_trim_cb() )
> >
> > Yes, commit caeadbc8ba4 was already wrong in assuming that there is
> > only one possible source for -EINVAL.
> >
> >>> Instead, I think it would be better to immediately account for invalid
> >>> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> >>> know for sure that indeed !ide_sect_range_ok() is the cause for the
> >>> -EINVAL return code.
> >>>
> >> So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
> >> acct_failed there, and filter off TRIM commands in the common
> >> accounting.
> >
> > blk_aio_discard() can fail with -EINVAL, too, so getting this error code
> > from a TRIM command doesn't mean anything. It can still have multiple
> > possible sources.
> >
>
> I meant that common ide_dma_cb() should account EINVAL (along with other
> errors) as failed, unless it's TRIM, which means it's already
> accounted (either invalid or failed)
Oh, you would already account for failure in ide_issue_trim_cb(), too,
but only if it's EINVAL? That feels like it would complicate the code
quite a bit.
And actually, didn't commit caeadbc8ba4 break werror=stop for requests
returning -EINVAL because we don't call ide_handle_rw_error() any more?
> > Maybe we just need to remember somewhere whether we already accounted
> > for a request (maybe an additional field in BlockAcctCookie? Or change
> > the type to BLOCK_ACCT_ALREADY_ACCOUNTED?) and then make an additional
> > block_account_one_io() call a no-op for such requests.
>
> Maybe even resetting to BLOCK_ACCT_NONE == 0. It should also protect
> from accounting uninitialized cookie.
That sounds good to me.
Kevin
Re: [Qemu-devel] [PATCH v2 4/7] block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Reduce number of structures ignored in overlap check: when checking > active table ignore active tables, when checking inactive table ignore > inactive ones. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain
On 8 October 2018 at 10:12, Peter Maydell wrote: > I looked back at the backtrace/etc that I posted earlier in this > thread, and it looked to me like maybe a memory corruption issue. > So I tried running the test under valgrind on Linux, and: ...which goes away if I do a complete build from clean, so presumably is the result of a stale .o file? The OSX version I'm running doesn't support valgrind, but the C compiler does have the clang sanitizers. Here's a log from a build with -fsanitize=address -fsanitize=undefined of commit df51a005192ee40b: $ ./tests/test-bdrv-drain /bdrv-drain/nested: ==60415==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffee500e000; bottom 0x00010fa0d000; size: 0x7ffdd5601000 (140728183296000) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 OK /bdrv-drain/multiparent: OK /bdrv-drain/driver-cb/drain_all: OK /bdrv-drain/driver-cb/drain: OK /bdrv-drain/driver-cb/drain_subtree: OK /bdrv-drain/driver-cb/co/drain_all: OK /bdrv-drain/driver-cb/co/drain: OK /bdrv-drain/driver-cb/co/drain_subtree: OK /bdrv-drain/quiesce/drain_all: OK /bdrv-drain/quiesce/drain: OK /bdrv-drain/quiesce/drain_subtree: OK /bdrv-drain/quiesce/co/drain_all: OK /bdrv-drain/quiesce/co/drain: OK /bdrv-drain/quiesce/co/drain_subtree: OK /bdrv-drain/graph-change/drain_subtree: OK /bdrv-drain/graph-change/drain_all: OK /bdrv-drain/iothread/drain_all: = ==60415==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d10060 at pc 0x00010b329270 bp 0x736c9d10 sp 0x736c9d08 READ of size 8 at 0x60d10060 thread T3 #0 0x10b32926f in notifier_list_notify notify.c:39 #1 0x10b2b8622 in qemu_thread_atexit_run qemu-thread-posix.c:473 #2 0x7fff5a0e1162 in _pthread_tsd_cleanup (libsystem_pthread.dylib:x86_64+0x5162) #3 0x7fff5a0e0ee8 in _pthread_exit (libsystem_pthread.dylib:x86_64+0x4ee8) #4 0x7fff5a0df66b in _pthread_body (libsystem_pthread.dylib:x86_64+0x366b) #5 0x7fff5a0df50c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c) #6 0x7fff5a0debf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8) 0x60d10060 is located 48 bytes inside of 144-byte region [0x60d10030,0x60d100c0) freed by thread T3 here: #0 0x10bcc51bd in wrap_free (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x551bd) #1 0x7fff5a0e1162 in _pthread_tsd_cleanup (libsystem_pthread.dylib:x86_64+0x5162) #2 0x7fff5a0e0ee8 in _pthread_exit (libsystem_pthread.dylib:x86_64+0x4ee8) #3 0x7fff5a0df66b in _pthread_body (libsystem_pthread.dylib:x86_64+0x366b) #4 0x7fff5a0df50c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c) #5 0x7fff5a0debf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8) previously allocated by thread T3 here: #0 0x10bcc5003 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x55003) #1 0x7fff59dc9969 in tlv_allocate_and_initialize_for_key (libdyld.dylib:x86_64+0x3969) #2 0x7fff59dca0eb in tlv_get_addr (libdyld.dylib:x86_64+0x40eb) #3 0x10b3558d6 in rcu_register_thread rcu.c:301 #4 0x10b131cb7 in iothread_run iothread.c:42 #5 0x10b2b8eff in qemu_thread_start qemu-thread-posix.c:504 #6 0x7fff5a0df660 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3660) #7 0x7fff5a0df50c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c) #8 0x7fff5a0debf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8) Thread T3 created by T0 here: #0 0x10bcbd00d in wrap_pthread_create (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x4d00d) #1 0x10b2b8bb5 in qemu_thread_create qemu-thread-posix.c:534 #2 0x10b131720 in iothread_new iothread.c:75 #3 0x10ac04edc in test_iothread_common test-bdrv-drain.c:668 #4 0x10abff44e in test_iothread_drain_all test-bdrv-drain.c:768 #5 0x10ba45b2b in g_test_run_suite_internal (libglib-2.0.0.dylib:x86_64+0x4fb2b) #6 0x10ba45cec in g_test_run_suite_internal (libglib-2.0.0.dylib:x86_64+0x4fcec) #7 0x10ba45cec in g_test_run_suite_internal (libglib-2.0.0.dylib:x86_64+0x4fcec) #8 0x10ba450fb in g_test_run_suite (libglib-2.0.0.dylib:x86_64+0x4f0fb) #9 0x10ba4504e in g_test_run (libglib-2.0.0.dylib:x86_64+0x4f04e) #10 0x10abf4515 in main test-bdrv-drain.c:1606 #11 0x7fff59dc7014 in start (libdyld.dylib:x86_64+0x1014) SUMMARY: AddressSanitizer: heap-use-after-free notify.c:39 in notifier_list_notify Shadow bytes around the buggy address: 0x1c1a1fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c1a1fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c1a1fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c1a1fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c1a1ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x1c1a2000: fa fa fa fa fa fa fd fd fd fd fd fd[fd]fd fd fd 0x1c1a2010: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
Re: [Qemu-devel] [PATCH v2 3/7] block/qcow2-refcount: check_refcounts_l2: refactor compressed case
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> Separate offset and size of compressed cluster.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2-refcount.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 566c19fbfa..0ea01e3ee2 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1570,7 +1570,7 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
> BDRVQcow2State *s = bs->opaque;
> uint64_t *l2_table, l2_entry;
> uint64_t next_contiguous_offset = 0;
> -int i, l2_size, nb_csectors, ret;
> +int i, l2_size, ret;
>
> /* Read L2 table from disk */
> l2_size = s->l2_size * sizeof(uint64_t);
> @@ -1589,6 +1589,9 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
>
> switch (qcow2_get_cluster_type(l2_entry)) {
> case QCOW2_CLUSTER_COMPRESSED:
> +{
> +int64_t csize, coffset;
> +
> /* Compressed clusters don't have QCOW_OFLAG_COPIED */
> if (l2_entry & QCOW_OFLAG_COPIED) {
> fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": "
> @@ -1599,12 +1602,13 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
>
> /* Mark cluster as used */
> -nb_csectors = ((l2_entry >> s->csize_shift) &
> - s->csize_mask) + 1;
> -l2_entry &= s->cluster_offset_mask;
> +csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
> +BDRV_SECTOR_SIZE;
> +coffset = l2_entry & s->cluster_offset_mask &
> + ~(BDRV_SECTOR_SIZE - 1);
This should actually be 512 instead of BDRV_SECTOR_SIZE (or the former
may be a shift by 9 bits), because this is about qcow2 and not about the
block layer's sector size.
(Other places in the qcow2 driver handling compressed clusters use 512
or shift by 9, too.)
Max
> ret = qcow2_inc_refcounts_imrt(bs, res,
> refcount_table,
> refcount_table_size,
> - l2_entry & ~511, nb_csectors *
> 512);
> + coffset, csize);
> if (ret < 0) {
> goto fail;
> }
> @@ -1621,6 +1625,7 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
> res->bfi.fragmented_clusters++;
> }
> break;
> +}
>
> case QCOW2_CLUSTER_ZERO_ALLOC:
> case QCOW2_CLUSTER_NORMAL:
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
On 2018-10-08 17:32, David Hildenbrand wrote:
> linux-user should always enable AFP, otherwise our emulated binary
> might crash once it tries to make use of additional floating-point
> registers or instructions.
>
> Cc: Peter Maydell
> Cc: Alex Bennée
> Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
> exceptions")
> Reported-by: Alex Bennée
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8ed4823d6e..18ba7f85a5 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -145,6 +145,11 @@ static void s390_cpu_full_reset(CPUState *s)
> env->cregs[0] = CR0_RESET;
> env->cregs[14] = CR14_RESET;
>
> +#if defined(CONFIG_USER_ONLY)
> +/* user mode should always be allowed to use the full FPU */
> +env->cregs[0] |= CR0_AFP;
> +#endif
> +
> /* architectured initial value for Breaking-Event-Address register */
> env->gbea = 1;
>
>
Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH] migration: invalidate cache before source start
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(>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?
>
>>
>> Thanks,
>>
>> Dave
>>
>>> Dave
>>>
> Dave
>
>> ---
>>
>> migration/migration.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1e99ec9b7e..8f39e0dc02 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2806,7 +2806,14 @@ static
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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?
> +
> +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?
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;
>
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH] s390x/tcg: always enable AFP for linux-user
linux-user should always enable AFP, otherwise our emulated binary
might crash once it tries to make use of additional floating-point
registers or instructions.
Cc: Peter Maydell
Cc: Alex Bennée
Fixes: db0504154e ("s390x/tcg: check for AFP-register, BFP and DFP data
exceptions")
Reported-by: Alex Bennée
Signed-off-by: David Hildenbrand
---
target/s390x/cpu.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8ed4823d6e..18ba7f85a5 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -145,6 +145,11 @@ static void s390_cpu_full_reset(CPUState *s)
env->cregs[0] = CR0_RESET;
env->cregs[14] = CR14_RESET;
+#if defined(CONFIG_USER_ONLY)
+/* user mode should always be allowed to use the full FPU */
+env->cregs[0] |= CR0_AFP;
+#endif
+
/* architectured initial value for Breaking-Event-Address register */
env->gbea = 1;
--
2.17.1
Re: [Qemu-devel] [PATCH v2 1/7] block/qcow2-refcount: fix check_oflag_copied
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> Increase corruptions_fixed only after successful fix.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2-refcount.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3c539f02e5..615847eb09 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1816,7 +1816,7 @@ static int check_oflag_copied(BlockDriverState *bs,
> BdrvCheckResult *res,
> for (i = 0; i < s->l1_size; i++) {
> uint64_t l1_entry = s->l1_table[i];
> uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
> -bool l2_dirty = false;
> +int l2_fixed_entries = 0;
I understand that this new variable kind of supersedes the other one,
but I think for the sake of readability it would be better to keep l2_dirty.
Max
> if (!l2_offset) {
> continue;
> @@ -1878,8 +1878,7 @@ static int check_oflag_copied(BlockDriverState *bs,
> BdrvCheckResult *res,
> l2_table[j] = cpu_to_be64(refcount == 1
> ? l2_entry | QCOW_OFLAG_COPIED
> : l2_entry & ~QCOW_OFLAG_COPIED);
> -l2_dirty = true;
> -res->corruptions_fixed++;
> +l2_fixed_entries++;
> } else {
> res->corruptions++;
> }
> @@ -1887,7 +1886,7 @@ static int check_oflag_copied(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
> }
>
> -if (l2_dirty) {
> +if (l2_fixed_entries > 0) {
> ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
> l2_offset, s->cluster_size);
> if (ret < 0) {
> @@ -1905,6 +1904,7 @@ static int check_oflag_copied(BlockDriverState *bs,
> BdrvCheckResult *res,
> res->check_errors++;
> goto fail;
> }
> +res->corruptions_fixed += l2_fixed_entries;
> }
> }
>
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
On 8/10/2018 6:03 PM, Kevin Wolf wrote:
> Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
>> On 4/10/2018 6:33 PM, Kevin Wolf wrote:
>>> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
Signed-off-by: Anton Nefedov
Reviewed-by: Alberto Garcia
---
hw/ide/core.c | 12
1 file changed, 12 insertions(+)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 2c62efc..352429b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
TrimAIOCB *iocb = opaque;
IDEState *s = iocb->s;
+if (iocb->i >= 0) {
+if (ret >= 0) {
+block_acct_done(blk_get_stats(s->blk), >acct);
+} else {
+block_acct_failed(blk_get_stats(s->blk), >acct);
+}
+}
+
if (ret >= 0) {
while (iocb->j < iocb->qiov->niov) {
int j = iocb->j;
@@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
goto done;
}
+block_acct_start(blk_get_stats(s->blk), >acct,
+ count << BDRV_SECTOR_BITS,
BLOCK_ACCT_UNMAP);
+
/* Got an entry! Submit and exit. */
iocb->aiocb = blk_aio_pdiscard(s->blk,
sector <<
BDRV_SECTOR_BITS,
@@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
}
if (ret == -EINVAL) {
+block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
>>>
>>> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
>>> also for reads and writes, and each of them could return -EINVAL.
>>>
>>
>> Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
>>
>>> Also, -EINVAL doesn't necessarily mean that the guest driver did
>>> something wrong, it could also be the result of a host problem.
>>> Therefore, it isn't right to call block_acct_invalid() here - especially
>>> since the request may already have been accounted for as either done or
>>> failed in ide_issue_trim_cb().
>>>
>>
>> Couldn't be accounted done with such retcode;
>> and it seems I shouldnt do block_acct_failed() there anyway - or it's
>> accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
>>
>> But if EINVAL (from further layers) should not be accounted as an
>> invalid op, then it should be accounted failed instead, the thing that
>> current code does not do.
>> (and which brings us back to possible double-accounting if we account
>> invalid in ide_issue_trim_cb() )
>
> Yes, commit caeadbc8ba4 was already wrong in assuming that there is
> only one possible source for -EINVAL.
>
>>> Instead, I think it would be better to immediately account for invalid
>>> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
>>> know for sure that indeed !ide_sect_range_ok() is the cause for the
>>> -EINVAL return code.
>>>
>> So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
>> acct_failed there, and filter off TRIM commands in the common
>> accounting.
>
> blk_aio_discard() can fail with -EINVAL, too, so getting this error code
> from a TRIM command doesn't mean anything. It can still have multiple
> possible sources.
>
I meant that common ide_dma_cb() should account EINVAL (along with other
errors) as failed, unless it's TRIM, which means it's already
accounted (either invalid or failed)
> Maybe we just need to remember somewhere whether we already accounted
> for a request (maybe an additional field in BlockAcctCookie? Or change
> the type to BLOCK_ACCT_ALREADY_ACCOUNTED?) and then make an additional
> block_account_one_io() call a no-op for such requests.
> > Kevin
>
Maybe even resetting to BLOCK_ACCT_NONE == 0. It should also protect
from accounting uninitialized cookie.
/Anton
[Qemu-devel] [PATCH] .travis.yml: split MacOSX builds and reduce target list
We have reached the point where the MacOSX build was regularly timing out. So as before I've reduced the target list to "major" architectures to try and bring the build time down. I've added an additional MacOSX build with the latest XCode with a minimal list of "most likely" targets on MacOS. Signed-off-by: Alex Bennée --- .travis.yml | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 95be6ec59f..13a09facd3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -107,8 +107,14 @@ matrix: - env: CONFIG="--disable-tcg" TEST_CMD="" compiler: gcc -- env: CONFIG="" +# MacOSX builds +- env: CONFIG="--target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu" os: osx + osx_image: xcode9.4 + compiler: clang +- env: CONFIG="--target-list=i386-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,x86_64-softmmu" + os: osx + osx_image: xcode10 compiler: clang # Python builds - env: CONFIG="--target-list=x86_64-softmmu" -- 2.17.1
Re: [Qemu-devel] [PATCH 0/2] linux-user: usbfs improvements
On Sun, Oct 7, 2018 at 11:59 PM Laurent Vivier wrote: > > Le 08/10/2018 à 06:27, Cortland Tölva a écrit : > > From: Cortland Setlow Tölva > > > > This patch series enables programs running under QEMU Linux user mode > > emulation to implement user-space USB drivers via the USBFS ioctl()s. > > Support is limited to control, bulk, and possibly interrupt transfers. > > > > Usbfs ioctl codes were incorrect whenever host and target disagreed on > > struct size. The submit, discard, and reap usbfs ioctls require special > > memory buffer handling and the second commit implements this but not for > > USB3 streams or isochronous transfers. > > > > Cortland Tölva (2): > > linux-user: Use calculated sizes for usbfs ioctls. > > linux-user: Implement special usbfs ioctls. > > > > linux-user/ioctls.h| 8 ++ > > linux-user/syscall.c | 177 + > > linux-user/syscall_defs.h | 42 - > > linux-user/syscall_types.h | 20 + > > 4 files changed, 227 insertions(+), 20 deletions(-) > > > > As I didn't push the previous series, could you merge the both in one on > top of master? Yes, I will do this as a v2 of this patch series. The version here is based on your linux user tree as of last night. > > I think it would be clearer. > > Thanks, > Laurent
Re: [Qemu-devel] [PULL 00/15] s390x updates
On 08/10/2018 17:02, Alex Bennée wrote: > > Peter Maydell writes: > >> On 4 October 2018 at 16:28, Cornelia Huck wrote: >>> The following changes since commit dafd95053611aa14dda40266857608d12ddce658: >>> >>> Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into >>> staging (2018-10-02 18:27:18 +0100) >>> >>> are available in the Git repository at: >>> >>> git://github.com/cohuck/qemu tags/s390x-20181004 >>> >>> for you to fetch changes up to b576d582ea2b03f4eada186fff59308d22b40a6a: >>> >>> hw/s390x/s390-pci-bus: Convert sysbus init function to realize function >>> (2018-10-04 12:10:40 +0200) >>> >>> >>> Various s390x updates: >>> - fix several struct definitions so that sparc hosts do not trip over >>> unaligned accesses >>> - fence enabling huge pages for pre-3.1 machines >>> - sysbus init -> realize conversion >>> - fixes and improvements in tcg (instruction flags and AFP registers) >> >> Applied, thanks. > > > This pull has broken s390x-linux-user tcg tests: > > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/multiarch/Makefile.target:30: > recipe for target 'run-test-mmap' failed > make[2]: *** [run-test-mmap] Error 132 > make[2]: *** Waiting for unfinished jobs > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-linux-test' failed > make[2]: *** [run-linux-test] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-hello-s390x' failed > make[2]: *** [run-hello-s390x] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-ipm' failed > make[2]: *** [run-ipm] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-exrl-trtr' failed > make[2]: *** [run-exrl-trtr] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-testthread' failed > make[2]: *** [run-testthread] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-exrl-trt' failed > make[2]: *** [run-exrl-trt] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-sha1' failed > make[2]: *** [run-sha1] Error 132 > timeout: the monitored command dumped core > Illegal instruction > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target > 'run-csst' failed > make[2]: *** [run-csst] Error 132 > /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile.include:71: recipe for > target 'run-guest-tests' failed > make[1]: *** [run-guest-tests] Error 2 > /home/alex/lsrc/qemu/qemu.git/tests/Makefile.include:968: recipe for target > 'run-tcg-tests-s390x-linux-user' failed > make: *** [run-tcg-tests-s390x-linux-user] Error 2 > > which are now showing up on Travis (admitidly hidden by the usual Travis > noise). I have bisected the breakage: > > git bisect start > git bisect bad b576d582ea2b03f4eada186fff59308d22b40a6a > b576d582ea bad hw/s390x/s390-pci-bus: Convert sysbus init function to > realize function > git bisect good dafd95053611aa14dda40266857608d12ddce658 > dafd950536 good Merge remote-tracking branch > 'remotes/bonzini/tags/for-upstream' into staging > git bisect good bbf6ea3bd961970785a583117ac1c0b84a8ee736 > bbf6ea3bd9 good s390x/tcg: factor out and fix DATA exception injection > git bisect bad db0504154ef1feedd135f25f8ca6e9abb5d00bbb > db0504154e bad s390x/tcg: check for AFP-register, BFP and DFP data > exceptions > git bisect good 82d179336dd696d04495963ca007fa961496d924 > 82d179336d good s390x/tcg: support flags for instructions > git bisect good ca5f31a0f27d265f2220ad43913f70280e8753a2 > ca5f31a0f2 good s390x/tcg: add instruction flags for floating point > instructions > > Which is points at: > > Bisect Rest (1) > db0504154e * bad @ s390x/tcg: check for AFP-register, BFP and DFP data > exceptions > Thanks for reporting. We have to enable AFP as default in linux-user (in control register 0). Will send a patch later this day. > -- > Alex Bennée > -- Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH 0/7] qcow2 check improvements
ping Hi, what about this? 17.08.2018 15:22, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > v2: > 02, 06: check bdrv_getlength error return code > > v1: > > We've faced the following problem: after host fs corruption, vm images > becomes invalid. And which is interesting, starting qemu-img check on > them led to allocating of the whole RAM and then killing qemu-img by > OOM Killer. > > This was due to corrupted l2 entries, which referenced clusters far-far > beyond the end of the qcow2 file. > 02 is a generic fix for the bug, 01 is unrelated improvement, 03-07 are > additional info and fixing for such corrupted table entries. > > Questions on 02, 06 and 07: > 1. Should restrictions be more or less strict? > 2. Are there valid cases, when such entries should not be considered as > corrupted? > > Vladimir Sementsov-Ogievskiy (7): >block/qcow2-refcount: fix check_oflag_copied >block/qcow2-refcount: avoid eating RAM >block/qcow2-refcount: check_refcounts_l2: refactor compressed case >block/qcow2-refcount: check_refcounts_l2: reduce ignored overlaps >block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero >block/qcow2-refcount: fix out-of-file L1 entries to be zero >block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero > > block/qcow2-refcount.c | 270 > +++-- > 1 file changed, 219 insertions(+), 51 deletions(-) > -- Best regards, Vladimir
[Qemu-devel] [PATCH v3 11/12] hw/mips: Add Data Scratch Pad RAM
From: Yongbok Kim
The optional Data Scratch Pad RAM (DSPRAM) block provides a
general scratch pad RAM used for temporary storage of data. The
DSPRAM provides a connection to on-chip memory or memory-mapped
registers, which are accessed in parallel with the L1 data
cache to minimize access latency.
Signed-off-by: Yongbok Kim
Signed-off-by: Aleksandar Markovic
---
default-configs/mips-softmmu-common.mak | 1 +
hw/mips/cps.c | 3 ++-
hw/mips/mips_malta.c| 31 +++
hw/misc/Makefile.objs | 1 +
include/hw/mips/cps.h | 2 ++
target/mips/cpu.h | 5 +
target/mips/internal.h | 1 +
target/mips/op_helper.c | 10 ++
target/mips/translate.c | 7 +++
9 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/default-configs/mips-softmmu-common.mak
b/default-configs/mips-softmmu-common.mak
index fae2347..45f2cbf 100644
--- a/default-configs/mips-softmmu-common.mak
+++ b/default-configs/mips-softmmu-common.mak
@@ -36,3 +36,4 @@ CONFIG_EMPTY_SLOT=y
CONFIG_MIPS_CPS=y
CONFIG_MIPS_ITU=y
CONFIG_I2C=y
+CONFIG_MIPS_DSPRAM=y
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index dd68795..93d3bea 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -102,7 +102,8 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
object_property_set_bool(OBJECT(>itu), saar_present, "saar-present",
);
if (saar_present) {
-qdev_prop_set_ptr(DEVICE(>itu), "saar", (void *)
>CP0_SAAR);
+qdev_prop_set_ptr(DEVICE(>itu), "saar",
+ (void *) >CP0_SAAR[0]);
}
object_property_set_bool(OBJECT(>itu), true, "realized", );
if (err != NULL) {
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 29b90ba..1b1bbd8 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1169,6 +1169,36 @@ static void create_cps(MaltaState *s, const char
*cpu_type,
*cbus_irq = NULL;
}
+static void create_dspram(void)
+{
+MIPSCPU *cpu = MIPS_CPU(first_cpu);
+CPUMIPSState *env = >env;
+bool dspram_present = (bool) env->dspramp;
+Error *err = NULL;
+
+env->dspram = g_new0(MIPSDSPRAMState, 1);
+
+/* DSPRAM */
+if (dspram_present) {
+if (!(bool) env->saarp) {
+error_report("%s: DSPRAM requires SAAR registers", __func__);
+exit(1);
+}
+object_initialize(env->dspram, sizeof(MIPSDSPRAMState),
+ TYPE_MIPS_DSPRAM);
+qdev_set_parent_bus(DEVICE(env->dspram), sysbus_get_default());
+qdev_prop_set_ptr(DEVICE(env->dspram), "saar",
+ (void *) >CP0_SAAR[1]);
+object_property_set_bool(OBJECT(env->dspram), true, "realized", );
+if (err != NULL) {
+error_report("%s: DSPRAM initialisation failed", __func__);
+exit(1);
+}
+memory_region_add_subregion(get_system_memory(), 0,
+sysbus_mmio_get_region(SYS_BUS_DEVICE(env->dspram), 0));
+}
+}
+
static void mips_create_cpu(MaltaState *s, const char *cpu_type,
qemu_irq *cbus_irq, qemu_irq *i8259_irq)
{
@@ -1177,6 +1207,7 @@ static void mips_create_cpu(MaltaState *s, const char
*cpu_type,
} else {
create_cpu_without_cps(cpu_type, cbus_irq, i8259_irq);
}
+create_dspram();
}
static
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 6d50b03..37a1b41 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -60,6 +60,7 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
obj-$(CONFIG_MIPS_CPS) += mips_cmgcr.o
obj-$(CONFIG_MIPS_CPS) += mips_cpc.o
obj-$(CONFIG_MIPS_ITU) += mips_itu.o
+obj-$(CONFIG_MIPS_DSPRAM) += mips_dspram.o
obj-$(CONFIG_MPS2_FPGAIO) += mps2-fpgaio.o
obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
diff --git a/include/hw/mips/cps.h b/include/hw/mips/cps.h
index aab1af9..a637036 100644
--- a/include/hw/mips/cps.h
+++ b/include/hw/mips/cps.h
@@ -25,6 +25,7 @@
#include "hw/intc/mips_gic.h"
#include "hw/misc/mips_cpc.h"
#include "hw/misc/mips_itu.h"
+#include "hw/misc/mips_dspram.h"
#define TYPE_MIPS_CPS "mips-cps"
#define MIPS_CPS(obj) OBJECT_CHECK(MIPSCPSState, (obj), TYPE_MIPS_CPS)
@@ -41,6 +42,7 @@ typedef struct MIPSCPSState {
MIPSGICState gic;
MIPSCPCState cpc;
MIPSITUState itu;
+MIPSDSPRAMState dspram;
} MIPSCPSState;
qemu_irq get_cps_irq(MIPSCPSState *cps, int pin_number);
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 7bc45d5..9e2eb19 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -625,6 +625,7 @@ struct CPUMIPSState {
uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits in CP0_TCStatus */
int insn_flags; /* Supported instruction set */
int saarp;
+int dspramp;
/* Fields up to this point are cleared
[Qemu-devel] [PATCH v3 07/12] target/mips: Implement emulation of nanoMIPS EVA instructions
From: Dimitrije Nikolic
Implement emulation of nanoMIPS EVA instructions. They are all
part of P.LS.E0 instruction pool, or one of its subpools.
Signed-off-by: Dimitrije Nikolic
Signed-off-by: Aleksandar Markovic
---
target/mips/translate.c | 79 +
1 file changed, 79 insertions(+)
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 7b1a6b1..d872fa7 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1979,6 +1979,17 @@ static inline void check_nms(DisasContext *ctx)
}
}
+/*
+ * This code generates a "reserved instruction" exception if the
+ * Config5 EVA bit is NOT set.
+ */
+static inline void check_eva(DisasContext *ctx)
+{
+if (!unlikely(ctx->CP0_Config5 & (1 << CP0C5_EVA))) {
+generate_exception_end(ctx, EXCP_RI);
+}
+}
+
/* Define small wrappers for gen_load_fpr* so that we have a uniform
calling interface for 32 and 64-bit FPRs. No sense in changing
@@ -20029,6 +20040,74 @@ static int decode_nanomips_32_48_opc(CPUMIPSState
*env, DisasContext *ctx)
break;
}
break;
+case NM_P_LS_E0:
+check_eva(ctx);
+switch (extract32(ctx->opcode, 11, 4)) {
+case NM_LBE:
+gen_ld(ctx, OPC_LBE, rt, rs, s);
+break;
+case NM_SBE:
+gen_st(ctx, OPC_SBE, rt, rs, s);
+break;
+case NM_LBUE:
+gen_ld(ctx, OPC_LBUE, rt, rs, s);
+break;
+case NM_P_PREFE:
+if (rt == 31) {
+/* SYNCIE */
+/* Break the TB to be able to sync copied instructions
+ immediately */
+ctx->base.is_jmp = DISAS_STOP;
+} else {
+/* PREF */
+/* Treat as NOP. */
+}
+break;
+case NM_LHE:
+gen_ld(ctx, OPC_LHE, rt, rs, s);
+break;
+case NM_SHE:
+gen_st(ctx, OPC_SHE, rt, rs, s);
+break;
+case NM_LHUE:
+gen_ld(ctx, OPC_LHUE, rt, rs, s);
+break;
+case NM_CACHEE:
+/* Treat as no-op */
+if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
+gen_cache_operation(ctx, rt, rs, s);
+}
+break;
+case NM_LWE:
+gen_ld(ctx, OPC_LWE, rt, rs, s);
+break;
+case NM_SWE:
+gen_st(ctx, OPC_SWE, rt, rs, s);
+break;
+case NM_P_LLE:
+switch (extract32(ctx->opcode, 2, 2)) {
+case NM_LL:
+gen_ld(ctx, OPC_LLE, rt, rs, s);
+break;
+case NM_LLWP:
+default:
+generate_exception_end(ctx, EXCP_RI);
+break;
+}
+break;
+case NM_P_SCE:
+switch (extract32(ctx->opcode, 2, 2)) {
+case NM_SC:
+gen_st_cond(ctx, OPC_SCE, rt, rs, s);
+break;
+case NM_SCWP:
+default:
+generate_exception_end(ctx, EXCP_RI);
+break;
+}
+break;
+}
+break;
case NM_P_LS_WM:
case NM_P_LS_UAWM:
check_nms(ctx);
--
2.7.4
[Qemu-devel] [PATCH v3 10/12] hw/mips: Update ITU to utilise SAARI/SAAR registers
From: Yongbok Kim
Update the ITU to utilise SAARI/SAAR registers and add new ITU
Control Register (ICR0).
Signed-off-by: Yongbok Kim
Signed-off-by: Aleksandar Markovic
---
hw/mips/cps.c | 8 ++
hw/misc/mips_itu.c | 72 +-
include/hw/misc/mips_itu.h | 7 +
target/mips/cpu.h | 5
target/mips/op_helper.c| 14 +
5 files changed, 99 insertions(+), 7 deletions(-)
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 4285d19..dd68795 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -69,6 +69,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
Error *err = NULL;
target_ulong gcr_base;
bool itu_present = false;
+bool saar_present = false;
for (i = 0; i < s->num_vp; i++) {
cpu = MIPS_CPU(cpu_create(s->cpu_type));
@@ -82,12 +83,14 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
itu_present = true;
/* Attach ITC Tag to the VP */
env->itc_tag = mips_itu_get_tag_region(>itu);
+env->itu = >itu;
}
qemu_register_reset(main_cpu_reset, cpu);
}
cpu = MIPS_CPU(first_cpu);
env = >env;
+saar_present = (bool) env->saarp;
/* Inter-Thread Communication Unit */
if (itu_present) {
@@ -96,6 +99,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
object_property_set_int(OBJECT(>itu), 16, "num-fifo", );
object_property_set_int(OBJECT(>itu), 16, "num-semaphores", );
+object_property_set_bool(OBJECT(>itu), saar_present, "saar-present",
+ );
+if (saar_present) {
+qdev_prop_set_ptr(DEVICE(>itu), "saar", (void *)
>CP0_SAAR);
+}
object_property_set_bool(OBJECT(>itu), true, "realized", );
if (err != NULL) {
error_propagate(errp, err);
diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
index 43bbec4..746e0c2 100644
--- a/hw/misc/mips_itu.c
+++ b/hw/misc/mips_itu.c
@@ -55,9 +55,17 @@ typedef enum ITCView {
ITCVIEW_EF_SYNC = 2,
ITCVIEW_EF_TRY = 3,
ITCVIEW_PV_SYNC = 4,
-ITCVIEW_PV_TRY = 5
+ITCVIEW_PV_TRY = 5,
+ITCVIEW_PV_ICR = 15
} ITCView;
+#define ITC_ICR0_CELL_NUM 16
+#define ITC_ICR0_BLK_GRAIN 8
+#define ITC_ICR0_BLK_GRAIN_MASK 0x7
+#define ITC_ICR0_ERR_AXI2
+#define ITC_ICR0_ERR_PARITY 1
+#define ITC_ICR0_ERR_EXEC 0
+
MemoryRegion *mips_itu_get_tag_region(MIPSITUState *itu)
{
return >tag_io;
@@ -76,7 +84,7 @@ static uint64_t itc_tag_read(void *opaque, hwaddr addr,
unsigned size)
return tag->ITCAddressMap[index];
}
-static void itc_reconfigure(MIPSITUState *tag)
+void itc_reconfigure(MIPSITUState *tag)
{
uint64_t *am = >ITCAddressMap[0];
MemoryRegion *mr = >storage_io;
@@ -84,6 +92,12 @@ static void itc_reconfigure(MIPSITUState *tag)
uint64_t size = (1 * KiB) + (am[1] & ITC_AM1_ADDR_MASK_MASK);
bool is_enabled = (am[0] & ITC_AM0_EN_MASK) != 0;
+if (tag->saar_present) {
+address = ((*(uint64_t *) tag->saar) & 0xE000ULL) << 4;
+size = 1 << ((*(uint64_t *) tag->saar >> 1) & 0x1f);
+is_enabled = *(uint64_t *) tag->saar & 1;
+}
+
memory_region_transaction_begin();
if (!(size & (size - 1))) {
memory_region_set_size(mr, size);
@@ -142,7 +156,12 @@ static inline ITCView get_itc_view(hwaddr addr)
static inline int get_cell_stride_shift(const MIPSITUState *s)
{
/* Minimum interval (for EntryGain = 0) is 128 B */
-return 7 + (s->ITCAddressMap[1] & ITC_AM1_ENTRY_GRAIN_MASK);
+if (s->saar_present) {
+return 7 + ((s->icr0 >> ITC_ICR0_BLK_GRAIN) &
+ITC_ICR0_BLK_GRAIN_MASK);
+} else {
+return 7 + (s->ITCAddressMap[1] & ITC_AM1_ENTRY_GRAIN_MASK);
+}
}
static inline ITCStorageCell *get_cell(MIPSITUState *s,
@@ -356,6 +375,12 @@ static void view_pv_try_write(ITCStorageCell *c)
view_pv_common_write(c);
}
+static void raise_exception(int excp)
+{
+current_cpu->exception_index = excp;
+cpu_loop_exit(current_cpu);
+}
+
static uint64_t itc_storage_read(void *opaque, hwaddr addr, unsigned size)
{
MIPSITUState *s = (MIPSITUState *)opaque;
@@ -363,6 +388,14 @@ static uint64_t itc_storage_read(void *opaque, hwaddr
addr, unsigned size)
ITCView view = get_itc_view(addr);
uint64_t ret = -1;
+switch (size) {
+case 1:
+case 2:
+s->icr0 |= 1 << ITC_ICR0_ERR_AXI;
+raise_exception(EXCP_DBE);
+return 0;
+}
+
switch (view) {
case ITCVIEW_BYPASS:
ret = view_bypass_read(cell);
@@ -382,6 +415,9 @@ static uint64_t itc_storage_read(void *opaque, hwaddr addr,
unsigned size)
case ITCVIEW_PV_TRY:
ret = view_pv_try_read(cell);
break;
+case ITCVIEW_PV_ICR:
+ret = s->icr0;
+break;
default:
qemu_log_mask(LOG_GUEST_ERROR,
[Qemu-devel] [PATCH v3 02/12] elf: Add MIPS_ABI_FP_XXX constants
From: Stefan Markovic Add MIPS_ABI_FP_XXX constants to elf.h. The source of information is kernel header arch/mips/include/asm/elf.h. Signed-off-by: Stefan Markovic Signed-off-by: Aleksandar Markovic --- include/elf.h | 8 1 file changed, 8 insertions(+) diff --git a/include/elf.h b/include/elf.h index decf210..eb5958d 100644 --- a/include/elf.h +++ b/include/elf.h @@ -87,6 +87,14 @@ typedef int64_t Elf64_Sxword; #define EF_MIPS_MACH_LS3A 0x00a2 /* ST Microelectronics Loongson 3A */ #define EF_MIPS_MACH 0x00ff /* EF_MIPS_MACH_xxx selection mask */ +#define MIPS_ABI_FP_ANY 0x0 /* FP ABI doesn't matter */ +#define MIPS_ABI_FP_DOUBLE0x1 /* -mdouble-float */ +#define MIPS_ABI_FP_SINGLE0x2 /* -msingle-float */ +#define MIPS_ABI_FP_SOFT 0x3 /* -msoft-float*/ +#define MIPS_ABI_FP_OLD_640x4 /* -mips32r2 -mfp64*/ +#define MIPS_ABI_FP_XX0x5 /* -mfpxx */ +#define MIPS_ABI_FP_640x6 /* -mips32r2 -mfp64*/ +#define MIPS_ABI_FP_64A 0x7 /* -mips32r2 -mfp64 -mno-odd-spreg */ /* These constants define the different elf file types */ #define ET_NONE 0 -- 2.7.4
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
> On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> > Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
> >> Signed-off-by: Anton Nefedov
> >> Reviewed-by: Alberto Garcia
> >> ---
> >> hw/ide/core.c | 12
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >> index 2c62efc..352429b 100644
> >> --- a/hw/ide/core.c
> >> +++ b/hw/ide/core.c
> >> @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> >> TrimAIOCB *iocb = opaque;
> >> IDEState *s = iocb->s;
> >>
> >> +if (iocb->i >= 0) {
> >> +if (ret >= 0) {
> >> +block_acct_done(blk_get_stats(s->blk), >acct);
> >> +} else {
> >> +block_acct_failed(blk_get_stats(s->blk), >acct);
> >> +}
> >> +}
> >> +
> >> if (ret >= 0) {
> >> while (iocb->j < iocb->qiov->niov) {
> >> int j = iocb->j;
> >> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> >> goto done;
> >> }
> >>
> >> +block_acct_start(blk_get_stats(s->blk), >acct,
> >> + count << BDRV_SECTOR_BITS,
> >> BLOCK_ACCT_UNMAP);
> >> +
> >> /* Got an entry! Submit and exit. */
> >> iocb->aiocb = blk_aio_pdiscard(s->blk,
> >> sector <<
> >> BDRV_SECTOR_BITS,
> >> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
> >> }
> >>
> >> if (ret == -EINVAL) {
> >> +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
> >
> > This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> > also for reads and writes, and each of them could return -EINVAL.
> >
>
> Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
>
> > Also, -EINVAL doesn't necessarily mean that the guest driver did
> > something wrong, it could also be the result of a host problem.
> > Therefore, it isn't right to call block_acct_invalid() here - especially
> > since the request may already have been accounted for as either done or
> > failed in ide_issue_trim_cb().
> >
>
> Couldn't be accounted done with such retcode;
> and it seems I shouldnt do block_acct_failed() there anyway - or it's
> accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
>
> But if EINVAL (from further layers) should not be accounted as an
> invalid op, then it should be accounted failed instead, the thing that
> current code does not do.
> (and which brings us back to possible double-accounting if we account
> invalid in ide_issue_trim_cb() )
Yes, commit caeadbc8ba4 was already wrong in assuming that there is
only one possible source for -EINVAL.
> > Instead, I think it would be better to immediately account for invalid
> > requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> > know for sure that indeed !ide_sect_range_ok() is the cause for the
> > -EINVAL return code.
> >
> So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
> acct_failed there, and filter off TRIM commands in the common
> accounting.
blk_aio_discard() can fail with -EINVAL, too, so getting this error code
from a TRIM command doesn't mean anything. It can still have multiple
possible sources.
Maybe we just need to remember somewhere whether we already accounted
for a request (maybe an additional field in BlockAcctCookie? Or change
the type to BLOCK_ACCT_ALREADY_ACCOUNTED?) and then make an additional
block_account_one_io() call a no-op for such requests.
Kevin
[Qemu-devel] [PATCH v3 09/12] target/mips: Implement MemoryMapID, SAARI, and SAAR registers
From: Yongbok Kim
SAARI (Special Address Access Register Index) provides an index
into the SAAR register to indicate whether the ITU or other block
is being accessed. SAAR (Special Address Access Register) stores
the base address where the ITU will be located, as well as the
block size.
Signed-off-by: Yongbok Kim
Signed-off-by: Aleksandar Markovic
---
target/mips/cpu.h | 8 ++
target/mips/helper.h| 6 +
target/mips/internal.h | 2 ++
target/mips/machine.c | 6 +
target/mips/op_helper.c | 51 +
target/mips/translate.c | 68 ++---
6 files changed, 137 insertions(+), 4 deletions(-)
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index e8ac057..87c0a93 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -252,6 +252,7 @@ struct CPUMIPSState {
#define CP0GN_VPId 0
target_ulong CP0_Context;
target_ulong CP0_KScratch[MIPS_KSCRATCH_NUM];
+int32_t CP0_MemoryMapID;
int32_t CP0_PageMask;
int32_t CP0_PageGrain_rw_bitmask;
int32_t CP0_PageGrain;
@@ -325,6 +326,12 @@ struct CPUMIPSState {
uint32_t CP0_BadInstrP;
uint32_t CP0_BadInstrX;
int32_t CP0_Count;
+#define CP0SAARI_IDX0
+uint32_t CP0_SAARI;
+#define CP0SAAR_BASE12
+#define CP0SAAR_SIZE1
+#define CP0SAAR_EN 0
+uint64_t CP0_SAAR[2];
target_ulong CP0_EntryHi;
#define CP0EnHi_EHINV 10
target_ulong CP0_EntryHi_ASID_mask;
@@ -616,6 +623,7 @@ struct CPUMIPSState {
uint32_t CP0_Status_rw_bitmask; /* Read/write bits in CP0_Status */
uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits in CP0_TCStatus */
int insn_flags; /* Supported instruction set */
+int saarp;
/* Fields up to this point are cleared by a CPU reset */
struct {} end_reset_fields;
diff --git a/target/mips/helper.h b/target/mips/helper.h
index fe2607d..4725c34 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -65,6 +65,8 @@ DEF_HELPER_1(mftc0_tcschedule, tl, env)
DEF_HELPER_1(mfc0_tcschefback, tl, env)
DEF_HELPER_1(mftc0_tcschefback, tl, env)
DEF_HELPER_1(mfc0_count, tl, env)
+DEF_HELPER_1(mfc0_saar, tl, env)
+DEF_HELPER_1(mfhc0_saar, tl, env)
DEF_HELPER_1(mftc0_entryhi, tl, env)
DEF_HELPER_1(mftc0_status, tl, env)
DEF_HELPER_1(mftc0_cause, tl, env)
@@ -89,6 +91,7 @@ DEF_HELPER_1(dmfc0_lladdr, tl, env)
DEF_HELPER_1(dmfc0_maar, tl, env)
DEF_HELPER_2(dmfc0_watchlo, tl, env, i32)
DEF_HELPER_2(dmfc0_watchhi, tl, env, i32)
+DEF_HELPER_1(dmfc0_saar, tl, env)
#endif /* TARGET_MIPS64 */
DEF_HELPER_2(mtc0_index, void, env, tl)
@@ -130,6 +133,9 @@ DEF_HELPER_2(mtc0_srsconf3, void, env, tl)
DEF_HELPER_2(mtc0_srsconf4, void, env, tl)
DEF_HELPER_2(mtc0_hwrena, void, env, tl)
DEF_HELPER_2(mtc0_count, void, env, tl)
+DEF_HELPER_2(mtc0_saari, void, env, tl)
+DEF_HELPER_2(mtc0_saar, void, env, tl)
+DEF_HELPER_2(mthc0_saar, void, env, tl)
DEF_HELPER_2(mtc0_entryhi, void, env, tl)
DEF_HELPER_2(mttc0_entryhi, void, env, tl)
DEF_HELPER_2(mtc0_compare, void, env, tl)
diff --git a/target/mips/internal.h b/target/mips/internal.h
index 3c5867e..c0a1144 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -61,6 +61,7 @@ struct mips_def_t {
target_ulong CP0_EBaseWG_rw_bitmask;
int insn_flags;
enum mips_mmu_types mmu_type;
+int32_t SAARP;
};
extern const struct mips_def_t mips_defs[];
@@ -91,6 +92,7 @@ struct r4k_tlb_t {
target_ulong VPN;
uint32_t PageMask;
uint16_t ASID;
+uint32_t MMID;
unsigned int G:1;
unsigned int C0:3;
unsigned int C1:3;
diff --git a/target/mips/machine.c b/target/mips/machine.c
index 8d7cecb..306e871 100644
--- a/target/mips/machine.c
+++ b/target/mips/machine.c
@@ -136,6 +136,7 @@ static int get_tlb(QEMUFile *f, void *pv, size_t size,
VMStateField *field)
qemu_get_betls(f, >VPN);
qemu_get_be32s(f, >PageMask);
qemu_get_be16s(f, >ASID);
+qemu_get_be32s(f, >MMID);
qemu_get_be16s(f, );
v->G = (flags >> 10) & 1;
v->C0 = (flags >> 7) & 3;
@@ -161,6 +162,7 @@ static int put_tlb(QEMUFile *f, void *pv, size_t size,
VMStateField *field,
r4k_tlb_t *v = pv;
uint16_t asid = v->ASID;
+uint32_t mmid = v->MMID;
uint16_t flags = ((v->EHINV << 15) |
(v->RI1 << 14) |
(v->RI0 << 13) |
@@ -177,6 +179,7 @@ static int put_tlb(QEMUFile *f, void *pv, size_t size,
VMStateField *field,
qemu_put_betls(f, >VPN);
qemu_put_be32s(f, >PageMask);
qemu_put_be16s(f, );
+qemu_put_be32s(f, );
qemu_put_be16s(f, );
qemu_put_be64s(f, >PFN[0]);
qemu_put_be64s(f, >PFN[1]);
@@ -251,6 +254,7 @@ const VMStateDescription vmstate_mips_cpu = {
VMSTATE_UINT64(env.CP0_EntryLo0, MIPSCPU),
VMSTATE_UINT64(env.CP0_EntryLo1, MIPSCPU),
VMSTATE_UINTTL(env.CP0_Context, MIPSCPU),
+VMSTATE_INT32(env.CP0_MemoryMapID, MIPSCPU),
VMSTATE_INT32(env.CP0_PageMask,
[Qemu-devel] [PATCH v3 04/12] target/mips: Add bit definitions for DSP R3 ASE
From: Stefan Markovic
Add DSP R3 ASE related bit definition for insn_flags and hflags.
Reviewed-by: Aleksandar Markovic
Signed-off-by: Stefan Markovic
Signed-off-by: Aleksandar Markovic
---
target/mips/cpu.h | 1 +
target/mips/mips-defs.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 28af4d1..4160699 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -598,6 +598,7 @@ struct CPUMIPSState {
/* MIPS DSP resources access. */
#define MIPS_HFLAG_DSP 0x08 /* Enable access to MIPS DSP resources. */
#define MIPS_HFLAG_DSPR2 0x10 /* Enable access to MIPS DSPR2 resources. */
+#define MIPS_HFLAG_DSPR3 0x2000 /* Enable access to MIPS DSPR3 resources.*/
/* Extra flag about HWREna register. */
#define MIPS_HFLAG_HWRENA_ULR 0x20 /* ULR bit from HWREna is set. */
#define MIPS_HFLAG_SBRI 0x40 /* R6 SDBBP causes RI excpt. in user mode */
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index c8e9979..b27b7ae 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -47,6 +47,7 @@
#define ASE_MDMX 0x0004
#define ASE_DSP 0x0008
#define ASE_DSPR2 0x0010
+#define ASE_DSPR3 0x0200
#define ASE_MT0x0020
#define ASE_SMARTMIPS 0x0040
#define ASE_MICROMIPS 0x0080
--
2.7.4
Re: [Qemu-devel] [PULL 00/15] s390x updates
Peter Maydell writes: > On 4 October 2018 at 16:28, Cornelia Huck wrote: >> The following changes since commit dafd95053611aa14dda40266857608d12ddce658: >> >> Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into >> staging (2018-10-02 18:27:18 +0100) >> >> are available in the Git repository at: >> >> git://github.com/cohuck/qemu tags/s390x-20181004 >> >> for you to fetch changes up to b576d582ea2b03f4eada186fff59308d22b40a6a: >> >> hw/s390x/s390-pci-bus: Convert sysbus init function to realize function >> (2018-10-04 12:10:40 +0200) >> >> >> Various s390x updates: >> - fix several struct definitions so that sparc hosts do not trip over >> unaligned accesses >> - fence enabling huge pages for pre-3.1 machines >> - sysbus init -> realize conversion >> - fixes and improvements in tcg (instruction flags and AFP registers) > > Applied, thanks. This pull has broken s390x-linux-user tcg tests: timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/multiarch/Makefile.target:30: recipe for target 'run-test-mmap' failed make[2]: *** [run-test-mmap] Error 132 make[2]: *** Waiting for unfinished jobs timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-linux-test' failed make[2]: *** [run-linux-test] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-hello-s390x' failed make[2]: *** [run-hello-s390x] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-ipm' failed make[2]: *** [run-ipm] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-exrl-trtr' failed make[2]: *** [run-exrl-trtr] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-testthread' failed make[2]: *** [run-testthread] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-exrl-trt' failed make[2]: *** [run-exrl-trt] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-sha1' failed make[2]: *** [run-sha1] Error 132 timeout: the monitored command dumped core Illegal instruction /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile:96: recipe for target 'run-csst' failed make[2]: *** [run-csst] Error 132 /home/alex/lsrc/qemu/qemu.git/tests/tcg/Makefile.include:71: recipe for target 'run-guest-tests' failed make[1]: *** [run-guest-tests] Error 2 /home/alex/lsrc/qemu/qemu.git/tests/Makefile.include:968: recipe for target 'run-tcg-tests-s390x-linux-user' failed make: *** [run-tcg-tests-s390x-linux-user] Error 2 which are now showing up on Travis (admitidly hidden by the usual Travis noise). I have bisected the breakage: git bisect start git bisect bad b576d582ea2b03f4eada186fff59308d22b40a6a b576d582ea bad hw/s390x/s390-pci-bus: Convert sysbus init function to realize function git bisect good dafd95053611aa14dda40266857608d12ddce658 dafd950536 good Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging git bisect good bbf6ea3bd961970785a583117ac1c0b84a8ee736 bbf6ea3bd9 good s390x/tcg: factor out and fix DATA exception injection git bisect bad db0504154ef1feedd135f25f8ca6e9abb5d00bbb db0504154e bad s390x/tcg: check for AFP-register, BFP and DFP data exceptions git bisect good 82d179336dd696d04495963ca007fa961496d924 82d179336d good s390x/tcg: support flags for instructions git bisect good ca5f31a0f27d265f2220ad43913f70280e8753a2 ca5f31a0f2 good s390x/tcg: add instruction flags for floating point instructions Which is points at: Bisect Rest (1) db0504154e * bad @ s390x/tcg: check for AFP-register, BFP and DFP data exceptions -- Alex Bennée
[Qemu-devel] [PATCH v3 08/12] target/mips: Extend WatchHi registers
From: Yongbok Kim
WatchHi is extended by the field MemoryMapID with the GINVT
instruction. The field is accessible by MTHC0/MFHC0 in 32-bit
architectures and DMTC0/DMFC0 in 64-bit architectures.
Signed-off-by: Yongbok Kim
Signed-off-by: Aleksandar Markovic
---
target/mips/cpu.h | 2 +-
target/mips/helper.h| 3 +++
target/mips/machine.c | 2 +-
target/mips/op_helper.c | 23 +--
target/mips/translate.c | 40 +++-
5 files changed, 65 insertions(+), 5 deletions(-)
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 4160699..e8ac057 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -512,7 +512,7 @@ struct CPUMIPSState {
uint64_t CP0_LLAddr_rw_bitmask;
int CP0_LLAddr_shift;
target_ulong CP0_WatchLo[8];
-int32_t CP0_WatchHi[8];
+uint64_t CP0_WatchHi[8];
#define CP0WH_ASID 16
target_ulong CP0_XContext;
int32_t CP0_Framemask;
diff --git a/target/mips/helper.h b/target/mips/helper.h
index b2a780a..fe2607d 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -76,6 +76,7 @@ DEF_HELPER_1(mfc0_maar, tl, env)
DEF_HELPER_1(mfhc0_maar, tl, env)
DEF_HELPER_2(mfc0_watchlo, tl, env, i32)
DEF_HELPER_2(mfc0_watchhi, tl, env, i32)
+DEF_HELPER_2(mfhc0_watchhi, tl, env, i32)
DEF_HELPER_1(mfc0_debug, tl, env)
DEF_HELPER_1(mftc0_debug, tl, env)
#ifdef TARGET_MIPS64
@@ -87,6 +88,7 @@ DEF_HELPER_1(dmfc0_tcschefback, tl, env)
DEF_HELPER_1(dmfc0_lladdr, tl, env)
DEF_HELPER_1(dmfc0_maar, tl, env)
DEF_HELPER_2(dmfc0_watchlo, tl, env, i32)
+DEF_HELPER_2(dmfc0_watchhi, tl, env, i32)
#endif /* TARGET_MIPS64 */
DEF_HELPER_2(mtc0_index, void, env, tl)
@@ -150,6 +152,7 @@ DEF_HELPER_2(mthc0_maar, void, env, tl)
DEF_HELPER_2(mtc0_maari, void, env, tl)
DEF_HELPER_3(mtc0_watchlo, void, env, tl, i32)
DEF_HELPER_3(mtc0_watchhi, void, env, tl, i32)
+DEF_HELPER_3(mthc0_watchhi, void, env, tl, i32)
DEF_HELPER_2(mtc0_xcontext, void, env, tl)
DEF_HELPER_2(mtc0_framemask, void, env, tl)
DEF_HELPER_2(mtc0_debug, void, env, tl)
diff --git a/target/mips/machine.c b/target/mips/machine.c
index 5ba78ac..8d7cecb 100644
--- a/target/mips/machine.c
+++ b/target/mips/machine.c
@@ -288,7 +288,7 @@ const VMStateDescription vmstate_mips_cpu = {
VMSTATE_INT32(env.CP0_MAARI, MIPSCPU),
VMSTATE_UINT64(env.lladdr, MIPSCPU),
VMSTATE_UINTTL_ARRAY(env.CP0_WatchLo, MIPSCPU, 8),
-VMSTATE_INT32_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
+VMSTATE_UINT64_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
VMSTATE_UINTTL(env.CP0_XContext, MIPSCPU),
VMSTATE_INT32(env.CP0_Framemask, MIPSCPU),
VMSTATE_INT32(env.CP0_Debug, MIPSCPU),
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index c148b31..14c9bcb 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -991,7 +991,12 @@ target_ulong helper_mfc0_watchlo(CPUMIPSState *env,
uint32_t sel)
target_ulong helper_mfc0_watchhi(CPUMIPSState *env, uint32_t sel)
{
-return env->CP0_WatchHi[sel];
+return (int32_t) env->CP0_WatchHi[sel];
+}
+
+target_ulong helper_mfhc0_watchhi(CPUMIPSState *env, uint32_t sel)
+{
+return env->CP0_WatchHi[sel] >> 32;
}
target_ulong helper_mfc0_debug(CPUMIPSState *env)
@@ -1059,6 +1064,11 @@ target_ulong helper_dmfc0_watchlo(CPUMIPSState *env,
uint32_t sel)
{
return env->CP0_WatchLo[sel];
}
+
+target_ulong helper_dmfc0_watchhi(CPUMIPSState *env, uint32_t sel)
+{
+return env->CP0_WatchHi[sel];
+}
#endif /* TARGET_MIPS64 */
void helper_mtc0_index(CPUMIPSState *env, target_ulong arg1)
@@ -1760,11 +1770,20 @@ void helper_mtc0_watchlo(CPUMIPSState *env,
target_ulong arg1, uint32_t sel)
void helper_mtc0_watchhi(CPUMIPSState *env, target_ulong arg1, uint32_t sel)
{
-int mask = 0x4FF8 | (env->CP0_EntryHi_ASID_mask << CP0WH_ASID);
+uint64_t mask = 0x4FF8 | (env->CP0_EntryHi_ASID_mask << CP0WH_ASID);
+if ((env->CP0_Config5 >> CP0C5_MI) & 1) {
+mask |= 0xULL; /* MMID */
+}
env->CP0_WatchHi[sel] = arg1 & mask;
env->CP0_WatchHi[sel] &= ~(env->CP0_WatchHi[sel] & arg1 & 0x7);
}
+void helper_mthc0_watchhi(CPUMIPSState *env, target_ulong arg1, uint32_t sel)
+{
+env->CP0_WatchHi[sel] = ((uint64_t) (arg1) << 32) |
+(env->CP0_WatchHi[sel] & 0xULL);
+}
+
void helper_mtc0_xcontext(CPUMIPSState *env, target_ulong arg1)
{
target_ulong mask = (1ULL << (env->SEGBITS - 7)) - 1;
diff --git a/target/mips/translate.c b/target/mips/translate.c
index d872fa7..e42cfa6 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -5234,6 +5234,25 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int
reg, int sel)
goto cp0_unimplemented;
}
break;
+case 19:
+switch (sel) {
+case 0:
+case 1:
+case 2:
+case 3:
+case 4:
+case 5:
+case 6:
+case 7:
+
[Qemu-devel] [PATCH v3 03/12] elf: Add Mips_elf_abiflags_v0 structure
From: Stefan Markovic
Add Mips_elf_abiflags_v0 structure to elf.h. The source of information
is kernel header arch/mips/include/asm/elf.h.
Signed-off-by: Stefan Markovic
Signed-off-by: Aleksandar Markovic
---
include/elf.h | 15 +++
1 file changed, 15 insertions(+)
diff --git a/include/elf.h b/include/elf.h
index eb5958d..75c60cc 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -96,6 +96,21 @@ typedef int64_t Elf64_Sxword;
#define MIPS_ABI_FP_640x6 /* -mips32r2 -mfp64*/
#define MIPS_ABI_FP_64A 0x7 /* -mips32r2 -mfp64 -mno-odd-spreg */
+typedef struct mips_elf_abiflags_v0 {
+ uint16_t version; /* Version of flags structure */
+ uint8_t isa_level; /* The level of the ISA: 1-5, 32, 64 */
+ uint8_t isa_rev;/* The revision of ISA: 0 for MIPS V and below,*/
+ /* 1-n otherwise */
+ uint8_t gpr_size; /* The size of general purpose registers */
+ uint8_t cpr1_size; /* The size of co-processor 1 registers*/
+ uint8_t cpr2_size; /* The size of co-processor 2 registers*/
+ uint8_t fp_abi; /* The floating-point ABI */
+ uint32_t isa_ext; /* Mask of processor-specific extensions */
+ uint32_t ases; /* Mask of ASEs used */
+ uint32_t flags1;/* Mask of general flags */
+ uint32_t flags2;
+} Mips_elf_abiflags_v0;
+
/* These constants define the different elf file types */
#define ET_NONE 0
#define ET_REL1
--
2.7.4
[Qemu-devel] [PATCH v3 12/12] target/mips: Add I6500 core configuration
From: Yongbok Kim
Add I6500 core configuration. Note that this configuration is
supported only by best-effort due to lack of certain features
in QEMU.
Signed-off-by: Yongbok Kim
Signed-off-by: Aleksandar Markovic
---
target/mips/translate_init.inc.c | 43
1 file changed, 43 insertions(+)
diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
index d7cd4ee..68a5c6b 100644
--- a/target/mips/translate_init.inc.c
+++ b/target/mips/translate_init.inc.c
@@ -722,6 +722,49 @@ const mips_def_t mips_defs[] =
.mmu_type = MMU_TYPE_R4000,
},
{
+.name = "I6500",
+.CP0_PRid = 0x1B000,
+.CP0_Config0 = MIPS_CONFIG0 | (0x2 << CP0C0_AR) | (0x2 << CP0C0_AT) |
+ (MMU_TYPE_R4000 << CP0C0_MT),
+.CP0_Config1 = MIPS_CONFIG1 | (1 << CP0C1_FP) | (15 << CP0C1_MMU) |
+ (2 << CP0C1_IS) | (5 << CP0C1_IL) | (3 << CP0C1_IA) |
+ (2 << CP0C1_DS) | (5 << CP0C1_DL) | (3 << CP0C1_DA) |
+ (0 << CP0C1_PC) | (1 << CP0C1_WR) | (1 << CP0C1_EP),
+.CP0_Config2 = MIPS_CONFIG2,
+.CP0_Config3 = MIPS_CONFIG3 | (1U << CP0C3_M) |
+ (1 << CP0C3_CMGCR) | (1 << CP0C3_MSAP) |
+ (1 << CP0C3_BP) | (1 << CP0C3_BI) | (1 << CP0C3_ULRI) |
+ (1 << CP0C3_RXI) | (1 << CP0C3_LPA) | (1 << CP0C3_VInt),
+.CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M) | (3 << CP0C4_IE) |
+ (1 << CP0C4_AE) | (0xfc << CP0C4_KScrExist),
+.CP0_Config5 = MIPS_CONFIG5 | (1 << CP0C5_ULS) | (1 << CP0C5_CRCP) |
+ (3 << CP0C5_GI) | (0 << CP0C5_XNP) | (1 << CP0C5_VP) |
+ (1 << CP0C5_MVH) | (1 << CP0C5_LLB) | (1 << CP0C5_MRP),
+.CP0_Config5_rw_bitmask = (1 << CP0C5_MSAEn) | (1 << CP0C5_MI) |
+ (1 << CP0C5_SBRI) | (1 << CP0C5_FRE) |
+ (1 << CP0C5_UFE),
+.CP0_LLAddr_rw_bitmask = 0,
+.CP0_LLAddr_shift = 0,
+.SYNCI_Step = 64,
+.CCRes = 2,
+.CP0_Status_rw_bitmask = 0x30D8,
+.CP0_PageGrain = (1 << CP0PG_IEC) | (1 << CP0PG_XIE) |
+ (1U << CP0PG_RIE),
+.CP0_PageGrain_rw_bitmask = (1 << CP0PG_ELPA),
+.CP1_fcr0 = (1 << FCR0_FREP) | (1 << FCR0_HAS2008) | (1 << FCR0_F64) |
+(1 << FCR0_L) | (1 << FCR0_W) | (1 << FCR0_D) |
+(1 << FCR0_S) | (0x03 << FCR0_PRID) | (0x0 << FCR0_REV),
+.CP1_fcr31 = (1 << FCR31_ABS2008) | (1 << FCR31_NAN2008),
+.CP1_fcr31_rw_bitmask = 0x0103,
+.MSAIR = 0x03 << MSAIR_ProcID,
+.SEGBITS = 48,
+.PABITS = 48,
+.insn_flags = CPU_MIPS64R6 | ASE_MSA,
+.mmu_type = MMU_TYPE_R4000,
+.SAARP = 1,
+.DSPRAMP = 1,
+},
+{
.name = "Loongson-2E",
.CP0_PRid = 0x6302,
/* 64KB I-cache and d-cache. 4 way with 32 bit cache line size. */
--
2.7.4
[Qemu-devel] [PATCH v3 05/12] target/mips: Add availability control for DSP R3 ASE
From: Stefan Markovic
Add infrastructure for availability control for DSP R3 ASE MIPS
instructions. Only BPOSGE32C currently belongs to DSP R3 ASE, but
this is likely to be changed in near future.
Reviewed-by: Aleksandar Markovic
Signed-off-by: Stefan Markovic
Signed-off-by: Aleksandar Markovic
---
target/mips/internal.h | 11 ---
target/mips/translate.c | 13 -
target/mips/translate_init.inc.c | 3 ++-
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/target/mips/internal.h b/target/mips/internal.h
index e41051f..3c5867e 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -307,8 +307,8 @@ static inline void compute_hflags(CPUMIPSState *env)
env->hflags &= ~(MIPS_HFLAG_COP1X | MIPS_HFLAG_64 | MIPS_HFLAG_CP0 |
MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU |
MIPS_HFLAG_AWRAP | MIPS_HFLAG_DSP | MIPS_HFLAG_DSPR2 |
- MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA | MIPS_HFLAG_FRE |
- MIPS_HFLAG_ELPA | MIPS_HFLAG_ERL);
+ MIPS_HFLAG_DSPR3 | MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA |
+ MIPS_HFLAG_FRE | MIPS_HFLAG_ELPA | MIPS_HFLAG_ERL);
if (env->CP0_Status & (1 << CP0St_ERL)) {
env->hflags |= MIPS_HFLAG_ERL;
}
@@ -355,7 +355,12 @@ static inline void compute_hflags(CPUMIPSState *env)
(env->CP0_Config5 & (1 << CP0C5_SBRI))) {
env->hflags |= MIPS_HFLAG_SBRI;
}
-if (env->insn_flags & ASE_DSPR2) {
+if (env->insn_flags & ASE_DSPR3) {
+if (env->CP0_Status & (1 << CP0St_MX)) {
+env->hflags |= MIPS_HFLAG_DSP | MIPS_HFLAG_DSPR2 |
+ MIPS_HFLAG_DSPR3;
+}
+} else if (env->insn_flags & ASE_DSPR2) {
/* Enables access MIPS DSP resources, now our cpu is DSP ASER2,
so enable to access DSPR2 resources. */
if (env->CP0_Status & (1 << CP0St_MX)) {
diff --git a/target/mips/translate.c b/target/mips/translate.c
index ab16cdb..d64a1da 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1868,6 +1868,17 @@ static inline void check_dspr2(DisasContext *ctx)
}
}
+static inline void check_dspr3(DisasContext *ctx)
+{
+if (unlikely(!(ctx->hflags & MIPS_HFLAG_DSPR3))) {
+if (ctx->insn_flags & ASE_DSP) {
+generate_exception_end(ctx, EXCP_DSPDIS);
+} else {
+generate_exception_end(ctx, EXCP_RI);
+}
+}
+}
+
/* This code generates a "reserved instruction" exception if the
CPU does not support the instruction set corresponding to flags. */
static inline void check_insn(DisasContext *ctx, int flags)
@@ -20098,7 +20109,7 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env,
DisasContext *ctx)
gen_compute_branch_cp1_nm(ctx, OPC_BC1NEZ, rt, s);
break;
case NM_BPOSGE32C:
-check_dspr2(ctx);
+check_dspr3(ctx);
{
int32_t imm = extract32(ctx->opcode, 1, 13) |
extract32(ctx->opcode, 0, 1) << 13;
diff --git a/target/mips/translate_init.inc.c b/target/mips/translate_init.inc.c
index b3320b9..d7cd4ee 100644
--- a/target/mips/translate_init.inc.c
+++ b/target/mips/translate_init.inc.c
@@ -485,7 +485,8 @@ const mips_def_t mips_defs[] =
.CP1_fcr31 = (1 << FCR31_ABS2008) | (1 << FCR31_NAN2008),
.SEGBITS = 32,
.PABITS = 32,
-.insn_flags = CPU_NANOMIPS32 | ASE_DSP | ASE_DSPR2 | ASE_MT,
+.insn_flags = CPU_NANOMIPS32 | ASE_DSP | ASE_DSPR2 | ASE_DSPR3 |
+ ASE_MT,
.mmu_type = MMU_TYPE_R4000,
},
#if defined(TARGET_MIPS64)
--
2.7.4
[Qemu-devel] [PATCH v3 06/12] target/mips: Add opcodes for nanoMIPS EVA instructions
From: Dimitrije Nikolic
Add opcodes for nanoMIPS EVA instructions: CACHEE, LBE, LBUE, LHE,
LHUE, LLE, LLWPE, LWE, PREFE, SBE, SCE, SCWPE, SHE, SWE.
Signed-off-by: Dimitrije Nikolic
Signed-off-by: Aleksandar Markovic
---
target/mips/translate.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/target/mips/translate.c b/target/mips/translate.c
index d64a1da..7b1a6b1 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -16499,6 +16499,40 @@ enum {
NM_P_SC = 0x0b,
};
+/* P.LS.E0 instruction pool */
+enum {
+NM_LBE = 0x00,
+NM_SBE = 0x01,
+NM_LBUE = 0x02,
+NM_P_PREFE = 0x03,
+NM_LHE = 0x04,
+NM_SHE = 0x05,
+NM_LHUE = 0x06,
+NM_CACHEE = 0x07,
+NM_LWE = 0x08,
+NM_SWE = 0x09,
+NM_P_LLE= 0x0a,
+NM_P_SCE= 0x0b,
+};
+
+/* P.PREFE instruction pool */
+enum {
+NM_SYNCIE = 0x00,
+NM_PREFE= 0x01,
+};
+
+/* P.LLE instruction pool */
+enum {
+NM_LLE = 0x00,
+NM_LLWPE= 0x01,
+};
+
+/* P.SCE instruction pool */
+enum {
+NM_SCE = 0x00,
+NM_SCWPE= 0x01,
+};
+
/* P.LS.WM instruction pool */
enum {
NM_LWM = 0x00,
--
2.7.4
[Qemu-devel] [PATCH v3 01/12] elf: Fix PT_MIPS_XXX constants
From: Stefan Markovic Fix existing and add missing PT_MIPS_XXX constants in elf.h. This is copied from kernel header arch/mips/include/asm/elf.h. Signed-off-by: Stefan Markovic Signed-off-by: Aleksandar Markovic --- include/elf.h | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/elf.h b/include/elf.h index 312f68a..decf210 100644 --- a/include/elf.h +++ b/include/elf.h @@ -28,8 +28,11 @@ typedef int64_t Elf64_Sxword; #define PT_PHDR6 #define PT_LOPROC 0x7000 #define PT_HIPROC 0x7fff -#define PT_MIPS_REGINFO0x7000 -#define PT_MIPS_OPTIONS0x7001 + +#define PT_MIPS_REGINFO 0x7000 +#define PT_MIPS_RTPROC0x7001 +#define PT_MIPS_OPTIONS 0x7002 +#define PT_MIPS_ABIFLAGS 0x7003 /* Flags in the e_flags field of the header */ /* MIPS architecture level. */ -- 2.7.4
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On 10/08/2018 10:35 AM, Cornelia Huck wrote:
On Mon, 8 Oct 2018 16:22:27 +0200
David Hildenbrand wrote:
On 08/10/2018 16:20, Tony Krowiak wrote:
On 09/27/2018 08:52 AM, Cornelia Huck wrote:
On Thu, 27 Sep 2018 14:29:01 +0200
Thomas Huth wrote:
On 2018-09-27 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Introduces the base object model for virtualizing AP devices.
Signed-off-by: Tony Krowiak
---
+typedef struct APBridge {
+SysBusDevice sysbus_dev;
+bool css_dev_path;
What is this css_dev_path variable good for? I don't see it used in any
of the other patches?
If you don't need it, I think you could get rid of this struct completely?
Huh, now I remember complaining about it before. Looks like a
copy-and-paste from the css bridge; that variable is used for compat
handling there (and should be ditched here).
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct APBus {
+BusState parent_obj;
+} APBus;
+
+#define TYPE_AP_BUS "ap-bus"
+#define AP_BUS(obj) \
+ OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
struct APBus.
If there's nothing interesting to put in these inherited structures,
probably yes.
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
new file mode 100644
index ..693df90cc041
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE "ap-device"
+
+typedef struct APDevice {
+DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
Do you really need any of these definitions except AP_DEVICE_TYPE ?
Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
AP_DEVICE_CLASS(klass), but aren't those typically included in all
QOM definitions?
Yes, we usually add all of them although only some might actually be
used. (adding a new device usually looks like filling out a template)
Much of this seems to be boilerplate in this case, and I'm not sure how
much sense it makes. On the plus side, however, it looks like
everything else :)
So, I would merge both a complete version or a
stripped-down-to-the-needed version, unless someone else has a strong
argument.
The 'I would merge both' implies you are asking for two versions, but
the 'or' implies you are asking for one or the other; I'm going to
assume you are asking for one or the other. I'll provide a stripped down
version in v10 which I am planning on posting today.
[Qemu-devel] [PATCH v3 00/12] Misc MIPS fixes and improvements for October 2018
From: Aleksandar Markovic This series contains support for MIPS ABI flags in elf.h, DSP R3 availability control, and emulation of nanoMIPS EVA instructions. MIPS ABI flags will be used in near future for linux user mode support for certain MIPS-specific parts of prctl() system call. This series also contains support for WatchHi, MemoryMapID, SAARI, and SAAR registers, ITU updates, implementation of DRAM, and I6500 core configuration. All patches were developed a while ago, but were not submitted to this list for various reasons. v2->v3: - added patch on extending WatchHi registers - added patch on MemoryMapID, SAARI, and SAAR registers - added patch on ITU updates wrt SAAR - added patch on Data Scratch Pad RAM - added patch on I6500 core configuration - corrected the patch on nanoMIPS EVA opcodes v1->v2: - added three patches on MIPS-specifics in elf.h Dimitrije Nikolic (2): target/mips: Add opcodes for nanoMIPS EVA instructions target/mips: Implement emulation of nanoMIPS EVA instructions Stefan Markovic (5): elf: Fix PT_MIPS_XXX constants elf: Add MIPS_ABI_FP_XXX constants elf: Add Mips_elf_abiflags_v0 structure target/mips: Add bit definitions for DSP R3 ASE target/mips: Add availability control for DSP R3 ASE Yongbok Kim (5): target/mips: Extend WatchHi registers target/mips: Implement MemoryMapID, SAARI, and SAAR registers hw/mips: Update ITU to utilise SAARI/SAAR registers hw/mips: Add Data Scratch Pad RAM target/mips: Add I6500 core configuration default-configs/mips-softmmu-common.mak | 1 + hw/mips/cps.c | 9 ++ hw/mips/mips_malta.c| 31 hw/misc/Makefile.objs | 1 + hw/misc/mips_itu.c | 72 +- include/elf.h | 30 +++- include/hw/mips/cps.h | 2 + include/hw/misc/mips_itu.h | 7 + target/mips/cpu.h | 21 ++- target/mips/helper.h| 9 ++ target/mips/internal.h | 14 +- target/mips/machine.c | 8 +- target/mips/mips-defs.h | 1 + target/mips/op_helper.c | 98 - target/mips/translate.c | 241 +++- target/mips/translate_init.inc.c| 46 +- 16 files changed, 568 insertions(+), 23 deletions(-) -- 2.7.4
Re: [Qemu-devel] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
>> Signed-off-by: Anton Nefedov
>> Reviewed-by: Alberto Garcia
>> ---
>> hw/ide/core.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 2c62efc..352429b 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>> TrimAIOCB *iocb = opaque;
>> IDEState *s = iocb->s;
>>
>> +if (iocb->i >= 0) {
>> +if (ret >= 0) {
>> +block_acct_done(blk_get_stats(s->blk), >acct);
>> +} else {
>> +block_acct_failed(blk_get_stats(s->blk), >acct);
>> +}
>> +}
>> +
>> if (ret >= 0) {
>> while (iocb->j < iocb->qiov->niov) {
>> int j = iocb->j;
>> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>> goto done;
>> }
>>
>> +block_acct_start(blk_get_stats(s->blk), >acct,
>> + count << BDRV_SECTOR_BITS,
>> BLOCK_ACCT_UNMAP);
>> +
>> /* Got an entry! Submit and exit. */
>> iocb->aiocb = blk_aio_pdiscard(s->blk,
>> sector << BDRV_SECTOR_BITS,
>> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
>> }
>>
>> if (ret == -EINVAL) {
>> +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
>
> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> also for reads and writes, and each of them could return -EINVAL.
>
Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
> Also, -EINVAL doesn't necessarily mean that the guest driver did
> something wrong, it could also be the result of a host problem.
> Therefore, it isn't right to call block_acct_invalid() here - especially
> since the request may already have been accounted for as either done or
> failed in ide_issue_trim_cb().
>
Couldn't be accounted done with such retcode;
and it seems I shouldnt do block_acct_failed() there anyway - or it's
accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
But if EINVAL (from further layers) should not be accounted as an
invalid op, then it should be accounted failed instead, the thing that
current code does not do.
(and which brings us back to possible double-accounting if we account
invalid in ide_issue_trim_cb() )
> Instead, I think it would be better to immediately account for invalid
> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> know for sure that indeed !ide_sect_range_ok() is the cause for the
> -EINVAL return code.
>
So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
acct_failed there, and filter off TRIM commands in the common
accounting.
/Anton
Re: [Qemu-devel] [PATCH v8 0/2] deprecate incorrect CPUs topolog
On Thu, Sep 13, 2018 at 01:06:00PM +0200, Igor Mammedov wrote: > Changelog since v7: > * drop repetitive sentence in deprication doc (Eric Blake > ) > Changelog since v5: > * add(v6) and then remove(v7) Notes section to/from deprication doc > (Eduardo Habkost ) > * fix up wording and math formating in deprication doc > (Eduardo Habkost ) > * drop !socket check as it always evaluates to true at that point > (Eduardo Habkost ) > Changelog since v4: > * extend deprication doc, adding that maxcpus should be multiple of > present on CLI [sockets/cores/threads] options > (Eduardo Habkost ) > > series bundles together 2 related patches posted separately earlier: > vl.c deprecate incorrect CPUs topology > vl:c: make sure that sockets are calculated correctly in '-smp X' case > > Goal is to depricate invalid topologies so we could make sure that topology > configuration is correct by forbidding invalid input once deprecation > period ends. Thanks, queued on machine-next. -- Eduardo
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On 09/27/2018 08:52 AM, Cornelia Huck wrote:
On Thu, 27 Sep 2018 14:29:01 +0200
Thomas Huth wrote:
On 2018-09-27 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Introduces the base object model for virtualizing AP devices.
Signed-off-by: Tony Krowiak
---
+typedef struct APBridge {
+SysBusDevice sysbus_dev;
+bool css_dev_path;
What is this css_dev_path variable good for? I don't see it used in any
of the other patches?
If you don't need it, I think you could get rid of this struct completely?
Huh, now I remember complaining about it before. Looks like a
copy-and-paste from the css bridge; that variable is used for compat
handling there (and should be ditched here).
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct APBus {
+BusState parent_obj;
+} APBus;
+
+#define TYPE_AP_BUS "ap-bus"
+#define AP_BUS(obj) \
+ OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
struct APBus.
If there's nothing interesting to put in these inherited structures,
probably yes.
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
new file mode 100644
index ..693df90cc041
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE "ap-device"
+
+typedef struct APDevice {
+DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
Do you really need any of these definitions except AP_DEVICE_TYPE ?
Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
AP_DEVICE_CLASS(klass), but aren't those typically included in all
QOM definitions?
Same here, I think.
Re: [Qemu-devel] [PATCH v4 6/8] scsi: account unmap operations
On 4/10/2018 6:47 PM, Kevin Wolf wrote:
> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
>> Signed-off-by: Anton Nefedov
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> Reviewed-by: Alberto Garcia
>> ---
>> hw/scsi/scsi-disk.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 9d10daf..0aac137 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1664,6 +1664,10 @@ static void scsi_unmap_complete_noio(UnmapCBData
>> *data, int ret)
>> goto done;
>> }
>>
>> +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
>> + r->sector_count * s->qdev.blocksize,
>> + BLOCK_ACCT_UNMAP);
>
> If the check just above this (check_lba_range) fails, we should account
> for an invalid request.
>
Done.
>> r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>> r->sector * s->qdev.blocksize,
>> r->sector_count *
>> s->qdev.blocksize,
>> @@ -1690,10 +1694,11 @@ static void scsi_unmap_complete(void *opaque, int
>> ret)
>> r->req.aiocb = NULL;
>>
>> aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> -if (scsi_disk_req_check_error(r, ret, false)) {
>> +if (scsi_disk_req_check_error(r, ret, true)) {
>> scsi_req_unref(>req);
>> g_free(data);
>> } else {
>> +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
>> scsi_unmap_complete_noio(data, ret);
>> }
>> aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> @@ -1740,10 +1745,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,
>> uint8_t *inbuf)
>> return;
>>
>> invalid_param_len:
>> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>> scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
>> return;
>>
>> invalid_field:
>> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>> scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>> }
>
> What about the blk_is_read_only() case which directly returns without
> jumping to one of the error labels?
>
So basically anything we don't bring to blk layer is invalid req.
Fixed.
Re: [Qemu-devel] [PULL 00/33] target-arm queue
On 8 October 2018 at 14:59, Peter Maydell wrote: > target-arm queue: the big things in here are SVE in system > emulation mode, and v8M stack limit checking; there are > also a handful of smaller fixes. > > thanks > -- PMM > > The following changes since commit 079911cb6e26898e16f5bb56ef4f9d33cf92d32d: > > Merge remote-tracking branch 'remotes/rth/tags/pull-fpu-20181005' into > staging (2018-10-08 12:44:35 +0100) > > are available in the Git repository at: > > https://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20181008 > > for you to fetch changes up to 74e2e59b8d0a68be0956310fc349179c89fd7be0: > > hw/display/bcm2835_fb: Silence Coverity warning about multiply overflow > (2018-10-08 14:55:05 +0100) > > > target-arm queue: > * target/arm: fix error in a code comment > * virt: Suppress external aborts on virt-2.10 and earlier > * target/arm: Correct condition for v8M callee stack push > * target/arm: Don't read r4 from v8M exception stackframe twice > * target/arm: Support SVE in system emulation mode > * target/arm: Implement v8M hardware stack limit checking > * hw/display/bcm2835_fb: Silence Coverity warning about multiply overflow Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH V2 1/1] tests: Add migration test for aarch64
On 04/10/2018 18:08, Wei Huang wrote:
> This patch adds migration test support for aarch64. The test code, which
> implements the same functionality as x86, is booted as a kernel in qemu.
> Here are the design choices we make for aarch64:
>
> * We choose this -kernel approach because aarch64 QEMU doesn't provide a
>built-in fw like x86 does. So instead of relying on a boot loader, we
>use -kernel approach for aarch64.
> * The serial output is sent to PL011 directly.
> * The physical memory base for mach-virt machine is 0x4000. We change
>the start_address and end_address for aarch64.
>
> In addition to providing the binary, this patch also includes the source
> code and the build script in tests/migration/aarch64. So users can change
> the source and/or re-compile the binary as they wish.
>
> Reviewed-by: Juan Quintela
> Reviewed-by: Andrew Jones
> Signed-off-by: Wei Huang
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
> ---
> tests/Makefile.include | 1 +
> tests/migration-test.c | 27 +++--
> tests/migration/Makefile | 2 +-
> tests/migration/aarch64/Makefile | 19 +
> tests/migration/aarch64/a-b-kernel.S | 75
>
> tests/migration/aarch64/a-b-kernel.h | 19 +
> tests/migration/migration-test.h | 9 +
> 7 files changed, 147 insertions(+), 5 deletions(-)
> create mode 100644 tests/migration/aarch64/Makefile
> create mode 100644 tests/migration/aarch64/a-b-kernel.S
> create mode 100644 tests/migration/aarch64/a-b-kernel.h
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 175d013..857e7cc 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -402,6 +402,7 @@ check-qtest-arm-y += tests/hexloader-test$(EXESUF)
> check-qtest-aarch64-y = tests/numa-test$(EXESUF)
> check-qtest-aarch64-$(CONFIG_SDHCI) += tests/sdhci-test$(EXESUF)
> check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-aarch64-y += tests/migration-test$(EXESUF)
>
> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 20f38f1..5bdc0bd 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -86,12 +86,13 @@ static const char *tmpfs;
> * repeatedly. It outputs a 'B' at a fixed rate while it's still running.
> */
> #include "tests/migration/i386/a-b-bootblock.h"
> +#include "tests/migration/aarch64/a-b-kernel.h"
>
> -static void init_bootfile_x86(const char *bootpath)
> +static void init_bootfile(const char *bootpath, void *content)
> {
> FILE *bootfile = fopen(bootpath, "wb");
>
> -g_assert_cmpint(fwrite(x86_bootsect, 512, 1, bootfile), ==, 1);
> +g_assert_cmpint(fwrite(content, 512, 1, bootfile), ==, 1);
> fclose(bootfile);
> }
>
> @@ -428,7 +429,7 @@ static int test_migrate_start(QTestState **from,
> QTestState **to,
> got_stop = false;
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -init_bootfile_x86(bootpath);
> +init_bootfile(bootpath, x86_bootsect);
> cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
>" -name source,debug-threads=on"
>" -serial file:%s/src_serial"
> @@ -459,6 +460,24 @@ static int test_migrate_start(QTestState **from,
> QTestState **to,
>
> start_address = PPC_TEST_MEM_START;
> end_address = PPC_TEST_MEM_END;
> +} else if (strcmp(arch, "aarch64") == 0) {
> +init_bootfile(bootpath, aarch64_kernel);
> +cmd_src = g_strdup_printf("-machine virt,accel=%s,gic-version=max "
> + "-name vmsource,debug-threads=on -cpu max "
> + "-m 150M -serial file:%s/src_serial "
> + "-kernel %s ",
> + accel, tmpfs, bootpath);
> +cmd_dst = g_strdup_printf("-machine virt,accel=%s,gic-version=max "
> + "-name vmdest,debug-threads=on -cpu max "
> + "-m 150M -serial file:%s/dest_serial "
> + "-kernel %s "
> + "-incoming %s ",
> + accel, tmpfs, bootpath, uri);
> +
> +start_address = ARM_TEST_MEM_START;
> +end_address = ARM_TEST_MEM_END;
> +
> +g_assert(sizeof(aarch64_kernel) <= ARM_TEST_MAX_KERNEL_SIZE);
> } else {
> g_assert_not_reached();
> }
> @@ -545,7 +564,7 @@ static void test_deprecated(void)
> {
> QTestState *from;
>
> -from = qtest_start("");
> +from = qtest_start("-machine none");
>
> deprecated_set_downtime(from, 0.12345);
> deprecated_set_speed(from, 12345);
> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> index
Re: [Qemu-devel] [RFC 2/6] cputlb: do not evict invalid entries to the vtlb
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.
Thanks for the clarification!
Emilio
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On 2018-10-08 16:20, Tony Krowiak wrote:
> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>> On Thu, 27 Sep 2018 14:29:01 +0200
>> Thomas Huth wrote:
>>
>>> On 2018-09-27 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Introduces the base object model for virtualizing AP devices.
Signed-off-by: Tony Krowiak
---
>>
+typedef struct APBridge {
+ SysBusDevice sysbus_dev;
+ bool css_dev_path;
>>>
>>> What is this css_dev_path variable good for? I don't see it used in any
>>> of the other patches?
>>> If you don't need it, I think you could get rid of this struct
>>> completely?
>>
>> Huh, now I remember complaining about it before. Looks like a
>> copy-and-paste from the css bridge; that variable is used for compat
>> handling there (and should be ditched here).
>>
>>>
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+ OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct APBus {
+ BusState parent_obj;
+} APBus;
+
+#define TYPE_AP_BUS "ap-bus"
+#define AP_BUS(obj) \
+ OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>
>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>> struct APBus.
>>
>> If there's nothing interesting to put in these inherited structures,
>> probably yes.
>>
>>>
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h
b/include/hw/s390x/ap-device.h
new file mode 100644
index ..693df90cc041
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
or (at
+ * your option) any later version. See the COPYING file in the
top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE "ap-device"
+
+typedef struct APDevice {
+ DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+ DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+ return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+ OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+ OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
>>>
>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
>
> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> patch 5/6.
Fine for me, if you replace the DO_UPCAST in patch 5 with AP_DEVICE().
> We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> AP_DEVICE_CLASS(klass), but aren't those typically included in all
> QOM definitions?
As long as you don't really need them, I'd simply remove them. They can
be added back when some code really needs them.
Thomas
[Qemu-devel] [PULL 01/33] target/arm: fix code comments error
From: Dongjiu Geng The parameter of kvm_arm_init_cpreg_list() is ARMCPU instead of CPUState, so correct the note to make it match the code. Signed-off-by: Dongjiu Geng Message-id: 1538069046-5757-1-git-send-email-gengdong...@huawei.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/kvm_arm.h | 4 ++-- target/arm/kvm.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h index 863f205822e..5948e8b5602 100644 --- a/target/arm/kvm_arm.h +++ b/target/arm/kvm_arm.h @@ -50,9 +50,9 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group, /** * kvm_arm_init_cpreg_list: - * @cs: CPUState + * @cpu: ARMCPU * - * Initialize the CPUState's cpreg list according to the kernel's + * Initialize the ARMCPU cpreg list according to the kernel's * definition of what CPU registers it knows about (and throw away * the previous TCG-created cpreg list). * diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 65f867d5692..54ef5f711b8 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -310,7 +310,7 @@ static int compare_u64(const void *a, const void *b) return 0; } -/* Initialize the CPUState's cpreg list according to the kernel's +/* Initialize the ARMCPU cpreg list according to the kernel's * definition of what CPU registers it knows about (and throw away * the previous TCG-created cpreg list). */ -- 2.19.0
Re: [Qemu-devel] [PATCH v3 3/4] cputlb: serialize tlb updates with env->tlb_lock
Emilio G. Cota writes:
> On Mon, Oct 08, 2018 at 14:57:18 +0100, Alex Bennée wrote:
>> Emilio G. Cota writes:
>> > The readers that do not hold tlb_lock must use atomic reads when
>> > reading .addr_write, since this field can be updated by other threads;
>> > the conversion to atomic reads is done in the next patch.
>>
>> We don't enforce this for the TCG code - but rely on the backend ISA's
>> to avoid torn reads from updates from cputlb that could invalidate an
>> entry.
>
> We do enforce it though; the TLB reads we emit in TCG backend
> code are appropriately sized to guarantee atomic reads.
>
>> > -/* For atomic correctness when running MTTCG we need to use the right
>> > - * primitives when copying entries */
>> > -static inline void copy_tlb_helper(CPUTLBEntry *d, CPUTLBEntry *s,
>> > - bool atomic_set)
>> > +/* Called with tlb_lock held */
>> > +static inline void copy_tlb_helper_locked(CPUTLBEntry *d, const
>> > CPUTLBEntry *s)
>> > {
>> > -#if TCG_OVERSIZED_GUEST
>> > *d = *s;
>>
>> In general I'm happy with the patch set but what ensures that this
>> always DRT with respect to the TCG code reads that race with it?
>
> copy_tlb_helper is only called by the "owner" CPU, so it cannot
> race with TCG code (i.e. the owner thread cannot race with itself).
>
> I wanted to add an assert_cpu_is_self(cpu) here, but that needs
> a CPUState pointer. Maybe I should just get rid of the function?
> All the callers have the assert, so that might make the code
> clearer.
I'm happy keeping the function and just expanding the comment:
/* Called with tlb_lock held and only ever from the vCPU context */
Reviewed-by: Alex Bennée
>
> Thanks,
>
> Emilio
--
Alex Bennée
[Qemu-devel] [PULL 06/33] target/arm: Adjust sve_exception_el
From: Richard Henderson
Check for EL3 before testing CPTR_EL3.EZ. Return 0 when the exception
should be routed via AdvSIMDFPAccessTrap. Mirror the structure of
CheckSVEEnabled more closely.
Fixes: 5be5e8eda78
Reviewed-by: Peter Maydell
Tested-by: Laurent Desnogues
Signed-off-by: Richard Henderson
Message-id: 20181005175350.30752-3-richard.hender...@linaro.org
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 96 ++---
1 file changed, 46 insertions(+), 50 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 050f3d444c6..9bb81da2c7f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4400,67 +4400,63 @@ static const ARMCPRegInfo debug_lpae_cp_reginfo[] = {
REGINFO_SENTINEL
};
-/* Return the exception level to which SVE-disabled exceptions should
- * be taken, or 0 if SVE is enabled.
+/* Return the exception level to which exceptions should be taken
+ * via SVEAccessTrap. If an exception should be routed through
+ * AArch64.AdvSIMDFPAccessTrap, return 0; fp_exception_el should
+ * take care of raising that exception.
+ * C.f. the ARM pseudocode function CheckSVEEnabled.
*/
static int sve_exception_el(CPUARMState *env)
{
#ifndef CONFIG_USER_ONLY
unsigned current_el = arm_current_el(env);
-/* The CPACR.ZEN controls traps to EL1:
- * 0, 2 : trap EL0 and EL1 accesses
- * 1: trap only EL0 accesses
- * 3: trap no accesses
+if (current_el <= 1) {
+bool disabled = false;
+
+/* The CPACR.ZEN controls traps to EL1:
+ * 0, 2 : trap EL0 and EL1 accesses
+ * 1: trap only EL0 accesses
+ * 3: trap no accesses
+ */
+if (!extract32(env->cp15.cpacr_el1, 16, 1)) {
+disabled = true;
+} else if (!extract32(env->cp15.cpacr_el1, 17, 1)) {
+disabled = current_el == 0;
+}
+if (disabled) {
+/* route_to_el2 */
+return (arm_feature(env, ARM_FEATURE_EL2)
+&& !arm_is_secure(env)
+&& (env->cp15.hcr_el2 & HCR_TGE) ? 2 : 1);
+}
+
+/* Check CPACR.FPEN. */
+if (!extract32(env->cp15.cpacr_el1, 20, 1)) {
+disabled = true;
+} else if (!extract32(env->cp15.cpacr_el1, 21, 1)) {
+disabled = current_el == 0;
+}
+if (disabled) {
+return 0;
+}
+}
+
+/* CPTR_EL2. Since TZ and TFP are positive,
+ * they will be zero when EL2 is not present.
*/
-switch (extract32(env->cp15.cpacr_el1, 16, 2)) {
-default:
-if (current_el <= 1) {
-/* Trap to PL1, which might be EL1 or EL3 */
-if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
-return 3;
-}
-return 1;
+if (current_el <= 2 && !arm_is_secure_below_el3(env)) {
+if (env->cp15.cptr_el[2] & CPTR_TZ) {
+return 2;
}
-break;
-case 1:
-if (current_el == 0) {
-return 1;
+if (env->cp15.cptr_el[2] & CPTR_TFP) {
+return 0;
}
-break;
-case 3:
-break;
}
-/* Similarly for CPACR.FPEN, after having checked ZEN. */
-switch (extract32(env->cp15.cpacr_el1, 20, 2)) {
-default:
-if (current_el <= 1) {
-if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
-return 3;
-}
-return 1;
-}
-break;
-case 1:
-if (current_el == 0) {
-return 1;
-}
-break;
-case 3:
-break;
-}
-
-/* CPTR_EL2. Check both TZ and TFP. */
-if (current_el <= 2
-&& (env->cp15.cptr_el[2] & (CPTR_TFP | CPTR_TZ))
-&& !arm_is_secure_below_el3(env)) {
-return 2;
-}
-
-/* CPTR_EL3. Check both EZ and TFP. */
-if (!(env->cp15.cptr_el[3] & CPTR_EZ)
-|| (env->cp15.cptr_el[3] & CPTR_TFP)) {
+/* CPTR_EL3. Since EZ is negative we must check for EL3. */
+if (arm_feature(env, ARM_FEATURE_EL3)
+&& !(env->cp15.cptr_el[3] & CPTR_EZ)) {
return 3;
}
#endif
--
2.19.0
Re: [Qemu-devel] [RFC PATCH 00/21] Trace updates and plugin RFC
Emilio G. Cota writes: > On Mon, Oct 08, 2018 at 11:28:38 +0100, Alex Bennée wrote: >> Emilio G. Cota writes: >> > Again, for performance you'd avoid the tracepoint (i.e. calling >> > a helper to call another function) and embed directly the >> > callback from TCG. Same thing applies to TB's. >> >> OK I see what you mean. I think that is doable although it might take a >> bit more tcg plumbing. > > I have patches to do it, it's not complicated. Right that would be useful. > >> >> So what do people think? Could this be a viable way to extend QEMU >> >> with plugins? >> > >> > For frequent events such as the ones mentioned above, I am >> > not sure plugins can be efficiently implemented under >> > tracing. >> >> I assume some form of helper-per-instrumented-event/insn is still going >> to be needed though? We are not considering some sort of EBF craziness? > > Helper, yes. But one that points directly to plugin code. It would be nice if the logic the inserts the trace helper vs a direct call could be shared. I guess I'd have to see the implementation to see how ugly it gets. > >> > For others (e.g. cpu_init events), sure, they could. >> > But still, differently from tracers, plugins can come and go >> > anytime, so I am not convinced that merging the two features >> > is a good idea. >> >> I don't think we have to mirror tracepoints and plugin points but I'm in >> favour of sharing the general mechanism and tooling rather than having a >> whole separate set of hooks. We certainly don't want anything like: >> >> trace_exec_tb(tb, pc); >> plugin_exec_tb(tb, pc); >> >> scattered throughout the code where the two do align. > > We could have something like > > plugin_trace_exec_tb(tb, pc); > > that would expand to the two lines above. Or similar. > > So I agree with you that in some cases the "trace points" > for both tracing and plugin might be the same, perhaps > identical. But that doesn't necessarily mean that making > plugins a subset of tracing is a good idea. But we can avoid having plugin-points and trace-events duplicating stuff as well? I guess you want to avoid having the generated code fragments for plugins? The other nice property was avoiding re-duplicating output logic for "filter" style operations. However I didn't actually included such an example in the series. I was pondering a QEMU powered PLT/library call tracer to demonstrate that sort of thing. > I think sharing my plugin implementation will help the > discussion. I'll share it as soon as I can (my QEMU plate > is full already trying to merge a couple of other features > first). Sounds good. > > Thanks, > > Emilio -- Alex Bennée
[Qemu-devel] [PULL 07/33] target/arm: Pass in current_el to fp and sve_exception_el
From: Richard Henderson
We are going to want to determine whether sve is enabled
for EL other than current.
Tested-by: Laurent Desnogues
Reviewed-by: Peter Maydell
Signed-off-by: Richard Henderson
Message-id: 20181005175350.30752-4-richard.hender...@linaro.org
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9bb81da2c7f..35458ad4a76 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4406,12 +4406,10 @@ static const ARMCPRegInfo debug_lpae_cp_reginfo[] = {
* take care of raising that exception.
* C.f. the ARM pseudocode function CheckSVEEnabled.
*/
-static int sve_exception_el(CPUARMState *env)
+static int sve_exception_el(CPUARMState *env, int el)
{
#ifndef CONFIG_USER_ONLY
-unsigned current_el = arm_current_el(env);
-
-if (current_el <= 1) {
+if (el <= 1) {
bool disabled = false;
/* The CPACR.ZEN controls traps to EL1:
@@ -4422,7 +4420,7 @@ static int sve_exception_el(CPUARMState *env)
if (!extract32(env->cp15.cpacr_el1, 16, 1)) {
disabled = true;
} else if (!extract32(env->cp15.cpacr_el1, 17, 1)) {
-disabled = current_el == 0;
+disabled = el == 0;
}
if (disabled) {
/* route_to_el2 */
@@ -4435,7 +4433,7 @@ static int sve_exception_el(CPUARMState *env)
if (!extract32(env->cp15.cpacr_el1, 20, 1)) {
disabled = true;
} else if (!extract32(env->cp15.cpacr_el1, 21, 1)) {
-disabled = current_el == 0;
+disabled = el == 0;
}
if (disabled) {
return 0;
@@ -4445,7 +4443,7 @@ static int sve_exception_el(CPUARMState *env)
/* CPTR_EL2. Since TZ and TFP are positive,
* they will be zero when EL2 is not present.
*/
-if (current_el <= 2 && !arm_is_secure_below_el3(env)) {
+if (el <= 2 && !arm_is_secure_below_el3(env)) {
if (env->cp15.cptr_el[2] & CPTR_TZ) {
return 2;
}
@@ -12512,11 +12510,10 @@ uint32_t HELPER(crc32c)(uint32_t acc, uint32_t val,
uint32_t bytes)
/* Return the exception level to which FP-disabled exceptions should
* be taken, or 0 if FP is enabled.
*/
-static inline int fp_exception_el(CPUARMState *env)
+static int fp_exception_el(CPUARMState *env, int cur_el)
{
#ifndef CONFIG_USER_ONLY
int fpen;
-int cur_el = arm_current_el(env);
/* CPACR and the CPTR registers don't exist before v6, so FP is
* always accessible
@@ -12579,7 +12576,8 @@ void cpu_get_tb_cpu_state(CPUARMState *env,
target_ulong *pc,
target_ulong *cs_base, uint32_t *pflags)
{
ARMMMUIdx mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
-int fp_el = fp_exception_el(env);
+int current_el = arm_current_el(env);
+int fp_el = fp_exception_el(env, current_el);
uint32_t flags;
if (is_a64(env)) {
@@ -12590,7 +12588,7 @@ void cpu_get_tb_cpu_state(CPUARMState *env,
target_ulong *pc,
flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
if (arm_feature(env, ARM_FEATURE_SVE)) {
-int sve_el = sve_exception_el(env);
+int sve_el = sve_exception_el(env, current_el);
uint32_t zcr_len;
/* If SVE is disabled, but FP is enabled,
@@ -12599,7 +12597,6 @@ void cpu_get_tb_cpu_state(CPUARMState *env,
target_ulong *pc,
if (sve_el != 0 && fp_el == 0) {
zcr_len = 0;
} else {
-int current_el = arm_current_el(env);
ARMCPU *cpu = arm_env_get_cpu(env);
zcr_len = cpu->sve_max_vq - 1;
--
2.19.0
[Qemu-devel] [PULL 27/33] target/arm: Add v8M stack checks for LDRD/STRD (imm)
Add the v8M stack checks for:
* LDRD (immediate)
* STRD (immediate)
Loads and stores are more complicated than ADD/SUB/MOV, because we
must ensure that memory accesses below the stack limit are not
performed, so we can't simply do the check when we actually update
SP.
For these instructions, if the stack limit check triggers
we must not:
* perform any memory access below the SP limit
* update PC, SP or the load/store base register
but it is IMPDEF whether we:
* perform any accesses above or equal to the SP limit
* update destination registers for loads
For QEMU we choose to always check the limit before doing any other
part of the load or store, so we won't update any registers or
perform any memory accesses.
It is UNKNOWN whether the limit check triggers for a load or store
where the initial SP value is below the limit and one of the stores
would be below the limit, but the writeback moves SP to above the
limit. For QEMU we choose to trigger the check in this situation.
Note that limit checks happen only for loads and stores which update
SP via writeback; they do not happen for loads and stores which
simply use SP as a base register.
Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
Message-id: 20181002163556.10279-9-peter.mayd...@linaro.org
---
target/arm/translate.c | 27 +--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index fcb33b8a503..c16d6075d94 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10278,6 +10278,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
* 0b_1001_x11x_____
* - load/store dual (pre-indexed)
*/
+bool wback = extract32(insn, 21, 1);
+
if (rn == 15) {
if (insn & (1 << 21)) {
/* UNPREDICTABLE */
@@ -10289,8 +10291,29 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
addr = load_reg(s, rn);
}
offset = (insn & 0xff) * 4;
-if ((insn & (1 << 23)) == 0)
+if ((insn & (1 << 23)) == 0) {
offset = -offset;
+}
+
+if (s->v8m_stackcheck && rn == 13 && wback) {
+/*
+ * Here 'addr' is the current SP; if offset is +ve we're
+ * moving SP up, else down. It is UNKNOWN whether the limit
+ * check triggers when SP starts below the limit and ends
+ * up above it; check whichever of the current and final
+ * SP is lower, so QEMU will trigger in that situation.
+ */
+if ((int32_t)offset < 0) {
+TCGv_i32 newsp = tcg_temp_new_i32();
+
+tcg_gen_addi_i32(newsp, addr, offset);
+gen_helper_v8m_stackcheck(cpu_env, newsp);
+tcg_temp_free_i32(newsp);
+} else {
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+}
+
if (insn & (1 << 24)) {
tcg_gen_addi_i32(addr, addr, offset);
offset = 0;
@@ -10314,7 +10337,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
gen_aa32_st32(s, tmp, addr, get_mem_index(s));
tcg_temp_free_i32(tmp);
}
-if (insn & (1 << 21)) {
+if (wback) {
/* Base writeback. */
tcg_gen_addi_i32(addr, addr, offset - 4);
store_reg(s, rn, addr);
--
2.19.0
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On Mon, 8 Oct 2018 16:22:27 +0200
David Hildenbrand wrote:
> On 08/10/2018 16:20, Tony Krowiak wrote:
> > On 09/27/2018 08:52 AM, Cornelia Huck wrote:
> >> On Thu, 27 Sep 2018 14:29:01 +0200
> >> Thomas Huth wrote:
> >>
> >>> On 2018-09-27 00:54, Tony Krowiak wrote:
> From: Tony Krowiak
>
> Introduces the base object model for virtualizing AP devices.
>
> Signed-off-by: Tony Krowiak
> ---
> >>
> +typedef struct APBridge {
> +SysBusDevice sysbus_dev;
> +bool css_dev_path;
> >>>
> >>> What is this css_dev_path variable good for? I don't see it used in any
> >>> of the other patches?
> >>> If you don't need it, I think you could get rid of this struct
> >>> completely?
> >>
> >> Huh, now I remember complaining about it before. Looks like a
> >> copy-and-paste from the css bridge; that variable is used for compat
> >> handling there (and should be ditched here).
> >>
> >>>
> +} APBridge;
> +
> +#define TYPE_AP_BRIDGE "ap-bridge"
> +#define AP_BRIDGE(obj) \
> +OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
> +
> +typedef struct APBus {
> +BusState parent_obj;
> +} APBus;
> +
> +#define TYPE_AP_BUS "ap-bus"
> +#define AP_BUS(obj) \
> + OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
> >>>
> >>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
> >>> struct APBus.
> >>
> >> If there's nothing interesting to put in these inherited structures,
> >> probably yes.
> >>
> >>>
> +void s390_init_ap(void);
> +
> +#endif
> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
> new file mode 100644
> index ..693df90cc041
> --- /dev/null
> +++ b/include/hw/s390x/ap-device.h
> @@ -0,0 +1,38 @@
> +/*
> + * Adjunct Processor (AP) matrix device interfaces
> + *
> + * Copyright 2018 IBM Corp.
> + * Author(s): Tony Krowiak
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#ifndef HW_S390X_AP_DEVICE_H
> +#define HW_S390X_AP_DEVICE_H
> +
> +#define AP_DEVICE_TYPE "ap-device"
> +
> +typedef struct APDevice {
> +DeviceState parent_obj;
> +} APDevice;
> +
> +typedef struct APDeviceClass {
> +DeviceClass parent_class;
> +} APDeviceClass;
> +
> +static inline APDevice *to_ap_dev(DeviceState *dev)
> +{
> +return container_of(dev, APDevice, parent_obj);
> +}
> +
> +#define AP_DEVICE(obj) \
> +OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
> +
> +#define AP_DEVICE_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
> +
> +#define AP_DEVICE_CLASS(klass) \
> +OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
> >>>
> >>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
> >
> > Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> > patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> > AP_DEVICE_CLASS(klass), but aren't those typically included in all
> > QOM definitions?
>
> Yes, we usually add all of them although only some might actually be
> used. (adding a new device usually looks like filling out a template)
Much of this seems to be boilerplate in this case, and I'm not sure how
much sense it makes. On the plus side, however, it looks like
everything else :)
So, I would merge both a complete version or a
stripped-down-to-the-needed version, unless someone else has a strong
argument.
[Qemu-devel] [PULL 30/33] target/arm: Add v8M stack checks for Thumb push/pop
Add v8M stack checks for the 16-bit Thumb push/pop
encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.
Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
Message-id: 20181002163556.10279-12-peter.mayd...@linaro.org
---
target/arm/translate.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 65df8d6975c..ef64d2559de 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12251,7 +12251,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
store_reg(s, rd, tmp);
break;
case 4: case 5: case 0xc: case 0xd:
-/* push/pop */
+/*
+ * 0b1011_x10x__
+ * - push/pop
+ */
addr = load_reg(s, 13);
if (insn & (1 << 8))
offset = 4;
@@ -12264,6 +12267,17 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
if ((insn & (1 << 11)) == 0) {
tcg_gen_addi_i32(addr, addr, -offset);
}
+
+if (s->v8m_stackcheck) {
+/*
+ * Here 'addr' is the lower of "old SP" and "new SP";
+ * if this is a pop that starts below the limit and ends
+ * above it, it is UNKNOWN whether the limit check triggers;
+ * we choose to trigger.
+ */
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+
for (i = 0; i < 8; i++) {
if (insn & (1 << i)) {
if (insn & (1 << 11)) {
--
2.19.0
Re: [Qemu-devel] [PATCH v1 00/12] arm: Add first models of Xilinx Versal SoC
On 3 October 2018 at 16:07, Edgar E. Iglesias wrote: > In QEMU we'd like to have a virtual developer board with the Versal SoC > and a selected set of peripherals under the control of QEMU. > We'd like to gradually extend this board as QEMU gains more support > for Versal hardware components. QEMU will generate a device-tree > describing only the components it supports and includes in the virtual > dev board. So, the SoC implementation and the GEM and HVC bugfix patches here are straightforward. What I'm less sure about is the "virtual" nature of the board model. What do we gain doing this rather than just modelling some particular Versal dev board? At the moment we have a fairly clear distinction: * most machine models are models of real hardware, and the real hardware is the litmus test for how things are supposed to work (and, like real hardware, the user provides the DTB) * the "virt" board is special, because it is purely virtual and contains only a few specific devices, so it can run Linux guests This would seem to be an odd hybrid, with an SoC that's a model of real hardware but also some virtual "QEMU controls what's present and creates the dtb" aspects. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 6/7] target/mips: Add opcodes for nanoMIPS EVA instructions
> From: Dimitrije Nikolic > > Add opcodes for nanoMIPS EVA instructions: CACHEE, LBE, LBUE, LHE, LHUE, LLE, LLWPE, LWE, PREFE, SBE, SCE, SCWPE, SHE, SWE. > > Signed-off-by: Aleksandar Markovic This patch is incomplete. The pool P.LS.E0 has three subpools, and, for each of them, corresponding opcodes should be created. Thanks, Aleksandar
[Qemu-devel] [PULL 33/33] hw/display/bcm2835_fb: Silence Coverity warning about multiply overflow
Coverity complains (CID 1395628) that the multiply in the calculation
of the framebuffer base is performed as 32x32 but then used in a
context that takes a 64-bit hwaddr. This can't actually ever
overflow the 32-bit result, because of the constraints placed on
the s->config values in bcm2835_fb_validate_config(). But we
can placate Coverity anyway, by explicitly casting one of the
inputs to a hwaddr, so the whole expression is calculated with
64-bit arithmetic.
Signed-off-by: Peter Maydell
Reviewed-by: Paolo Bonzini
Message-id: 20181005133012.26490-1-peter.mayd...@linaro.org
---
hw/display/bcm2835_fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index d534d00a65f..599863e4e17 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -190,7 +190,7 @@ static void fb_update_display(void *opaque)
}
if (s->invalidate) {
-hwaddr base = s->config.base + xoff + yoff * src_width;
+hwaddr base = s->config.base + xoff + (hwaddr)yoff * src_width;
framebuffer_update_memory_section(>fbsection, s->dma_mr,
base,
s->config.yres, src_width);
--
2.19.0
Re: [Qemu-devel] [qemu-s390x] [PATCH v9 4/6] s390x/ap: base Adjunct Processor (AP) object model
On 08/10/2018 16:20, Tony Krowiak wrote:
> On 09/27/2018 08:52 AM, Cornelia Huck wrote:
>> On Thu, 27 Sep 2018 14:29:01 +0200
>> Thomas Huth wrote:
>>
>>> On 2018-09-27 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Introduces the base object model for virtualizing AP devices.
Signed-off-by: Tony Krowiak
---
>>
+typedef struct APBridge {
+SysBusDevice sysbus_dev;
+bool css_dev_path;
>>>
>>> What is this css_dev_path variable good for? I don't see it used in any
>>> of the other patches?
>>> If you don't need it, I think you could get rid of this struct completely?
>>
>> Huh, now I remember complaining about it before. Looks like a
>> copy-and-paste from the css bridge; that variable is used for compat
>> handling there (and should be ditched here).
>>
>>>
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct APBus {
+BusState parent_obj;
+} APBus;
+
+#define TYPE_AP_BUS "ap-bus"
+#define AP_BUS(obj) \
+ OBJECT_CHECK(APBus, (obj), TYPE_AP_BUS)
>>>
>>> I think you could also get rid of AP_BRIDGE(), AP_BUS() and maybe even
>>> struct APBus.
>>
>> If there's nothing interesting to put in these inherited structures,
>> probably yes.
>>
>>>
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
new file mode 100644
index ..693df90cc041
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE "ap-device"
+
+typedef struct APDevice {
+DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
>>>
>>> Do you really need any of these definitions except AP_DEVICE_TYPE ?
>
> Yes, we need AP_DEVICE(obj) and struct APDevice; they are both used in
> patch 5/6. We can probably get rid of AP_DEVICE_GET_CLASS(obj) and
> AP_DEVICE_CLASS(klass), but aren't those typically included in all
> QOM definitions?
Yes, we usually add all of them although only some might actually be
used. (adding a new device usually looks like filling out a template)
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler
On Mon, 8 Oct 2018 14:41:50 +0200 David Hildenbrand wrote: > On 08/10/2018 14:19, Igor Mammedov wrote: > > On Mon, 8 Oct 2018 13:47:53 +0200 > > David Hildenbrand wrote: > > > >>> That way using [2] and [1 - modulo it should match only concrete type] > >>> machine would be able to override hotplug handlers for > >>> TYPE_VIRTIO_PMEM_PCI > >>> and explicitly call machine + pci hotplug handlers in necessary order. > >>> > >>> flow would look like: > >>> [acpi|shcp|native pci-e eject]-> > >>>hotplug_ctrl = qdev_get_hotplug_handler(dev); > >>>hotplug_handler_unplug(hotplug_ctrl, dev, _err); -> > >>> machine_unplug() > >>>machine_virtio_pci_pmem_cb(): > >>> // we now that's device has 2 stage hotplug handlers, > >>> // so we can arrange hotplug sequence in necessary order > >>> hotplug_ctrl2 = qdev_get_bus_hotplug_handler(dev); > >>> > >>> //then do unplug in whatever order that's correct, > >>> // I'd assume tear down/stop PCI device first, flushing > >>> // command virtio command queues and that unplug memory > >>> itself. > >>> hotplug_handler_unplug(hotplug_ctrl2, dev, _err); > >>> memory_device_unplug() > >>> > >> > >> Looking into the details, this order is not possible. The unplug will > >> essentially do a device_unparent() leading to the whole hierarchy > >> getting destroyed. The memory_device part always has to come first. > > > > Question here is if there are anything that should be handled first on > > virtio level before memory_device/pmem part is called? > > If there isn't it might be fine to swap the order of unplug sequence. > > > > Was asking myself the same thing, but as we are effectively holding the > iothread lock and the guest triggered the unplug, I guess it is fine to > unregister the memory region at this point. It looks the same to me but I'm not familiar with virtio or PCI. I'd ask Michael if it's safe thing to do.
