Re: [PATCH 1/2] char: Skip CLI aliases in query-chardev-backends
Kevin Wolf writes: > Am 12.11.2020 um 09:22 hat Markus Armbruster geschrieben: >> Kevin Wolf writes: >> >> > The aliases "tty" and "parport" are only valid on the command line, QMP >> > commands like chardev-add don't know them. query-chardev-backends should >> > describe QMP and therefore not include them in the list of available >> > backends. >> > >> > Signed-off-by: Kevin Wolf >> >> I'd call that a bug. > > Which is why I'm fixing it, separate from the deprecation. Thank you :) >> In the light of PATCH 2, I propose to put that one first (with the >> help_string_append() hunk dropped), then remove the aliases from CLI >> help, too, like this: [...] > > Going one step back without thinking in solutions immediately, what > you're suggesting is that deprecated options should become undocumented > instead of just annotated as deprecated? "Should become" is open to debate. "Have become" is a fact: 3478eae990 "qemu-options: Deprecate -nodefconfig", 2017-10-09 80f52a6694 "Deprecate -M command line options", 2011-07-23 and more. I don't see the why *help* output should mention deprecated options. It's distracting. Tell me how to use this thing, not how I could use it, but really shouldn't. Mentioning them in the reference manual may have value. Covering them in deprecated.rst is of course mandatory.
[PATCH 05/10] ui: Improve a client_migrate_info error message
client_migrate_info reports spice_server_migrate_connect() failure as
"An undefined error has occurred". Improve to "Could not set up
display for migration".
QERR_UNDEFINED_ERROR is now unused. Drop.
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
monitor/misc.c| 2 +-
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index d8267129bc..596fce0c54 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -65,9 +65,6 @@
#define QERR_REPLAY_NOT_SUPPORTED \
"Record/replay feature is not supported for '%s'"
-#define QERR_UNDEFINED_ERROR \
-"An undefined error has occurred"
-
#define QERR_UNSUPPORTED \
"this feature or command is not currently supported"
diff --git a/monitor/misc.c b/monitor/misc.c
index 9aa2505cfa..f0d3d41753 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -441,7 +441,7 @@ void qmp_client_migrate_info(const char *protocol, const
char *hostname,
has_port ? port : -1,
has_tls_port ? tls_port : -1,
cert_subject)) {
-error_setg(errp, QERR_UNDEFINED_ERROR);
+error_setg(errp, "Could not set up display for migration");
return;
}
return;
--
2.26.2
[PATCH 00/10] Chipping away at qerror.h
Obviously not for 5.2. Please review anyway.
Markus Armbruster (10):
qerror: Drop unused QERR_ macros
qerror: Eliminate QERR_ macros used in just one place
block: Improve some block-commit, block-stream error messages
ui: Improve some set_passwd, expire_password error messages
ui: Improve a client_migrate_info error message
ui: Tweak a client_migrate_info error message
qga: Replace an unreachable error by abort()
qga: Tweak a guest-shutdown error message
qom: Improve {qom,device}-list-properties error messages
Tweak a few "Parameter 'NAME' expects THING" error message
include/qapi/qmp/qerror.h| 23 ---
block/quorum.c | 2 +-
blockdev.c | 17 --
chardev/char.c | 2 +-
hw/core/qdev-properties-system.c | 2 +-
monitor/misc.c | 12 +-
monitor/qmp-cmds.c | 38 +---
net/net.c| 2 +-
qga/commands-win32.c | 5 ++---
qom/qom-qmp-cmds.c | 17 +-
softmmu/qdev-monitor.c | 4 ++--
tests/qemu-iotests/040 | 12 +-
12 files changed, 51 insertions(+), 85 deletions(-)
--
2.26.2
[PATCH 01/10] qerror: Drop unused QERR_ macros
QERR_INVALID_BLOCK_FORMAT is dead since commit e6641719fe "block: Always pass NULL as drv for bdrv_open()", 2015-09-14. QERR_INVALID_PASSWORD is dead since commit c01c214b69 "block: remove all encryption handling APIs", 2017-07-11. Bury them. Signed-off-by: Markus Armbruster --- include/qapi/qmp/qerror.h | 6 -- 1 file changed, 6 deletions(-) diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 7c76e24aa7..3eabd451d8 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -43,9 +43,6 @@ #define QERR_FEATURE_DISABLED \ "The feature '%s' is not enabled" -#define QERR_INVALID_BLOCK_FORMAT \ -"Invalid block format '%s'" - #define QERR_INVALID_PARAMETER \ "Invalid parameter '%s'" @@ -55,9 +52,6 @@ #define QERR_INVALID_PARAMETER_VALUE \ "Parameter '%s' expects %s" -#define QERR_INVALID_PASSWORD \ -"Password incorrect" - #define QERR_IO_ERROR \ "An IO error has occurred" -- 2.26.2
[PATCH 03/10] block: Improve some block-commit, block-stream error messages
block-commit defaults @base-node to the deepest backing image. When
there is none, it fails with "Base 'NULL' not found". Improve to
"There is no backing image".
block-commit and block-stream reject a @base argument that doesn't
resolve with "Base 'BASE' not found". Commit 6b33f3ae8b "qemu-img:
Improve commit invalid base message" improved this message in
qemu-img. Improve it here, too: "Can't find '%s' in the backing
chain".
QERR_BASE_NOT_FOUND is now unused. Drop.
Cc: Kevin Wolf
Cc: Max Reitz
Cc: qemu-bl...@nongnu.org
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 2 --
blockdev.c| 15 +--
tests/qemu-iotests/040| 12 ++--
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index c272e3fc29..5d7e69cc1f 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -16,8 +16,6 @@
* These macros will go away, please don't use in new code, and do not
* add new ones!
*/
-#define QERR_BASE_NOT_FOUND \
-"Base '%s' not found"
#define QERR_BUS_NO_HOTPLUG \
"Bus '%s' does not support hotplugging"
diff --git a/blockdev.c b/blockdev.c
index fe6fb5dc1d..d05a8740f4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2531,7 +2531,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
if (has_base) {
base_bs = bdrv_find_backing_image(bs, base);
if (base_bs == NULL) {
-error_setg(errp, QERR_BASE_NOT_FOUND, base);
+error_setg(errp, "Can't find '%s' in the backing chain", base);
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
@@ -2703,13 +2703,16 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
}
} else if (has_base && base) {
base_bs = bdrv_find_backing_image(top_bs, base);
+if (base_bs == NULL) {
+error_setg(errp, "Can't find '%s' in the backing chain", base);
+goto out;
+}
} else {
base_bs = bdrv_find_base(top_bs);
-}
-
-if (base_bs == NULL) {
-error_setg(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-goto out;
+if (base_bs == NULL) {
+error_setg(errp, "There is no backimg image");
+goto out;
+}
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index caf286571a..dc6069edc0 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -156,7 +156,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top='%s' %
backing_img, base='%s' % backing_img)
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' %
backing_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing
chain" % backing_img)
def test_top_invalid(self):
self.assert_no_active_block_jobs()
@@ -168,7 +168,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top='%s' %
mid_img, base='badfile')
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
+self.assert_qmp(result, 'error/desc', "Can't find 'badfile' in the
backing chain")
def test_top_node_invalid(self):
self.assert_no_active_block_jobs()
@@ -208,7 +208,7 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top='%s' %
backing_img, base='%s' % mid_img)
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' %
mid_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing
chain" % mid_img)
def test_top_and_base_node_reversed(self):
self.assert_no_active_block_jobs()
@@ -349,7 +349,7 @@ class TestRelativePaths(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top='%s' %
self.mid_img, base='%s' % self.mid_img)
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' %
self.mid_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing
chain" % self.mid_img)
def test_top_invalid(self):
self.assert_no_active_block_jobs()
@@ -361,7 +361,7 @@ class TestRelativePaths(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top='%s' %
self.mid_img, bas
[PATCH 07/10] qga: Replace an unreachable error by abort()
check_suspend_mode()'s error message Parameter 'mode' expects GuestSuspendMode makes no sense to users: GuestSuspendMode is a C enum. Fortunately, it is unreachable. Replace it by abort(). Cc: Michael Roth Signed-off-by: Markus Armbruster --- qga/commands-win32.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 300b87c859..87dc43e837 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1441,8 +1441,7 @@ static void check_suspend_mode(GuestSuspendMode mode, Error **errp) } break; default: -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "mode", - "GuestSuspendMode"); +abort(); } } -- 2.26.2
[PATCH 09/10] qom: Improve {qom, device}-list-properties error messages
qom-list-properties reports
Parameter 'typename' expects device
when @typename exists, but isn't a TYPE_DEVICE. Improve this to
Parameter 'typename' expects a non-abstract device type
device-list-properties reports
Parameter 'typename' expects object
when @typename exists, but isn't a TYPE_OBJECT. Improve this to
Parameter 'typename' expects a QOM type
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Signed-off-by: Markus Armbruster
---
qom/qom-qmp-cmds.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 310ab2d048..2dd233f293 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -138,15 +138,10 @@ ObjectPropertyInfoList *qmp_device_list_properties(const
char *typename,
return NULL;
}
-klass = object_class_dynamic_cast(klass, TYPE_DEVICE);
-if (klass == NULL) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
TYPE_DEVICE);
-return NULL;
-}
-
-if (object_class_is_abstract(klass)) {
+if (!object_class_dynamic_cast(klass, TYPE_DEVICE)
+|| object_class_is_abstract(klass)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
- "non-abstract device type");
+ "a non-abstract device type");
return NULL;
}
@@ -208,9 +203,9 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char
*typename,
return NULL;
}
-klass = object_class_dynamic_cast(klass, TYPE_OBJECT);
-if (klass == NULL) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
TYPE_OBJECT);
+if (!object_class_dynamic_cast(klass, TYPE_OBJECT)) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
+ "a QOM type");
return NULL;
}
--
2.26.2
[PATCH 08/10] qga: Tweak a guest-shutdown error message
Change
Parameter 'mode' expects halt|powerdown|reboot
to
Parameter 'mode' expects 'halt', 'powerdown', or 'reboot'
for consistency with similar error messages elsewhere.
Cc: Michael Roth
Signed-off-by: Markus Armbruster
---
qga/commands-win32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 87dc43e837..ba1fd07d06 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -334,7 +334,7 @@ void qmp_guest_shutdown(bool has_mode, const char *mode,
Error **errp)
shutdown_flag |= EWX_REBOOT;
} else {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "mode",
- "halt|powerdown|reboot");
+ "'halt', 'powerdown', or 'reboot'");
return;
}
--
2.26.2
[PATCH 10/10] Tweak a few "Parameter 'NAME' expects THING" error message
Change to "expects a THING" where that's an obvious improvement
Signed-off-by: Markus Armbruster
---
block/quorum.c | 2 +-
blockdev.c | 2 +-
chardev/char.c | 2 +-
hw/core/qdev-properties-system.c | 2 +-
softmmu/qdev-monitor.c | 4 ++--
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/quorum.c b/block/quorum.c
index e846a7e892..656a80f93a 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -856,7 +856,7 @@ static int quorum_valid_threshold(int threshold, int
num_children, Error **errp)
if (threshold < 1) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- "vote-threshold", "value >= 1");
+ "vote-threshold", "a value >= 1");
return -ERANGE;
}
diff --git a/blockdev.c b/blockdev.c
index d05a8740f4..6c7be7c522 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2991,7 +2991,7 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
}
if (granularity & (granularity - 1)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
- "power of 2");
+ "a power of 2");
return;
}
diff --git a/chardev/char.c b/chardev/char.c
index aa4282164a..a9b8c5a9aa 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -521,7 +521,7 @@ static const ChardevClass *char_get_class(const char
*driver, Error **errp)
if (object_class_is_abstract(oc)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
- "abstract device type");
+ "an abstract device type");
return NULL;
}
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index b81a4e8d14..93061b5bf1 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -776,7 +776,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, const
char *name,
}
if (value < -1 || value > 255) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "pci_devfn");
+ name ? name : "null", "a value between -1 and 255");
return;
}
*ptr = value;
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index bcfb90a08f..08318c5d9d 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -237,7 +237,7 @@ static DeviceClass *qdev_get_device_class(const char
**driver, Error **errp)
if (object_class_is_abstract(oc)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
- "non-abstract device type");
+ "a non-abstract device type");
return NULL;
}
@@ -245,7 +245,7 @@ static DeviceClass *qdev_get_device_class(const char
**driver, Error **errp)
if (!dc->user_creatable ||
(qdev_hotplug && !dc->hotpluggable)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
- "pluggable device type");
+ "a pluggable device type");
return NULL;
}
--
2.26.2
[PATCH 02/10] qerror: Eliminate QERR_ macros used in just one place
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 9 -
monitor/misc.c| 8
net/net.c | 2 +-
3 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 3eabd451d8..c272e3fc29 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -25,21 +25,12 @@
#define QERR_DEVICE_HAS_NO_MEDIUM \
"Device '%s' has no medium"
-#define QERR_DEVICE_INIT_FAILED \
-"Device '%s' could not be initialized"
-
#define QERR_DEVICE_IN_USE \
"Device '%s' is in use"
#define QERR_DEVICE_NO_HOTPLUG \
"Device '%s' does not support hotplugging"
-#define QERR_FD_NOT_FOUND \
-"File descriptor named '%s' not found"
-
-#define QERR_FD_NOT_SUPPLIED \
-"No file descriptor supplied via SCM_RIGHTS"
-
#define QERR_FEATURE_DISABLED \
"The feature '%s' is not enabled"
diff --git a/monitor/misc.c b/monitor/misc.c
index 32e6a8c13d..9aa2505cfa 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1232,7 +1232,7 @@ void qmp_getfd(const char *fdname, Error **errp)
fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
if (fd == -1) {
-error_setg(errp, QERR_FD_NOT_SUPPLIED);
+error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
return;
}
@@ -1286,7 +1286,7 @@ void qmp_closefd(const char *fdname, Error **errp)
}
qemu_mutex_unlock(&cur_mon->mon_lock);
-error_setg(errp, QERR_FD_NOT_FOUND, fdname);
+error_setg(errp, "File descriptor named '%s' not found", fdname);
}
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
@@ -1357,7 +1357,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t
fdset_id, bool has_opaque,
fd = qemu_chr_fe_get_msgfd(&mon->chr);
if (fd == -1) {
-error_setg(errp, QERR_FD_NOT_SUPPLIED);
+error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
goto error;
}
@@ -1410,7 +1410,7 @@ error:
} else {
snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64, fdset_id);
}
-error_setg(errp, QERR_FD_NOT_FOUND, fd_str);
+error_setg(errp, "File descriptor named '%s' not found", fd_str);
}
FdsetInfoList *qmp_query_fdsets(Error **errp)
diff --git a/net/net.c b/net/net.c
index 794c652282..5e1b57a608 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1008,7 +1008,7 @@ static int net_client_init1(const Netdev *netdev, bool
is_netdev, Error **errp)
if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) < 0)
{
/* FIXME drop when all init functions store an Error */
if (errp && !*errp) {
-error_setg(errp, QERR_DEVICE_INIT_FAILED,
+error_setg(errp, "Device '%s' could not be initialized",
NetClientDriver_str(netdev->type));
}
return -1;
--
2.26.2
[PATCH 04/10] ui: Improve some set_passwd, expire_password error messages
set_passwd and expire_password reject invalid "protocol" with "Invalid
parameter 'protocol'". Misleading; the parameter is valid, its value
isn't. Improve to "Parameter 'protocol' expects 'vnc' or 'spice'".
expire_password fails with "Could not set password". Misleading;
improve to "Could not set password expire time".
QERR_SET_PASSWD_FAILED is now unused. Drop.
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
monitor/qmp-cmds.c| 38 +++---
2 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 5d7e69cc1f..d8267129bc 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -65,9 +65,6 @@
#define QERR_REPLAY_NOT_SUPPORTED \
"Record/replay feature is not supported for '%s'"
-#define QERR_SET_PASSWD_FAILED \
-"Could not set password"
-
#define QERR_UNDEFINED_ERROR \
"An undefined error has occurred"
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index a08143b323..ffbf948d55 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -199,13 +199,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-if (rc != 0) {
-error_setg(errp, QERR_SET_PASSWD_FAILED);
-}
-return;
-}
-
-if (strcmp(protocol, "vnc") == 0) {
+} else if (strcmp(protocol, "vnc") == 0) {
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -214,13 +208,15 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-if (rc < 0) {
-error_setg(errp, QERR_SET_PASSWD_FAILED);
-}
+} else {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
+ "'vnc' or 'spice'");
return;
}
-error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
+if (rc != 0) {
+error_setg(errp, "Could not set password");
+}
}
void qmp_expire_password(const char *protocol, const char *whenstr,
@@ -244,28 +240,24 @@ void qmp_expire_password(const char *protocol, const char
*whenstr,
return;
}
rc = qemu_spice.set_pw_expire(when);
-if (rc != 0) {
-error_setg(errp, QERR_SET_PASSWD_FAILED);
-}
-return;
-}
-
-if (strcmp(protocol, "vnc") == 0) {
+} else if (strcmp(protocol, "vnc") == 0) {
rc = vnc_display_pw_expire(NULL, when);
-if (rc != 0) {
-error_setg(errp, QERR_SET_PASSWD_FAILED);
-}
+} else {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
+ "'vnc' or 'spice'");
return;
}
-error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
+if (rc != 0) {
+error_setg(errp, "Could not set password expire time");
+}
}
#ifdef CONFIG_VNC
void qmp_change_vnc_password(const char *password, Error **errp)
{
if (vnc_display_password(NULL, password) < 0) {
-error_setg(errp, QERR_SET_PASSWD_FAILED);
+error_setg(errp, "Could not set password");
}
}
--
2.26.2
[PATCH 06/10] ui: Tweak a client_migrate_info error message
Change Parameter 'protocol' expects spice to Parameter 'protocol' expects 'spice' for consistency with similar error messages elsewhere. Cc: Gerd Hoffmann Signed-off-by: Markus Armbruster --- monitor/misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monitor/misc.c b/monitor/misc.c index f0d3d41753..354ac87736 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -447,7 +447,7 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname, return; } -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", "spice"); +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", "'spice'"); } static void hmp_logfile(Monitor *mon, const QDict *qdict) -- 2.26.2
Re: [PATCH] Clean up includes
On 13/11/20 07:12, Markus Armbruster wrote:
Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.
This commit was created with scripts/clean-includes, with the changes
to the following files manually reverted:
contrib/plugins/, tests/plugin/, and tests/test-rcu-slist.c appear not
to include osdep.h intentionally. The remaining reverts are the same
as in commit bbfff19688d.
Acked-by: Paolo Bonzini
Paolo
Signed-off-by: Markus Armbruster
---
No real need to get this into 5.2 at this stage. No real risk either.
contrib/vhost-user-gpu/vugbm.h | 2 --
contrib/vhost-user-gpu/vugpu.h | 1 -
include/hw/block/swim.h | 1 -
include/hw/display/macfb.h | 1 -
include/qemu/nvdimm-utils.h | 1 -
tests/qtest/fuzz/fuzz.h | 1 -
tests/qtest/fuzz/generic_fuzz_configs.h | 1 -
tools/virtiofsd/fuse_common.h | 2 --
tools/virtiofsd/fuse_log.h | 1 -
tools/virtiofsd/fuse_lowlevel.h | 3 ---
tools/virtiofsd/fuse_misc.h | 1 -
tools/virtiofsd/passthrough_seccomp.h | 1 -
contrib/vhost-user-gpu/virgl.c | 1 +
contrib/vhost-user-gpu/vugbm.c | 1 +
contrib/vhost-user-input/main.c | 1 -
hw/display/artist.c | 1 -
hw/s390x/s390-pci-vfio.c| 3 ++-
tools/virtiofsd/buffer.c| 5 -
tools/virtiofsd/fuse_log.c | 2 --
tools/virtiofsd/fuse_lowlevel.c | 10 --
tools/virtiofsd/fuse_opt.c | 4
tools/virtiofsd/fuse_signals.c | 5 -
tools/virtiofsd/fuse_virtio.c | 10 --
tools/virtiofsd/helper.c| 8
tools/virtiofsd/passthrough_ll.c| 12
tools/virtiofsd/passthrough_seccomp.c | 3 ---
util/nvdimm-utils.c | 1 +
27 files changed, 5 insertions(+), 78 deletions(-)
diff --git a/contrib/vhost-user-gpu/vugbm.h b/contrib/vhost-user-gpu/vugbm.h
index 07e698fcd7..66f1520764 100644
--- a/contrib/vhost-user-gpu/vugbm.h
+++ b/contrib/vhost-user-gpu/vugbm.h
@@ -10,10 +10,8 @@
#ifndef VHOST_USER_GPU_VUGBM_H
#define VHOST_USER_GPU_VUGBM_H
-#include "qemu/osdep.h"
#ifdef CONFIG_MEMFD
-#include
#include
#endif
diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
index 3153c9a6de..5eca2a96ab 100644
--- a/contrib/vhost-user-gpu/vugpu.h
+++ b/contrib/vhost-user-gpu/vugpu.h
@@ -15,7 +15,6 @@
#ifndef VUGPU_H
#define VUGPU_H
-#include "qemu/osdep.h"
#include "contrib/libvhost-user/libvhost-user-glib.h"
#include "standard-headers/linux/virtio_gpu.h"
diff --git a/include/hw/block/swim.h b/include/hw/block/swim.h
index 5a49029543..c1bd5f6555 100644
--- a/include/hw/block/swim.h
+++ b/include/hw/block/swim.h
@@ -11,7 +11,6 @@
#ifndef SWIM_H
#define SWIM_H
-#include "qemu/osdep.h"
#include "hw/sysbus.h"
#include "qom/object.h"
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index c133fa271e..80806b0306 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -13,7 +13,6 @@
#ifndef MACFB_H
#define MACFB_H
-#include "qemu/osdep.h"
#include "exec/memory.h"
#include "ui/console.h"
#include "qom/object.h"
diff --git a/include/qemu/nvdimm-utils.h b/include/qemu/nvdimm-utils.h
index 4b8b198ba7..5f45774c2c 100644
--- a/include/qemu/nvdimm-utils.h
+++ b/include/qemu/nvdimm-utils.h
@@ -1,7 +1,6 @@
#ifndef NVDIMM_UTILS_H
#define NVDIMM_UTILS_H
-#include "qemu/osdep.h"
GSList *nvdimm_get_device_list(void);
#endif
diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
index 08e9560a79..3a8570e84c 100644
--- a/tests/qtest/fuzz/fuzz.h
+++ b/tests/qtest/fuzz/fuzz.h
@@ -14,7 +14,6 @@
#ifndef FUZZER_H_
#define FUZZER_H_
-#include "qemu/osdep.h"
#include "qemu/units.h"
#include "qapi/error.h"
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index c4d925f9e6..b4c5fefeca 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -13,7 +13,6 @@
#ifndef GENERIC_FUZZ_CONFIGS_H
#define GENERIC_FUZZ_CONFIGS_H
-#include "qemu/osdep.h"
typedef struct generic_fuzz_config {
const char *name, *args, *objects;
diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index 5aee5193eb..30b18b4966 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -18,8 +18,6 @@
#include "fuse_log.h"
#include "fuse_opt.h"
-#include
-#include
/** Major version of FUSE library interface */
#define FUSE_MAJOR_VERSION 3
diff --git a/tools/virtiofsd/fuse_log.h b/tools/virtiofsd/fuse_log.h
index bf6c11ff11..8d7091bd4d 100644
--- a/tools/virtiofsd/fuse_log.h
+++ b/tools/virtiofsd/fuse_log.h
@@ -14,7 +14,6 @@
* This file defines the
Re: [PATCH 09/10] qom: Improve {qom,device}-list-properties error messages
On 13/11/20 09:26, Markus Armbruster wrote: qom-list-properties reports Parameter 'typename' expects device when @typename exists, but isn't a TYPE_DEVICE. Improve this to Parameter 'typename' expects a non-abstract device type device-list-properties reports Parameter 'typename' expects object when @typename exists, but isn't a TYPE_OBJECT. Improve this to Parameter 'typename' expects a QOM type Silly mistake: device-list-properties and qom-list-properties are exchanged in the commit message. Can be fixed without reposting. Paolo
Re: [PATCH] Clean up includes
* Markus Armbruster (arm...@redhat.com) wrote: > Clean up includes so that osdep.h is included first and headers > which it implies are not included manually. > > This commit was created with scripts/clean-includes, with the changes > to the following files manually reverted: > > contrib/libvhost-user/libvhost-user-glib.h > contrib/libvhost-user/libvhost-user.c > contrib/libvhost-user/libvhost-user.h > contrib/plugins/hotblocks.c > contrib/plugins/hotpages.c > contrib/plugins/howvec.c > contrib/plugins/lockstep.c > linux-user/mips64/cpu_loop.c > linux-user/mips64/signal.c > linux-user/sparc64/cpu_loop.c > linux-user/sparc64/signal.c > linux-user/x86_64/cpu_loop.c > linux-user/x86_64/signal.c > target/s390x/gen-features.c > tests/fp/platform.h > tests/migration/s390x/a-b-bios.c > tests/plugin/bb.c > tests/plugin/empty.c > tests/plugin/insn.c > tests/plugin/mem.c > tests/test-rcu-simpleq.c > tests/test-rcu-slist.c > tests/test-rcu-tailq.c > tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c > > contrib/plugins/, tests/plugin/, and tests/test-rcu-slist.c appear not > to include osdep.h intentionally. The remaining reverts are the same > as in commit bbfff19688d. > > Signed-off-by: Markus Armbruster Yeh that looks ok for virtiofsd I think. Acked-by: Dr. David Alan Gilbert > --- > No real need to get this into 5.2 at this stage. No real risk either. > > contrib/vhost-user-gpu/vugbm.h | 2 -- > contrib/vhost-user-gpu/vugpu.h | 1 - > include/hw/block/swim.h | 1 - > include/hw/display/macfb.h | 1 - > include/qemu/nvdimm-utils.h | 1 - > tests/qtest/fuzz/fuzz.h | 1 - > tests/qtest/fuzz/generic_fuzz_configs.h | 1 - > tools/virtiofsd/fuse_common.h | 2 -- > tools/virtiofsd/fuse_log.h | 1 - > tools/virtiofsd/fuse_lowlevel.h | 3 --- > tools/virtiofsd/fuse_misc.h | 1 - > tools/virtiofsd/passthrough_seccomp.h | 1 - > contrib/vhost-user-gpu/virgl.c | 1 + > contrib/vhost-user-gpu/vugbm.c | 1 + > contrib/vhost-user-input/main.c | 1 - > hw/display/artist.c | 1 - > hw/s390x/s390-pci-vfio.c| 3 ++- > tools/virtiofsd/buffer.c| 5 - > tools/virtiofsd/fuse_log.c | 2 -- > tools/virtiofsd/fuse_lowlevel.c | 10 -- > tools/virtiofsd/fuse_opt.c | 4 > tools/virtiofsd/fuse_signals.c | 5 - > tools/virtiofsd/fuse_virtio.c | 10 -- > tools/virtiofsd/helper.c| 8 > tools/virtiofsd/passthrough_ll.c| 12 > tools/virtiofsd/passthrough_seccomp.c | 3 --- > util/nvdimm-utils.c | 1 + > 27 files changed, 5 insertions(+), 78 deletions(-) > > diff --git a/contrib/vhost-user-gpu/vugbm.h b/contrib/vhost-user-gpu/vugbm.h > index 07e698fcd7..66f1520764 100644 > --- a/contrib/vhost-user-gpu/vugbm.h > +++ b/contrib/vhost-user-gpu/vugbm.h > @@ -10,10 +10,8 @@ > #ifndef VHOST_USER_GPU_VUGBM_H > #define VHOST_USER_GPU_VUGBM_H > > -#include "qemu/osdep.h" > > #ifdef CONFIG_MEMFD > -#include > #include > #endif > > diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h > index 3153c9a6de..5eca2a96ab 100644 > --- a/contrib/vhost-user-gpu/vugpu.h > +++ b/contrib/vhost-user-gpu/vugpu.h > @@ -15,7 +15,6 @@ > #ifndef VUGPU_H > #define VUGPU_H > > -#include "qemu/osdep.h" > > #include "contrib/libvhost-user/libvhost-user-glib.h" > #include "standard-headers/linux/virtio_gpu.h" > diff --git a/include/hw/block/swim.h b/include/hw/block/swim.h > index 5a49029543..c1bd5f6555 100644 > --- a/include/hw/block/swim.h > +++ b/include/hw/block/swim.h > @@ -11,7 +11,6 @@ > #ifndef SWIM_H > #define SWIM_H > > -#include "qemu/osdep.h" > #include "hw/sysbus.h" > #include "qom/object.h" > > diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h > index c133fa271e..80806b0306 100644 > --- a/include/hw/display/macfb.h > +++ b/include/hw/display/macfb.h > @@ -13,7 +13,6 @@ > #ifndef MACFB_H > #define MACFB_H > > -#include "qemu/osdep.h" > #include "exec/memory.h" > #include "ui/console.h" > #include "qom/object.h" > diff --git a/include/qemu/nvdimm-utils.h b/include/qemu/nvdimm-utils.h > index 4b8b198ba7..5f45774c2c 100644 > --- a/include/qemu/nvdimm-utils.h > +++ b/include/qemu/nvdimm-utils.h > @@ -1,7 +1,6 @@ > #ifndef NVDIMM_UTILS_H > #define NVDIMM_UTILS_H > > -#include "qemu/osdep.h" > > GSList *nvdimm_get_device_list(void); > #endif > diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h > index 08e9560a79..3a8570e84c 100644 > --- a/tests/qtest/fuzz/fuzz.h > +++ b/tests/qtest/fuzz/fuzz.h > @@ -14,7 +14,6 @@ > #ifndef FUZZER_H_ > #define FUZZER_H_ > > -#include "qemu/osdep.h" >
Re: [PATCH] Clean up includes
On 13/11/2020 07.12, Markus Armbruster wrote: > Clean up includes so that osdep.h is included first and headers > which it implies are not included manually. > > This commit was created with scripts/clean-includes, with the changes > to the following files manually reverted: > > contrib/libvhost-user/libvhost-user-glib.h > contrib/libvhost-user/libvhost-user.c > contrib/libvhost-user/libvhost-user.h > contrib/plugins/hotblocks.c > contrib/plugins/hotpages.c > contrib/plugins/howvec.c > contrib/plugins/lockstep.c > linux-user/mips64/cpu_loop.c > linux-user/mips64/signal.c > linux-user/sparc64/cpu_loop.c > linux-user/sparc64/signal.c > linux-user/x86_64/cpu_loop.c > linux-user/x86_64/signal.c > target/s390x/gen-features.c > tests/fp/platform.h > tests/migration/s390x/a-b-bios.c > tests/plugin/bb.c > tests/plugin/empty.c > tests/plugin/insn.c > tests/plugin/mem.c > tests/test-rcu-simpleq.c > tests/test-rcu-slist.c > tests/test-rcu-tailq.c > tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c > > contrib/plugins/, tests/plugin/, and tests/test-rcu-slist.c appear not > to include osdep.h intentionally. The remaining reverts are the same > as in commit bbfff19688d. > > Signed-off-by: Markus Armbruster > --- > No real need to get this into 5.2 at this stage. No real risk either. Seems to compile fine: https://gitlab.com/huth/qemu/-/pipelines/215598144 https://cirrus-ci.com/build/6127772462481408 https://travis-ci.com/github/huth/qemu/builds/201194948 Tested-by: Thomas Huth
Re: [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()
Am 13.11.2020 um 12:13 hat lichun geschrieben:
> monitor_parse_arguments() also need to known the current monitoar:
> (gdb) bt
> #0 0x55ac6a6d in mon_get_cpu_sync (mon=0x0,
> synchronize=synchronize@entry=true) at ../monitor/misc.c:270
> #1 0x55ac6b4a in mon_get_cpu () at ../monitor/misc.c:294
> #2 0x55ac80fd in get_monitor_def (pval=pval@entry=0x7fffcc78,
> name=name@entry=0x7fffcc80 "pc") at ../monitor/misc.c:1669
> #3 0x5583fa8a in expr_unary (mon=mon@entry=0x568a75a0) at
> ../monitor/hmp.c:387
> #4 0x5583fb32 in expr_prod (mon=mon@entry=0x568a75a0) at
> ../monitor/hmp.c:421
> #5 0x5583fbcc in expr_logic (mon=mon@entry=0x568a75a0) at
> ../monitor/hmp.c:455
> #6 0x5583f82c in expr_sum (mon=mon@entry=0x568a75a0) at
> ../monitor/hmp.c:484
> #7 0x5583fc97 in get_expr (mon=mon@entry=0x568a75a0,
> pval=pval@entry=0x7fffce18, pp=pp@entry=0x7fffce08) at
> ../monitor/hmp.c:511
> #8 0x558409b1 in monitor_parse_arguments
> (mon=mon@entry=0x568a75a0, cmd=0x56561e40 ,
> cmd=0x56561e40 , endp=0x7fffd288) at
> ../monitor/hmp.c:876
> #9 0x55841796 in handle_hmp_command (mon=mon@entry=0x568a75a0,
> cmdline=0x568b12b3 "$pc", cmdline@entry=0x568b12b0 "xp $pc") at
> ../monitor/hmp.c:1073
> Therefore update current monitor as soon as possible to avoid
> hmp/xp command failure.
>
> Fixes: ff04108a0e36 ("hmp: Update current monitor only in
> handle_hmp_command()")
> Signed-off-by: lichun
> ---
> monitor/hmp.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index c5cd9d3..ee5413e 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1072,52 +1072,52 @@ static void handle_hmp_command_co(void *opaque)
> }
>
> void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> {
> QDict *qdict;
> const HMPCommand *cmd;
> const char *cmd_start = cmdline;
>
> trace_handle_hmp_command(mon, cmdline);
>
> +/* old_mon is non-NULL when called from qmp_human_monitor_command() */
> +Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common);
> +
> cmd = monitor_parse_command(mon, cmdline, &cmdline, hmp_cmds);
> if (!cmd) {
> return;
> }
Now the monitor isn't changed back in all early return cases.
>
> qdict = monitor_parse_arguments(&mon->common, &cmdline, cmd);
> if (!qdict) {
> while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> cmdline--;
> }
> monitor_printf(&mon->common, "Try \"help %.*s\" for more
> information\n",
> (int)(cmdline - cmd_start), cmd_start);
> return;
> }
>
> if (!cmd->coroutine) {
> -/* old_mon is non-NULL when called from qmp_human_monitor_command()
> */
> -Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(),
> &mon->common);
> cmd->cmd(&mon->common, qdict);
> -monitor_set_cur(qemu_coroutine_self(), old_mon);
> } else {
> HandleHmpCommandCo data = {
> .mon = &mon->common,
> .cmd = cmd,
> .qdict = qdict,
> .done = false,
> };
> Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> -monitor_set_cur(co, &mon->common);
Removing this line is wrong, we still need to set the current monitor
for co, which is not qemu_coroutine_self() self.
> aio_co_enter(qemu_get_aio_context(), co);
> AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> }
> +monitor_set_cur(qemu_coroutine_self(), old_mon);
>
> qobject_unref(qdict);
> }
With the above bugs fixed, this approach is one option to fix the bug.
Personally, if it's possible with reasonable effort, I would prefer the
other way, which is making sure that monitor_cur() isn't used, but the
Monitor pointer is just passed down. This would be a bigger change, but
it wouldn't only fix the bug, but also clean up the code and make it
more maintainable.
I can try to write a patch series to do it this way and see how it goes.
Kevin
RE: [PATCH v6] introduce vfio-user protocol specification
> +Version Data Format > +^^^ > + > +The version data is an optional JSON byte array with the following format: > + > ++--+--+---+ > +| Name | Type | Description > | > ++==+==+== > =+ > +| capabilities | collection of| Contains common capabilities that the > | > +| | name/value pairs | sender supports. Optional. > | > ++--+--+---+ > + > +Capabilities: > + > ++---+--+--+ > +| Name | Type | Description > | > ++===+==+= > =+ > +| ``max_fds`` | number | Maximum number of file descriptors that > | > +| | | can be received by the sender. > Optional. | > +| | | If not specified then the receiver must > | > +| | | assume ``max_fds=1``. > | > ++---+--+--+ > +| ``migration`` | collection of| Migration capability parameters. If > | > +| | name/value pairs | missing then migration is not supported > | > +| | | by the sender. > | > ++---+--+--+ Can we introduce the following parameters that can help simplify the server implementation? * max_dma_regions: maximum number of DMA regions per VFIO_USER_DMA_MAP/UNMAP message * max_dirty_bitmaps: maximum number of dirty bitmaps requested per VFIO_USER_DIRTY_PAGES message If these parameters are missing then they can be unlimited. The vfio-user client will have to break a single operation into multiple messages. > +VFIO_USER_DEVICE_GET_REGION_INFO > + > + > +Message format > +^^ > + > ++--++ > +| Name | Value | > ++==++ > +| Message ID || > ++--++ > +| Command | 5 | > ++--++ > +| Message size | 48 + any caps | > ++--++ > +| Flags| Reply bit set in reply | > ++--++ > +| Error| 0/errno| > ++--++ > +| Region info | VFIO region info | > ++--++ > + > +This command message is sent by the client to the server to query for > +information about device memory regions. The VFIO region info structure is > +defined in (``struct vfio_region_info``). Since the client > +does not know the size of the capabilities, the size of the reply it should > +expect is 48 plus any capabilities whose size is indicated in the size field > of > +the reply header. > + > +VFIO region info format > +^^^ > + > ++++--+ > +| Name | Offset | Size | > ++++==+ > +| argsz | 16 | 4| > ++++--+ > +| flags | 20 | 4| > ++++--+ > +|| +-+-+ | > +|| | Bit | Definition | | > +|| +=+=+ | > +|| | 0 | VFIO_REGION_INFO_FLAG_READ | | > +|| +-+-+ | > +|| | 1 | VFIO_REGION_INFO_FLAG_WRITE | | > +|| +-+-+ | > +|| | 2 | VFIO_REGION_INFO_FLAG_MMAP | | > +|| +-+-+ | > +|| | 3 | VFIO_REGION_INFO_FLAG_CAPS | | > +|| +-+-+ | > ++++--+ > +| index | 24 | 4| > ++++--+ > +| cap_offset | 28 | 4| > ++++--+ > +| size | 32 | 8| > ++++--+ > +| offset | 40 | 8| > ++-
Re: [PATCH 2/2] authz-list-file: Improve an error message
On Fri, Nov 13, 2020 at 07:23:58AM +0100, Markus Armbruster wrote:
> When qauthz_list_file_load() rejects JSON values other than JSON
> object with a rather confusing error message:
>
> $ echo 1 | qemu-system-x86_64 -nodefaults -S -display none -object
> authz-list-file,id=authz0,filename=/dev/stdin
> qemu-system-x86_64: -object
> authz-list-file,id=authz0,filename=/dev/stdin: Invalid parameter type for
> 'obj', expected: dict
>
> Improve to
>
> qemu-system-x86_64: -object
> authz-list-file,id=authz0,filename=/dev/stdin: File '/dev/stdin' must contain
> a JSON object
>
> Signed-off-by: Markus Armbruster
> ---
> authz/listfile.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/authz/listfile.c b/authz/listfile.c
> index 1421e674a4..da3a0e69a2 100644
> --- a/authz/listfile.c
> +++ b/authz/listfile.c
> @@ -73,7 +73,8 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
>
> pdict = qobject_to(QDict, obj);
> if (!pdict) {
> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "obj", "dict");
> +error_setg(errp, "File '%s' must contain a JSON object",
> + fauthz->filename);
This code pattern was copied from other places in QEMU which use the
same QERR_INVALID_PARAMETER_TYPE. There are another 10 or so examples
of this error message pattern.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 1/2] authz-list-file: Fix file read error handling
On Fri, Nov 13, 2020 at 07:23:57AM +0100, Markus Armbruster wrote: > The Error ** argument must be NULL, &error_abort, &error_fatal, or a > pointer to a variable containing NULL. Passing an argument of the > latter kind twice without clearing it in between is wrong: if the > first call sets an error, it no longer points to NULL for the second > call. > > qauthz_list_file_complete() is wrong that way: it passes @errp to > qauthz_list_file_complete() without checking for failure. If it runs > into another failure, it trips error_setv()'s assertion. Reproducer: > > $ qemu-system-x86_64 -nodefaults -S -display none -object > authz-list-file,id=authz0,filename= > qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == > NULL' failed. > Aborted (core dumped) > > Fix it to check for failure. > > Fixes: 55d869846de802a16af1a50584c51737bd664387 > Signed-off-by: Markus Armbruster > --- > authz/listfile.c | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PULL 8/8] target/mips: Preparing for adding MMI instructions
Hi Mateja,
(Cc'ing Fredrik)
On 2/27/19 3:00 PM, Aleksandar Markovic wrote:
> From: Mateja Marjanovic
>
> Set up MMI code to be compiled only for TARGET_MIPS64. This is
> needed so that GPRs are 64 bit, and combined with MMI registers,
> they will form full 128 bit registers.
>
> Signed-off-by: Mateja Marjanovic
> Signed-off-by: Aleksandar Markovic
> Reviewed-by: Aleksandar Rikalo
> Message-Id: <1551183797-13570-2-git-send-email-mateja.marjano...@rt-rk.com>
> ---
> target/mips/translate.c | 43 +--
> 1 file changed, 41 insertions(+), 2 deletions(-)
...
> static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
> {
> int rs, rt, rd, sa;
> @@ -28796,10 +28828,11 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
> decode_opc_special(env, ctx);
> break;
> case OPC_SPECIAL2:
> +#if defined(TARGET_MIPS64)
> if ((ctx->insn_flags & INSN_R5900) && (ctx->insn_flags & ASE_MMI)) {
> decode_mmi(env, ctx);
This change is incorrect, you removed support for the
MADD[U] and MULT[U] instructions on TXx9 32-bit targets
(TX79 still works).
> -#if !defined(TARGET_MIPS64)
> -} else if (ctx->insn_flags & ASE_MXU) {
> +#else
> +if (ctx->insn_flags & ASE_MXU) {
> decode_opc_mxu(env, ctx);
> #endif
...
Re: [PATCH 5/6] qapi: Add support for aliases
Am 12.11.2020 um 19:34 hat Eric Blake geschrieben:
> On 11/12/20 11:28 AM, Kevin Wolf wrote:
> > Introduce alias definitions for object types (structs and unions). This
> > allows using the same QAPI type and visitor for many syntax variations
> > that exist in the external representation, like between QMP and the
> > command line. It also provides a new tool for evolving the schema while
> > maintaining backwards compatibility during a deprecation period.
>
> Cool! Obvious followup patch series: deprecate all QAPI members spelled
> with _ by making them aliases of new members spelled with -, so that we
> can finally have consistent spellings.
Ah, that's a nice one, too. I didn't even think of it. Another one I'd
like to see is deprecation of SocketAddressLegacy.
There is one part missing in this series that we would first need to
address before we can actually use it to evolve parts of the schema that
are visible in QMP: Exposing aliases in introspection and expressing
that the original name of something is deprecated, but the alias will
stay around (and probably also deprecating an alias without the original
name or other aliases).
If we can easily figure out a way to express this that everyone agrees
with, I'm happy to include it in this series. Otherwise, since the first
user is the command line and not QMP, I'd leave that for the future.
For example, imagine we have an option 'foo' with a new alias 'bar'. If
we just directly expose the alias rule (which would be the simplest
solution from the QEMU perspective), management will check if the alias
exists before accessing 'bar'. But in the final state, we remove 'foo'
and 'bar' is not an alias any more, so the introspection for 'bar' would
change. Is this desirable?
On the other hand, we can't specify both as normal options because you
must set (at most) one of them, but not both. Also exposing things as
normal options becomes hard with wildcard aliases (mapping all fields
from a nested struct), especially if unions are involved where some
options exist in one or two variants, but not in others.
Given this, I think just exposing the alias rules and letting the
management tool check both alternatives - if the alias rule or the
future option exists - might actually still be the least bad option.
Hmm, I guess I should CC libvirt for this discussion, actually. :-)
> > +=== Aliases ===
> > +
> > +Object types, including structs and unions, can contain alias
> > +definitions.
> > +
> > +Aliases define alternative member names that may be used in the
> > +external representation to provide a value for a member in the same
> > +object or in a nested object.
> > +
> > +Syntax:
> > +ALIAS = { '*alias': STRING,
> > + 'source': [ STRING, ... ] }
> > +
> > +'source' is a list of member names representing the path to an object
> > +member, starting from the type where the alias definition is
> > +specified. It may refer to another alias name. It is allowed to use
> > +a path that doesn't necessarily match an existing member in every
> > +variant or even at all; in this case, the alias remains unused.
> > +
> > +If 'alias' is present, then the single member referred to by 'source'
> > +is made accessible with the name given in 'alias' in the type where
> > +the alias definition is specified.
> > +
> > +If 'alias' is not present, then all members in the object referred to
> > +by 'source' are made accessible in the type where the alias definition
> > +is specified with the same name as they have in 'source'.
>
> Is it worth an example of how to use this?
Yes, I should have included an example. Or actually, probably one
example for aliasing a single field and another one for a wildcard alias
that maps all fields in a struct.
Kevin
Re: [PATCH] Clean up includes
On Fri, 13 Nov 2020 07:12:16 +0100 Markus Armbruster wrote: > Clean up includes so that osdep.h is included first and headers > which it implies are not included manually. > > This commit was created with scripts/clean-includes, with the changes > to the following files manually reverted: > > contrib/libvhost-user/libvhost-user-glib.h > contrib/libvhost-user/libvhost-user.c > contrib/libvhost-user/libvhost-user.h > contrib/plugins/hotblocks.c > contrib/plugins/hotpages.c > contrib/plugins/howvec.c > contrib/plugins/lockstep.c > linux-user/mips64/cpu_loop.c > linux-user/mips64/signal.c > linux-user/sparc64/cpu_loop.c > linux-user/sparc64/signal.c > linux-user/x86_64/cpu_loop.c > linux-user/x86_64/signal.c > target/s390x/gen-features.c > tests/fp/platform.h > tests/migration/s390x/a-b-bios.c > tests/plugin/bb.c > tests/plugin/empty.c > tests/plugin/insn.c > tests/plugin/mem.c > tests/test-rcu-simpleq.c > tests/test-rcu-slist.c > tests/test-rcu-tailq.c > tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c > > contrib/plugins/, tests/plugin/, and tests/test-rcu-slist.c appear not > to include osdep.h intentionally. The remaining reverts are the same > as in commit bbfff19688d. > > Signed-off-by: Markus Armbruster > --- > No real need to get this into 5.2 at this stage. No real risk either. > > contrib/vhost-user-gpu/vugbm.h | 2 -- > contrib/vhost-user-gpu/vugpu.h | 1 - > include/hw/block/swim.h | 1 - > include/hw/display/macfb.h | 1 - > include/qemu/nvdimm-utils.h | 1 - > tests/qtest/fuzz/fuzz.h | 1 - > tests/qtest/fuzz/generic_fuzz_configs.h | 1 - > tools/virtiofsd/fuse_common.h | 2 -- > tools/virtiofsd/fuse_log.h | 1 - > tools/virtiofsd/fuse_lowlevel.h | 3 --- > tools/virtiofsd/fuse_misc.h | 1 - > tools/virtiofsd/passthrough_seccomp.h | 1 - > contrib/vhost-user-gpu/virgl.c | 1 + > contrib/vhost-user-gpu/vugbm.c | 1 + > contrib/vhost-user-input/main.c | 1 - > hw/display/artist.c | 1 - > hw/s390x/s390-pci-vfio.c| 3 ++- > tools/virtiofsd/buffer.c| 5 - > tools/virtiofsd/fuse_log.c | 2 -- > tools/virtiofsd/fuse_lowlevel.c | 10 -- > tools/virtiofsd/fuse_opt.c | 4 > tools/virtiofsd/fuse_signals.c | 5 - > tools/virtiofsd/fuse_virtio.c | 10 -- > tools/virtiofsd/helper.c| 8 > tools/virtiofsd/passthrough_ll.c| 12 > tools/virtiofsd/passthrough_seccomp.c | 3 --- > util/nvdimm-utils.c | 1 + > 27 files changed, 5 insertions(+), 78 deletions(-) Acked-by: Cornelia Huck
Re: [PATCH] docs: Better mention of qemu-img amend limitations
Am 23.09.2020 um 22:37 hat Eric Blake geschrieben: > Missed during merge resolution of commit bc5ee6da71. > > Signed-off-by: Eric Blake > --- > docs/tools/qemu-img.rst | 4 > 1 file changed, 4 insertions(+) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index c35bd6482203..2b5891b54db7 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -265,6 +265,10 @@ Command description: Adding a little more context: >The set of options that can be amended are dependent on the image >format, but note that amending the backing chain relationship should >instead be performed with ``qemu-img rebase``. > >--force allows some unsafe operations. Currently for -f luks, it allows to >erase the last encryption key, and to overwrite an active encryption key. > > + The set of options that can be amended are dependent on the image > + format, but note that amending the backing chain relationship should > + instead be performed with ``qemu-img rebase``. > + I think the problem is your local conflict resolution. This patch would duplicate the paragraph. Kevin
[PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper
From: Bin Meng
Per the SDM, when returning to outer privilege level, for segment
registers (ES, FS, GS, and DS) if the check fails, the segment
selector becomes null, but QEMU clears the base/limit/flags as well
as nullifying the segment selector, which should be a spec violation.
Real hardware seems to be compliant with the spec, at least on one
Coffee Lake board I tested.
Signed-off-by: Bin Meng
---
Changes in v2:
- clearing the DESC_P bit in the segment descriptor
target/i386/seg_helper.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index be88938..d539573 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env, int
seg_reg, int cpl)
if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) {
/* data or non conforming code segment */
if (dpl < cpl) {
-cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0);
+cpu_x86_load_seg_cache(env, seg_reg, 0,
+ env->segs[seg_reg].base,
+ env->segs[seg_reg].limit,
+ env->segs[seg_reg].flags & ~DESC_P_MASK);
}
}
}
--
2.7.4
[PATCH] arm/monitor: Add support for 'info tlb' command
This adds hmp 'info tlb' command support for the arm platform.
The limitation is that this only implements a page walker for
ARMv8-A AArch64 Long Descriptor format, 32bit addressing is
not supported yet.
Signed-off-by: Changbin Du
---
hmp-commands-info.hx | 3 +-
target/arm/helper.c| 17 +---
target/arm/internals.h | 16
target/arm/monitor.c | 187 +
4 files changed, 206 insertions(+), 17 deletions(-)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 117ba25f91..1b5b3f2616 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -222,7 +222,8 @@ SRST
ERST
#if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC) || \
-defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
+defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K) || \
+defined(TARGET_ARM)
{
.name = "tlb",
.args_type = "",
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 11b0803df7..c73a08943b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9974,8 +9974,7 @@ static inline uint64_t regime_sctlr(CPUARMState *env,
ARMMMUIdx mmu_idx)
#ifndef CONFIG_USER_ONLY
/* Return true if the specified stage of address translation is disabled */
-static inline bool regime_translation_disabled(CPUARMState *env,
- ARMMMUIdx mmu_idx)
+bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx)
{
if (arm_feature(env, ARM_FEATURE_M)) {
switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] &
@@ -10021,20 +10020,6 @@ static inline bool
regime_translation_big_endian(CPUARMState *env,
return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
}
-/* Return the TTBR associated with this translation regime */
-static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
- int ttbrn)
-{
-if (mmu_idx == ARMMMUIdx_Stage2) {
-return env->cp15.vttbr_el2;
-}
-if (ttbrn == 0) {
-return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
-} else {
-return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
-}
-}
-
#endif /* !CONFIG_USER_ONLY */
/* Convert a possible stage1+2 MMU index into the appropriate
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 5460678756..12f883c636 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -949,6 +949,8 @@ static inline uint32_t regime_el(CPUARMState *env,
ARMMMUIdx mmu_idx)
}
}
+bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx);
+
/* Return the TCR controlling this translation regime */
static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
{
@@ -958,6 +960,20 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx
mmu_idx)
return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
}
+/* Return the TTBR associated with this translation regime */
+static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
+ int ttbrn)
+{
+if (mmu_idx == ARMMMUIdx_Stage2) {
+return env->cp15.vttbr_el2;
+}
+if (ttbrn == 0) {
+return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
+} else {
+return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
+}
+}
+
/* Return the FSR value for a debug exception (watchpoint, hardware
* breakpoint or BKPT insn) targeting the specified exception level.
*/
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 169d8a64b6..d6b64ea3b6 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -31,6 +31,9 @@
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "qom/qom-qobject.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp-target.h"
+#include "internals.h"
static GICCapability *gic_cap_new(int version)
{
@@ -236,3 +239,187 @@ CpuModelExpansionInfo
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return expansion_info;
}
+
+/* Perform linear address sign extension */
+static target_ulong addr_canonical(int va_bits, target_ulong addr)
+{
+#ifdef TARGET_AARCH64
+if (addr & (1UL << (va_bits - 1))) {
+addr |= (hwaddr)-(1L << va_bits);
+}
+#endif
+
+return addr;
+}
+
+#define PTE_HEADER_FIELDS "vaddrpaddr"\
+"size attr\n"
+#define PTE_HEADER_DELIMITER" "\
+"
--\n"
+
+static void print_pte_header(Monitor *mon)
+{
+monitor_printf(mon, PTE_HEADER_FIELDS);
+monitor_printf(mon, PTE_HEADER_DELIMITER);
+}
+
+static void
+print_pte_lpae(Monitor *mon, uint32_t tableattrs, int va_bits, target_ulong
vaddr,
+ hwaddr paddr, target_ulong size, target_ulong pte)
+{
+uint32_t ns = extract64(pte, 5, 1) | extract32(tableattrs, 4, 1);
+u
Re: [PATCH] arm/monitor: Add support for 'info tlb' command
Patchew URL: https://patchew.org/QEMU/20201113095854.67668-1-changbin...@gmail.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20201113095854.67668-1-changbin...@gmail.com Type: series Subject: [PATCH] arm/monitor: Add support for 'info tlb' command === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20201113095854.67668-1-changbin...@gmail.com -> patchew/20201113095854.67668-1-changbin...@gmail.com Switched to a new branch 'test' a33e6e7 arm/monitor: Add support for 'info tlb' command === OUTPUT BEGIN === WARNING: line over 80 characters #141: FILE: target/arm/monitor.c:267: +print_pte_lpae(Monitor *mon, uint32_t tableattrs, int va_bits, target_ulong vaddr, ERROR: spaces required around that '=' (ctx:VxW) #174: FILE: target/arm/monitor.c:300: +int ptshift= pg_shift + (max_level - cur_level) * stride; ^ WARNING: line over 80 characters #195: FILE: target/arm/monitor.c:321: +print_pte_lpae(mon, tableattrs, va_bits, vstart, paddr, pgsize, pte); WARNING: line over 80 characters #268: FILE: target/arm/monitor.c:394: +walk_pte_lpae(mon, true, 0, base, vstart, cur_level, stride, va_bits); total: 1 errors, 3 warnings, 262 lines checked Commit a33e6e7e0edd (arm/monitor: Add support for 'info tlb' command) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20201113095854.67668-1-changbin...@gmail.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH for-5.2] iotests: Replace deprecated ConfigParser.readfp()
iotest 277 fails on Fedora 33 (Python 3.9) because a deprecation warning changes the output: nbd-fault-injector.py:230: DeprecationWarning: This method will be removed in future versions. Use 'parser.read_file()' instead. In fact, readfp() has already been deprecated in Python 3.2 and the replacement has existed since the same version, so we can now unconditionally switch to read_file(). Signed-off-by: Kevin Wolf --- tests/qemu-iotests/nbd-fault-injector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py index 78f42c4214..6e11ef89b8 100755 --- a/tests/qemu-iotests/nbd-fault-injector.py +++ b/tests/qemu-iotests/nbd-fault-injector.py @@ -227,7 +227,7 @@ def parse_config(config): def load_rules(filename): config = configparser.RawConfigParser() with open(filename, 'rt') as f: -config.readfp(f, filename) +config.read_file(f, filename) return parse_config(config) def open_socket(path): -- 2.28.0
Re: [PATCH 0/2] RFC: Issue with discards on raw block device without O_DIRECT
On Thu 12-11-20 17:38:36, Maxim Levitsky wrote: > On Thu, 2020-11-12 at 12:19 +0100, Jan Kara wrote: > > [added some relevant people and lists to CC] > > > > On Wed 11-11-20 17:44:05, Maxim Levitsky wrote: > > > On Wed, 2020-11-11 at 17:39 +0200, Maxim Levitsky wrote: > > > > clone of "starship_production" > > > > > > The git-publish destroyed the cover letter: > > > > > > For the reference this is for bz #1872633 > > > > > > The issue is that current kernel code that implements 'fallocate' > > > on kernel block devices roughly works like that: > > > > > > 1. Flush the page cache on the range that is about to be discarded. > > > 2. Issue the discard and wait for it to finish. > > >(as far as I can see the discard doesn't go through the > > >page cache). > > > > > > 3. Check if the page cache is dirty for this range, > > >if it is dirty (meaning that someone wrote to it meanwhile) > > >return -EBUSY. > > > > > > This means that if qemu (or qemu-img) issues a write, and then > > > discard to the area that shares a page, -EBUSY can be returned by > > > the kernel. > > > > Indeed, if you don't submit PAGE_SIZE aligned discards, you can get back > > EBUSY which seems wrong to me. IMO we should handle this gracefully in the > > kernel so we need to fix this. > > > > > On the other hand, for example, the ext4 implementation of discard > > > doesn't seem to be affected. It does take a lock on the inode to avoid > > > concurrent IO and flushes O_DIRECT writers prior to doing discard thought. > > > > Well, filesystem hole punching is somewhat different beast than block device > > discard (at least implementation wise). > > > > > Doing fsync and retrying is seems to resolve this issue, but it might be > > > a too big hammer. Just retrying doesn't work, indicating that maybe the > > > code that flushes the page cache in (1) doesn't do this correctly ? > > > > > > It also can be racy unless special means are done to block IO from > > > happening > > > from qemu during this fsync. > > > > > > This patch series contains two patches: > > > > > > First patch just lets the file-posix ignore the -EBUSY errors, which is > > > technically enough to fail back to plain write in this case, but seems > > > wrong. > > > > > > And the second patch adds an optimization to qemu-img to avoid such a > > > fragmented write/discard in the first place. > > > > > > Both patches make the reproducer work for this particular bugzilla, > > > but I don't think they are enough. > > > > > > What do you think? > > > > So if the EBUSY error happens because something happened to the page cache > > outside of discarded range (like you describe above), that is a kernel bug > > than needs to get fixed. EBUSY should really mean - someone wrote to the > > discarded range while discard was running and userspace app has to deal > > with that depending on what it aims to do... > I double checked this, those are the writes/discards according to my debug > prints (I print start and then start+len-1 for each request) > I have attached the patch for this for reference. > > ZERO: 0x7fe0 7fffefff (len:0x1ff000) >fallocate 7fe0 7fffefff Yeah, the end at 7000 is indeed not 4k aligned... > WRITE: 0x7000 7dff (len:0xe00) >write 7000 7dff .. and this write is following discarded area in the same page (7000..7dff). Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH v2 1/2] fuzz: add virtio-blk fuzz target
On Mon, Nov 09, 2020 at 01:24:20PM +0100, Thomas Huth wrote: > On 09/11/2020 12.25, Dima Stepanov wrote: > > The virtio-blk fuzz target sets up and fuzzes the available virtio-blk > > queues. The implementation is based on two files: > > - tests/qtest/fuzz/virtio_scsi_fuzz.c > > - tests/qtest/virtio_blk_test.c > > > > Signed-off-by: Dima Stepanov > > Reviewed-by: Alexander Bulekov > > --- > > tests/qtest/fuzz/meson.build | 1 + > > tests/qtest/fuzz/virtio_blk_fuzz.c | 234 > > + > > 2 files changed, 235 insertions(+) > > create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c > > Thanks, I can add this to my next qtest-related pull request. > Note: I just spotted your patch by accident ... please put me on CC: if you > want me to merge qtest/fuzzer related patches. Hi Thomas, Thanks for handling it. Got it! Will add you to CC for the next fuzzing pull requests. Dima. > > Thomas >
Re: [PATCH] arm/monitor: Add support for 'info tlb' command
On Fri, 13 Nov 2020 at 09:59, Changbin Du wrote: > > This adds hmp 'info tlb' command support for the arm platform. > The limitation is that this only implements a page walker for > ARMv8-A AArch64 Long Descriptor format, 32bit addressing is > not supported yet. "info tlb" needs its own entirely independent implementation of a page-table walk? I see this is how x86 has done it ,but it seems like a recipe for the info command being perpetually behind what we actually implement... thanks -- PMM
Re: [PATCH v3 2/4] ads7846: put it into the 'input' category
On Fri, 13 Nov 2020 at 03:32, Gan Qixin wrote:
>
> The category of the ads7846 device is not set, put it into the 'input'
> category.
>
> Signed-off-by: Gan Qixin
> ---
> Cc: Peter Maydell
> ---
> hw/display/ads7846.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c
> index 023165b2a3..cb3a431cfd 100644
> --- a/hw/display/ads7846.c
> +++ b/hw/display/ads7846.c
> @@ -163,10 +163,12 @@ static void ads7846_realize(SSISlave *d, Error **errp)
>
> static void ads7846_class_init(ObjectClass *klass, void *data)
> {
> +DeviceClass *dc = DEVICE_CLASS(klass);
> SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
>
> k->realize = ads7846_realize;
> k->transfer = ads7846_transfer;
> +set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> }
>
> static const TypeInfo ads7846_info = {
Reviewed-by: Peter Maydell
Really we should move the file too...
thanks
-- PMM
[PATCH v2 002/122] arm: do not use ram_size global
Use the machine properties instead.
Cc: qemu-...@nongnu.org
Signed-off-by: Paolo Bonzini
---
hw/arm/aspeed.c | 8
hw/display/pxa2xx_lcd.c | 5 +++--
target/arm/arm-semi.c | 3 ++-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0ef3f6b412..cc8ed6ec9c 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -295,7 +295,7 @@ static void aspeed_machine_init(MachineState *machine)
/*
* This will error out if isize is not supported by memory controller.
*/
-object_property_set_uint(OBJECT(&bmc->soc), "ram-size", ram_size,
+object_property_set_uint(OBJECT(&bmc->soc), "ram-size", machine->ram_size,
&error_fatal);
for (i = 0; i < sc->macs_num; i++) {
@@ -332,8 +332,8 @@ static void aspeed_machine_init(MachineState *machine)
max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
&error_abort);
memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
- "max_ram", max_ram_size - ram_size);
-memory_region_add_subregion(&bmc->ram_container, ram_size, &bmc->max_ram);
+ "max_ram", max_ram_size - machine->ram_size);
+memory_region_add_subregion(&bmc->ram_container, machine->ram_size,
&bmc->max_ram);
aspeed_board_init_flashes(&bmc->soc.fmc, bmc->fmc_model ?
bmc->fmc_model : amc->fmc_model);
@@ -378,7 +378,7 @@ static void aspeed_machine_init(MachineState *machine)
aspeed_board_binfo.smp_loader_start = AST_SMP_MBOX_CODE;
}
-aspeed_board_binfo.ram_size = ram_size;
+aspeed_board_binfo.ram_size = machine->ram_size;
aspeed_board_binfo.loader_start = sc->memmap[ASPEED_DEV_SDRAM];
aspeed_board_binfo.nb_cpus = sc->num_cpus;
diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index ff90104b80..dfff994962 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -17,6 +17,7 @@
#include "ui/console.h"
#include "hw/arm/pxa.h"
#include "ui/pixel_ops.h"
+#include "hw/boards.h"
/* FIXME: For graphic_rotate. Should probably be done in common code. */
#include "sysemu/sysemu.h"
#include "framebuffer.h"
@@ -305,7 +306,7 @@ static void pxa2xx_descriptor_load(PXA2xxLCDState *s)
descptr = s->dma_ch[i].descriptor;
if (!((descptr >= PXA2XX_SDRAM_BASE && descptr +
- sizeof(desc) <= PXA2XX_SDRAM_BASE + ram_size) ||
+ sizeof(desc) <= PXA2XX_SDRAM_BASE +
current_machine->ram_size) ||
(descptr >= PXA2XX_INTERNAL_BASE && descptr + sizeof(desc) <=
PXA2XX_INTERNAL_BASE + PXA2XX_INTERNAL_SIZE))) {
continue;
@@ -850,7 +851,7 @@ static void pxa2xx_update_display(void *opaque)
}
fbptr = s->dma_ch[ch].source;
if (!((fbptr >= PXA2XX_SDRAM_BASE &&
- fbptr <= PXA2XX_SDRAM_BASE + ram_size) ||
+ fbptr <= PXA2XX_SDRAM_BASE + current_machine->ram_size) ||
(fbptr >= PXA2XX_INTERNAL_BASE &&
fbptr <= PXA2XX_INTERNAL_BASE + PXA2XX_INTERNAL_SIZE))) {
pxa2xx_dma_ber_set(s, ch);
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index c1df664f7e..8a5cd49df5 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -36,6 +36,7 @@
#else
#include "exec/gdbstub.h"
#include "qemu/cutils.h"
+#include "hw/boards.h"
#endif
#define TARGET_SYS_OPEN0x01
@@ -1044,7 +1045,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
retvals[2] = ts->stack_base;
retvals[3] = 0; /* Stack limit. */
#else
-limit = ram_size;
+limit = current_machine->ram_size;
/* TODO: Make this use the limit of the loaded application. */
retvals[0] = limit / 2;
retvals[1] = limit;
--
2.26.2
[PATCH v2 003/122] cris: do not use ram_size global
Use the machine properties instead.
Signed-off-by: Paolo Bonzini
---
hw/cris/axis_dev88.c | 1 +
hw/cris/boot.c | 2 +-
hw/cris/boot.h | 1 +
3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
index dab7423c73..b0cb6d84af 100644
--- a/hw/cris/axis_dev88.c
+++ b/hw/cris/axis_dev88.c
@@ -333,6 +333,7 @@ void axisdev88_init(MachineState *machine)
if (kernel_filename) {
li.image_filename = kernel_filename;
li.cmdline = kernel_cmdline;
+li.ram_size = machine->ram_size;
cris_load_image(cpu, &li);
} else if (!qtest_enabled()) {
fprintf(stderr, "Kernel image must be specified\n");
diff --git a/hw/cris/boot.c b/hw/cris/boot.c
index aa8d2756d6..9fa09cfd83 100644
--- a/hw/cris/boot.c
+++ b/hw/cris/boot.c
@@ -81,7 +81,7 @@ void cris_load_image(CRISCPU *cpu, struct cris_load_info *li)
if (image_size < 0) {
/* Takes a kimage from the axis devboard SDK. */
image_size = load_image_targphys(li->image_filename, 0x40004000,
- ram_size);
+ li->ram_size);
li->entry = 0x40004000;
}
diff --git a/hw/cris/boot.h b/hw/cris/boot.h
index 218854e5d1..9f1e0e340c 100644
--- a/hw/cris/boot.h
+++ b/hw/cris/boot.h
@@ -6,6 +6,7 @@ struct cris_load_info
const char *image_filename;
const char *cmdline;
int image_size;
+ram_addr_t ram_size;
hwaddr entry;
};
--
2.26.2
[PATCH v2 005/122] i386: do not use ram_size global
Use the loader parameters instead.
Signed-off-by: Paolo Bonzini
---
hw/i386/fw_cfg.c | 2 +-
hw/i386/vmport.c | 3 ++-
hw/i386/xen/xen-hvm.c | 2 +-
hw/intc/apic_common.c | 3 ++-
hw/smbios/smbios.c| 8
5 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index e06579490c..b87f0e5070 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -118,7 +118,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
* "etc/max-cpus" actually being apic_id_limit
*/
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, apic_id_limit);
-fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, ms->ram_size);
#ifdef CONFIG_ACPI
fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
acpi_tables, acpi_tables_len);
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 20d605506b..490a57f52c 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -32,6 +32,7 @@
#include "hw/isa/isa.h"
#include "hw/i386/vmport.h"
#include "hw/qdev-properties.h"
+#include "hw/boards.h"
#include "sysemu/sysemu.h"
#include "sysemu/hw_accel.h"
#include "sysemu/qtest.h"
@@ -188,7 +189,7 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t
addr)
return -1;
}
cpu->env.regs[R_EBX] = 0x1177;
-return ram_size;
+return current_machine->ram_size;
}
static uint32_t vmport_cmd_get_hz(void *opaque, uint32_t addr)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 9519c33c09..096c46fef1 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1493,7 +1493,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion
**ram_memory)
#else
xen_map_cache_init(NULL, state);
#endif
-xen_ram_init(pcms, ram_size, ram_memory);
+xen_ram_init(pcms, ms->ram_size, ram_memory);
qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 81addd6390..67c24238b0 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -27,6 +27,7 @@
#include "hw/i386/apic.h"
#include "hw/i386/apic_internal.h"
#include "trace.h"
+#include "hw/boards.h"
#include "sysemu/hax.h"
#include "sysemu/kvm.h"
#include "hw/qdev-properties.h"
@@ -297,7 +298,7 @@ static void apic_common_realize(DeviceState *dev, Error
**errp)
/* Note: We need at least 1M to map the VAPIC option ROM */
if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
-!hax_enabled() && ram_size >= 1024 * 1024) {
+!hax_enabled() && current_machine->ram_size >= 1024 * 1024) {
vapic = sysbus_create_simple("kvmvapic", -1, NULL);
}
s->vapic = vapic;
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 6a3d39793b..f22c4f5b73 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -678,13 +678,13 @@ static void smbios_build_type_16_table(unsigned dimm_cnt)
t->location = 0x01; /* Other */
t->use = 0x03; /* System memory */
t->error_correction = 0x06; /* Multi-bit ECC (for Microsoft, per SeaBIOS)
*/
-size_kb = QEMU_ALIGN_UP(ram_size, KiB) / KiB;
+size_kb = QEMU_ALIGN_UP(current_machine->ram_size, KiB) / KiB;
if (size_kb < MAX_T16_STD_SZ) {
t->maximum_capacity = cpu_to_le32(size_kb);
t->extended_maximum_capacity = cpu_to_le64(0);
} else {
t->maximum_capacity = cpu_to_le32(MAX_T16_STD_SZ);
-t->extended_maximum_capacity = cpu_to_le64(ram_size);
+t->extended_maximum_capacity = cpu_to_le64(current_machine->ram_size);
}
t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided
*/
t->number_of_memory_devices = cpu_to_le16(dimm_cnt);
@@ -911,9 +911,9 @@ void smbios_get_tables(MachineState *ms,
#define MAX_DIMM_SZ (16 * GiB)
#define GET_DIMM_SZ ((i < dimm_cnt - 1) ? MAX_DIMM_SZ \
-: ((ram_size - 1) % MAX_DIMM_SZ) + 1)
+: ((current_machine->ram_size - 1) %
MAX_DIMM_SZ) + 1)
-dimm_cnt = QEMU_ALIGN_UP(ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
+dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) /
MAX_DIMM_SZ;
smbios_build_type_16_table(dimm_cnt);
--
2.26.2
[PATCH v2 001/122] vl: remove bios_name
bios_name was a legacy variable used by machine code, but it is
no more.
Signed-off-by: Paolo Bonzini
Reviewed-by: Alex Bennée
Message-Id: <20201026143028.3034018-16-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini
---
include/sysemu/sysemu.h | 1 -
softmmu/vl.c| 2 --
2 files changed, 3 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 817ff4cf75..1336b4264a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -8,7 +8,6 @@
/* vl.c */
-extern const char *bios_name;
extern int only_migratable;
extern const char *qemu_name;
extern QemuUUID qemu_uuid;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e6e0ad5a92..ab79c361b9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -119,7 +119,6 @@
static const char *data_dir[16];
static int data_dir_idx;
-const char *bios_name = NULL;
enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
int display_opengl;
const char* keyboard_layout = NULL;
@@ -4208,7 +4207,6 @@ void qemu_init(int argc, char **argv, char **envp)
kernel_filename = qemu_opt_get(machine_opts, "kernel");
initrd_filename = qemu_opt_get(machine_opts, "initrd");
kernel_cmdline = qemu_opt_get(machine_opts, "append");
-bios_name = qemu_opt_get(machine_opts, "firmware");
opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
if (opts) {
--
2.26.2
[PATCH v2 006/122] m68k: do not use ram_size global
Use the machine properties instead.
Cc: Laurent Vivier
Signed-off-by: Paolo Bonzini
---
hw/m68k/mcf5206.c | 4 +++-
hw/m68k/mcf5208.c | 3 ++-
target/m68k/m68k-semi.c | 5 +++--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index 51d2e0da1c..8708aa4480 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -10,6 +10,7 @@
#include "qemu/error-report.h"
#include "qemu/log.h"
#include "cpu.h"
+#include "hw/boards.h"
#include "hw/irq.h"
#include "hw/m68k/mcf.h"
#include "qemu/timer.h"
@@ -311,8 +312,9 @@ static uint64_t m5206_mbar_read(m5206_mbar_state *s,
/* FIXME: currently hardcoded to 128Mb. */
{
uint32_t mask = ~0;
-while (mask > ram_size)
+while (mask > current_machine->ram_size) {
mask >>= 1;
+}
return mask & 0x0ffe;
}
case 0x5c: return 1; /* DRAM bank 1 empty. */
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 7c8ca5ddf6..2205145ecc 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -157,8 +157,9 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
{
int n;
for (n = 0; n < 32; n++) {
-if (ram_size < (2u << n))
+if (current_machine->ram_size < (2u << n)) {
break;
+}
}
return (n - 1) | 0x4000;
}
diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
index 8e5fbfc8fa..27600e0cc0 100644
--- a/target/m68k/m68k-semi.c
+++ b/target/m68k/m68k-semi.c
@@ -26,6 +26,7 @@
#else
#include "exec/gdbstub.h"
#include "exec/softmmu-semi.h"
+#include "hw/boards.h"
#endif
#include "qemu/log.h"
@@ -455,8 +456,8 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
* FIXME: This is wrong for boards where RAM does not start at
* address zero.
*/
-env->dregs[1] = ram_size;
-env->aregs[7] = ram_size;
+env->dregs[1] = current_machine->ram_size;
+env->aregs[7] = current_machine->ram_size;
#endif
return;
default:
--
2.26.2
[PATCH v2 004/122] hppa: do not use ram_size global
Use the machine properties instead.
Cc: Richard Henderson
Signed-off-by: Paolo Bonzini
---
hw/hppa/machine.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5e745d5ea9..7e41cb2462 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -97,7 +97,7 @@ static FWCfgState *create_fw_cfg(MachineState *ms)
fw_cfg = fw_cfg_init_mem(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4);
fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, ms->smp.cpus);
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, HPPA_MAX_CPUS);
-fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, ram_size);
+fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, ms->ram_size);
val = cpu_to_le64(MIN_SEABIOS_HPPA_VERSION);
fw_cfg_add_file(fw_cfg, "/etc/firmware-min-version",
@@ -288,7 +288,7 @@ static void machine_hppa_init(MachineState *machine)
(1) Due to sign-extension problems and PDC,
put the initrd no higher than 1G.
(2) Reserve 64k for stack. */
-initrd_base = MIN(ram_size, 1 * GiB);
+initrd_base = MIN(machine->ram_size, 1 * GiB);
initrd_base = initrd_base - 64 * KiB;
initrd_base = (initrd_base - initrd_size) & TARGET_PAGE_MASK;
@@ -316,7 +316,7 @@ static void machine_hppa_init(MachineState *machine)
* various parameters in registers. After firmware initialization,
* firmware will start the Linux kernel with ramdisk and cmdline.
*/
-cpu[0]->env.gr[26] = ram_size;
+cpu[0]->env.gr[26] = machine->ram_size;
cpu[0]->env.gr[25] = kernel_entry;
/* tell firmware how many SMP CPUs to present in inventory table */
@@ -342,11 +342,11 @@ static void hppa_machine_reset(MachineState *ms)
}
/* already initialized by machine_hppa_init()? */
-if (cpu[0]->env.gr[26] == ram_size) {
+if (cpu[0]->env.gr[26] == ms->ram_size) {
return;
}
-cpu[0]->env.gr[26] = ram_size;
+cpu[0]->env.gr[26] = ms->ram_size;
cpu[0]->env.gr[25] = 0; /* no firmware boot menu */
cpu[0]->env.gr[24] = 'c';
/* gr22/gr23 unused, no initrd while reboot. */
--
2.26.2
Re: [PATCH v2] digic: remove bios_name
On Fri, 13 Nov 2020 at 10:17, Paolo Bonzini wrote: > > Pull defaults to digic4_board_init so that a MachineState is available. > > Cc: Peter Maydell > Signed-off-by: Paolo Bonzini > --- > hw/arm/digic_boards.c | 19 +++ > 1 file changed, 7 insertions(+), 12 deletions(-) Reviewed-by: Peter Maydell Did you want me to take this via target-arm.next or are you planning to include it in some other series/pull ? thanks -- PMM
[PATCH v2] digic: remove bios_name
Pull defaults to digic4_board_init so that a MachineState is available.
Cc: Peter Maydell
Signed-off-by: Paolo Bonzini
---
hw/arm/digic_boards.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index d5524d3e72..fd228fa96f 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -70,19 +70,20 @@ static void digic4_board_init(MachineState *machine,
DigicBoard *board)
memory_region_add_subregion(get_system_memory(), 0, machine->ram);
if (board->add_rom0) {
-board->add_rom0(s, DIGIC4_ROM0_BASE, board->rom0_def_filename);
+board->add_rom0(s, DIGIC4_ROM0_BASE,
+machine->firmware ?: board->rom0_def_filename);
}
if (board->add_rom1) {
-board->add_rom1(s, DIGIC4_ROM1_BASE, board->rom1_def_filename);
+board->add_rom1(s, DIGIC4_ROM1_BASE,
+machine->firmware ?: board->rom1_def_filename);
}
}
static void digic_load_rom(DigicState *s, hwaddr addr,
- hwaddr max_size, const char *def_filename)
+ hwaddr max_size, const char *filename)
{
target_long rom_size;
-const char *filename;
if (qtest_enabled()) {
/* qtest runs no code so don't attempt a ROM load which
@@ -91,12 +92,6 @@ static void digic_load_rom(DigicState *s, hwaddr addr,
return;
}
-if (bios_name) {
-filename = bios_name;
-} else {
-filename = def_filename;
-}
-
if (filename) {
char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, filename);
@@ -119,7 +114,7 @@ static void digic_load_rom(DigicState *s, hwaddr addr,
* 64M Bit (4Mx16) Page Mode / Multi-Bank NOR Flash Memory
*/
static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
- const char *def_filename)
+ const char *filename)
{
#define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
#define FLASH_K8P3215UQB_SECTOR_SIZE (64 * 1024)
@@ -131,7 +126,7 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr
addr,
0x00EC, 0x007E, 0x0003, 0x0001,
0x0555, 0x2aa, 0);
-digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, def_filename);
+digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, filename);
}
static DigicBoard digic4_board_canon_a1100 = {
--
2.26.2
Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper
Hi Paolo,
On Fri, Nov 13, 2020 at 6:18 PM Paolo Bonzini wrote:
>
> On 13/11/20 10:56, Bin Meng wrote:
> > From: Bin Meng
> >
> > Per the SDM, when returning to outer privilege level, for segment
> > registers (ES, FS, GS, and DS) if the check fails, the segment
> > selector becomes null, but QEMU clears the base/limit/flags as well
> > as nullifying the segment selector, which should be a spec violation.
> >
> > Real hardware seems to be compliant with the spec, at least on one
> > Coffee Lake board I tested.
> >
> > Signed-off-by: Bin Meng
> >
> > ---
> >
> > Changes in v2:
> > - clearing the DESC_P bit in the segment descriptor
> >
> > target/i386/seg_helper.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> > index be88938..d539573 100644
> > --- a/target/i386/seg_helper.c
> > +++ b/target/i386/seg_helper.c
> > @@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env,
> > int seg_reg, int cpl)
> > if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) {
> > /* data or non conforming code segment */
> > if (dpl < cpl) {
> > -cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0);
> > +cpu_x86_load_seg_cache(env, seg_reg, 0,
> > + env->segs[seg_reg].base,
> > + env->segs[seg_reg].limit,
> > + env->segs[seg_reg].flags &
> > ~DESC_P_MASK);
> > }
> > }
> > }
> >
>
> Queued, thanks.
Thanks!
> It would be nicer if the commit message explained how
> the guest can notice the difference.
The commit message says "Per the SDM" :) The actual failure case
involves a special code sequence that is exposed in VxWorks guest
testing. Linux does not expose this however.
Regards,
Bin
Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper
On 13/11/20 10:56, Bin Meng wrote:
From: Bin Meng
Per the SDM, when returning to outer privilege level, for segment
registers (ES, FS, GS, and DS) if the check fails, the segment
selector becomes null, but QEMU clears the base/limit/flags as well
as nullifying the segment selector, which should be a spec violation.
Real hardware seems to be compliant with the spec, at least on one
Coffee Lake board I tested.
Signed-off-by: Bin Meng
---
Changes in v2:
- clearing the DESC_P bit in the segment descriptor
target/i386/seg_helper.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index be88938..d539573 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env, int
seg_reg, int cpl)
if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) {
/* data or non conforming code segment */
if (dpl < cpl) {
-cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0);
+cpu_x86_load_seg_cache(env, seg_reg, 0,
+ env->segs[seg_reg].base,
+ env->segs[seg_reg].limit,
+ env->segs[seg_reg].flags & ~DESC_P_MASK);
}
}
}
Queued, thanks. It would be nicer if the commit message explained how
the guest can notice the difference.
Paolo
[PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()
The e1000e_write_packet_to_guest() function iterates over a set of
receive descriptors by advancing rx descriptor head register (RDH) from
its initial value to rx descriptor tail register (RDT). The check in
e1000e_ring_empty() is responsible for detecting whether RDH has reached
RDT, terminating the loop if that's the case. Additional checks have
been added in the past to deal with bogus values submitted by the guest
to prevent possible infinite loop. This is done by "wrapping around" RDH
at some point and detecting whether it assumes the original value during
the loop.
However, when e1000e is configured to use the packet split feature, RDH is
incremented by two instead of one, as the packet split descriptors are
32 bytes while regular descriptors are 16 bytes. A malicious or buggy
guest may set RDT to an odd value and transmit only null RX descriptors.
This corner case would prevent RDH from ever matching RDT, leading to an
infinite loop. This patch adds a check in e1000e_ring_advance() to make sure
RDH does not exceed RDT in a single incremental step, adjusting the count
value accordingly.
This issue was independently reported by Gaoning Pan (Zhejiang University)
and Cheolwoo Myung.
Signed-off-by: Mauro Matteo Cascella
Tested-by: Mauro Matteo Cascella
Reported-by: Gaoning Pan
Reported-by: Cheolwoo Myung <330cj...@gmail.com>
---
Changes since previous version:
Take the initial values of RDH/RDT into account. Address the case where RDH is
less than RDT and (RDH + count) would exceed RDT.
Patch v1:
https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg01492.html
References:
https://git.qemu.org/?p=qemu.git;a=commit;h=dd793a74882477ca38d49e191110c17dfe
https://git.qemu.org/?p=qemu.git;a=commit;h=4154c7e03fa55b4cf52509a83d50d6c09d743b7
http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
hw/net/e1000e_core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcfd46696f..324cc14ffb 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -829,6 +829,11 @@ e1000e_ring_head_descr(E1000ECore *core, const
E1000E_RingInfo *r)
static inline void
e1000e_ring_advance(E1000ECore *core, const E1000E_RingInfo *r, uint32_t count)
{
+if (count > 1 && core->mac[r->dh] < core->mac[r->dt] &&
+core->mac[r->dh] + count > core->mac[r->dt]) {
+count = core->mac[r->dt] - core->mac[r->dh];
+}
+
core->mac[r->dh] += count;
if (core->mac[r->dh] * E1000_RING_DESC_LEN >= core->mac[r->dlen]) {
--
2.28.0
Re: [PATCH v2] digic: remove bios_name
On 13/11/20 11:22, Peter Maydell wrote: Pull defaults to digic4_board_init so that a MachineState is available. Cc: Peter Maydell Signed-off-by: Paolo Bonzini --- hw/arm/digic_boards.c | 19 +++ 1 file changed, 7 insertions(+), 12 deletions(-) Reviewed-by: Peter Maydell Did you want me to take this via target-arm.next or are you planning to include it in some other series/pull ? I have ~15 patches each for bios_name and ram_size and I was planning to send them all myself (most have already received acks/reviews from maintainers), but it's the same. I did want to run this through you since this particular board was one of very few that didn't have a purely mechanical change, and you had a suggestion on how to improve on v1. Paolo
Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper
On 13/11/20 11:23, Bin Meng wrote: It would be nicer if the commit message explained how the guest can notice the difference. The commit message says "Per the SDM" :) The actual failure case involves a special code sequence that is exposed in VxWorks guest testing. Linux does not expose this however. I see. Is there any chance you could write a testcase for kvm-unit-tests? Or just explain how to write such a test, and then I can write it myself; it's not clear to me how the guest can observe the base and limit of a non-present segment. Paolo
Re: [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size
* Markus Armbruster (arm...@redhat.com) wrote:
> 73af8dd8d7 "migration: Make xbzrle_cache_size a migration
> parameter" (v2.11.0) made the new parameter unsigned (QAPI type
> 'size', uint64_t in C). It neglected to update existing code, which
> continues to use int64_t.
>
> migrate_xbzrle_cache_size() returns the new parameter. Adjust its
> return type.
>
> QMP query-migrate-cache-size returns migrate_xbzrle_cache_size().
> Adjust its return type.
>
> migrate-set-parameters passes the new parameter to
> xbzrle_cache_resize(). Adjust its parameter type.
>
> xbzrle_cache_resize() passes it on to cache_init(). Adjust its
> parameter type.
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Dr. David Alan Gilbert
> ---
> qapi/migration.json| 4 ++--
> migration/migration.h | 2 +-
> migration/page_cache.h | 2 +-
> migration/ram.h| 2 +-
> migration/migration.c | 4 ++--
> migration/page_cache.c | 2 +-
> migration/ram.c| 2 +-
> 7 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3ad3720cf0..e8a4dfecce 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -78,7 +78,7 @@
> # Since: 1.2
> ##
> { 'struct': 'XBZRLECacheStats',
> - 'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> + 'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
> 'cache-miss': 'int', 'cache-miss-rate': 'number',
> 'encoding-rate': 'number', 'overflow': 'int' } }
>
> @@ -1465,7 +1465,7 @@
> # <- { "return": 67108864 }
> #
> ##
> -{ 'command': 'query-migrate-cache-size', 'returns': 'int',
> +{ 'command': 'query-migrate-cache-size', 'returns': 'size',
>'features': [ 'deprecated' ] }
>
> ##
> diff --git a/migration/migration.h b/migration/migration.h
> index d096b77f74..0387dc40d6 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -324,7 +324,7 @@ int migrate_multifd_zlib_level(void);
> int migrate_multifd_zstd_level(void);
>
> int migrate_use_xbzrle(void);
> -int64_t migrate_xbzrle_cache_size(void);
> +uint64_t migrate_xbzrle_cache_size(void);
> bool migrate_colo_enabled(void);
>
> bool migrate_use_block(void);
> diff --git a/migration/page_cache.h b/migration/page_cache.h
> index 0cb94498a0..8733b4df6e 100644
> --- a/migration/page_cache.h
> +++ b/migration/page_cache.h
> @@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
> * @page_size: cache page size
> * @errp: set *errp if the check failed, with reason
> */
> -PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
> +PageCache *cache_init(uint64_t cache_size, size_t page_size, Error **errp);
> /**
> * cache_fini: free all cache resources
> * @cache pointer to the PageCache struct
> diff --git a/migration/ram.h b/migration/ram.h
> index 011e85414e..016eaa3378 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -47,7 +47,7 @@ bool ramblock_is_ignored(RAMBlock *block);
> INTERNAL_RAMBLOCK_FOREACH(block) \
> if (!qemu_ram_is_migratable(block)) {} else
>
> -int xbzrle_cache_resize(int64_t new_size, Error **errp);
> +int xbzrle_cache_resize(uint64_t new_size, Error **errp);
> uint64_t ram_bytes_remaining(void);
> uint64_t ram_bytes_total(void);
>
> diff --git a/migration/migration.c b/migration/migration.c
> index cad56fbf8c..3daed2da0e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2226,7 +2226,7 @@ void qmp_migrate_set_cache_size(int64_t value, Error
> **errp)
> qmp_migrate_set_parameters(&p, errp);
> }
>
> -int64_t qmp_query_migrate_cache_size(Error **errp)
> +uint64_t qmp_query_migrate_cache_size(Error **errp)
> {
> return migrate_xbzrle_cache_size();
> }
> @@ -2456,7 +2456,7 @@ int migrate_use_xbzrle(void)
> return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
> }
>
> -int64_t migrate_xbzrle_cache_size(void)
> +uint64_t migrate_xbzrle_cache_size(void)
> {
> MigrationState *s;
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index 098b436223..b384f265fb 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -38,7 +38,7 @@ struct PageCache {
> size_t num_items;
> };
>
> -PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
> +PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> {
> int64_t i;
> size_t num_pages = new_size / page_size;
> diff --git a/migration/ram.c b/migration/ram.c
> index add5396a62..a84425d04f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -126,7 +126,7 @@ static void XBZRLE_cache_unlock(void)
> * @new_size: new cache size
> * @errp: set *errp if the check failed, with reason
> */
> -int xbzrle_cache_resize(int64_t new_size, Error **errp)
> +int xbzrle_cache_resize(uint64_t new_size, Error **errp)
> {
> PageCache *new_cache;
> int64_t ret = 0;
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@re
Re: [PATCH v2] digic: remove bios_name
On Fri, 13 Nov 2020 at 10:37, Paolo Bonzini wrote: > > On 13/11/20 11:22, Peter Maydell wrote: > >> Pull defaults to digic4_board_init so that a MachineState is available. > >> > >> Cc: Peter Maydell > >> Signed-off-by: Paolo Bonzini > >> --- > >> hw/arm/digic_boards.c | 19 +++ > >> 1 file changed, 7 insertions(+), 12 deletions(-) > > Reviewed-by: Peter Maydell > > > > Did you want me to take this via target-arm.next or are you > > planning to include it in some other series/pull ? > > I have ~15 patches each for bios_name and ram_size and I was planning to > send them all myself (most have already received acks/reviews from > maintainers), but it's the same. Yeah, I though you might. That's fine (less work for me!), just making sure we were on the same page. -- PMM
Re: [PATCH v2 006/122] m68k: do not use ram_size global
On 13/11/2020 11.15, Paolo Bonzini wrote:
> Use the machine properties instead.
>
> Cc: Laurent Vivier
> Signed-off-by: Paolo Bonzini
> ---
> hw/m68k/mcf5206.c | 4 +++-
> hw/m68k/mcf5208.c | 3 ++-
> target/m68k/m68k-semi.c | 5 +++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index 51d2e0da1c..8708aa4480 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -10,6 +10,7 @@
> #include "qemu/error-report.h"
> #include "qemu/log.h"
> #include "cpu.h"
> +#include "hw/boards.h"
> #include "hw/irq.h"
> #include "hw/m68k/mcf.h"
> #include "qemu/timer.h"
> @@ -311,8 +312,9 @@ static uint64_t m5206_mbar_read(m5206_mbar_state *s,
> /* FIXME: currently hardcoded to 128Mb. */
> {
> uint32_t mask = ~0;
> -while (mask > ram_size)
> +while (mask > current_machine->ram_size) {
> mask >>= 1;
> +}
> return mask & 0x0ffe;
> }
> case 0x5c: return 1; /* DRAM bank 1 empty. */
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index 7c8ca5ddf6..2205145ecc 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -157,8 +157,9 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
> {
> int n;
> for (n = 0; n < 32; n++) {
> -if (ram_size < (2u << n))
> +if (current_machine->ram_size < (2u << n)) {
> break;
> +}
> }
> return (n - 1) | 0x4000;
> }
> diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
> index 8e5fbfc8fa..27600e0cc0 100644
> --- a/target/m68k/m68k-semi.c
> +++ b/target/m68k/m68k-semi.c
> @@ -26,6 +26,7 @@
> #else
> #include "exec/gdbstub.h"
> #include "exec/softmmu-semi.h"
> +#include "hw/boards.h"
> #endif
> #include "qemu/log.h"
>
> @@ -455,8 +456,8 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
> * FIXME: This is wrong for boards where RAM does not start at
> * address zero.
> */
> -env->dregs[1] = ram_size;
> -env->aregs[7] = ram_size;
> +env->dregs[1] = current_machine->ram_size;
> +env->aregs[7] = current_machine->ram_size;
> #endif
> return;
> default:
>
Reviewed-by: Thomas Huth
Re: [PULL 00/11] migration queue
On Thu, 12 Nov 2020 at 18:41, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > The following changes since commit cb5d19e8294486551c422759260883ed290226d9: > > Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201112' > into staging (2020-11-12 11:33:26 +) > > are available in the Git repository at: > > git://github.com/dagrh/qemu.git tags/pull-migration-20201112a > > for you to fetch changes up to 7632b56c8f880a8f86cf049a3785069e1ffd2997: > > virtiofsd: check whether strdup lo.source return NULL in main func > (2020-11-12 16:25:38 +) > > > Migration & virtiofs fixes for 5.2 > > A bunch of small fixes. > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully
* Markus Armbruster (arm...@redhat.com) wrote:
> migrate-set-parameters passes the size to xbzrle_cache_resize().
> xbzrle_cache_resize() checks it fits into size_t before it passes it
> on to cache_init(). cache_init() checks it is a power of two no
> smaller than @page_size.
>
> cache_init() is also called from xbzrle_init(), bypassing
> xbzrle_cache_resize()'s check.
>
> Drop xbzrle_cache_resize()'s check, and check more carefully in
> cache_init().
>
> Signed-off-by: Markus Armbruster
> ---
> migration/page_cache.c | 15 ---
> migration/ram.c| 7 ---
> 2 files changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index b384f265fb..e07f4ad1dc 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -41,17 +41,10 @@ struct PageCache {
> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> {
> int64_t i;
> -size_t num_pages = new_size / page_size;
> +uint64_t num_pages = new_size / page_size;
> PageCache *cache;
>
> -if (new_size < page_size) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "is smaller than one target page size");
> -return NULL;
> -}
> -
> -/* round down to the nearest power of 2 */
> -if (!is_power_of_2(num_pages)) {
> +if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> "is not a power of two number of pages");
That error message is now wrong since it includes a whole bunch of
reasons.
Also, the comparison is now on the divided num_pages, it's not that
obvious to me that checking the num_pages makes sense in acomparison to
checking the actual cache size.
(Arguably the check should also happen in migrate_params_test_apply)
Dave
> return NULL;
> @@ -71,8 +64,8 @@ PageCache *cache_init(uint64_t new_size, size_t page_size,
> Error **errp)
> trace_migration_pagecache_init(cache->max_num_items);
>
> /* We prefer not to abort if there is no memory */
> -cache->page_cache = g_try_malloc((cache->max_num_items) *
> - sizeof(*cache->page_cache));
> +cache->page_cache = g_try_malloc_n(cache->max_num_items,
> + sizeof(*cache->page_cache));
> if (!cache->page_cache) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> "Failed to allocate page cache");
> diff --git a/migration/ram.c b/migration/ram.c
> index a84425d04f..d632ae694c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -131,13 +131,6 @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp)
> PageCache *new_cache;
> int64_t ret = 0;
>
> -/* Check for truncation */
> -if (new_size != (size_t)new_size) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "exceeding address space");
> -return -1;
> -}
> -
> if (new_size == migrate_xbzrle_cache_size()) {
> /* nothing to do */
> return 0;
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages
* Markus Armbruster (arm...@redhat.com) wrote:
> cache_init() attempts to handle allocation failure.. The two error
> messages are garbage, as untested error messages commonly are:
>
> Parameter 'cache size' expects Failed to allocate cache
> Parameter 'cache size' expects Failed to allocate page cache
>
> Fix them to just
>
> Failed to allocate cache
> Failed to allocate page cache
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/page_cache.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index e07f4ad1dc..ed979eeb45 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -53,8 +53,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size,
> Error **errp)
> /* We prefer not to abort if there is no memory */
> cache = g_try_malloc(sizeof(*cache));
> if (!cache) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "Failed to allocate cache");
> +error_setg(errp, "Failed to allocate cache");
> return NULL;
> }
> cache->page_size = page_size;
> @@ -67,8 +66,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size,
> Error **errp)
> cache->page_cache = g_try_malloc_n(cache->max_num_items,
> sizeof(*cache->page_cache));
> if (!cache->page_cache) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "Failed to allocate page cache");
> +error_setg(errp, "Failed to allocate page cache");
> g_free(cache);
> return NULL;
> }
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()
>Am 13.11.2020 um 12:13 hat lichun geschrieben:
>> monitor_parse_arguments() also need to known the current monitoar:
>> (gdb) bt
>> #0 0x55ac6a6d in mon_get_cpu_sync (mon=0x0,
>>synchronize=synchronize@entry=true) at ../monitor/misc.c:270
>> #1 0x55ac6b4a in mon_get_cpu () at ../monitor/misc.c:294
>> #2 0x55ac80fd in get_monitor_def (pval=pval@entry=0x7fffcc78,
>>name=name@entry=0x7fffcc80 "pc") at ../monitor/misc.c:1669
>> #3 0x5583fa8a in expr_unary (mon=mon@entry=0x568a75a0) at
>>../monitor/hmp.c:387
>> #4 0x5583fb32 in expr_prod (mon=mon@entry=0x568a75a0) at
>>../monitor/hmp.c:421
>> #5 0x5583fbcc in expr_logic (mon=mon@entry=0x568a75a0) at
>>../monitor/hmp.c:455
>> #6 0x5583f82c in expr_sum (mon=mon@entry=0x568a75a0) at
>>../monitor/hmp.c:484
>> #7 0x5583fc97 in get_expr (mon=mon@entry=0x568a75a0,
>>pval=pval@entry=0x7fffce18, pp=pp@entry=0x7fffce08) at
>>../monitor/hmp.c:511
>> #8 0x558409b1 in monitor_parse_arguments
>>(mon=mon@entry=0x568a75a0, cmd=0x56561e40 ,
>>cmd=0x56561e40 , endp=0x7fffd288) at
>>../monitor/hmp.c:876
>> #9 0x55841796 in handle_hmp_command (mon=mon@entry=0x568a75a0,
>>cmdline=0x568b12b3 "$pc", cmdline@entry=0x568b12b0 "xp $pc") at
>>../monitor/hmp.c:1073
>> Therefore update current monitor as soon as possible to avoid
>> hmp/xp command failure.
>>
>> Fixes: ff04108a0e36 ("hmp: Update current monitor only in
>> handle_hmp_command()")
>> Signed-off-by: lichun
>> ---
>> monitor/hmp.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>> index c5cd9d3..ee5413e 100644
>> --- a/monitor/hmp.c
>> +++ b/monitor/hmp.c
>> @@ -1072,52 +1072,52 @@ static void handle_hmp_command_co(void *opaque)
>> }
>>
>> void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>> {
>> QDict *qdict;
>> const HMPCommand *cmd;
>> const char *cmd_start = cmdline;
>>
>> trace_handle_hmp_command(mon, cmdline);
>>
>> + /* old_mon is non-NULL when called from qmp_human_monitor_command() */
>> + Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common);
>> +
>> cmd = monitor_parse_command(mon, cmdline, &cmdline, hmp_cmds);
>> if (!cmd) {
>> return;
>> }
>
>Now the monitor isn't changed back in all early return cases.
>
>>
>> qdict = monitor_parse_arguments(&mon->common, &cmdline, cmd);
>> if (!qdict) {
>> while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
>> cmdline--;
>> }
>> monitor_printf(&mon->common, "Try \"help %.*s\" for more
>>information\n",
>> (int)(cmdline - cmd_start), cmd_start);
>> return;
>> }
>>
>> if (!cmd->coroutine) {
>> - /* old_mon is non-NULL when called from qmp_human_monitor_command()
>> */
>> - Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(),
>> &mon->common);
>> cmd->cmd(&mon->common, qdict);
>> - monitor_set_cur(qemu_coroutine_self(), old_mon);
>> } else {
>> HandleHmpCommandCo data = {
>> .mon = &mon->common,
>> .cmd = cmd,
>> .qdict = qdict,
>> .done = false,
>> };
>> Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
>> - monitor_set_cur(co, &mon->common);
>
>Removing this line is wrong, we still need to set the current monitor
>for co, which is not qemu_coroutine_self() self.
>
>> aio_co_enter(qemu_get_aio_context(), co);
>> AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
>> }
>> + monitor_set_cur(qemu_coroutine_self(), old_mon);
>>
>> qobject_unref(qdict);
>> }
>
>With the above bugs fixed, this approach is one option to fix the bug.
>
>Personally, if it's possible with reasonable effort, I would prefer the
>other way, which is making sure that monitor_cur() isn't used, but the
>Monitor pointer is just passed down. This would be a bigger change, but
>it wouldn't only fix the bug, but also clean up the code and make it
>more maintainable.
>
>I can try to write a patch series to do it this way and see how it goes.
This is the best way, I will not post v2. This bug will be fixed by that
series.
Transfer the work to you Kevin.
>
>Kevin
>
RE: [PATCH v3 2/4] ads7846: put it into the 'input' category
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Friday, November 13, 2020 6:13 PM
> To: ganqixin
> Cc: QEMU Developers ; QEMU Trivial
> ; Thomas Huth ; Laurent
> Vivier ; Philippe Mathieu-Daudé ;
> Markus Armbruster ; Michael S. Tsirkin
> ; Chenqun (kuhn) ;
> Zhanghailiang
> Subject: Re: [PATCH v3 2/4] ads7846: put it into the 'input' category
>
> On Fri, 13 Nov 2020 at 03:32, Gan Qixin wrote:
> >
> > The category of the ads7846 device is not set, put it into the 'input'
> > category.
> >
> > Signed-off-by: Gan Qixin
> > ---
> > Cc: Peter Maydell
> > ---
> > hw/display/ads7846.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c index
> > 023165b2a3..cb3a431cfd 100644
> > --- a/hw/display/ads7846.c
> > +++ b/hw/display/ads7846.c
> > @@ -163,10 +163,12 @@ static void ads7846_realize(SSISlave *d, Error
> > **errp)
> >
> > static void ads7846_class_init(ObjectClass *klass, void *data) {
> > +DeviceClass *dc = DEVICE_CLASS(klass);
> > SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
> >
> > k->realize = ads7846_realize;
> > k->transfer = ads7846_transfer;
> > +set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> > }
> >
> > static const TypeInfo ads7846_info = {
>
> Reviewed-by: Peter Maydell
>
> Really we should move the file too...
>
OK, I will try to move the file to the correct folder:)
Gan Qixin
Re: [PATCH v2 006/122] m68k: do not use ram_size global
On 13/11/20 11:44, Thomas Huth wrote: On 13/11/2020 11.15, Paolo Bonzini wrote: Use the machine properties instead. Cc: Laurent Vivier Signed-off-by: Paolo Bonzini Thanks -- of course this was just an unintentional send, but I'll note your Reviewed-by nevertheless. Paolo Reviewed-by: Thomas Huth
Re: [PATCH v3 09/53] qdev: Make qdev_get_prop_ptr() get Object* arg
On Thu, 12 Nov 2020 16:43:06 -0500 Eduardo Habkost wrote: > Make the code more generic and not specific to TYPE_DEVICE. > > Reviewed-by: Marc-André Lureau > Signed-off-by: Eduardo Habkost > --- > Changes v1 -> v2: > - Fix build error with CONFIG_XEN > I took the liberty of keeping the Reviewed-by line from > Marc-André as the build fix is a trivial one line change > --- > Cc: Stefan Berger > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: Kevin Wolf > Cc: Max Reitz > Cc: Paolo Bonzini > Cc: "Daniel P. Berrangé" > Cc: Eduardo Habkost > Cc: Cornelia Huck > Cc: Thomas Huth > Cc: Richard Henderson > Cc: David Hildenbrand > Cc: Halil Pasic > Cc: Christian Borntraeger > Cc: Matthew Rosato > Cc: Alex Williamson > Cc: qemu-devel@nongnu.org > Cc: xen-de...@lists.xenproject.org > Cc: qemu-bl...@nongnu.org > Cc: qemu-s3...@nongnu.org > --- > include/hw/qdev-properties.h | 2 +- > backends/tpm/tpm_util.c | 8 ++-- > hw/block/xen-block.c | 5 +- > hw/core/qdev-properties-system.c | 57 +- > hw/core/qdev-properties.c| 82 +--- > hw/s390x/css.c | 5 +- > hw/s390x/s390-pci-bus.c | 4 +- > hw/vfio/pci-quirks.c | 5 +- > 8 files changed, 68 insertions(+), 100 deletions(-) Reviewed-by: Cornelia Huck #s390 parts
Re: [PATCH v3 12/53] qdev: Make error_set_from_qdev_prop_error() get Object* argument
On Thu, 12 Nov 2020 16:43:09 -0500 Eduardo Habkost wrote: > Make the code more generic and not specific to TYPE_DEVICE. > > Reviewed-by: Marc-André Lureau > Signed-off-by: Eduardo Habkost > --- > Cc: Paolo Bonzini > Cc: "Daniel P. Berrangé" > Cc: Eduardo Habkost > Cc: Cornelia Huck > Cc: Thomas Huth > Cc: Richard Henderson > Cc: David Hildenbrand > Cc: Halil Pasic > Cc: Christian Borntraeger > Cc: qemu-devel@nongnu.org > Cc: qemu-s3...@nongnu.org > --- > include/hw/qdev-properties.h | 2 +- > hw/core/qdev-properties-system.c | 10 +- > hw/core/qdev-properties.c| 10 +- > hw/s390x/css.c | 2 +- > 4 files changed, 12 insertions(+), 12 deletions(-) Reviewed-by: Cornelia Huck #s390 parts
Re: [PATCH 6/6] migration: Fix a few absurdly defective error messages
* Markus Armbruster (arm...@redhat.com) wrote:
> migrate_params_check() has a number of error messages of the form
>
> Parameter 'NAME' expects is invalid, it should be ...
>
> Fix them to something like
>
> Parameter 'NAME' expects a ...
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/migration.c | 23 +++
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 3daed2da0e..5f9a10909d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1240,21 +1240,21 @@ static bool migrate_params_check(MigrationParameters
> *params, Error **errp)
> if (params->has_compress_level &&
> (params->compress_level > 9)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> - "is invalid, it should be in the range of 0 to 9");
> + "a value between 0 and 9");
> return false;
> }
>
> if (params->has_compress_threads && (params->compress_threads < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "compress_threads",
> - "is invalid, it should be in the range of 1 to 255");
> + "a value between 1 and 255");
> return false;
> }
>
> if (params->has_decompress_threads && (params->decompress_threads < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "decompress_threads",
> - "is invalid, it should be in the range of 1 to 255");
> + "a value between 1 and 255");
> return false;
> }
>
> @@ -1307,21 +1307,21 @@ static bool migrate_params_check(MigrationParameters
> *params, Error **errp)
> if (params->has_multifd_channels && (params->multifd_channels < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "multifd_channels",
> - "is invalid, it should be in the range of 1 to 255");
> + "a value between 1 and 255");
> return false;
> }
>
> if (params->has_multifd_zlib_level &&
> (params->multifd_zlib_level > 9)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level",
> - "is invalid, it should be in the range of 0 to 9");
> + "a value between 0 and 9");
> return false;
> }
>
> if (params->has_multifd_zstd_level &&
> (params->multifd_zstd_level > 20)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
> - "is invalid, it should be in the range of 0 to 20");
> + "a value between 0 and 20");
> return false;
> }
>
> @@ -1330,8 +1330,7 @@ static bool migrate_params_check(MigrationParameters
> *params, Error **errp)
> !is_power_of_2(params->xbzrle_cache_size))) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "xbzrle_cache_size",
> - "is invalid, it should be bigger than target page size"
> - " and a power of 2");
> + "a power of two no less than the target page size");
> return false;
> }
>
> @@ -1348,21 +1347,21 @@ static bool migrate_params_check(MigrationParameters
> *params, Error **errp)
> params->announce_initial > 10) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "announce_initial",
> - "is invalid, it must be less than 10 ms");
> + "a value between 0 and 10");
> return false;
> }
> if (params->has_announce_max &&
> params->announce_max > 10) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "announce_max",
> - "is invalid, it must be less than 10 ms");
> + "a value between 0 and 10");
> return false;
> }
> if (params->has_announce_rounds &&
> params->announce_rounds > 1000) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "announce_rounds",
> - "is invalid, it must be in the range of 0 to 1000");
> + "a value between 0 and 1000");
> return false;
> }
> if (params->has_announce_step &&
> @@ -1370,7 +1369,7 @@ static bool migrate_params_check(MigrationParameters
> *params, Error **errp)
> params->announce_step > 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "announce_step",
> - "is invalid, it must be in the range of 1 to 1 ms");
> + "a value between 0 and 1");
> return false;
> }
>
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PULL 0/1] vfio fix for QEMU 5.2-rc2
On Thu, 12 Nov 2020 at 23:07, Alex Williamson wrote: > > The following changes since commit cb5d19e8294486551c422759260883ed290226d9: > > Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201112' > into staging (2020-11-12 11:33:26 +) > > are available in the Git repository at: > > git://github.com/awilliam/qemu-vfio.git tags/vfio-update-20201112.0 > > for you to fetch changes up to e408aeef8663fd6e3075aef252404c55d710a75e: > > Fix use after free in vfio_migration_probe (2020-11-12 15:58:16 -0700) > > > VFIO update 2020-11-12 > > * Fix coverity reported use-after-free (Kirti Wankhede) > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
Re: [PATCH v3 1/2] quorum: Implement bdrv_co_block_status()
On 11.11.20 17:53, Alberto Garcia wrote: The quorum driver does not implement bdrv_co_block_status() and because of that it always reports to contain data even if all its children are known to be empty. One consequence of this is that if we for example create a quorum with a size of 10GB and we mirror it to a new image the operation will write 10GB of actual zeroes to the destination image wasting a lot of time and disk space. Since a quorum has an arbitrary number of children of potentially different formats there is no way to report all possible allocation status flags in a way that makes sense, so this implementation only reports when a given region is known to contain zeroes (BDRV_BLOCK_ZERO) or not (BDRV_BLOCK_DATA). If all children agree that a region contains zeroes then we can return BDRV_BLOCK_ZERO using the smallest size reported by the children (because all agree that a region of at least that size contains zeroes). If at least one child disagrees we have to return BDRV_BLOCK_DATA. In this case we use the largest of the sizes reported by the children that didn't return BDRV_BLOCK_ZERO (because we know that there won't be an agreement for at least that size). Signed-off-by: Alberto Garcia Tested-by: Tao Xu --- block/quorum.c | 52 + tests/qemu-iotests/312 | 148 + tests/qemu-iotests/312.out | 67 + tests/qemu-iotests/group | 1 + 4 files changed, 268 insertions(+) create mode 100755 tests/qemu-iotests/312 create mode 100644 tests/qemu-iotests/312.out Reviewed-by: Max Reitz
[PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
When I restricted the section where the current monitor is set to only the command handler, I missed that monitor_parse_arguments() can use it indirectly, too, when evaluating register variables. These cases get NULL now and crash (easy to reproduce with "x $pc"). This series passes the right monitor object down instead of using monitor_cur(), which fixes the crash. Kevin Wolf (3): hmp: Pass monitor to mon_get_cpu() hmp: Pass monitor to MonitorDef.get_value() hmp: Pass monitor to mon_get_cpu_env() include/monitor/hmp-target.h | 7 --- monitor/monitor-internal.h | 2 +- monitor/hmp.c| 2 +- monitor/misc.c | 24 target/i386/monitor.c| 11 ++- target/m68k/monitor.c| 2 +- target/nios2/monitor.c | 2 +- target/ppc/monitor.c | 22 +- target/riscv/monitor.c | 2 +- target/sh4/monitor.c | 2 +- target/sparc/monitor.c | 12 +++- target/xtensa/monitor.c | 2 +- 12 files changed, 49 insertions(+), 41 deletions(-) -- 2.28.0
[PATCH for-5.2 2/3] hmp: Pass monitor to MonitorDef.get_value()
All of these callbacks use mon_get_cpu_env(). Pass the Monitor
pointer to them it in preparation for adding a monitor argument to
mon_get_cpu_env().
Signed-off-by: Kevin Wolf
---
include/monitor/hmp-target.h | 3 ++-
monitor/misc.c | 2 +-
target/i386/monitor.c| 3 ++-
target/ppc/monitor.c | 12
target/sparc/monitor.c | 6 --
5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 519616d1fb..385fb18664 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -33,7 +33,8 @@
struct MonitorDef {
const char *name;
int offset;
-target_long (*get_value)(const struct MonitorDef *md, int val);
+target_long (*get_value)(Monitor *mon, const struct MonitorDef *md,
+ int val);
int type;
};
diff --git a/monitor/misc.c b/monitor/misc.c
index c918d6bd08..f566e28174 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1678,7 +1678,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const
char *name)
for(; md->name != NULL; md++) {
if (hmp_compare_cmd(name, md->name)) {
if (md->get_value) {
-*pval = md->get_value(md, md->offset);
+*pval = md->get_value(mon, md, md->offset);
} else {
CPUArchState *env = mon_get_cpu_env();
ptr = (uint8_t *)env + md->offset;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 5ca3cab4ec..fed4606aeb 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -601,7 +601,8 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
}
}
-static target_long monitor_get_pc(const struct MonitorDef *md, int val)
+static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return env->eip + env->segs[R_CS].base;
diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index a5a177d717..9c0fc2b8c3 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -29,7 +29,8 @@
#include "monitor/hmp-target.h"
#include "monitor/hmp.h"
-static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
+static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
unsigned int u;
@@ -43,19 +44,22 @@ static target_long monitor_get_ccr(const struct MonitorDef
*md, int val)
return u;
}
-static target_long monitor_get_decr(const struct MonitorDef *md, int val)
+static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
+int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_decr(env);
}
-static target_long monitor_get_tbu(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_tbu(env);
}
-static target_long monitor_get_tbl(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_tbl(env);
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index a7ea287cbc..bf979d6520 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -40,7 +40,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
}
#ifndef TARGET_SPARC64
-static target_long monitor_get_psr (const struct MonitorDef *md, int val)
+static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
@@ -48,7 +49,8 @@ static target_long monitor_get_psr (const struct MonitorDef
*md, int val)
}
#endif
-static target_long monitor_get_reg(const struct MonitorDef *md, int val)
+static target_long monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return env->regwptr[val];
--
2.28.0
[PATCH for-5.2 1/3] hmp: Pass monitor to mon_get_cpu()
mon_get_cpu() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur(),
explicitly pass the Monitor pointer to the function.
Signed-off-by: Kevin Wolf
---
include/monitor/hmp-target.h | 2 +-
monitor/monitor-internal.h | 2 +-
monitor/hmp.c| 2 +-
monitor/misc.c | 18 +-
target/i386/monitor.c| 2 +-
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 8b7820a3ad..519616d1fb 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ const MonitorDef *target_monitor_defs(void);
int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
CPUArchState *mon_get_cpu_env(void);
-CPUState *mon_get_cpu(void);
+CPUState *mon_get_cpu(Monitor *mon);
void hmp_info_mem(Monitor *mon, const QDict *qdict);
void hmp_info_tlb(Monitor *mon, const QDict *qdict);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index ad2e64be13..a6131554da 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -178,7 +178,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
void monitor_data_destroy_qmp(MonitorQMP *mon);
void coroutine_fn monitor_qmp_dispatcher_co(void *data);
-int get_monitor_def(int64_t *pval, const char *name);
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
void help_cmd(Monitor *mon, const char *name);
void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
int hmp_compare_cmd(const char *name, const char *list);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index c5cd9d372b..1204233999 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -384,7 +384,7 @@ static int64_t expr_unary(Monitor *mon)
pch++;
}
*q = 0;
-ret = get_monitor_def(®, buf);
+ret = get_monitor_def(mon, ®, buf);
if (ret < 0) {
expr_error(mon, "unknown register");
}
diff --git a/monitor/misc.c b/monitor/misc.c
index 32e6a8c13d..c918d6bd08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -289,14 +289,14 @@ static CPUState *mon_get_cpu_sync(Monitor *mon, bool
synchronize)
return cpu;
}
-CPUState *mon_get_cpu(void)
+CPUState *mon_get_cpu(Monitor *mon)
{
-return mon_get_cpu_sync(monitor_cur(), true);
+return mon_get_cpu_sync(mon, true);
}
CPUArchState *mon_get_cpu_env(void)
{
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(monitor_cur());
return cs ? cs->env_ptr : NULL;
}
@@ -319,7 +319,7 @@ static void hmp_info_registers(Monitor *mon, const QDict
*qdict)
cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
}
} else {
-cs = mon_get_cpu();
+cs = mon_get_cpu(mon);
if (!cs) {
monitor_printf(mon, "No CPU available\n");
@@ -381,7 +381,7 @@ static void hmp_info_history(Monitor *mon, const QDict
*qdict)
static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
{
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
if (!cs) {
monitor_printf(mon, "No CPU available\n");
@@ -546,7 +546,7 @@ static void memory_dump(Monitor *mon, int count, int
format, int wsize,
int l, line_size, i, max_digits, len;
uint8_t buf[16];
uint64_t v;
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
if (!cs && (format == 'i' || !is_physical)) {
monitor_printf(mon, "Can not dump without CPU\n");
@@ -711,7 +711,7 @@ static void hmp_gva2gpa(Monitor *mon, const QDict *qdict)
{
target_ulong addr = qdict_get_int(qdict, "addr");
MemTxAttrs attrs;
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
hwaddr gpa;
if (!cs) {
@@ -1663,10 +1663,10 @@ HMPCommand hmp_cmds[] = {
* Set @pval to the value in the register identified by @name.
* return 0 if OK, -1 if not found
*/
-int get_monitor_def(int64_t *pval, const char *name)
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
{
const MonitorDef *md = target_monitor_defs();
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
void *ptr;
uint64_t tmp = 0;
int ret;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 7abae3c8df..5ca3cab4ec 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -656,7 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
int id = qdict_get_try_int(qdict, "apic-id", 0);
cs = cpu_by_arch_id(id);
} else {
-cs = mon_get_cpu();
+cs = mon_get_cpu(mon);
}
--
2.28.0
[PATCH for-5.2 3/3] hmp: Pass monitor to mon_get_cpu_env()
mon_get_cpu_env() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur_env(),
explicitly pass the Monitor pointer to the function.
Without this fix, an HMP command like "x $pc" crashes like this:
#0 0x55caa01f in mon_get_cpu_sync (mon=0x0, synchronize=true) at
../monitor/misc.c:270
#1 0x55caa141 in mon_get_cpu (mon=0x0) at ../monitor/misc.c:294
#2 0x55caa158 in mon_get_cpu_env () at ../monitor/misc.c:299
#3 0x55b19739 in monitor_get_pc (mon=0x56ad2de0,
md=0x565d2d40 , val=0) at ../target/i386/monitor.c:607
#4 0x55cadbec in get_monitor_def (mon=0x56ad2de0,
pval=0x7fffc208, name=0x7fffc220 "pc") at ../monitor/misc.c:1681
#5 0x5582ec4f in expr_unary (mon=0x56ad2de0) at
../monitor/hmp.c:387
#6 0x5582edbb in expr_prod (mon=0x56ad2de0) at
../monitor/hmp.c:421
#7 0x5582ee79 in expr_logic (mon=0x56ad2de0) at
../monitor/hmp.c:455
#8 0x5582eefe in expr_sum (mon=0x56ad2de0) at
../monitor/hmp.c:484
#9 0x5582efe8 in get_expr (mon=0x56ad2de0, pval=0x7fffc418,
pp=0x7fffc408) at ../monitor/hmp.c:511
#10 0x5582fcd4 in monitor_parse_arguments (mon=0x56ad2de0,
endp=0x7fffc890, cmd=0x56675b50 ) at ../monitor/hmp.c:876
#11 0x558306a8 in handle_hmp_command (mon=0x56ad2de0,
cmdline=0x56ada452 "$pc") at ../monitor/hmp.c:1087
#12 0x5582df14 in monitor_command_cb (opaque=0x56ad2de0,
cmdline=0x56ada450 "x $pc", readline_opaque=0x0) at ../monitor/hmp.c:47
After this fix, nothing is left in monitor_parse_arguments() that can
indirectly call monitor_cur(), so the fix is complete.
Fixes: ff04108a0e36e822519c517bd3bddbc1c7747c18
Reported-by: lichun
Signed-off-by: Kevin Wolf
---
include/monitor/hmp-target.h | 2 +-
monitor/misc.c | 6 +++---
target/i386/monitor.c| 6 +++---
target/m68k/monitor.c| 2 +-
target/nios2/monitor.c | 2 +-
target/ppc/monitor.c | 10 +-
target/riscv/monitor.c | 2 +-
target/sh4/monitor.c | 2 +-
target/sparc/monitor.c | 6 +++---
target/xtensa/monitor.c | 2 +-
10 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 385fb18664..60fc92722a 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ struct MonitorDef {
const MonitorDef *target_monitor_defs(void);
int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
-CPUArchState *mon_get_cpu_env(void);
+CPUArchState *mon_get_cpu_env(Monitor *mon);
CPUState *mon_get_cpu(Monitor *mon);
void hmp_info_mem(Monitor *mon, const QDict *qdict);
diff --git a/monitor/misc.c b/monitor/misc.c
index f566e28174..398211a034 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -294,9 +294,9 @@ CPUState *mon_get_cpu(Monitor *mon)
return mon_get_cpu_sync(mon, true);
}
-CPUArchState *mon_get_cpu_env(void)
+CPUArchState *mon_get_cpu_env(Monitor *mon)
{
-CPUState *cs = mon_get_cpu(monitor_cur());
+CPUState *cs = mon_get_cpu(mon);
return cs ? cs->env_ptr : NULL;
}
@@ -1680,7 +1680,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const
char *name)
if (md->get_value) {
*pval = md->get_value(mon, md, md->offset);
} else {
-CPUArchState *env = mon_get_cpu_env();
+CPUArchState *env = mon_get_cpu_env(mon);
ptr = (uint8_t *)env + md->offset;
switch(md->type) {
case MD_I32:
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index fed4606aeb..9f9e1c42f4 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -222,7 +222,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
{
CPUArchState *env;
-env = mon_get_cpu_env();
+env = mon_get_cpu_env(mon);
if (!env) {
monitor_printf(mon, "No CPU available\n");
return;
@@ -550,7 +550,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
{
CPUArchState *env;
-env = mon_get_cpu_env();
+env = mon_get_cpu_env(mon);
if (!env) {
monitor_printf(mon, "No CPU available\n");
return;
@@ -604,7 +604,7 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
int val)
{
-CPUArchState *env = mon_get_cpu_env();
+CPUArchState *env = mon_get_cpu_env(mon);
return env->eip + env->segs[R_CS].base;
}
diff --git a/target/m68k/monitor.c b/target/m68k/monitor.c
index 2055fe8a00..2bdf6acae0 100644
--- a/target/m68k/monitor.c
+++ b/target/m68k/monitor.c
@@ -12,7 +12,7 @@
void hmp_info_tlb(Monitor *mon, const QDict *qdict)
{
-CPUArchState *env1 = mon_get_cpu_env();
+CPUArchState *en
Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()
On 11.11.20 17:53, Alberto Garcia wrote:
This simply calls bdrv_co_pwrite_zeroes() in all children
Signed-off-by: Alberto Garcia
---
block/quorum.c | 18 --
tests/qemu-iotests/312 | 7 +++
tests/qemu-iotests/312.out | 4
3 files changed, 27 insertions(+), 2 deletions(-)
Should we set supported_zero_flags to something? I think we can at
least set BDRV_REQ_NO_FALLBACK. We could also try BDRV_REQ_FUA.
diff --git a/block/quorum.c b/block/quorum.c
index 9691a9bee9..c81572f513 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -692,8 +692,13 @@ static void write_quorum_entry(void *opaque)
QuorumChildRequest *sacb = &acb->qcrs[i];
sacb->bs = s->children[i]->bs;
-sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
-acb->qiov, acb->flags);
+if (acb->flags & BDRV_REQ_ZERO_WRITE) {
+sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
+ acb->bytes, acb->flags);
+} else {
+sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
+acb->qiov, acb->flags);
+}
Seems unnecessary (bdrv_co_pwritev() can handle BDRV_REQ_ZERO_WRITE),
but perhaps it’s good to be explicit.
if (sacb->ret == 0) {
acb->success_count++;
} else {
[...]
diff --git a/tests/qemu-iotests/312 b/tests/qemu-iotests/312
index 1b08f1552f..93046393e7 100755
--- a/tests/qemu-iotests/312
+++ b/tests/qemu-iotests/312
@@ -114,6 +114,13 @@ $QEMU_IO -c "write -P 0 $((0x20)) $((0x1))"
"$TEST_IMG.0" | _filter_qemu
$QEMU_IO -c "write -z $((0x20)) $((0x3))" "$TEST_IMG.1" |
_filter_qemu_io
$QEMU_IO -c "write -P 0 $((0x20)) $((0x2))" "$TEST_IMG.2" |
_filter_qemu_io
+# Test 5: write data to a region and then zeroize it, doing it
+# directly on the quorum device instead of the individual images.
+# This has no effect on the end result but proves that the quorum driver
+# supports 'write -z'.
+$QEMU_IO -c "open -o $quorum" -c "write $((0x25)) $((0x1))" |
_filter_qemu_io
+$QEMU_IO -c "open -o $quorum" -c "write -z $((0x25)) $((0x1))" |
_filter_qemu_io
+
My gut would have preferred a test where the data region is larger than
the zeroed region (so we can see that the first write has done
something), but who cares about my gut.
I don’t mind not setting supported_zero_flags enough to warrant
withholding a
Reviewed-by: Max Reitz
But I’ll give you some time to reply before I’d take this patch to
block-next. (That is, unless Kevin takes it during my two-week PTO...)
echo
echo '### Launch the drive-mirror job'
echo
Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation
* Markus Armbruster (arm...@redhat.com) wrote:
> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
> switched MigrationParameters to narrower integer types, and removed
> the simplified qmp_migrate_set_parameters()'s argument checking
> accordingly.
>
> Good idea, except qmp_migrate_set_parameters() takes
> MigrateSetParameters, not MigrationParameters. Its job is updating
> migrate_get_current()->parameters (which *is* of type
> MigrationParameters) according to its argument. The integers now get
> truncated silently. Reproducer:
>
> ---> {'execute': 'query-migrate-parameters'}
> <--- {"return": {[...] "compress-threads": 8, [...]}}
> ---> {"execute": "migrate-set-parameters", "arguments":
> {"compress-threads": 257}}
> <--- {"return": {}}
> ---> {'execute': 'query-migrate-parameters'}
> <--- {"return": {[...] "compress-threads": 1, [...]}}
>
> Fix by resynchronizing MigrateSetParameters with MigrationParameters.
Having those two separate types is a pain!
> Fixes: 741d4086c856320807a2575389d7c0505578270b
> Signed-off-by: Markus Armbruster
Reviewed-by: Dr. David Alan Gilbert
> ---
> qapi/migration.json | 28 ++--
> monitor/hmp-cmds.c | 24
> 2 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 688e8da749..3ad3720cf0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -885,28 +885,28 @@
> '*announce-max': 'size',
> '*announce-rounds': 'size',
> '*announce-step': 'size',
> -'*compress-level': 'int',
> -'*compress-threads': 'int',
> +'*compress-level': 'uint8',
> +'*compress-threads': 'uint8',
> '*compress-wait-thread': 'bool',
> -'*decompress-threads': 'int',
> -'*throttle-trigger-threshold': 'int',
> -'*cpu-throttle-initial': 'int',
> -'*cpu-throttle-increment': 'int',
> +'*decompress-threads': 'uint8',
> +'*throttle-trigger-threshold': 'uint8',
> +'*cpu-throttle-initial': 'uint8',
> +'*cpu-throttle-increment': 'uint8',
> '*cpu-throttle-tailslow': 'bool',
> '*tls-creds': 'StrOrNull',
> '*tls-hostname': 'StrOrNull',
> '*tls-authz': 'StrOrNull',
> -'*max-bandwidth': 'int',
> -'*downtime-limit': 'int',
> -'*x-checkpoint-delay': 'int',
> +'*max-bandwidth': 'size',
> +'*downtime-limit': 'uint64',
> +'*x-checkpoint-delay': 'uint32',
> '*block-incremental': 'bool',
> -'*multifd-channels': 'int',
> +'*multifd-channels': 'uint8',
> '*xbzrle-cache-size': 'size',
> '*max-postcopy-bandwidth': 'size',
> -'*max-cpu-throttle': 'int',
> +'*max-cpu-throttle': 'uint8',
> '*multifd-compression': 'MultiFDCompression',
> -'*multifd-zlib-level': 'int',
> -'*multifd-zstd-level': 'int',
> +'*multifd-zlib-level': 'uint8',
> +'*multifd-zstd-level': 'uint8',
> '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>
> ##
> @@ -1093,7 +1093,7 @@
> '*max-bandwidth': 'size',
> '*downtime-limit': 'uint64',
> '*x-checkpoint-delay': 'uint32',
> -'*block-incremental': 'bool' ,
> +'*block-incremental': 'bool',
> '*multifd-channels': 'uint8',
> '*xbzrle-cache-size': 'size',
> '*max-postcopy-bandwidth': 'size',
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 492789248f..f8ef061510 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1292,11 +1292,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
> switch (val) {
> case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> p->has_compress_level = true;
> -visit_type_int(v, param, &p->compress_level, &err);
> +visit_type_uint8(v, param, &p->compress_level, &err);
> break;
> case MIGRATION_PARAMETER_COMPRESS_THREADS:
> p->has_compress_threads = true;
> -visit_type_int(v, param, &p->compress_threads, &err);
> +visit_type_uint8(v, param, &p->compress_threads, &err);
> break;
> case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
> p->has_compress_wait_thread = true;
> @@ -1304,19 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
> break;
> case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
> p->has_decompress_threads = true;
> -visit_type_int(v, param, &p->decompress_threads, &err);
> +visit_type_uint8(v, param, &p->decompress_threads, &err);
> break;
> case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
> p->has_throttle_tri
Re: [PATCH 08/13] char: Add mux option to ChardevOptions
On 12/11/20 18:59, Kevin Wolf wrote:
The final missing piece to achieve compatibility between
qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
is support for the 'mux' option. Implement it.
Signed-off-by: Kevin Wolf
---
qapi/char.json | 4 +++-
chardev/char.c | 41 +++--
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/qapi/char.json b/qapi/char.json
index e1f9347044..d6733a5473 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -453,12 +453,14 @@
#
# @id: the chardev's ID, must be unique
# @backend: backend type and parameters
+# @mux: enable multiplexing mode (default: false)
#
# Since: 6.0
##
{ 'struct': 'ChardevOptions',
'data': { 'id': 'str',
-'backend': 'ChardevBackend' },
+'backend': 'ChardevBackend',
+'*mux': 'bool' },
'aliases': [ { 'source': ['backend'] } ] }
I think this shows that -chardev and chardev-add are both kind of
unrepairable. The right way to do a mux with a serial and monitor on
top should be explicit:
stdio
↑
mux-controller
↑↑
mux mux
↑↑
serial monitor
-object chardev-stdio,id=stdio
-object chardev-mux-controller,id=mux,backend=stdio
-object chardev-mux,id=serial-chardev,controller=mux
-object chardev-mux,id=mon-chardev,controller=mux
-serial chardev:serial-chardev
-monitor chardev:mon-chardev
Adding automagic "mux" creation to -chardev is wrong. I don't entirely
object to the series since it's quite nice, but I see it as more of a
cleanup than the final stage. It hinges on what Markus thinks of
aliases, I guess.
Paolo
Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz
* Markus Armbruster (arm...@redhat.com) wrote:
> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> parameter" added MigrationParameters member @tls-authz. Whereas the
> other members aren't really optional (see commit 1bda8b3c695), this
> one is genuinely optional: migration_instance_init() leaves it absent,
> and migration_tls_channel_process_incoming() passes it to
> qcrypto_tls_session_new(), which checks for null.
>
> Commit d2f1d29b95 has a number of issues, though:
>
> * When qmp_query_migrate_parameters() copies migration parameters into
> its reply, it ignores has_tls_authz, and assumes true instead. When
> it is false,
>
> - HMP info migrate_parameters prints the null pointer (crash bug on
> some systems), and
>
> - QMP query-migrate-parameters replies "tls-authz": "" (because the
> QObject output visitor silently maps null pointer to "", which it
> really shouldn't).
>
> The HMP defect was noticed and fixed in commit 7cd75cbdb8
> 'migration: use "" instead of (null) for tls-authz'. Unfortunately,
> the fix papered over the real bug: it made
> qmp_query_migrate_parameters() map null tls_authz to "". It also
> dropped the check for has_tls_authz from
> hmp_info_migrate_parameters().
>
> Revert, and fix qmp_query_migrate_parameters() not to screw up
> has_tls_authz. No change to HMP. QMP now has "tls-authz" in the
> reply only when it's actually present in
> migrate_get_current()->parameters. If we prefer to remain
> bug-compatible, we should make tls_authz non-optional there.
>
> * migrate_params_test_apply() neglects to apply tls_authz. Currently
> harmless, because migrate_params_check() doesn't care. Fix it
> anyway.
>
> * qmp_migrate_set_parameters() crashes:
>
> {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
>
> Add the necessary rewrite of null to "". For background
> information, see commit 01fa559826 "migration: Use JSON null instead
> of "" to reset parameter to default".
>
> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> Cc: Daniel P. Berrangé
> Signed-off-by: Markus Armbruster
Yes, but I'd like an Ack from Dan as well for this one
Reviewed-by: Dr. David Alan Gilbert
> ---
> qapi/migration.json | 2 +-
> migration/migration.c | 17 ++---
> monitor/hmp-cmds.c| 2 +-
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..688e8da749 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -928,7 +928,7 @@
> ##
> # @MigrationParameters:
> #
> -# The optional members aren't actually optional.
> +# The optional members aren't actually optional, except for @tls-authz.
> #
> # @announce-initial: Initial delay (in milliseconds) before sending the
> #first announce (Since 4.0)
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa55a9..cad56fbf8c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error
> **errp)
> params->tls_creds = g_strdup(s->parameters.tls_creds);
> params->has_tls_hostname = true;
> params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> -params->has_tls_authz = true;
> -params->tls_authz = g_strdup(s->parameters.tls_authz ?
> - s->parameters.tls_authz : "");
> +params->has_tls_authz = s->parameters.has_tls_authz;
> +params->tls_authz = g_strdup(s->parameters.tls_authz);
> params->has_max_bandwidth = true;
> params->max_bandwidth = s->parameters.max_bandwidth;
> params->has_downtime_limit = true;
> @@ -1433,6 +1432,11 @@ static void
> migrate_params_test_apply(MigrateSetParameters *params,
> dest->tls_hostname = params->tls_hostname->u.s;
> }
>
> +if (params->has_tls_authz) {
> +assert(params->tls_authz->type == QTYPE_QSTRING);
> +dest->tls_authz = params->tls_authz->u.s;
> +}
> +
> if (params->has_max_bandwidth) {
> dest->max_bandwidth = params->max_bandwidth;
> }
> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters
> *params, Error **errp)
> params->tls_hostname->type = QTYPE_QSTRING;
> params->tls_hostname->u.s = strdup("");
> }
> +/* TODO Rewrite "" to null instead */
> +if (params->has_tls_authz
> +&& params->tls_authz->type == QTYPE_QNULL) {
> +qobject_unref(params->tls_authz->u.n);
> +params->tls_authz->type = QTYPE_QSTRING;
> +params->tls_authz->u.s = strdup("");
> +}
>
> migrate_params_test_apply(params, &tmp);
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..492789248f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const
> QDict *qdict)
>
Re: [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters
* Markus Armbruster (arm...@redhat.com) wrote: > Not sure about 5.2. The bugs aren't recent regressions. Lets leave it till 5.3 > Markus Armbruster (6): > migration: Fix and clean up around @tls-authz > migration: Fix migrate-set-parameters argument validation > migration: Clean up signed vs. unsigned XBZRLE cache-size > migration: Check xbzrle-cache-size more carefully > migration: Fix cache_init()'s "Failed to allocate" error messages > migration: Fix a few absurdly defective error messages > > qapi/migration.json| 34 > migration/migration.h | 2 +- > migration/page_cache.h | 2 +- > migration/ram.h| 2 +- > migration/migration.c | 44 ++ > migration/page_cache.c | 23 +++--- > migration/ram.c| 9 + > monitor/hmp-cmds.c | 26 - > 8 files changed, 68 insertions(+), 74 deletions(-) > > -- > 2.26.2 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
* Kevin Wolf (kw...@redhat.com) wrote: > When I restricted the section where the current monitor is set to only > the command handler, I missed that monitor_parse_arguments() can use it > indirectly, too, when evaluating register variables. These cases get > NULL now and crash (easy to reproduce with "x $pc"). > > This series passes the right monitor object down instead of using > monitor_cur(), which fixes the crash. Why didn't the test-hmp.c find this? It has a 'p $pc + 8' Dave > Kevin Wolf (3): > hmp: Pass monitor to mon_get_cpu() > hmp: Pass monitor to MonitorDef.get_value() > hmp: Pass monitor to mon_get_cpu_env() > > include/monitor/hmp-target.h | 7 --- > monitor/monitor-internal.h | 2 +- > monitor/hmp.c| 2 +- > monitor/misc.c | 24 > target/i386/monitor.c| 11 ++- > target/m68k/monitor.c| 2 +- > target/nios2/monitor.c | 2 +- > target/ppc/monitor.c | 22 +- > target/riscv/monitor.c | 2 +- > target/sh4/monitor.c | 2 +- > target/sparc/monitor.c | 12 +++- > target/xtensa/monitor.c | 2 +- > 12 files changed, 49 insertions(+), 41 deletions(-) > > -- > 2.28.0 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PULL 5/6] hw/display/cirrus_vga: Fix hexadecimal format string specifier
From: Philippe Mathieu-Daudé
The '%u' conversion specifier is for decimal notation.
When prefixing a format with '0x', we want the hexadecimal
specifier ('%x').
Inspired-by: Dov Murik
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Dr. David Alan Gilbert
Message-id: 20201103112558.2554390-3-phi...@redhat.com
Signed-off-by: Gerd Hoffmann
---
hw/display/cirrus_vga.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index e14096deb46a..fdca6ca659f9 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2105,7 +2105,7 @@ static void cirrus_vga_mem_write(void *opaque,
} else {
qemu_log_mask(LOG_GUEST_ERROR,
"cirrus: mem_writeb 0x" TARGET_FMT_plx " "
- "value 0x%02" PRIu64 "\n", addr, mem_value);
+ "value 0x%02" PRIx64 "\n", addr, mem_value);
}
}
--
2.27.0
[PULL 4/6] hw/display/cirrus_vga: Remove debugging code commented out
From: Philippe Mathieu-Daudé
Commit ec87f206d70 ("cirrus: replace debug printf with trace points")
forgot to remove this code once replaced. Do it now.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Dr. David Alan Gilbert
Message-id: 20201103112558.2554390-2-phi...@redhat.com
Signed-off-by: Gerd Hoffmann
---
hw/display/cirrus_vga.c | 18 --
1 file changed, 18 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 722b9e7004cd..e14096deb46a 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2532,9 +2532,6 @@ static uint64_t cirrus_vga_ioport_read(void *opaque,
hwaddr addr,
case 0x3c5:
val = cirrus_vga_read_sr(c);
break;
-#ifdef DEBUG_VGA_REG
- printf("vga: read SR%x = 0x%02x\n", s->sr_index, val);
-#endif
break;
case 0x3c6:
val = cirrus_read_hidden_dac(c);
@@ -2560,9 +2557,6 @@ static uint64_t cirrus_vga_ioport_read(void *opaque,
hwaddr addr,
break;
case 0x3cf:
val = cirrus_vga_read_gr(c, s->gr_index);
-#ifdef DEBUG_VGA_REG
- printf("vga: read GR%x = 0x%02x\n", s->gr_index, val);
-#endif
break;
case 0x3b4:
case 0x3d4:
@@ -2571,9 +2565,6 @@ static uint64_t cirrus_vga_ioport_read(void *opaque,
hwaddr addr,
case 0x3b5:
case 0x3d5:
val = cirrus_vga_read_cr(c, s->cr_index);
-#ifdef DEBUG_VGA_REG
- printf("vga: read CR%x = 0x%02x\n", s->cr_index, val);
-#endif
break;
case 0x3ba:
case 0x3da:
@@ -2645,9 +2636,6 @@ static void cirrus_vga_ioport_write(void *opaque, hwaddr
addr, uint64_t val,
s->sr_index = val;
break;
case 0x3c5:
-#ifdef DEBUG_VGA_REG
- printf("vga: write SR%x = 0x%02" PRIu64 "\n", s->sr_index, val);
-#endif
cirrus_vga_write_sr(c, val);
break;
case 0x3c6:
@@ -2670,9 +2658,6 @@ static void cirrus_vga_ioport_write(void *opaque, hwaddr
addr, uint64_t val,
s->gr_index = val;
break;
case 0x3cf:
-#ifdef DEBUG_VGA_REG
- printf("vga: write GR%x = 0x%02" PRIu64 "\n", s->gr_index, val);
-#endif
cirrus_vga_write_gr(c, s->gr_index, val);
break;
case 0x3b4:
@@ -2681,9 +2666,6 @@ static void cirrus_vga_ioport_write(void *opaque, hwaddr
addr, uint64_t val,
break;
case 0x3b5:
case 0x3d5:
-#ifdef DEBUG_VGA_REG
- printf("vga: write CR%x = 0x%02"PRIu64"\n", s->cr_index, val);
-#endif
cirrus_vga_write_cr(c, val);
break;
case 0x3ba:
--
2.27.0
[PULL 0/6] Fixes 20201113 patches
The following changes since commit cb5d19e8294486551c422759260883ed290226d9: Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201112' i= nto staging (2020-11-12 11:33:26 +) are available in the Git repository at: git://git.kraxel.org/qemu tags/fixes-20201113-pull-request for you to fetch changes up to 172bc8520db1cb98d09b367360068a675fbc9413: xhci: fix nec-usb-xhci properties (2020-11-13 07:36:33 +0100) fixes for console, audio, usb, vga. Geoffrey McRae (1): audio/jack: fix use after free segfault Gerd Hoffmann (1): xhci: fix nec-usb-xhci properties Philippe Mathieu-Daud=C3=A9 (3): hw/usb/Kconfig: Fix USB_XHCI_NEC (depends on USB_XHCI_PCI) hw/display/cirrus_vga: Remove debugging code commented out hw/display/cirrus_vga: Fix hexadecimal format string specifier lichun (1): console: avoid passing con=3DNULL to graphic_hw_update_done() audio/jackaudio.c | 50 ++--- hw/display/cirrus_vga.c | 20 + hw/usb/hcd-xhci-nec.c | 31 - ui/console.c| 5 +++-- hw/usb/Kconfig | 3 +-- 5 files changed, 68 insertions(+), 41 deletions(-) --=20 2.27.0
[PULL 3/6] hw/usb/Kconfig: Fix USB_XHCI_NEC (depends on USB_XHCI_PCI)
From: Philippe Mathieu-Daudé
Since commit 755fba11fbc and 8ddab8dd3d8 we can not build
USB_XHCI_NEC without USB_XHCI_PCI. Correct the Kconfig
dependency.
Fixes: 755fba11fbc ("usb/hcd-xhci: Move qemu-xhci device to hcd-xhci-pci.c")
Reviewed-by: Sai Pavan Boddu
Reviewed-by: Richard Henderson
Signed-off-by: Philippe Mathieu-Daudé
Message-id: 20201109135300.2592982-2-phi...@redhat.com
[ kraxel: restore "default y if PCI_DEVICES" because
"qemu-system-ppc64 -M pseries,usb=on" needs USB_XHCI_NEC=y ]
Signed-off-by: Gerd Hoffmann
---
hw/usb/Kconfig | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index a674ce4c542e..3b07d9cf6879 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -43,8 +43,7 @@ config USB_XHCI_PCI
config USB_XHCI_NEC
bool
default y if PCI_DEVICES
-depends on PCI
-select USB_XHCI
+select USB_XHCI_PCI
config USB_XHCI_SYSBUS
bool
--
2.27.0
[PULL 6/6] xhci: fix nec-usb-xhci properties
Storing properties directly in XHCIPciState.xhci doesn't work,
the object_initialize_child() call in xhci_instance_init() will
overwrite them.
This changes the defaults for some properties, which in turn breaks
live migration and possibly other things as well.
So add XHCINecState, store properties there, copy them over on
instance init.
Fixes: 8ddab8dd3d81 ("usb/hcd-xhci: Split pci wrapper for xhci base model")
Reported-by: Dr. David Alan Gilbert
Signed-off-by: Gerd Hoffmann
Message-id: 20201112103741.2335-1-kra...@redhat.com
---
hw/usb/hcd-xhci-nec.c | 31 ++-
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
index 5707b2cabd16..13a125afe2f7 100644
--- a/hw/usb/hcd-xhci-nec.c
+++ b/hw/usb/hcd-xhci-nec.c
@@ -27,18 +27,37 @@
#include "hcd-xhci-pci.h"
+typedef struct XHCINecState {
+/*< private >*/
+XHCIPciState parent_obj;
+/*< public >*/
+uint32_t flags;
+uint32_t intrs;
+uint32_t slots;
+} XHCINecState;
+
static Property nec_xhci_properties[] = {
DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
-DEFINE_PROP_BIT("superspeed-ports-first", XHCIPciState,
-xhci.flags, XHCI_FLAG_SS_FIRST, true),
-DEFINE_PROP_BIT("force-pcie-endcap", XHCIPciState, xhci.flags,
+DEFINE_PROP_BIT("superspeed-ports-first", XHCINecState, flags,
+XHCI_FLAG_SS_FIRST, true),
+DEFINE_PROP_BIT("force-pcie-endcap", XHCINecState, flags,
XHCI_FLAG_FORCE_PCIE_ENDCAP, false),
-DEFINE_PROP_UINT32("intrs", XHCIPciState, xhci.numintrs, XHCI_MAXINTRS),
-DEFINE_PROP_UINT32("slots", XHCIPciState, xhci.numslots, XHCI_MAXSLOTS),
+DEFINE_PROP_UINT32("intrs", XHCINecState, intrs, XHCI_MAXINTRS),
+DEFINE_PROP_UINT32("slots", XHCINecState, slots, XHCI_MAXSLOTS),
DEFINE_PROP_END_OF_LIST(),
};
+static void nec_xhci_instance_init(Object *obj)
+{
+XHCIPciState *pci = XHCI_PCI(obj);
+XHCINecState *nec = container_of(pci, XHCINecState, parent_obj);
+
+pci->xhci.flags= nec->flags;
+pci->xhci.numintrs = nec->intrs;
+pci->xhci.numslots = nec->slots;
+}
+
static void nec_xhci_class_init(ObjectClass *klass, void *data)
{
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -53,6 +72,8 @@ static void nec_xhci_class_init(ObjectClass *klass, void
*data)
static const TypeInfo nec_xhci_info = {
.name = TYPE_NEC_XHCI,
.parent= TYPE_XHCI_PCI,
+.instance_size = sizeof(XHCINecState),
+.instance_init = nec_xhci_instance_init,
.class_init= nec_xhci_class_init,
};
--
2.27.0
Re: [PULL for-5.2 0/1] MAINTAINERS: Replace my twiddle.net address
On Fri, 13 Nov 2020 at 04:40, Richard Henderson wrote: > > The following changes since commit cb5d19e8294486551c422759260883ed290226d9: > > Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201112' > into staging (2020-11-12 11:33:26 +) > > are available in the Git repository at: > > https://github.com/rth7680/qemu.git tags/pull-tcg-20201112 > > for you to fetch changes up to 336f744e148a7b9d50ebf205d5dba7b0fec271d9: > > MAINTAINERS: Replace my twiddle.net address (2020-11-12 20:35:43 -0800) > > > Use richard.hender...@linaro.org in MAINTAINERS Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
[PULL 1/6] audio/jack: fix use after free segfault
From: Geoffrey McRae
This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is received. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.
Signed-off-by: Geoffrey McRae
Reviewed-by: Christian Schoenebeck
Message-Id: <20201108063351.35804-2-ge...@hostfission.com>
Signed-off-by: Gerd Hoffmann
---
audio/jackaudio.c | 50 +++
1 file changed, 37 insertions(+), 13 deletions(-)
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 1e714b30bc8a..3b7c18443dbe 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "qemu/module.h"
#include "qemu/atomic.h"
+#include "qemu/main-loop.h"
#include "qemu-common.h"
#include "audio.h"
@@ -63,6 +64,7 @@ typedef struct QJackClient {
QJackState state;
jack_client_t *client;
jack_nframes_t freq;
+QEMUBH *shutdown_bh;
struct QJack *j;
int nchannels;
@@ -87,6 +89,7 @@ QJackIn;
static int qjack_client_init(QJackClient *c);
static void qjack_client_connect_ports(QJackClient *c);
static void qjack_client_fini(QJackClient *c);
+static QemuMutex qjack_shutdown_lock;
static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
{
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
return 0;
}
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
static void qjack_shutdown(void *arg)
{
QJackClient *c = (QJackClient *)arg;
c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
}
static void qjack_client_recover(QJackClient *c)
{
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
}
/* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
/* if enabled then attempt to recover */
if (c->enabled) {
@@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct
audsettings *as,
QJackOut *jo = (QJackOut *)hw;
Audiodev *dev = (Audiodev *)drv_opaque;
-qjack_client_fini(&jo->c);
-
jo->c.out = true;
jo->c.enabled = false;
jo->c.nchannels = as->nchannels;
jo->c.opt = dev->u.jack.out;
+jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
+
int ret = qjack_client_init(&jo->c);
if (ret != 0) {
+qemu_bh_delete(jo->c.shutdown_bh);
return ret;
}
@@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct
audsettings *as,
QJackIn *ji = (QJackIn *)hw;
Audiodev *dev = (Audiodev *)drv_opaque;
-qjack_client_fini(&ji->c);
-
ji->c.out = false;
ji->c.enabled = false;
ji->c.nchannels = as->nchannels;
ji->c.opt = dev->u.jack.in;
+ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
+
int ret = qjack_client_init(&ji->c);
if (ret != 0) {
+qemu_bh_delete(ji->c.shutdown_bh);
return ret;
}
@@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings
*as,
return 0;
}
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
{
switch (c->state) {
case QJACK_STATE_RUNNING:
@@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
case QJACK_STATE_SHUTDOWN:
jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(&c->fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
/* fallthrough */
case QJACK_STATE_DISCONNECTED:
break;
}
+}
-qjack_buffer_free(&c->fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+qemu_mutex_lock(&qjack_shutdown_lock);
+qjack_client_fini_locked(c);
+qemu_mutex_unlock(&qjack_shutdown_lock);
}
static void qjack_fini_out(HWVoiceOut *hw)
{
QJackOut *jo = (QJackOut *)hw;
qjack_client_fini(&jo->c);
+
+qemu_bh_delete(jo->c.shutdown_bh);
}
static void qjack_fini_in(HWVoiceIn *hw)
{
QJackIn *ji = (QJackIn *)hw;
qjack_client_fini(&ji->c);
+
+qemu_bh_delete(ji->c.shutdown_bh);
}
static void qjack_enable_out(HWVoiceOut *hw, bool enable)
@@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
static void register_audio_jack(void)
{
+qemu_mutex_init(&qjack_shutdown_lock);
audio_driver_register(&jack_driver);
jack_set_thread_creator(qjack_thread_creator);
jack_set_error_function(qjack_error);
--
2.27.0
[PULL 2/6] console: avoid passing con=NULL to graphic_hw_update_done()
From: lichun
In graphic_hw_update(), first select an existing console,
a specific-console or active_console(if not specified),
then updating the console.
Signed-off-by: lichun
Message-id: 1604682219-114389-1-git-send-email-lic...@ruijie.com.cn
Signed-off-by: Gerd Hoffmann
---
ui/console.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ui/console.c b/ui/console.c
index e8e59707d38c..e07d2c380df7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -270,10 +270,11 @@ void graphic_hw_update_done(QemuConsole *con)
void graphic_hw_update(QemuConsole *con)
{
bool async = false;
+con = con ? con : active_console;
if (!con) {
-con = active_console;
+return;
}
-if (con && con->hw_ops->gfx_update) {
+if (con->hw_ops->gfx_update) {
con->hw_ops->gfx_update(con->hw);
async = con->hw_ops->gfx_update_async;
}
--
2.27.0
Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben: > * Kevin Wolf (kw...@redhat.com) wrote: > > When I restricted the section where the current monitor is set to only > > the command handler, I missed that monitor_parse_arguments() can use it > > indirectly, too, when evaluating register variables. These cases get > > NULL now and crash (easy to reproduce with "x $pc"). > > > > This series passes the right monitor object down instead of using > > monitor_cur(), which fixes the crash. > > Why didn't the test-hmp.c find this? It has a 'p $pc + 8' Good question, a manual 'p $pc + 8' crashes for me on master. Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command. Then it would just get the wrong monitor (the QMP one instead of the temporary HMP monitor) and not NULL. The accessed CPU is even the same because neither QMP nor the temporary HMP monitor have a current CPU set, so even if the test case did check the result, it wouldn't catch this. Only if the test case were using multiple CPUs and cpu-index had been set for human-monitor-command (to something other than the default), we would get a wrong result. But of course, it still wouldn't crash. Kevin
Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
* Kevin Wolf (kw...@redhat.com) wrote: > Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben: > > * Kevin Wolf (kw...@redhat.com) wrote: > > > When I restricted the section where the current monitor is set to only > > > the command handler, I missed that monitor_parse_arguments() can use it > > > indirectly, too, when evaluating register variables. These cases get > > > NULL now and crash (easy to reproduce with "x $pc"). > > > > > > This series passes the right monitor object down instead of using > > > monitor_cur(), which fixes the crash. > > > > Why didn't the test-hmp.c find this? It has a 'p $pc + 8' > > Good question, a manual 'p $pc + 8' crashes for me on master. > > Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command. > Then it would just get the wrong monitor (the QMP one instead of the > temporary HMP monitor) and not NULL. The accessed CPU is even the same > because neither QMP nor the temporary HMP monitor have a current CPU > set, so even if the test case did check the result, it wouldn't catch > this. > > Only if the test case were using multiple CPUs and cpu-index had been > set for human-monitor-command (to something other than the default), we > would get a wrong result. But of course, it still wouldn't crash. Ah, fair enough. Dave > Kevin -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
* Kevin Wolf (kw...@redhat.com) wrote: > When I restricted the section where the current monitor is set to only > the command handler, I missed that monitor_parse_arguments() can use it > indirectly, too, when evaluating register variables. These cases get > NULL now and crash (easy to reproduce with "x $pc"). > > This series passes the right monitor object down instead of using > monitor_cur(), which fixes the crash. > Reviewed-by: Dr. David Alan Gilbert and queued; I'll send out an HMP pull shortly. > Kevin Wolf (3): > hmp: Pass monitor to mon_get_cpu() > hmp: Pass monitor to MonitorDef.get_value() > hmp: Pass monitor to mon_get_cpu_env() > > include/monitor/hmp-target.h | 7 --- > monitor/monitor-internal.h | 2 +- > monitor/hmp.c| 2 +- > monitor/misc.c | 24 > target/i386/monitor.c| 11 ++- > target/m68k/monitor.c| 2 +- > target/nios2/monitor.c | 2 +- > target/ppc/monitor.c | 22 +- > target/riscv/monitor.c | 2 +- > target/sh4/monitor.c | 2 +- > target/sparc/monitor.c | 12 +++- > target/xtensa/monitor.c | 2 +- > 12 files changed, 49 insertions(+), 41 deletions(-) > > -- > 2.28.0 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 2/2] authz-list-file: Improve an error message
Daniel P. Berrangé writes:
> On Fri, Nov 13, 2020 at 07:23:58AM +0100, Markus Armbruster wrote:
>> When qauthz_list_file_load() rejects JSON values other than JSON
>> object with a rather confusing error message:
>>
>> $ echo 1 | qemu-system-x86_64 -nodefaults -S -display none -object
>> authz-list-file,id=authz0,filename=/dev/stdin
>> qemu-system-x86_64: -object
>> authz-list-file,id=authz0,filename=/dev/stdin: Invalid parameter type for
>> 'obj', expected: dict
>>
>> Improve to
>>
>> qemu-system-x86_64: -object
>> authz-list-file,id=authz0,filename=/dev/stdin: File '/dev/stdin' must
>> contain a JSON object
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> authz/listfile.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé
Thanks!
>> diff --git a/authz/listfile.c b/authz/listfile.c
>> index 1421e674a4..da3a0e69a2 100644
>> --- a/authz/listfile.c
>> +++ b/authz/listfile.c
>> @@ -73,7 +73,8 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
>>
>> pdict = qobject_to(QDict, obj);
>> if (!pdict) {
>> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "obj", "dict");
>> +error_setg(errp, "File '%s' must contain a JSON object",
>> + fauthz->filename);
>
> This code pattern was copied from other places in QEMU which use the
> same QERR_INVALID_PARAMETER_TYPE. There are another 10 or so examples
> of this error message pattern.
Yes. My "[PATCH 00/10] Chipping away at qerror.h" fixes other patterns,
but not this one. I intend to chip some more, as time permits.
[PATCH 1/5] block/prl-xml: add Parallels xml BlockDriver
From: Klim Kireev
This patch introduces new BlockDriver: prl-xml.
It adds opening and closing capabilities.
All operations are performed using libxml2.
Signed-off-by: Klim Kireev
---
block/prl-xml.c | 492
block/Makefile.objs | 5 +-
2 files changed, 495 insertions(+), 2 deletions(-)
create mode 100644 block/prl-xml.c
diff --git a/block/prl-xml.c b/block/prl-xml.c
new file mode 100644
index 00..fa9c4fd5fa
--- /dev/null
+++ b/block/prl-xml.c
@@ -0,0 +1,492 @@
+/*
+* Block driver for Parallels disk image format
+* Copyright (c) 2015-2017, Virtuozzo, Inc.
+* Authors:
+* 2016-2017 Klim S. Kireev
+* 2015 Denis V. Lunev
+*
+* This code was originally based on comparing different disk images created
+* by Parallels. Currently it is based on opened OpenVZ sources
+* available at
+* https://github.com/OpenVZ/ploop
+*
+* Permission is hereby granted, free of charge, to any person obtaining a copy
+* of this software and associated documentation files (the "Software"), to deal
+* in the Software without restriction, including without limitation the rights
+* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+* copies of the Software, and to permit persons to whom the Software is
+* furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included in
+* all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+* THE SOFTWARE.
+*/
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "qemu/uuid.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
+#include "qapi/qmp/qdict.h"
+
+#include
+
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include /* For basename */
+
+#define DEF_TOP_SNAPSHOT "5fbaabe3-6958-40ff-92a7-860e329aab41"
+#define GUID_LEN strlen(DEF_TOP_SNAPSHOT)
+#define PRL_XML_FILENAME "DiskDescriptor.xml"
+
+typedef struct BDRVPrlXmlState {
+xmlDoc *xml;
+BdrvChild *image;
+} BDRVPrlXmlState;
+
+enum TopSnapMode {
+ERROR_MODE = -1,
+NODE_MODE,
+GUID_MODE
+};
+
+static QemuOptsList prl_xml_create_opts = {
+.name = "prl-xml-create-opts",
+.head = QTAILQ_HEAD_INITIALIZER(prl_xml_create_opts.head),
+.desc = {
+{
+.name = BLOCK_OPT_SIZE,
+.type = QEMU_OPT_SIZE,
+.help = "Virtual disk size",
+},
+{
+.name = BLOCK_OPT_CLUSTER_SIZE,
+.type = QEMU_OPT_SIZE,
+.help = "Parallels XML back image cluster size",
+.def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
+},
+{ /* end of list */ }
+}
+};
+
+static xmlNodePtr xml_find_element_child(xmlNodePtr node, const char *elem)
+{
+xmlNodePtr child;
+
+for (child = node->xmlChildrenNode; child != NULL; child = child->next) {
+if (child->type == XML_ELEMENT_NODE &&
+!xmlStrcmp(child->name, (const xmlChar *)elem))
+{
+return child;
+}
+}
+return NULL;
+}
+
+static xmlNodePtr xml_seek_va(xmlNodePtr root, va_list args)
+{
+const char *elem;
+
+while ((elem = va_arg(args, const char *)) != NULL) {
+root = xml_find_element_child(root, elem);
+if (root == NULL) {
+return NULL;
+}
+}
+return root;
+}
+
+static xmlNodePtr xml_seek(xmlNodePtr root, ...)
+{
+va_list args;
+va_start(args, root);
+root = xml_seek_va(root, args);
+va_end(args);
+return root;
+}
+
+static const char *xml_get_text(xmlNodePtr node, ...)
+{
+xmlNodePtr child;
+va_list args;
+
+if (node == NULL) {
+return NULL;
+}
+
+va_start(args, node);
+node = xml_seek_va(node, args);
+va_end(args);
+
+if (node == NULL) {
+return NULL;
+}
+
+for (child = node->xmlChildrenNode; child; child = child->next) {
+if (child->type == XML_TEXT_NODE) {
+return (const char *)child->content;
+}
+}
+return NULL;
+}
+
+static inline int get_addr_mode(xmlDocPtr doc)
+{
+xmlNodePtr root = xmlDocGetRootElement(doc);
+if (root == NULL) {
+return ERROR_MODE;
+}
+
+xmlNodePtr cur = xml_seek(root, "Snapshots", "TopGUID", NULL);
+if (cur == NULL) {
+return GUID_MODE;
+} else {
+return NODE_MODE;
+}
+};
+
+static int xml_check(xmlNodePtr root, Error **errp)
+{
+xmlNodePtr image;
+const char *data;
+
+data = (const char *)xmlGetProp(roo
[PATCH 3/5] block/prl-xml: add bdrv_probe
From: Klim Kireev
This commit adds bdrv_probe implementation.
It checks the filename (it must be DiskDescriptor.xml).
Then it checks correctness of the xml file using libxml2.
Signed-off-by: Klim Kireev
---
block/prl-xml.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/block/prl-xml.c b/block/prl-xml.c
index 5ab32bb6ab..023651342c 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -467,6 +467,30 @@ fail:
return ret;
}
+static int prl_probe_xml(const uint8_t *data, int buf_size,
+ const char *filename)
+{
+xmlDoc *doc = NULL;
+xmlNodePtr root;
+
+if (strcmp(basename(filename), PRL_XML_FILENAME) != 0) {
+return 0;
+}
+
+doc = xmlReadFile(filename, NULL, XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
+if (doc == NULL) {
+return 0;
+}
+
+root = xmlDocGetRootElement(doc);
+if (root == NULL) {
+return 0;
+}
+
+xmlFreeDoc(doc);
+return 100;
+}
+
static coroutine_fn int
prl_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov)
@@ -499,6 +523,7 @@ static coroutine_fn int prl_co_flush_to_os(BlockDriverState
*bs)
static BlockDriver bdrv_prl_xml = {
.format_name= "prl-xml",
.instance_size = sizeof(BDRVPrlXmlState),
+.bdrv_probe = prl_probe_xml,
.bdrv_open = prl_open_xml,
.bdrv_co_readv = prl_co_readv,
.bdrv_co_writev = prl_co_writev,
--
2.21.3
[PATCH 2/5] block/prl-xml: add bdrv_co_readv/writev and flush
From: Klim Kireev
This commit adds bdrv_co_readv, bdrv_co_writev, and bdrv_co_flush_to_os
implementation. It merely passes these functions down
to top BlockDriverState in the snapshot chain.
Signed-off-by: Klim Kireev
Signed-off-by: Edgar Kaziakhmedov
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/prl-xml.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/block/prl-xml.c b/block/prl-xml.c
index fa9c4fd5fa..5ab32bb6ab 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -467,6 +467,22 @@ fail:
return ret;
}
+static coroutine_fn int
+prl_co_readv(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_readv(s->image, sector_num, nb_sectors, qiov);
+}
+
+static coroutine_fn int
+prl_co_writev(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_writev(s->image, sector_num, nb_sectors, qiov);
+}
+
static void prl_close_xml(BlockDriverState *bs)
{
BDRVPrlXmlState *s = bs->opaque;
@@ -474,11 +490,20 @@ static void prl_close_xml(BlockDriverState *bs)
xmlFreeDoc(s->xml);
}
+static coroutine_fn int prl_co_flush_to_os(BlockDriverState *bs)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_flush(s->image->bs);
+}
+
static BlockDriver bdrv_prl_xml = {
.format_name= "prl-xml",
.instance_size = sizeof(BDRVPrlXmlState),
.bdrv_open = prl_open_xml,
+.bdrv_co_readv = prl_co_readv,
+.bdrv_co_writev = prl_co_writev,
.bdrv_close = prl_close_xml,
+.bdrv_co_flush_to_os= prl_co_flush_to_os,
.create_opts= &prl_xml_create_opts,
.bdrv_child_perm= bdrv_filter_default_perms,
.is_filter = true
--
2.21.3
[PATCH 5/5] iotests: add test for prl-xml format
From: Edgar Kaziakhmedov
Signed-off-by: Edgar Kaziakhmedov
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/164| 98 ++
tests/qemu-iotests/164.out| 54 ++
tests/qemu-iotests/check | 7 ++
tests/qemu-iotests/group | 1 +
.../prl-xml/DiskDescriptor.xml.bz2| Bin 0 -> 457 bytes
.../sample_images/prl-xml/Snapshots.xml.bz2 | Bin 0 -> 307 bytes
...aabe3-6958-40ff-92a7-860e329aab41}.hds.bz2 | Bin 0 -> 93 bytes
...476cf-d62e-45d1-b355-86feca91376e}.hds.bz2 | Bin 0 -> 93 bytes
8 files changed, 160 insertions(+)
create mode 100755 tests/qemu-iotests/164
create mode 100644 tests/qemu-iotests/164.out
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/DiskDescriptor.xml.bz2
create mode 100644 tests/qemu-iotests/sample_images/prl-xml/Snapshots.xml.bz2
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/parallels.{5fbaabe3-6958-40ff-92a7-860e329aab41}.hds.bz2
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/parallels.{986476cf-d62e-45d1-b355-86feca91376e}.hds.bz2
diff --git a/tests/qemu-iotests/164 b/tests/qemu-iotests/164
new file mode 100755
index 00..a55ab5d7d8
--- /dev/null
+++ b/tests/qemu-iotests/164
@@ -0,0 +1,98 @@
+#!/bin/bash
+#
+# prl-xml format validation test
+#
+# Copyright (C) 2017 Edgar Kaziakhmedov
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
[PATCH 4/5] block/prl-xml: add bdrv_check
From: Klim Kireev
Add bdrv_check, which just checks child image and all backings.
Signed-off-by: Klim Kireev
---
block/prl-xml.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/prl-xml.c b/block/prl-xml.c
index 023651342c..736cd98469 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -520,6 +520,23 @@ static coroutine_fn int
prl_co_flush_to_os(BlockDriverState *bs)
return bdrv_co_flush(s->image->bs);
}
+static coroutine_fn int prl_co_check_xml(BlockDriverState *bs,
+BdrvCheckResult *result,
+BdrvCheckMode fix) {
+BDRVPrlXmlState *s = bs->opaque;
+BdrvChild *cur = s->image;
+int ret = 0;
+
+while (cur != NULL && ret >= 0) {
+if (cur->bs->drv->bdrv_co_check != NULL) {
+ret = bdrv_check(cur->bs, result, fix);
+}
+cur = cur->bs->backing;
+}
+
+return ret;
+}
+
static BlockDriver bdrv_prl_xml = {
.format_name= "prl-xml",
.instance_size = sizeof(BDRVPrlXmlState),
@@ -529,6 +546,7 @@ static BlockDriver bdrv_prl_xml = {
.bdrv_co_writev = prl_co_writev,
.bdrv_close = prl_close_xml,
.bdrv_co_flush_to_os= prl_co_flush_to_os,
+.bdrv_co_check = prl_co_check_xml,
.create_opts= &prl_xml_create_opts,
.bdrv_child_perm= bdrv_filter_default_perms,
.is_filter = true
--
2.21.3
[PULL 1/3] hmp: Pass monitor to mon_get_cpu()
From: Kevin Wolf
mon_get_cpu() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur(),
explicitly pass the Monitor pointer to the function.
Signed-off-by: Kevin Wolf
Message-Id: <20201113114326.97663-2-kw...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Dr. David Alan Gilbert
---
include/monitor/hmp-target.h | 2 +-
monitor/hmp.c| 2 +-
monitor/misc.c | 18 +-
monitor/monitor-internal.h | 2 +-
target/i386/monitor.c| 2 +-
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 8b7820a3ad..519616d1fb 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ const MonitorDef *target_monitor_defs(void);
int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
CPUArchState *mon_get_cpu_env(void);
-CPUState *mon_get_cpu(void);
+CPUState *mon_get_cpu(Monitor *mon);
void hmp_info_mem(Monitor *mon, const QDict *qdict);
void hmp_info_tlb(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index c5cd9d372b..1204233999 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -384,7 +384,7 @@ static int64_t expr_unary(Monitor *mon)
pch++;
}
*q = 0;
-ret = get_monitor_def(®, buf);
+ret = get_monitor_def(mon, ®, buf);
if (ret < 0) {
expr_error(mon, "unknown register");
}
diff --git a/monitor/misc.c b/monitor/misc.c
index 32e6a8c13d..c918d6bd08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -289,14 +289,14 @@ static CPUState *mon_get_cpu_sync(Monitor *mon, bool
synchronize)
return cpu;
}
-CPUState *mon_get_cpu(void)
+CPUState *mon_get_cpu(Monitor *mon)
{
-return mon_get_cpu_sync(monitor_cur(), true);
+return mon_get_cpu_sync(mon, true);
}
CPUArchState *mon_get_cpu_env(void)
{
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(monitor_cur());
return cs ? cs->env_ptr : NULL;
}
@@ -319,7 +319,7 @@ static void hmp_info_registers(Monitor *mon, const QDict
*qdict)
cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
}
} else {
-cs = mon_get_cpu();
+cs = mon_get_cpu(mon);
if (!cs) {
monitor_printf(mon, "No CPU available\n");
@@ -381,7 +381,7 @@ static void hmp_info_history(Monitor *mon, const QDict
*qdict)
static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
{
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
if (!cs) {
monitor_printf(mon, "No CPU available\n");
@@ -546,7 +546,7 @@ static void memory_dump(Monitor *mon, int count, int
format, int wsize,
int l, line_size, i, max_digits, len;
uint8_t buf[16];
uint64_t v;
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
if (!cs && (format == 'i' || !is_physical)) {
monitor_printf(mon, "Can not dump without CPU\n");
@@ -711,7 +711,7 @@ static void hmp_gva2gpa(Monitor *mon, const QDict *qdict)
{
target_ulong addr = qdict_get_int(qdict, "addr");
MemTxAttrs attrs;
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
hwaddr gpa;
if (!cs) {
@@ -1663,10 +1663,10 @@ HMPCommand hmp_cmds[] = {
* Set @pval to the value in the register identified by @name.
* return 0 if OK, -1 if not found
*/
-int get_monitor_def(int64_t *pval, const char *name)
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
{
const MonitorDef *md = target_monitor_defs();
-CPUState *cs = mon_get_cpu();
+CPUState *cs = mon_get_cpu(mon);
void *ptr;
uint64_t tmp = 0;
int ret;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index ad2e64be13..a6131554da 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -178,7 +178,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
void monitor_data_destroy_qmp(MonitorQMP *mon);
void coroutine_fn monitor_qmp_dispatcher_co(void *data);
-int get_monitor_def(int64_t *pval, const char *name);
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
void help_cmd(Monitor *mon, const char *name);
void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
int hmp_compare_cmd(const char *name, const char *list);
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 7abae3c8df..5ca3cab4ec 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -656,7 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
int id = qdict_get_try_int(qdict, "apic-id", 0);
cs = cpu_by_arch_id(id);
} else {
-cs = mon_get_cpu();
+cs = mon_get_cpu(mon);
}
--
2.28.0
[PULL 3/3] hmp: Pass monitor to mon_get_cpu_env()
From: Kevin Wolf
mon_get_cpu_env() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur_env(),
explicitly pass the Monitor pointer to the function.
Without this fix, an HMP command like "x $pc" crashes like this:
#0 0x55caa01f in mon_get_cpu_sync (mon=0x0, synchronize=true) at
../monitor/misc.c:270
#1 0x55caa141 in mon_get_cpu (mon=0x0) at ../monitor/misc.c:294
#2 0x55caa158 in mon_get_cpu_env () at ../monitor/misc.c:299
#3 0x55b19739 in monitor_get_pc (mon=0x56ad2de0,
md=0x565d2d40 , val=0) at ../target/i386/monitor.c:607
#4 0x55cadbec in get_monitor_def (mon=0x56ad2de0,
pval=0x7fffc208, name=0x7fffc220 "pc") at ../monitor/misc.c:1681
#5 0x5582ec4f in expr_unary (mon=0x56ad2de0) at
../monitor/hmp.c:387
#6 0x5582edbb in expr_prod (mon=0x56ad2de0) at
../monitor/hmp.c:421
#7 0x5582ee79 in expr_logic (mon=0x56ad2de0) at
../monitor/hmp.c:455
#8 0x5582eefe in expr_sum (mon=0x56ad2de0) at
../monitor/hmp.c:484
#9 0x5582efe8 in get_expr (mon=0x56ad2de0, pval=0x7fffc418,
pp=0x7fffc408) at ../monitor/hmp.c:511
#10 0x5582fcd4 in monitor_parse_arguments (mon=0x56ad2de0,
endp=0x7fffc890, cmd=0x56675b50 ) at ../monitor/hmp.c:876
#11 0x558306a8 in handle_hmp_command (mon=0x56ad2de0,
cmdline=0x56ada452 "$pc") at ../monitor/hmp.c:1087
#12 0x5582df14 in monitor_command_cb (opaque=0x56ad2de0,
cmdline=0x56ada450 "x $pc", readline_opaque=0x0) at ../monitor/hmp.c:47
After this fix, nothing is left in monitor_parse_arguments() that can
indirectly call monitor_cur(), so the fix is complete.
Fixes: ff04108a0e36e822519c517bd3bddbc1c7747c18
Reported-by: lichun
Signed-off-by: Kevin Wolf
Message-Id: <20201113114326.97663-4-kw...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Dr. David Alan Gilbert
---
include/monitor/hmp-target.h | 2 +-
monitor/misc.c | 6 +++---
target/i386/monitor.c| 6 +++---
target/m68k/monitor.c| 2 +-
target/nios2/monitor.c | 2 +-
target/ppc/monitor.c | 10 +-
target/riscv/monitor.c | 2 +-
target/sh4/monitor.c | 2 +-
target/sparc/monitor.c | 6 +++---
target/xtensa/monitor.c | 2 +-
10 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 385fb18664..60fc92722a 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ struct MonitorDef {
const MonitorDef *target_monitor_defs(void);
int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
-CPUArchState *mon_get_cpu_env(void);
+CPUArchState *mon_get_cpu_env(Monitor *mon);
CPUState *mon_get_cpu(Monitor *mon);
void hmp_info_mem(Monitor *mon, const QDict *qdict);
diff --git a/monitor/misc.c b/monitor/misc.c
index f566e28174..398211a034 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -294,9 +294,9 @@ CPUState *mon_get_cpu(Monitor *mon)
return mon_get_cpu_sync(mon, true);
}
-CPUArchState *mon_get_cpu_env(void)
+CPUArchState *mon_get_cpu_env(Monitor *mon)
{
-CPUState *cs = mon_get_cpu(monitor_cur());
+CPUState *cs = mon_get_cpu(mon);
return cs ? cs->env_ptr : NULL;
}
@@ -1680,7 +1680,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const
char *name)
if (md->get_value) {
*pval = md->get_value(mon, md, md->offset);
} else {
-CPUArchState *env = mon_get_cpu_env();
+CPUArchState *env = mon_get_cpu_env(mon);
ptr = (uint8_t *)env + md->offset;
switch(md->type) {
case MD_I32:
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index fed4606aeb..9f9e1c42f4 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -222,7 +222,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
{
CPUArchState *env;
-env = mon_get_cpu_env();
+env = mon_get_cpu_env(mon);
if (!env) {
monitor_printf(mon, "No CPU available\n");
return;
@@ -550,7 +550,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
{
CPUArchState *env;
-env = mon_get_cpu_env();
+env = mon_get_cpu_env(mon);
if (!env) {
monitor_printf(mon, "No CPU available\n");
return;
@@ -604,7 +604,7 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
int val)
{
-CPUArchState *env = mon_get_cpu_env();
+CPUArchState *env = mon_get_cpu_env(mon);
return env->eip + env->segs[R_CS].base;
}
diff --git a/target/m68k/monitor.c b/target/m68k/monitor.c
index 2055fe8a00..2bdf6acae0 100644
--- a/target/m68k/monitor.c
+++ b/target/m68k/mon
[PULL 0/3] hmp queue
From: "Dr. David Alan Gilbert" The following changes since commit be2df2ac6f6b921cc057de0a119ac30fbc60: Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20201112' into staging (2020-11-13 11:36:30 +) are available in the Git repository at: git://github.com/dagrh/qemu.git tags/pull-hmp-20201113 for you to fetch changes up to e7cff9c68d4a46343861fbc3cc6b2a0b63b2dbb8: hmp: Pass monitor to mon_get_cpu_env() (2020-11-13 12:45:51 +) HMP fixes Kevin's HMP fixes Kevin Wolf (3): hmp: Pass monitor to mon_get_cpu() hmp: Pass monitor to MonitorDef.get_value() hmp: Pass monitor to mon_get_cpu_env() include/monitor/hmp-target.h | 7 --- monitor/hmp.c| 2 +- monitor/misc.c | 24 monitor/monitor-internal.h | 2 +- target/i386/monitor.c| 11 ++- target/m68k/monitor.c| 2 +- target/nios2/monitor.c | 2 +- target/ppc/monitor.c | 22 +- target/riscv/monitor.c | 2 +- target/sh4/monitor.c | 2 +- target/sparc/monitor.c | 12 +++- target/xtensa/monitor.c | 2 +- 12 files changed, 49 insertions(+), 41 deletions(-)
[PULL 2/3] hmp: Pass monitor to MonitorDef.get_value()
From: Kevin Wolf
All of these callbacks use mon_get_cpu_env(). Pass the Monitor
pointer to them it in preparation for adding a monitor argument to
mon_get_cpu_env().
Signed-off-by: Kevin Wolf
Message-Id: <20201113114326.97663-3-kw...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Dr. David Alan Gilbert
---
include/monitor/hmp-target.h | 3 ++-
monitor/misc.c | 2 +-
target/i386/monitor.c| 3 ++-
target/ppc/monitor.c | 12
target/sparc/monitor.c | 6 --
5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 519616d1fb..385fb18664 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -33,7 +33,8 @@
struct MonitorDef {
const char *name;
int offset;
-target_long (*get_value)(const struct MonitorDef *md, int val);
+target_long (*get_value)(Monitor *mon, const struct MonitorDef *md,
+ int val);
int type;
};
diff --git a/monitor/misc.c b/monitor/misc.c
index c918d6bd08..f566e28174 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1678,7 +1678,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const
char *name)
for(; md->name != NULL; md++) {
if (hmp_compare_cmd(name, md->name)) {
if (md->get_value) {
-*pval = md->get_value(md, md->offset);
+*pval = md->get_value(mon, md, md->offset);
} else {
CPUArchState *env = mon_get_cpu_env();
ptr = (uint8_t *)env + md->offset;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 5ca3cab4ec..fed4606aeb 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -601,7 +601,8 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
}
}
-static target_long monitor_get_pc(const struct MonitorDef *md, int val)
+static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return env->eip + env->segs[R_CS].base;
diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index a5a177d717..9c0fc2b8c3 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -29,7 +29,8 @@
#include "monitor/hmp-target.h"
#include "monitor/hmp.h"
-static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
+static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
unsigned int u;
@@ -43,19 +44,22 @@ static target_long monitor_get_ccr(const struct MonitorDef
*md, int val)
return u;
}
-static target_long monitor_get_decr(const struct MonitorDef *md, int val)
+static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
+int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_decr(env);
}
-static target_long monitor_get_tbu(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_tbu(env);
}
-static target_long monitor_get_tbl(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return cpu_ppc_load_tbl(env);
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index a7ea287cbc..bf979d6520 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -40,7 +40,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
}
#ifndef TARGET_SPARC64
-static target_long monitor_get_psr (const struct MonitorDef *md, int val)
+static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
@@ -48,7 +49,8 @@ static target_long monitor_get_psr (const struct MonitorDef
*md, int val)
}
#endif
-static target_long monitor_get_reg(const struct MonitorDef *md, int val)
+static target_long monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env();
return env->regwptr[val];
--
2.28.0
Re: [PATCH] arm/monitor: Add support for 'info tlb' command
* Peter Maydell (peter.mayd...@linaro.org) wrote: > On Fri, 13 Nov 2020 at 09:59, Changbin Du wrote: > > > > This adds hmp 'info tlb' command support for the arm platform. > > The limitation is that this only implements a page walker for > > ARMv8-A AArch64 Long Descriptor format, 32bit addressing is > > not supported yet. > > "info tlb" needs its own entirely independent implementation > of a page-table walk? I see this is how x86 has done it ,but > it seems like a recipe for the info command being perpetually > behind what we actually implement... I think the challenge is you want 'info tlb' to be quite easy to read from a debuggers point of view and robust at pointing out things that are odd; I'd hope at least you can use most of the macros to extract fields etc Dave > thanks > -- PMM > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH RFC 0/5] block: implement Parallels Disk format driver
Hi all!
I just send these old patches as they can be useful. I'm not the author
of the code and not going to discuss them. "RFC" is here just to mark
the series as "not-for-applying-to-master". So, please don't answer
here. If you want to continue this work, post v2 first.
Virtuozzo doesn't have plans on improving Parallels disk format
support in Qemu. Still, if someone want to work on it, these patches may
help.
Note that patches don't apply to master, they need a rebase. I've
applied them onto b0292b851b85ba81c0cfedf5b576c32189cfaa11, to run git
send-email. So, you may start from applying to same commit and rebasing
onto current master if you want.
Note also that Edgar and Klim are not in Virtuozzo team for now and
their @virtuozzo.com emails are invalid.
Edgar Kaziakhmedov (1):
iotests: add test for prl-xml format
Klim Kireev (4):
block/prl-xml: add Parallels xml BlockDriver
block/prl-xml: add bdrv_co_readv/writev and flush
block/prl-xml: add bdrv_probe
block/prl-xml: add bdrv_check
block/prl-xml.c | 560 ++
block/Makefile.objs | 5 +-
tests/qemu-iotests/164| 98 +++
tests/qemu-iotests/164.out| 54 ++
tests/qemu-iotests/check | 7 +
tests/qemu-iotests/group | 1 +
.../prl-xml/DiskDescriptor.xml.bz2| Bin 0 -> 457 bytes
.../sample_images/prl-xml/Snapshots.xml.bz2 | Bin 0 -> 307 bytes
...aabe3-6958-40ff-92a7-860e329aab41}.hds.bz2 | Bin 0 -> 93 bytes
...476cf-d62e-45d1-b355-86feca91376e}.hds.bz2 | Bin 0 -> 93 bytes
10 files changed, 723 insertions(+), 2 deletions(-)
create mode 100644 block/prl-xml.c
create mode 100755 tests/qemu-iotests/164
create mode 100644 tests/qemu-iotests/164.out
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/DiskDescriptor.xml.bz2
create mode 100644 tests/qemu-iotests/sample_images/prl-xml/Snapshots.xml.bz2
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/parallels.{5fbaabe3-6958-40ff-92a7-860e329aab41}.hds.bz2
create mode 100644
tests/qemu-iotests/sample_images/prl-xml/parallels.{986476cf-d62e-45d1-b355-86feca91376e}.hds.bz2
--
2.21.3
Re: [PATCH 08/13] char: Add mux option to ChardevOptions
Am 13.11.2020 um 12:50 hat Paolo Bonzini geschrieben:
> On 12/11/20 18:59, Kevin Wolf wrote:
> > The final missing piece to achieve compatibility between
> > qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
> > is support for the 'mux' option. Implement it.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > qapi/char.json | 4 +++-
> > chardev/char.c | 41 +++--
> > 2 files changed, 38 insertions(+), 7 deletions(-)
> >
> > diff --git a/qapi/char.json b/qapi/char.json
> > index e1f9347044..d6733a5473 100644
> > --- a/qapi/char.json
> > +++ b/qapi/char.json
> > @@ -453,12 +453,14 @@
> > #
> > # @id: the chardev's ID, must be unique
> > # @backend: backend type and parameters
> > +# @mux: enable multiplexing mode (default: false)
> > #
> > # Since: 6.0
> > ##
> > { 'struct': 'ChardevOptions',
> > 'data': { 'id': 'str',
> > -'backend': 'ChardevBackend' },
> > +'backend': 'ChardevBackend',
> > +'*mux': 'bool' },
> > 'aliases': [ { 'source': ['backend'] } ] }
>
> I think this shows that -chardev and chardev-add are both kind of
> unrepairable. The right way to do a mux with a serial and monitor on
> top should be explicit:
>
> stdio
>↑
> mux-controller
> ↑↑
> mux mux
> ↑↑
>serial monitor
>
> -object chardev-stdio,id=stdio
> -object chardev-mux-controller,id=mux,backend=stdio
> -object chardev-mux,id=serial-chardev,controller=mux
> -object chardev-mux,id=mon-chardev,controller=mux
> -serial chardev:serial-chardev
> -monitor chardev:mon-chardev
I don't think these "mux" chardevs plugged to a "mux-controller"
actually exist, at least today. You can directly plug serial and monitor
to a mux chardev that sits on top of stdio.
This is the current syntax for explicitly configuring things:
-chardev stdio,id=stdio
-chardev mux,chardev=stdio,id=mux
-mon chardev=mux
-serial chardev:mux
And of course this is still possible after my series, and it is what
management tools should be using.
> Adding automagic "mux" creation to -chardev is wrong.
I'm not really adding it, it's already there.
While the code is temporarily duplicated and it looks like an addition
here, at the end of the series it's effectively just some preexisting
code moved (and QAPIfied) from qemu_chr_new_from_opts().
Of course, we can deprecate it, but I don't think it's really in the way
because it just desugars to two normal chardev definitions. On the other
hand, I've never used it (apart from testing this patch), so I don't
really care in practice if it exists or not.
> I don't entirely object to the series since it's quite nice, but I see
> it as more of a cleanup than the final stage. It hinges on what
> Markus thinks of aliases, I guess.
Yes, I completely agree that this is mostly a cleanup. Most QAPIfication
actually is, because QemuOpts and hand written parsers do work and we
have been successfully using them for years. They just work in less
consistent ways and take a bit more code.
I never said it has to be the final stage, but I think whatever the
final stage is, having external interfaces that are consistent and use
the QAPI schema as the one source of truth will be helpful.
This is basically what I meant when I said your QOM conversion and my
QAPIfication work aren't conflicting (conceptually), but addressing
separate problems.
Kevin
Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation
"Dr. David Alan Gilbert" writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
>> switched MigrationParameters to narrower integer types, and removed
>> the simplified qmp_migrate_set_parameters()'s argument checking
>> accordingly.
>>
>> Good idea, except qmp_migrate_set_parameters() takes
>> MigrateSetParameters, not MigrationParameters. Its job is updating
>> migrate_get_current()->parameters (which *is* of type
>> MigrationParameters) according to its argument. The integers now get
>> truncated silently. Reproducer:
>>
>> ---> {'execute': 'query-migrate-parameters'}
>> <--- {"return": {[...] "compress-threads": 8, [...]}}
>> ---> {"execute": "migrate-set-parameters", "arguments":
>> {"compress-threads": 257}}
>> <--- {"return": {}}
>> ---> {'execute': 'query-migrate-parameters'}
>> <--- {"return": {[...] "compress-threads": 1, [...]}}
>>
>> Fix by resynchronizing MigrateSetParameters with MigrationParameters.
>
> Having those two separate types is a pain!
It is!
MigrateSetParameters is the argument of migrate-set-parameters,
MigrationParameters is the result of query-migrate-parameters and part
of the internal migration state.
Differences:
(1) Optional members
For migrate-set-parameters, we need *all* members to be optional.
For migration state and query-migrate-parameters, we want only some
members to be optional (currently only @tls-authz, I think).
(2) Special values
migrate-set-parameters has a "reset to default, whatever that may
be" feature for some members (currently only @tls-creds,
@tls-hostname, @tls-authz, I think). Doing that cleanly requires an
additonal value.
The first attempt to fuse the two types (commit de63ab6124 "migrate:
Share common MigrationParameters struct", 2016-10-13) took care of (1).
Introspection of query-migrate-parameters became mildly misleading (it
claims members are optional that aren't), and C code dealing with
migration state had to take care to set the has_FOO = true. Tolerable.
I had to revert it to address (2) cleanly and in time: commit 01fa559826
"migration: Use JSON null instead of "" to reset parameter to default",
2017-07-24.
A second try needs to take care of (2) as well. Messes up
query-migrate-parameters some more: introspection claims a few members
can be null that can't.
Is the "reset" feature is worth all that trouble? Is overloading
migrate-set-parameters a good idea?
>> Fixes: 741d4086c856320807a2575389d7c0505578270b
>> Signed-off-by: Markus Armbruster
>
> Reviewed-by: Dr. David Alan Gilbert
Thanks!
Re: [PATCH 03/10] block: Improve some block-commit, block-stream error messages
On 13.11.20 09:26, Markus Armbruster wrote: block-commit defaults @base-node to the deepest backing image. When there is none, it fails with "Base 'NULL' not found". Improve to "There is no backing image". block-commit and block-stream reject a @base argument that doesn't resolve with "Base 'BASE' not found". Commit 6b33f3ae8b "qemu-img: Improve commit invalid base message" improved this message in qemu-img. Improve it here, too: "Can't find '%s' in the backing chain". QERR_BASE_NOT_FOUND is now unused. Drop. Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-bl...@nongnu.org Signed-off-by: Markus Armbruster --- include/qapi/qmp/qerror.h | 2 -- blockdev.c| 15 +-- tests/qemu-iotests/040| 12 ++-- 3 files changed, 15 insertions(+), 14 deletions(-) Reviewed-by: Max Reitz
[PATCH] tests: add prefixes to the bare mkdtemp calls
The first step to debug a thing is to know what created the thing in
the first place. Add some prefixes so random tmpdir's have something
grep in the code.
Signed-off-by: Alex Bennée
---
python/qemu/machine.py| 2 +-
tests/acceptance/avocado_qemu/__init__.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6420f01bed..052a77592c 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -303,7 +303,7 @@ class QEMUMachine:
return args
def _pre_launch(self) -> None:
-self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
+self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
dir=self._test_dir)
self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
self._qemu_log_file = open(self._qemu_log_path, 'wb')
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index 4cda037187..8496a0c43d 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -171,7 +171,7 @@ class Test(avocado.Test):
self.cancel("No QEMU binary defined or found in the build tree")
def _new_vm(self, *args):
-vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
+vm = QEMUMachine(self.qemu_bin,
sock_dir=tempfile.mkdtemp(prefix="avocado_qemu_sock_"))
if args:
vm.add_args(*args)
return vm
--
2.20.1
Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully
"Dr. David Alan Gilbert" writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> migrate-set-parameters passes the size to xbzrle_cache_resize().
>> xbzrle_cache_resize() checks it fits into size_t before it passes it
>> on to cache_init(). cache_init() checks it is a power of two no
>> smaller than @page_size.
>>
>> cache_init() is also called from xbzrle_init(), bypassing
>> xbzrle_cache_resize()'s check.
>>
>> Drop xbzrle_cache_resize()'s check, and check more carefully in
>> cache_init().
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> migration/page_cache.c | 15 ---
>> migration/ram.c| 7 ---
>> 2 files changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> index b384f265fb..e07f4ad1dc 100644
>> --- a/migration/page_cache.c
>> +++ b/migration/page_cache.c
>> @@ -41,17 +41,10 @@ struct PageCache {
>> PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>> {
>> int64_t i;
>> -size_t num_pages = new_size / page_size;
>> +uint64_t num_pages = new_size / page_size;
>> PageCache *cache;
>>
>> -if (new_size < page_size) {
>> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> - "is smaller than one target page size");
>> -return NULL;
>> -}
>> -
>> -/* round down to the nearest power of 2 */
>> -if (!is_power_of_2(num_pages)) {
>> +if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> "is not a power of two number of pages");
>
> That error message is now wrong since it includes a whole bunch of
> reasons.
We could argue about "wrong", but I readily concedede it needs
improvement:
Parameter 'cache size' expects is not a power of two number of pages
is crap. I fixed similar crap in my "[PATCH 00/10] Chipping away at
qerror.h", but missed this one.
What about
Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size
?
> Also, the comparison is now on the divided num_pages, it's not that
> obvious to me that checking the num_pages makes sense in acomparison to
> checking the actual cache size.
Would you accept
if (!is_power_of_2(new_size)
|| !num_pages || num_pages != (size_t)num_pages) {
?
If not, please propose something you like better.
> (Arguably the check should also happen in migrate_params_test_apply)
Feels like one bridge too far for this patch.
Re: [PATCH 09/10] qom: Improve {qom,device}-list-properties error messages
Paolo Bonzini writes: > On 13/11/20 09:26, Markus Armbruster wrote: >> qom-list-properties reports >> Parameter 'typename' expects device >> when @typename exists, but isn't a TYPE_DEVICE. Improve this to >> Parameter 'typename' expects a non-abstract device type >> device-list-properties reports >> Parameter 'typename' expects object >> when @typename exists, but isn't a TYPE_OBJECT. Improve this to >> Parameter 'typename' expects a QOM type > > Silly mistake: device-list-properties and qom-list-properties are > exchanged in the commit message. Can be fixed without reposting. You're right. Thanks!
Re: [PATCH] tests: add prefixes to the bare mkdtemp calls
Patchew URL: https://patchew.org/QEMU/20201113133424.8723-1-alex.ben...@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20201113133424.8723-1-alex.ben...@linaro.org Type: series Subject: [PATCH] tests: add prefixes to the bare mkdtemp calls === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20201113082626.2725812-1-arm...@redhat.com -> patchew/20201113082626.2725812-1-arm...@redhat.com * [new tag] patchew/20201113133424.8723-1-alex.ben...@linaro.org -> patchew/20201113133424.8723-1-alex.ben...@linaro.org Switched to a new branch 'test' 0a6d11e tests: add prefixes to the bare mkdtemp calls === OUTPUT BEGIN === WARNING: line over 80 characters #23: FILE: python/qemu/machine.py:306: +self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", dir=self._test_dir) ERROR: line over 90 characters #36: FILE: tests/acceptance/avocado_qemu/__init__.py:174: +vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp(prefix="avocado_qemu_sock_")) total: 1 errors, 1 warnings, 16 lines checked Commit 0a6d11e02824 (tests: add prefixes to the bare mkdtemp calls) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20201113133424.8723-1-alex.ben...@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL 0/6] Fixes 20201113 patches
On Fri, 13 Nov 2020 at 12:31, Gerd Hoffmann wrote: > > The following changes since commit cb5d19e8294486551c422759260883ed290226d9: > > Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201112' i= > nto staging (2020-11-12 11:33:26 +) > > are available in the Git repository at: > > git://git.kraxel.org/qemu tags/fixes-20201113-pull-request > > for you to fetch changes up to 172bc8520db1cb98d09b367360068a675fbc9413: > > xhci: fix nec-usb-xhci properties (2020-11-13 07:36:33 +0100) > > > fixes for console, audio, usb, vga. > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
