[Qemu-devel] [PATCH][Outreachy]
util/envlist.c:This patch replaces malloc with g_malloc
This replacement was suggested as part of the bite-sized tasks.
Signed-off-by: Sarah Khan
--
diff --git a/util/envlist.c b/util/envlist.c
index e86857e..0324fe2 100644
--- a/util/envlist.c
+++ b/util/envlist.c
@@ -25,7 +25,7 @@ envlist_create(void)
{
envlist_t *envlist;
- if ((envlist = malloc(sizeof (*envlist))) == NULL)
+ if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
return (NULL);
QLIST_INIT(&envlist->el_entries);
@@ -48,10 +48,10 @@ envlist_free(envlist_t *envlist)
entry = envlist->el_entries.lh_first;
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
}
- free(envlist);
+ g_free(envlist);
}
/*
@@ -155,16 +155,16 @@ envlist_setenv(envlist_t *envlist, const char *env)
if (entry != NULL) {
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
} else {
envlist->el_count++;
}
- if ((entry = malloc(sizeof (*entry))) == NULL)
+ if ((entry = g_malloc(sizeof (*entry))) == NULL)
return (errno);
if ((entry->ev_var = strdup(env)) == NULL) {
- free(entry);
+ g_free(entry);
return (errno);
}
QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
@@ -201,8 +201,8 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
}
if (entry != NULL) {
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
envlist->el_count--;
}
@@ -225,7 +225,7 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
struct envlist_entry *entry;
char **env, **penv;
- penv = env = malloc((envlist->el_count + 1) * sizeof (char *));
+ penv = env = g_malloc((envlist->el_count + 1) * sizeof (char *));
if (env == NULL)
return (NULL);
---
util/envlist.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/util/envlist.c b/util/envlist.c
index e86857e..0324fe2 100644
--- a/util/envlist.c
+++ b/util/envlist.c
@@ -25,7 +25,7 @@ envlist_create(void)
{
envlist_t *envlist;
- if ((envlist = malloc(sizeof (*envlist))) == NULL)
+ if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
return (NULL);
QLIST_INIT(&envlist->el_entries);
@@ -48,10 +48,10 @@ envlist_free(envlist_t *envlist)
entry = envlist->el_entries.lh_first;
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
}
- free(envlist);
+ g_free(envlist);
}
/*
@@ -155,16 +155,16 @@ envlist_setenv(envlist_t *envlist, const char *env)
if (entry != NULL) {
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
} else {
envlist->el_count++;
}
- if ((entry = malloc(sizeof (*entry))) == NULL)
+ if ((entry = g_malloc(sizeof (*entry))) == NULL)
return (errno);
if ((entry->ev_var = strdup(env)) == NULL) {
- free(entry);
+ g_free(entry);
return (errno);
}
QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
@@ -201,8 +201,8 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
}
if (entry != NULL) {
QLIST_REMOVE(entry, ev_link);
- free((char *)entry->ev_var);
- free(entry);
+ g_free((char *)entry->ev_var);
+ g_free(entry);
envlist->el_count--;
}
@@ -225,7 +225,7 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
struct envlist_entry *entry;
char **env, **penv;
- penv = env = malloc((envlist->el_count + 1) * sizeof (char *));
+ penv = env = g_malloc((envlist->el_count + 1) * sizeof (char *));
if (env == NULL)
return (NULL);
--
1.9.1
[Qemu-devel] [PATCH v3 4/4] ui/cocoa.m: switch to QKeyCode
This patch removes the pc/xt keycode map and replaces it with the QKeyCode
keymap.
Signed-off-by: John Arbuckle
---
Removed the LARGEST_KEYCODE marco.
Changed macToQKeyCodeMap to mac_to_qkeycode_map.
ui/cocoa.m | 317 +++--
1 file changed, 141 insertions(+), 176 deletions(-)
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 3ee5549..e57b9cc 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -33,6 +33,7 @@
#include "sysemu/sysemu.h"
#include "qmp-commands.h"
#include "sysemu/blockdev.h"
+#include
#ifndef MAC_OS_X_VERSION_10_5
#define MAC_OS_X_VERSION_10_5 1050
@@ -72,178 +73,139 @@ bool stretch_video;
NSTextField *pauseLabel;
NSArray * supportedImageFileTypes;
-// keymap conversion
-int keymap[] =
-{
-// SdlImacImacHSdlH104xtH 104xtC sdl
-30, // 0 0x000x1eA QZ_a
-31, // 1 0x010x1fS QZ_s
-32, // 2 0x020x20D QZ_d
-33, // 3 0x030x21F QZ_f
-35, // 4 0x040x23H QZ_h
-34, // 5 0x050x22G QZ_g
-44, // 6 0x060x2cZ QZ_z
-45, // 7 0x070x2dX QZ_x
-46, // 8 0x080x2eC QZ_c
-47, // 9 0x090x2fV QZ_v
-0, // 10 0x0AUndefined
-48, // 11 0x0B0x30B QZ_b
-16, // 12 0x0C0x10Q QZ_q
-17, // 13 0x0D0x11W QZ_w
-18, // 14 0x0E0x12E QZ_e
-19, // 15 0x0F0x13R QZ_r
-21, // 16 0x100x15Y QZ_y
-20, // 17 0x110x14T QZ_t
-2, // 18 0x120x021 QZ_1
-3, // 19 0x130x032 QZ_2
-4, // 20 0x140x043 QZ_3
-5, // 21 0x150x054 QZ_4
-7, // 22 0x160x076 QZ_6
-6, // 23 0x170x065 QZ_5
-13, // 24 0x180x0d= QZ_EQUALS
-10, // 25 0x190x0a9 QZ_9
-8, // 26 0x1A0x087 QZ_7
-12, // 27 0x1B0x0c- QZ_MINUS
-9, // 28 0x1C0x098 QZ_8
-11, // 29 0x1D0x0b0 QZ_0
-27, // 30 0x1E0x1b] QZ_RIGHTBRACKET
-24, // 31 0x1F0x18O QZ_o
-22, // 32 0x200x16U QZ_u
-26, // 33 0x210x1a[ QZ_LEFTBRACKET
-23, // 34 0x220x17I QZ_i
-25, // 35 0x230x19P QZ_p
-28, // 36 0x240x1cENTER QZ_RETURN
-38, // 37 0x250x26L QZ_l
-36, // 38 0x260x24J QZ_j
-40, // 39 0x270x28' QZ_QUOTE
-37, // 40 0x280x25K QZ_k
-39, // 41 0x290x27; QZ_SEMICOLON
-43, // 42 0x2A0x2b\ QZ_BACKSLASH
-51, // 43 0x2B0x33, QZ_COMMA
-53, // 44 0x2C0x35/ QZ_SLASH
-49, // 45 0x2D0x31N QZ_n
-50, // 46 0x2E0x32M QZ_m
-52, // 47 0x2F0x34. QZ_PERIOD
-15, // 48 0x300x0fTAB QZ_TAB
-57, // 49 0x310x39SPACE QZ_SPACE
-41, // 50 0x320x29` QZ_BACKQUOTE
-14, // 51 0x330x0eBKSPQZ_BACKSPACE
-0, // 52 0x34Undefined
-1, // 53 0x350x01ESC QZ_ESCAPE
-220, // 54 0x360xdcE0,5C R GUI QZ_RMETA
-219, // 55 0x370xdbE0,5B L GUI QZ_LMETA
-42, // 56 0x380x2aL SHFT QZ_LSHIFT
-58, // 57 0x390x3aCAPSQZ_CAPSLOCK
-56, // 58 0x3A0x38L ALT QZ_LALT
-29, // 59 0x3B0x1dL CTRL QZ_LCTRL
-54, // 60 0x3C0x36R SHFT QZ_RSHIFT
-184,// 61 0x3D0xb8E0,38 R ALT QZ_RALT
-157,// 62 0x3E0x9dE0,1D R CTRL QZ_RCTRL
-0, // 63 0x3FUndefined
-0, // 64 0x40Undefined
-0, // 65 0x41Undefined
-0, // 66 0x42Undefined
-55, // 67 0x430x37KP *QZ_KP_MULTIPLY
-0, // 68 0x44Undefined
-78, // 69 0x450x4eKP +QZ_KP_PLUS
-0, // 70 0x46Undefined
-69, // 71 0x470x45NUM QZ_N
[Qemu-devel] [PATCH v3 3/4] hw/input/adb.c: implement QKeyCode support
Remove the old pc_to_adb_keycode array and replace it with QKeyCode support.
Signed-off-by: John Arbuckle
---
Some of the keys do not translate as logically as we would think they would. For
example the Q_KEY_CODE_CTRL_R does not work with ADB_KEY_RIGHT_CONTROL. The
wrong key would show up in the guest. These problem keys are commmented out and
replaced with the number that does work correctly. This patch can be easily
tested with the Linux command xev or Mac OS's Key Caps.
hw/input/adb.c | 223 +
1 file changed, 177 insertions(+), 46 deletions(-)
diff --git a/hw/input/adb.c b/hw/input/adb.c
index f0ad0d4..d176d39 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -25,6 +25,9 @@
#include "hw/hw.h"
#include "hw/input/adb.h"
#include "ui/console.h"
+#include "include/hw/input/adb-keys.h"
+#include "ui/input.h"
+#include "sysemu/sysemu.h"
/* debug ADB */
//#define DEBUG_ADB
@@ -59,6 +62,9 @@ do { printf("ADB: " fmt , ## __VA_ARGS__); } while (0)
/* error codes */
#define ADB_RET_NOTPRESENT (-2)
+/* The adb keyboard doesn't have every key imaginable */
+#define NO_KEY 0xff
+
static void adb_device_reset(ADBDevice *d)
{
qdev_reset_all(DEVICE(d));
@@ -187,23 +193,138 @@ typedef struct ADBKeyboardClass {
DeviceRealize parent_realize;
} ADBKeyboardClass;
-static const uint8_t pc_to_adb_keycode[256] = {
- 0, 53, 18, 19, 20, 21, 23, 22, 26, 28, 25, 29, 27, 24, 51, 48,
- 12, 13, 14, 15, 17, 16, 32, 34, 31, 35, 33, 30, 36, 54, 0, 1,
- 2, 3, 5, 4, 38, 40, 37, 41, 39, 50, 56, 42, 6, 7, 8, 9,
- 11, 45, 46, 43, 47, 44,123, 67, 58, 49, 57,122,120, 99,118, 96,
- 97, 98,100,101,109, 71,107, 89, 91, 92, 78, 86, 87, 88, 69, 83,
- 84, 85, 82, 65, 0, 0, 10,103,111, 0, 0,110, 81, 0, 0, 0,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
- 0, 0, 0, 94, 0, 93, 0, 0, 0, 0, 0, 0,104,102, 0, 0,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 76,125, 0, 0,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,105, 0, 0, 0, 0, 0,
- 0, 0, 0, 0, 0, 75, 0, 0,124, 0, 0, 0, 0, 0, 0, 0,
- 0, 0, 0, 0, 0, 0, 0,115, 62,116, 0, 59, 0, 60, 0,119,
- 61,121,114,117, 0, 0, 0, 0, 0, 0, 0, 55,126, 0,127, 0,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
- 0, 0, 0, 0, 0, 95, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+int qcode_to_adb_keycode[] = {
+[Q_KEY_CODE_SHIFT] = ADB_KEY_LEFT_SHIFT,
+[Q_KEY_CODE_SHIFT_R] = 123, /* ADB_KEY_RIGHT_SHIFT, */
+[Q_KEY_CODE_ALT] = ADB_KEY_LEFT_OPTION,
+[Q_KEY_CODE_ALT_R] = 124, /* ADB_KEY_RIGHT_OPTION,*/
+[Q_KEY_CODE_ALTGR] = ADB_KEY_RIGHT_OPTION,
+[Q_KEY_CODE_CTRL] = 54, /* ADB_KEY_LEFT_CONTROL, */
+[Q_KEY_CODE_CTRL_R]= 125, /* ADB_KEY_RIGHT_CONTROL, */
+[Q_KEY_CODE_META_L]= ADB_KEY_LEFT_COMMAND,
+
+ /* 126 works as right super in Linux */
+ /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
+[Q_KEY_CODE_META_R]= ADB_KEY_LEFT_COMMAND,
+[Q_KEY_CODE_SPC] = ADB_KEY_SPACEBAR,
+
+[Q_KEY_CODE_ESC] = ADB_KEY_ESC,
+[Q_KEY_CODE_1] = ADB_KEY_1,
+[Q_KEY_CODE_2] = ADB_KEY_2,
+[Q_KEY_CODE_3] = ADB_KEY_3,
+[Q_KEY_CODE_4] = ADB_KEY_4,
+[Q_KEY_CODE_5] = ADB_KEY_5,
+[Q_KEY_CODE_6] = ADB_KEY_6,
+[Q_KEY_CODE_7] = ADB_KEY_7,
+[Q_KEY_CODE_8] = ADB_KEY_8,
+[Q_KEY_CODE_9] = ADB_KEY_9,
+[Q_KEY_CODE_0] = ADB_KEY_0,
+[Q_KEY_CODE_MINUS] = ADB_KEY_MINUS,
+[Q_KEY_CODE_EQUAL] = ADB_KEY_EQUAL,
+[Q_KEY_CODE_BACKSPACE] = ADB_KEY_DELETE,
+[Q_KEY_CODE_TAB] = ADB_KEY_TAB,
+[Q_KEY_CODE_Q] = ADB_KEY_Q,
+[Q_KEY_CODE_W] = ADB_KEY_W,
+[Q_KEY_CODE_E] = ADB_KEY_E,
+[Q_KEY_CODE_R] = ADB_KEY_R,
+[Q_KEY_CODE_T] = ADB_KEY_T,
+[Q_KEY_CODE_Y] = ADB_KEY_Y,
+[Q_KEY_CODE_U] = ADB_KEY_U,
+[Q_KEY_CODE_I] = ADB_KEY_I,
+[Q_KEY_CODE_O] = ADB_KEY_O,
+[Q_KEY_CODE_P] = ADB_KEY_P,
+[Q_KEY_CODE_BRACKET_LEFT] = ADB_KEY_LEFT_BRACKET,
+[Q_KEY_CODE_BRACKET_RIGHT] = ADB_KEY_RIGHT_BRACKET,
+[Q_KEY_CODE_RET] = ADB_KEY_RETURN,
+[Q_KEY_CODE_A] = ADB_KEY_A,
+[Q_KEY_CODE_S] = ADB_KEY_S,
+[Q_KEY_CODE_D] = ADB_KEY_D,
+[Q_KEY_CODE_F] = ADB_KEY_F,
+[Q_KEY_CODE_G] = ADB_KEY_G,
+[Q_KEY_CODE_H] = ADB_KEY_H,
+[Q_KEY_CODE_J] = ADB_KEY_J,
+[Q_KEY_CODE_K] = ADB_KEY_K,
+[Q_KEY_CODE_L] = ADB_KEY_L,
+[Q_KEY_CODE_SEMICOLON] = ADB_KEY_SEMICOLON,
+[Q_KEY_CODE_APOSTROPHE]= A
[Qemu-devel] [PATCH v3 2/4] qapi-schema.json: Add power and keypad equal keys
Add the power and keypad equal keys. These keys are found on a real Macintosh
keyboard.
Signed-off-by: John Arbuckle
---
qapi-schema.json | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 8d04897..5cf34bf 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3026,6 +3026,7 @@
#
# 'unmapped' and 'pause' since 2.0
# 'ro' and 'kp_comma' since 2.4
+# 'kp_equals' and 'power' since 2.6
##
{ 'enum': 'QKeyCode',
'data': [ 'unmapped',
@@ -3044,7 +3045,7 @@
'left', 'up', 'down', 'right', 'insert', 'delete', 'stop', 'again',
'props', 'undo', 'front', 'copy', 'open', 'paste', 'find', 'cut',
'lf', 'help', 'meta_l', 'meta_r', 'compose', 'pause', 'ro',
-'kp_comma' ] }
+'kp_comma', 'kp_equals', 'power' ] }
##
# @KeyValue
--
2.7.2
[Qemu-devel] [PATCH v3 1/4] adb-keys.h: initial commit
This commit implements the adb-keys.h file. It holds information on adb keycode
values.
Signed-off-by: John Arbuckle
---
Changed name of file from MacKeys.h to adb-keys.h.
Changed name of constants from MAC_KEYS_ to ADB_KEYS_.
include/hw/input/adb-keys.h | 160
1 file changed, 160 insertions(+)
create mode 100644 include/hw/input/adb-keys.h
diff --git a/include/hw/input/adb-keys.h b/include/hw/input/adb-keys.h
new file mode 100644
index 000..6e009ee
--- /dev/null
+++ b/include/hw/input/adb-keys.h
@@ -0,0 +1,160 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * 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.
+ */
+
+/*
+ * adb-keys.h
+ *
+ * Provides a enum of all the Macintosh keycodes.
+ * Note: keys like Power, volume related, and eject are handled at a lower
+ *level and are not available to QEMU. That doesn't mean we can't
+ *substitute one key for another. The function keys like F1 make a good
+ *substitute for these keys. This can be done in the GTK, SDL, or Cocoa
+ *code.
+ */
+
+#ifndef __ADBKEYS__
+#define __ADBKEYS__
+
+enum {
+ADB_KEY_A = 0,
+ADB_KEY_B = 11,
+ADB_KEY_C = 8,
+ADB_KEY_D = 2,
+ADB_KEY_E = 14,
+ADB_KEY_F = 3,
+ADB_KEY_G = 5,
+ADB_KEY_H = 4,
+ADB_KEY_I = 34,
+ADB_KEY_J = 38,
+ADB_KEY_K = 40,
+ADB_KEY_L = 37,
+ADB_KEY_M = 46,
+ADB_KEY_N = 45,
+ADB_KEY_O = 31,
+ADB_KEY_P = 35,
+ADB_KEY_Q = 12,
+ADB_KEY_R = 15,
+ADB_KEY_S = 1,
+ADB_KEY_T = 17,
+ADB_KEY_U = 32,
+ADB_KEY_V = 9,
+ADB_KEY_W = 13,
+ADB_KEY_X = 7,
+ADB_KEY_Y = 16,
+ADB_KEY_Z = 6,
+
+ADB_KEY_0 = 29,
+ADB_KEY_1 = 18,
+ADB_KEY_2 = 19,
+ADB_KEY_3 = 20,
+ADB_KEY_4 = 21,
+ADB_KEY_5 = 23,
+ADB_KEY_6 = 22,
+ADB_KEY_7 = 26,
+ADB_KEY_8 = 28,
+ADB_KEY_9 = 25,
+
+ADB_KEY_GRAVE_ACCENT = 50,
+ADB_KEY_MINUS = 27,
+ADB_KEY_EQUAL = 24,
+ADB_KEY_DELETE = 51,
+ADB_KEY_CAPS_LOCK = 57,
+ADB_KEY_TAB = 48,
+ADB_KEY_RETURN = 36,
+ADB_KEY_LEFT_BRACKET = 33,
+ADB_KEY_RIGHT_BRACKET = 30,
+ADB_KEY_BACKSLASH = 42,
+ADB_KEY_SEMICOLON = 41,
+ADB_KEY_APOSTROPHE = 39,
+ADB_KEY_COMMA = 43,
+ADB_KEY_PERIOD = 47,
+ADB_KEY_FORWARD_SLASH = 44,
+ADB_KEY_LEFT_SHIFT = 56,
+ADB_KEY_RIGHT_SHIFT = 60,
+ADB_KEY_SPACEBAR = 49,
+ADB_KEY_LEFT_CONTROL = 59,
+ADB_KEY_RIGHT_CONTROL = 62,
+ADB_KEY_LEFT_OPTION = 58,
+ADB_KEY_RIGHT_OPTION = 61,
+ADB_KEY_LEFT_COMMAND = 55,
+ADB_KEY_RIGHT_COMMAND = 54,
+
+ADB_KEY_KP_0 = 82,
+ADB_KEY_KP_1 = 83,
+ADB_KEY_KP_2 = 84,
+ADB_KEY_KP_3 = 85,
+ADB_KEY_KP_4 = 86,
+ADB_KEY_KP_5 = 87,
+ADB_KEY_KP_6 = 88,
+ADB_KEY_KP_7 = 89,
+ADB_KEY_KP_8 = 91,
+ADB_KEY_KP_9 = 92,
+ADB_KEY_KP_PERIOD = 65,
+ADB_KEY_KP_ENTER = 76,
+ADB_KEY_KP_PLUS = 69,
+ADB_KEY_KP_SUBTRACT = 78,
+ADB_KEY_KP_MULTIPLY = 67,
+ADB_KEY_KP_DIVIDE = 75,
+ADB_KEY_KP_EQUAL = 81,
+ADB_KEY_KP_CLEAR = 71,
+
+ADB_KEY_UP = 126,
+ADB_KEY_DOWN = 125,
+ADB_KEY_LEFT = 123,
+ADB_KEY_RIGHT = 124,
+
+ADB_KEY_HELP = 114,
+ADB_KEY_HOME = 115,
+ADB_KEY_PAGE_UP = 116,
+ADB_KEY_PAGE_DOWN = 121,
+ADB_KEY_END = 119,
+ADB_KEY_FORWARD_DELETE = 117,
+
+ADB_KEY_ESC = 53,
+ADB_KEY_F1 = 122,
+ADB_KEY_F2 = 120,
+ADB_KEY_F3 = 99,
+ADB_KEY_F4 = 118,
+ADB_KEY_F5 = 96,
+ADB_KEY_F6 = 97,
+ADB_KEY_F7 = 98,
+ADB_KEY_F8 = 100,
+ADB_KEY_F9 = 101,
+ADB_KEY_F10 = 109,
+ADB_KEY_F11 = 103,
+ADB_KEY_F12 = 111,
+ADB_KEY_F13 = 105,
+ADB_KEY_F14 = 107,
+ADB_KEY_F15 = 113,
+
+ADB_KEY_VOLUME_UP = 72,
+ADB_KEY_VOLUME_DOWN = 73,
+ADB_KEY_VOLUME_MUTE = 74,
+ADB_KEY_POWER = 32639,
+};
+
+/* Could not find the value for this key. */
+/* #define ADB_KEY_EJECT */
+
+#en
[Qemu-devel] [PATCH v3 0/4] Implement some QKeyCode support
This patchset adds QKeyCode support the adb and cocoa code. Note: you do not need to be on a Mac to test out the adb.c, qapi-schema.json, and adb-keys.h files. Only the cocoa.m file changes are Mac specific. If you are using Linux, then the xev command is what you could use to test out these patches. For Mac users the Key Caps application would help with testing out these patches. John Arbuckle (4): adb-keys.h qapi-schema.json adb.c cocoa.m hw/input/adb.c | 223 --- include/hw/input/adb-keys.h | 160 ++ qapi-schema.json| 3 +- ui/cocoa.m | 317 4 files changed, 480 insertions(+), 223 deletions(-) create mode 100644 include/hw/input/adb-keys.h -- 2.7.2
[Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model
Implement basic AST2400 timer functionality: Up to 8 timers can
independently be configured, enabled, reset and disabled. A couple of
hardware features are not implemented, namely clock value matching and
pulse generation, but the implementation is enough to boot the Linux
kernel configured with aspeed_defconfig.
Signed-off-by: Andrew Jeffery
---
Since v2:
* Improve handling of timer configuration with respect to enabled state
* Remove redundant enabled member from AspeedTimer
* Implement VMStateDescriptions
* Fix interrupt behaviour (edge triggered, both edges)
* Fix various issues with trace-event declarations
* Include qemu/osdep.h
Since v1:
* Refactor initialisation of and respect requested clock rates (APB/External)
* Simplify some index calculations
* Use tracing infrastructure instead of internal DPRINTF
* Enforce access size constraints and alignment in MemoryRegionOps
default-configs/arm-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/aspeed_timer.c | 452
include/hw/timer/aspeed_timer.h | 59 ++
trace-events| 9 +
5 files changed, 522 insertions(+)
create mode 100644 hw/timer/aspeed_timer.c
create mode 100644 include/hw/timer/aspeed_timer.h
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index a9f82a1..2bcd236 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -110,3 +110,4 @@ CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_ACPI=y
CONFIG_SMBIOS=y
+CONFIG_ASPEED_SOC=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 5cfea6e..003c14f 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
new file mode 100644
index 000..f1d232c
--- /dev/null
+++ b/hw/timer/aspeed_timer.c
@@ -0,0 +1,452 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery
+ *
+ * Copyright (C) 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ptimer.h"
+#include "hw/sysbus.h"
+#include "hw/timer/aspeed_timer.h"
+#include "qemu-common.h"
+#include "qemu/bitops.h"
+#include "qemu/main-loop.h"
+#include "qemu/timer.h"
+#include "trace.h"
+
+#define TIMER_NR_REGS 4
+
+#define TIMER_CTRL_BITS 4
+#define TIMER_CTRL_MASK ((1 << TIMER_CTRL_BITS) - 1)
+
+#define TIMER_CLOCK_USE_EXT true
+#define TIMER_CLOCK_EXT_HZ 100
+#define TIMER_CLOCK_USE_APB false
+#define TIMER_CLOCK_APB_HZ 2400
+
+#define TIMER_REG_STATUS 0
+#define TIMER_REG_RELOAD 1
+#define TIMER_REG_MATCH_FIRST 2
+#define TIMER_REG_MATCH_SECOND 3
+
+#define TIMER_FIRST_CAP_PULSE 4
+
+enum timer_ctrl_op {
+op_enable = 0,
+op_external_clock,
+op_overflow_interrupt,
+op_pulse_enable
+};
+
+/**
+ * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer
+ * structs, as it's a waste of memory and it makes implementing
+ * VMStateDescription a little clunky. The ptimer BH callback needs to know
+ * whether a specific AspeedTimer is enabled, but this information is held in
+ * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
+ * arbitrary AspeedTimer to AspeedTimerCtrlState.
+ */
+static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
+{
+AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
+return container_of(timers, AspeedTimerCtrlState, timers);
+}
+
+static inline bool timer_ctrl_status(AspeedTimer *t, enum timer_ctrl_op op)
+{
+return !!(timer_to_ctrl(t)->ctrl & BIT(t->id * TIMER_CTRL_BITS + op));
+}
+
+static inline bool timer_enabled(AspeedTimer *t)
+{
+return timer_ctrl_status(t, op_enable);
+}
+
+static inline bool timer_overflow_interrupt(AspeedTimer *t)
+{
+return timer_ctrl_status(t, op_overflow_interrupt);
+}
+
+static inline bool timer_can_pulse(AspeedTimer *t)
+{
+return t->id >= TIMER_FIRST_CAP_PULSE;
+}
+
+static void aspeed_timer_expire(void *opaque)
+{
+AspeedTimer *t = opaque;
+
+/* Only support interrupts on match values of zero for the moment - this is
+ * sufficient to boot an aspeed_defconfig Linux kernel. Non-zero match
+ * values need some further consideration given the current ptimer API.
+ * Maybe run multiple ptimers?
+ */
+bool match = !(t->match[0] && t->match[1]);
+bool interrupt = timer_overflow_interrupt(t) || match;
+if (timer_enabled(t) && interrupt) {
+t->level = !t->level;
+qemu_set_irq(t->irq, t->level);
+}
+}
+
+static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
+{
+uint64_t value;
+
+switch (reg) {
+case TIMER_REG_STATUS:
+
[Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model
Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
configured with aspeed_defconfig. The model implements the 'new'
(revised) register set and while the hardware exposes both the new and
legacy register sets, accesses to the legacy register set will not
be serviced (though the access will be logged).
Signed-off-by: Andrew Jeffery
---
Since v2:
* Implement all supported interrupt types and configurations
* Implement a VMStateDescription
* Log accesses to legacy IO space
* Add documentation on some implementation and hardware details
* Switch to extract64/deposit64 where possible
* Drop int_ prefix from some struct member names
* Fix various issues with trace-event declarations
* Include qemu/osdep.h
hw/intc/Makefile.objs| 1 +
hw/intc/aspeed_vic.c | 335 +++
include/hw/intc/aspeed_vic.h | 48 +++
trace-events | 7 +
4 files changed, 391 insertions(+)
create mode 100644 hw/intc/aspeed_vic.c
create mode 100644 include/hw/intc/aspeed_vic.h
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 6a13a39..0e47f0f 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -31,3 +31,4 @@ obj-$(CONFIG_XICS_KVM) += xics_kvm.o
obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
obj-$(CONFIG_S390_FLIC) += s390_flic.o
obj-$(CONFIG_S390_FLIC_KVM) += s390_flic_kvm.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_vic.o
diff --git a/hw/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
new file mode 100644
index 000..8cfaa02
--- /dev/null
+++ b/hw/intc/aspeed_vic.c
@@ -0,0 +1,335 @@
+/*
+ * ASPEED Interrupt Controller (New)
+ *
+ * Andrew Jeffery
+ *
+ * Copyright 2015, 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+/* The hardware exposes two register sets, a legacy set and a 'new' set. The
+ * model implements the 'new' register set, and logs warnings on accesses to
+ * the legacy IO space.
+ *
+ * The hardware uses 32bit registers to manage 51 IRQs, with low and high
+ * registers for each conceptual register. The device model's implementation
+ * uses 64bit data types to store both low and high register values (in the one
+ * member), but must cope with access offset values in multiples of 4 passed to
+ * the callbacks. As such the read() and write() implementations process the
+ * provided offset to understand whether the access is requesting the lower or
+ * upper 32 bits of the 64bit member.
+ *
+ * Additionally, the "Interrupt Enable", "Edge Status" and "Software Interrupt"
+ * fields have separate "enable"/"status" and "clear" registers, where set bits
+ * are written to one or the other to change state (avoiding a
+ * read-modify-write sequence).
+ */
+
+#include "qemu/osdep.h"
+#include
+#include "hw/intc/aspeed_vic.h"
+#include "qemu/bitops.h"
+#include "trace.h"
+
+#define AVIC_NEW_BASE_OFFSET 0x80
+
+#define AVIC_L_MASK 0xU
+#define AVIC_H_MASK 0x0007U
+#define AVIC_EVENT_W_MASK (0x78000ULL << 32)
+
+static void aspeed_vic_update(AspeedVICState *s)
+{
+uint64_t new = (s->raw & s->enable);
+uint64_t flags;
+
+flags = new & s->select;
+trace_aspeed_vic_update_fiq(!!flags);
+qemu_set_irq(s->fiq, !!flags);
+
+flags = new & ~s->select;
+trace_aspeed_vic_update_irq(!!flags);
+qemu_set_irq(s->irq, !!flags);
+}
+
+static void aspeed_vic_set_irq(void *opaque, int irq, int level)
+{
+uint64_t irq_mask;
+bool raise;
+AspeedVICState *s = (AspeedVICState *)opaque;
+
+if (irq > ASPEED_VIC_NR_IRQS) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Out-of-range interrupt number:
%d\n",
+ __func__, irq);
+return;
+}
+
+trace_aspeed_vic_set_irq(irq, level);
+
+irq_mask = BIT(irq);
+if (s->sense & irq_mask) {
+/* level-triggered */
+if (s->event & irq_mask) {
+/* high-sensitive */
+raise = level;
+} else {
+/* low-sensitive */
+raise = !level;
+}
+s->raw = deposit64(s->raw, irq, 1, raise);
+} else {
+uint64_t old_level = s->level & irq_mask;
+
+/* edge-triggered */
+if (s->dual_edge & irq_mask) {
+raise = (!!old_level) != (!!level);
+} else {
+if (s->event & irq_mask) {
+/* rising-sensitive */
+raise = !old_level && level;
+} else {
+/* falling-sensitive */
+raise = old_level && !level;
+}
+}
+if (raise) {
+s->raw = deposit64(s->raw, irq, 1, raise);
+}
+}
+s->level = deposit64(s->level, irq, 1, level);
+aspeed_vic_update(s);
+}
+
+static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
+{
+uint64_t val;
+const bool high = !!(offset & 0x4);
+hwaddr n_offset = (offset & ~0x4);
+AspeedVICState
[Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model
Adds an AST2400 ARM machine[1], based around an AST2400 SOC containing an
ARM926 processor, ASPEED VIC and timer devices, and a 8250 UART. The new
machine type is functional enough to boot an aspeed_defconfig Linux
kernel to userspace.
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery
---
Since v2:
* Implement a SOC model to move code out from the machine definition
* Rework the machine to better use QOM
* Include qemu/osdep.h
* Revert back to qemu_log_mask(LOG_UNIMP, ...) in IO handlers
Since v1:
hw/arm/Makefile.objs | 1 +
hw/arm/ast2400.c | 208 +++
2 files changed, 209 insertions(+)
create mode 100644 hw/arm/ast2400.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index a711e4d..f333b7f 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -16,3 +16,4 @@ obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
+obj-$(CONFIG_ASPEED_SOC) += ast2400.o
diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
new file mode 100644
index 000..74aca49
--- /dev/null
+++ b/hw/arm/ast2400.c
@@ -0,0 +1,208 @@
+/*
+ * ASPEED AST2400
+ *
+ * Andrew Jeffery
+ * Jeremy Kerr
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "exec/address-spaces.h"
+#include "hw/arm/arm.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/sysbus.h"
+#include "hw/intc/aspeed_vic.h"
+#include "hw/timer/aspeed_timer.h"
+#include "target-arm/cpu.h"
+#include "trace.h"
+
+#define AST2400_UART_5_BASE 0x00184000
+#define AST2400_IOMEM_SIZE 0x0020
+#define AST2400_IOMEM_BASE 0x1E60
+#define AST2400_VIC_BASE 0x1E6C
+#define AST2400_TIMER_BASE 0x1E782000
+#define AST2400_SDRAM_BASE 0x4000
+
+static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
+static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, };
+
+static struct arm_boot_info ast2400_binfo = {
+.loader_start = AST2400_SDRAM_BASE,
+.board_id = 0,
+.nb_cpus = 1,
+};
+
+/*
+ * IO handlers: simply catch any reads/writes to IO addresses that aren't
+ * handled by a device mapping.
+ */
+
+static uint64_t ast2400_soc_io_read(void *p, hwaddr offset, unsigned size)
+{
+qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
+ __func__, offset, size);
+return 0;
+}
+
+static void ast2400_soc_io_write(void *opaque, hwaddr offset, uint64_t value,
+unsigned size)
+{
+qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+ __func__, offset, value, size);
+}
+
+static const MemoryRegionOps ast2400_soc_io_ops = {
+.read = ast2400_soc_io_read,
+.write = ast2400_soc_io_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+typedef struct AST2400SOCState {
+/*< private >*/
+DeviceState parent;
+
+/*< public >*/
+ARMCPU *cpu;
+MemoryRegion iomem;
+AspeedVICState vic;
+AspeedTimerCtrlState timerctrl;
+} AST2400SOCState;
+
+#define TYPE_AST2400_SOC "ast2400-soc"
+#define AST2400_SOC(obj) OBJECT_CHECK(AST2400SOCState, (obj), TYPE_AST2400_SOC)
+
+static void ast2400_soc_init(Object *obj)
+{
+AST2400SOCState *s = AST2400_SOC(obj);
+
+s->cpu = cpu_arm_init("arm926");
+
+object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
+object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
+qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
+
+object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
+object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
+}
+
+static void ast2400_soc_realize(DeviceState *dev, Error **errp)
+{
+int i;
+AST2400SOCState *s = AST2400_SOC(dev);
+Error *err = NULL;
+
+/* IO space */
+memory_region_init_io(&s->iomem, NULL, &ast2400_soc_io_ops, NULL,
+"ast2400.io", AST2400_IOMEM_SIZE);
+memory_region_add_subregion(get_system_memory(), AST2400_IOMEM_BASE,
+&s->iomem);
+
+/* VIC */
+object_property_set_bool(OBJECT(&s->vic), true, "realized", &err);
+if (err) {
+error_propagate(errp, err);
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(&s->vic), 0, AST2400_VIC_BASE);
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 0,
+ qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 1,
+ qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_FIQ));
+
+/* Timer */
+object_property_set_bool(OBJECT(&s->timerctrl), true, "realized", &err);
+if (err) {
+error_propagate(errp
[Qemu-devel] [PATCH v3 0/3] Add ASPEED AST2400 machine model
This patch series models enough of the ASPEED AST2400 ARM9 SoC[0] to boot an aspeed_defconfig Linux kernel[1]. Specifically, the series implements the timer and VIC devices and integrates them into a new ast2400 machine through a AST2400SOC model. The device model patches only partially implement the hardware features of the timer and VIC, again mostly just enough to boot Linux. Unfortunately the datasheet describing the devices is not generally available, but I'll try to add comments to any unclear areas. The addition of the AST2400 to QEMU is motivated by use of the SoC as a BMC in OpenPOWER[2][3] machines and the ongoing development of OpenBMC[4]. The presence of a machine model for the AST2400 will help with development and testing of the OpenBMC stack. Cheers, Andrew [0] http://www.aspeedtech.com/products.php?fPath=20&rId=376 [1] git fetch g...@github.com:openbmc/linux.git dev-4.3 [2] http://openpowerfoundation.org/ [3] https://github.com/open-power/ [4] https://github.com/openbmc/openbmc Changes since v2: This re-roll is a reasonable rework of the patches in the series, which may make it difficult to compare v1 to v2. Addressed reviews/comments from: * Peter Maydell * Alexey Kardashevskiy * Joel Stanley Changes since v1: Addressed reviews/comments from: * Cédric Le Goater Andrew Jeffery (3): hw/timer: Add ASPEED timer device model hw/intc: Add (new) ASPEED VIC device model hw/arm: Add ASPEED AST2400 machine model default-configs/arm-softmmu.mak | 1 + hw/arm/Makefile.objs| 1 + hw/arm/ast2400.c| 208 ++ hw/intc/Makefile.objs | 1 + hw/intc/aspeed_vic.c| 335 + hw/timer/Makefile.objs | 1 + hw/timer/aspeed_timer.c | 452 include/hw/intc/aspeed_vic.h| 48 + include/hw/timer/aspeed_timer.h | 59 ++ trace-events| 16 ++ 10 files changed, 1122 insertions(+) create mode 100644 hw/arm/ast2400.c create mode 100644 hw/intc/aspeed_vic.c create mode 100644 hw/timer/aspeed_timer.c create mode 100644 include/hw/intc/aspeed_vic.h create mode 100644 include/hw/timer/aspeed_timer.h -- 2.5.0
Re: [Qemu-devel] QCow2 compression
Eric Blake wrote:
> [any way you could convince your mailer to not break threading?]
>
> On 03/03/2016 09:24 PM, mgre...@cinci.rr.com wrote:
> >>
> >> The zeros are not part of the compressed data, though, that's why the
> >> Compressed Cluster Descriptor indicates a shorter size. Had another
> >> compressed cluster been written to the same image, it might have ended
> >> up where you are seeing the zero padding now. (The trick with
> >> compression is that multiple guest clusters can end up in a single host
> >> cluster.)
> >>
> >
> > Thanks, but the given length of 0x5600 is still short by 160(decimal) bytes
> > compared to the
> > non-zero data (which occupies an additional 133 bytes beyond the expected
> > end at
> > 0x3DED50) and zero
> > padding (an additional 27 bytes beyond that). Could there be an off-by-one
> > error
> > somewhere?
> > The file doesn't even end on a sector boundary let alone a cluster
> > boundary.
>
> Based on an IRC conversation I had when you first asked the question, I
> think the spec is indeed weak, and that we DO have some fishy code.
>
> Look at what the code does:
>
> uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>uint64_t offset,
>int compressed_size)
> ...
> nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
> (cluster_offset >> 9);
>
> cluster_offset |= QCOW_OFLAG_COMPRESSED |
> ((uint64_t)nb_csectors << s->csize_shift);
>
> That sure does sound like an off-by-one. cluster_offset does indeed
> look like a byte offset (from qcow2_alloc_bytes); so let's consider what
> happens if we've already allocated one compressed cluster in the past,
> going from 65536 to 66999. So on this call, cluster_offset would be
> 67000, and if compressed size is 1025 (just round numbers to make
> discussion easy; no idea if gzip would really do this on any particular
> data), we are computing ((67000+1025-1)>>9) - (67000>>9) == 2, but 1025
> bytes occupies 3 sectors, not 2.
>
> But we offset it by another off-by-one:
>
> int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
> {
> ...
> nb_csectors = ((cluster_offset >> s->csize_shift) &
> s->csize_mask) + 1;
>
> Yuck. That is horribly ugly.
>
> We need to fix the documentation to make it obvious that the sector
> count is a _LOWER BOUND_ on the number of sectors occupied, and that you
> need to read at least one more cluster's worth of data before decompressing.
>
> It would also be nice to fix qcow2 code to not have quite so many
> off-by-one computations, but be more precise about what data is going where.
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Thanks for verifying the behavior for me. This will
allow me to add compression support to the code I've
already written.
I've implemented QCow2 support for a fork of the popular
DOSBox emulator.
The project (not mine), is called DOSBox-X, and is geared
towards emulating Windows9x era hardware with a specific
focus on games: http://dosbox-x.com/
If you'd like to give me any feedback on the QCow2 code I've written
please feel free to e-mail me directly. The relevant files are
qcow2_disk.cpp and qcow_disk.h. If you do, please be kind as
my C++ skills are very rusty. Overall I think it is pretty readable.
It took me about 2 and a half weeks working in my spare time to go
from reading the spec to the final code integrated into DOSBox-X.
Thanks,
Michael Greger
Re: [Qemu-devel] [PATCH v4 08/16] bitops: Add ONES macro
On Mon, Feb 29, 2016 at 3:51 AM, Alex Bennée wrote: > > Alistair Francis writes: > >> From: Peter Crosthwaite >> >> Little macro that just gives you N ones (justified to LSB). >> >> Signed-off-by: Peter Crosthwaite >> Signed-off-by: Alistair Francis >> --- >> >> include/qemu/bitops.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h >> index 8164225..27bf98d 100644 >> --- a/include/qemu/bitops.h >> +++ b/include/qemu/bitops.h >> @@ -430,4 +430,6 @@ static inline uint64_t deposit64(uint64_t value, int >> start, int length, >> return (value & ~mask) | ((fieldval << start) & mask); >> } >> >> +#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1) >> + > > I'm a little ambivalent about this one. The definition should probably > be up at the top of the file with the other #defines. I don't like the > name as it is a little too generic. I also notice in all the actual uses > you immediately invert the result because you want to mask the top bits. > > I suspect what's needed here is a more generic helper: > > #define MAKE_64BIT_MASK (shift, length) \ > ... > > And then the: > > .ro = ~ONES(27) }, > > Becomes: > > .ro = MAKE_64BIT_MASK(27, 64 - 27); > > >> #endif I like that idea. I have updated the implementation to use this style instead of the ONES() style. Thanks, Alistair > > > -- > Alex Bennée >
Re: [Qemu-devel] QCow2 compression
[any way you could convince your mailer to not break threading?]
On 03/03/2016 09:24 PM, mgre...@cinci.rr.com wrote:
>>
>> The zeros are not part of the compressed data, though, that's why the
>> Compressed Cluster Descriptor indicates a shorter size. Had another
>> compressed cluster been written to the same image, it might have ended
>> up where you are seeing the zero padding now. (The trick with
>> compression is that multiple guest clusters can end up in a single host
>> cluster.)
>>
>
> Thanks, but the given length of 0x5600 is still short by 160(decimal) bytes
> compared to the
> non-zero data (which occupies an additional 133 bytes beyond the expected end
> at
> 0x3DED50) and zero
> padding (an additional 27 bytes beyond that). Could there be an off-by-one
> error
> somewhere?
> The file doesn't even end on a sector boundary let alone a cluster boundary.
Based on an IRC conversation I had when you first asked the question, I
think the spec is indeed weak, and that we DO have some fishy code.
Look at what the code does:
uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size)
...
nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
(cluster_offset >> 9);
cluster_offset |= QCOW_OFLAG_COMPRESSED |
((uint64_t)nb_csectors << s->csize_shift);
That sure does sound like an off-by-one. cluster_offset does indeed
look like a byte offset (from qcow2_alloc_bytes); so let's consider what
happens if we've already allocated one compressed cluster in the past,
going from 65536 to 66999. So on this call, cluster_offset would be
67000, and if compressed size is 1025 (just round numbers to make
discussion easy; no idea if gzip would really do this on any particular
data), we are computing ((67000+1025-1)>>9) - (67000>>9) == 2, but 1025
bytes occupies 3 sectors, not 2.
But we offset it by another off-by-one:
int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
{
...
nb_csectors = ((cluster_offset >> s->csize_shift) &
s->csize_mask) + 1;
Yuck. That is horribly ugly.
We need to fix the documentation to make it obvious that the sector
count is a _LOWER BOUND_ on the number of sectors occupied, and that you
need to read at least one more cluster's worth of data before decompressing.
It would also be nice to fix qcow2 code to not have quite so many
off-by-one computations, but be more precise about what data is going where.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 09/16] dma: Add Xilinx Zynq devcfg device model
On Mon, Feb 29, 2016 at 4:20 AM, Alex Bennée wrote:
>
> Alistair Francis writes:
>
>> From: Peter Crosthwaite
>>
>> Minimal device model for devcfg module of Zynq. DMA capabilities and
>> interrupt generation supported.
>>
>> Signed-off-by: Peter Crosthwaite
>> Signed-off-by: Alistair Francis
>> ---
>>
>> default-configs/arm-softmmu.mak | 1 +
>> hw/dma/Makefile.objs | 1 +
>> hw/dma/xlnx-zynq-devcfg.c | 397
>> ++
>> include/hw/dma/xlnx-zynq-devcfg.h | 62 ++
>> 4 files changed, 461 insertions(+)
>> create mode 100644 hw/dma/xlnx-zynq-devcfg.c
>> create mode 100644 include/hw/dma/xlnx-zynq-devcfg.h
>>
>> diff --git a/default-configs/arm-softmmu.mak
>> b/default-configs/arm-softmmu.mak
>> index a9f82a1..d524584 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
>> CONFIG_BITBANG_I2C=y
>> CONFIG_FRAMEBUFFER=y
>> CONFIG_XILINX_SPIPS=y
>> +CONFIG_ZYNQ_DEVCFG=y
>>
>> CONFIG_ARM11SCU=y
>> CONFIG_A9SCU=y
>> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
>> index 0e65ed0..eaf0a81 100644
>> --- a/hw/dma/Makefile.objs
>> +++ b/hw/dma/Makefile.objs
>> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
>> common-obj-$(CONFIG_I82374) += i82374.o
>> common-obj-$(CONFIG_I8257) += i8257.o
>> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
>> +common-obj-$(CONFIG_ZYNQ_DEVCFG) += xlnx-zynq-devcfg.o
>> common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
>> common-obj-$(CONFIG_STP2000) += sparc32_dma.o
>> common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
>> diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
>> new file mode 100644
>> index 000..0420123
>> --- /dev/null
>> +++ b/hw/dma/xlnx-zynq-devcfg.c
>> @@ -0,0 +1,397 @@
>> +/*
>> + * QEMU model of the Xilinx Zynq Devcfg Interface
>> + *
>> + * (C) 2011 PetaLogix Pty Ltd
>> + * (C) 2014 Xilinx Inc.
>> + * Written by Peter Crosthwaite
>> + *
>> + * 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 "hw/dma/xlnx-zynq-devcfg.h"
>> +#include "qemu/bitops.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/dma.h"
>> +
>> +#define FREQ_HZ 9
>> +
>> +#define BTT_MAX 0x400
>> +
>> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
>> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
>> +#endif
>> +
>> +#define DB_PRINT(...) do { \
>> +if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
>> +qemu_log("%s: ", __func__); \
>> +qemu_log(__VA_ARGS__); \
>> +} \
>> +} while (0);
>
> This can be done in one qemu_log invocation as per other patches.
Fixed.
>
>> +
>> +REG32(CTRL, 0x00)
>> +FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr
>> ignored */
>> +FIELD(CTRL, PCAP_PR,27, 1) /* Forced to 0 on bad
>> unlock */
>> +FIELD(CTRL, PCAP_MODE, 26, 1)
>> +FIELD(CTRL, MULTIBOOT_EN, 24, 1)
>> +FIELD(CTRL, USER_MODE, 15, 1)
>> +FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
>> +FIELD(CTRL, PCFG_AES_EN, 9, 3)
>> +FIELD(CTRL, SEU_EN, 8, 1)
>> +FIELD(CTRL, SEC_EN, 7, 1)
>> +FIELD(CTRL, SPNIDEN, 6, 1)
>> +FIELD(CTRL, SPIDEN, 5, 1)
>> +FIELD(CTRL, NIDEN, 4, 1)
>> +FIELD(CTRL, DBGEN, 3, 1)
>> +FIELD(CTRL, DAP_EN, 0, 3)
>> +
>> +REG32(LOCK, 0x04)
>> +#define AES_FUSE_LOCK4
>> +#define AES_EN_LOCK 3
>> +#define SEU_LOCK 2
>> +#define SEC_LOCK 1
>> +#define DBG_LOCK 0
>> +
>> +/* mapping bits in R_LOCK to what they lock in R_CTRL */
>> +static const uint32_t lock_ctrl_map[] = {
>> +[AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
>> +[AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MA
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
* Paolo Bonzini (pbonz...@redhat.com) wrote: > > > On 04/03/2016 15:26, Li, Liang Z wrote: > >> > > >> > The memory usage will keep increasing due to ever growing caches, etc, so > >> > you'll be left with very little free memory fairly soon. > >> > > > I don't think so. > > > > Roman is right. For example, here I am looking at a 64 GB (physical) > machine which was booted about 30 minutes ago, and which is running > disk-heavy workloads (installing VMs). > > Since I have started writing this email (2 minutes?), the amount of free > memory has already gone down from 37 GB to 33 GB. I expect that by the > time I have finished running the workload, in two hours, it will not > have any free memory. But what about a VM sitting idle, or that just has more RAM assigned to it than is currently using. I've got a host here that's been up for 46 days and has been doing some heavy VM debugging a few days ago, but today: # free -m totalusedfree shared buff/cache available Mem: 965361146 44834 184 50555 94735 I very rarely use all it's RAM, so it's got a big chunk of free RAM, and yes it's got a big chunk of cache as well. Dave > > Paolo -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 01/38] exec: Fix memory allocation when memory path names new file
Paolo Bonzini writes:
> On 29/02/2016 19:40, Markus Armbruster wrote:
>> -if (!stat(path, &st) && S_ISDIR(st.st_mode)) {
>> +ret = stat(path, &st);
>> +if (!ret && S_ISDIR(st.st_mode)) {
>> +/* path names a directory -> create a temporary file there */
>> /* Make name safe to use with mkstemp by replacing '/' with '_'. */
>> sanitized_name = g_strdup(memory_region_name(block->mr));
>> for (c = sanitized_name; *c != '\0'; c++) {
>> @@ -1282,13 +1271,32 @@ static void *file_ram_alloc(RAMBlock *block,
>> unlink(filename);
>> }
>> g_free(filename);
>> +} else if (!ret) {
>> +/* path names an existing file -> use it */
>> +fd = open(path, O_RDWR);
>> } else {
>> +/* create a new file */
>> fd = open(path, O_RDWR | O_CREAT, 0644);
>> +unlink_on_error = true;
>> }
>
> While at it, let's avoid TOCTTOU conditions:
>
> for (;;) {
> fd = open(path, O_RDWR);
> if (fd != -1) {
> break;
> }
> if (errno == ENOENT) {
> fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
> if (fd != -1) {
> unlink_on_error = true;
> break;
> }
> } else if (errno == EISDIR) {
> ... mkstemp ...
> if (fd != -1) {
> unlink_on_error = true;
> break;
> }
> }
> if (errno != EEXIST && errno != EINTR) {
> goto error;
> }
> }
>
> and use fstatfs in gethugepagesize.
A question on gethugepagesize(). We have a couple of copies.
Here's target-ppc/kvm.c's:
static long gethugepagesize(const char *mem_path)
{
struct statfs fs;
int ret;
do {
ret = statfs(mem_path, &fs);
} while (ret != 0 && errno == EINTR);
if (ret != 0) {
fprintf(stderr, "Couldn't statfs() memory path: %s\n",
strerror(errno));
exit(1);
}
#define HUGETLBFS_MAGIC 0x958458f6
if (fs.f_type != HUGETLBFS_MAGIC) {
/* Explicit mempath, but it's ordinary pages */
return getpagesize();
}
/* It's hugepage, return the huge page size */
return fs.f_bsize;
}
I guess the use of HUGETLBFS_MAGIC is fine since kvm.c is Linux-specific.
There's another one in ivshmem_server.c, functionally identical and
wrapped in CONFIG_LINUX.
Here's exec.c's:
#define HUGETLBFS_MAGIC 0x958458f6
static long gethugepagesize(const char *path, Error **errp)
{
struct statfs fs;
int ret;
do {
ret = statfs(path, &fs);
} while (ret != 0 && errno == EINTR);
if (ret != 0) {
error_setg_errno(errp, errno, "failed to get page size of file %s",
path);
return 0;
}
return fs.f_bsize;
}
Before commit bfc2a1a, it additionally had
if (fs.f_type != HUGETLBFS_MAGIC)
fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
Note the lack of "if not hugetlbfs, use getpagesize()" logic.
Here's util/mmap-alloc.c's:
#define HUGETLBFS_MAGIC 0x958458f6
#ifdef CONFIG_LINUX
#include
#endif
size_t qemu_fd_getpagesize(int fd)
{
#ifdef CONFIG_LINUX
struct statfs fs;
int ret;
if (fd != -1) {
do {
ret = fstatfs(fd, &fs);
} while (ret != 0 && errno == EINTR);
if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) {
return fs.f_bsize;
}
}
#endif
return getpagesize();
}
Would you like me to convert the others users to this one and drop the
dupes?
Re: [Qemu-devel] [PATCH v4 14/16] register: Add GPIO API
On Mon, Feb 29, 2016 at 4:22 AM, Alex Bennée wrote:
>
> Alistair Francis writes:
>
>> From: Peter Crosthwaite
>>
>> Add GPIO functionality to the register API. This allows association
>> and automatic connection of GPIOs to bits in registers. GPIO inputs
>> will attach to handlers that automatically set read-only bits in
>> registers. GPIO outputs will be updated to reflect their field value
>> when their respective registers are written (or reset). Supports
>> active low GPIOs.
>>
>> This is particularly effective for implementing system level
>> controllers, where heterogenous collections of control signals are
>> placed is a SoC specific peripheral then propagated all over the
>> system.
>>
>> Signed-off-by: Peter Crosthwaite
>> [ EI Changes:
>> * register: Add a polarity field to GPIO connections
>> Makes it possible to directly connect active low signals
>> to generic interrupt pins.
>> ]
>> Signed-off-by: Edgar E. Iglesias
>> Signed-off-by: Alistair Francis
>> ---
>>
>> hw/core/register.c| 96
>> +++
>> include/hw/register.h | 37
>> 2 files changed, 133 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index ac866f6..1dc7ccc 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -106,6 +106,7 @@ void register_write(RegisterInfo *reg, uint64_t val,
>> uint64_t we)
>> }
>>
>> register_write_val(reg, new_val);
>> +register_refresh_gpios(reg, old_val);
>>
>> if (ac->post_write) {
>> ac->post_write(reg, new_val);
>> @@ -147,19 +148,113 @@ void register_reset(RegisterInfo *reg)
>> g_assert(reg);
>> g_assert(reg->data);
>> g_assert(reg->access);
>> +uint64_t old_val;
>> +
>> +old_val = register_read_val(reg);
>>
>> register_write_val(reg, reg->access->reset);
>> +register_refresh_gpios(reg, old_val);
>> +}
>> +
>> +void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value)
>> +{
>> +const RegisterAccessInfo *ac;
>> +const RegisterGPIOMapping *gpio;
>> +
>> +ac = reg->access;
>> +for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
>> +int i;
>> +
>> +if (gpio->input) {
>> +continue;
>> +}
>> +
>> +for (i = 0; i < gpio->num; ++i) {
>> +uint64_t gpio_value, gpio_value_old;
>> +
>> +qemu_irq gpo = qdev_get_gpio_out_named(DEVICE(reg), gpio->name,
>> i);
>> +gpio_value_old = extract64(old_value,
>> + gpio->bit_pos + i * gpio->width,
>> + gpio->width) ^ gpio->polarity;
>> +gpio_value = extract64(register_read_val(reg),
>> + gpio->bit_pos + i * gpio->width,
>> + gpio->width) ^ gpio->polarity;
>> +if (!(gpio_value_old ^ gpio_value)) {
>> +continue;
>> +}
>> +if (reg->debug && gpo) {
>> +qemu_log("refreshing gpio out %s to %" PRIx64 "\n",
>> + gpio->name, gpio_value);
>> +}
>> +qemu_set_irq(gpo, gpio_value);
>> +}
>> +}
>> +}
>> +
>> +typedef struct DeviceNamedGPIOHandlerOpaque {
>> +DeviceState *dev;
>> +const char *name;
>> +} DeviceNamedGPIOHandlerOpaque;
>> +
>> +static void register_gpio_handler(void *opaque, int n, int level)
>> +{
>> +DeviceNamedGPIOHandlerOpaque *gho = opaque;
>> +RegisterInfo *reg = REGISTER(gho->dev);
>> +
>> +const RegisterAccessInfo *ac;
>> +const RegisterGPIOMapping *gpio;
>> +
>> +ac = reg->access;
>> +for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
>> +if (gpio->input && !strcmp(gho->name, gpio->name)) {
>> +register_write_val(reg, deposit64(register_read_val(reg),
>> + gpio->bit_pos + n *
>> gpio->width,
>> + gpio->width,
>> + level ^ gpio->polarity));
>> +return;
>> +}
>> +}
>> +
>> +abort();
>> }
>>
>> void register_init(RegisterInfo *reg)
>> {
>> assert(reg);
>> +const RegisterAccessInfo *ac;
>> +const RegisterGPIOMapping *gpio;
>>
>> if (!reg->data || !reg->access) {
>> return;
>> }
>>
>> object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
>> +
>> +ac = reg->access;
>> +for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
>> +if (!gpio->num) {
>> +((RegisterGPIOMapping *)gpio)->num = 1;
>> +}
>> +if (!gpio->width) {
>> +((RegisterGPIOMapping *)gpio)->width = 1;
>> +}
>> +if (gpio->input) {
>> +DeviceNamedGPIOHandlerOpaque gho = {
>> +.name = gpio->name,
>> +.dev = DEVICE(reg),
>> +};
>> +
Re: [Qemu-devel] [PATCH 4/7] target-i386: Dump illegal opcodes with -d unimp
On Mar 4, 2016 04:41, Paolo Bonzini wrote: > Any thoughts about patch 7? > > Paolo I think I like my version, since it unifies the code testing and setting the bit. I was planning to refresh the series on Saturday; feel free to do a pull request earlier if you want. r~
Re: [Qemu-devel] [RFC PATCH v1 05/10] cpu: Abstract CPU core type
On Fri, 4 Mar 2016 16:32:53 +0530
Bharata B Rao wrote:
> On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:
> > On Fri, 4 Mar 2016 12:24:16 +0530
> > Bharata B Rao wrote:
> >
> > > Add an abstract CPU core type that could be used by machines that want
> > > to define and hotplug CPUs in core granularity.
> > >
> > > Signed-off-by: Bharata B Rao
> > > ---
> > > hw/cpu/Makefile.objs | 1 +
> > > hw/cpu/core.c | 44
> > > include/hw/cpu/core.h | 30 ++
> > > 3 files changed, 75 insertions(+)
> > > create mode 100644 hw/cpu/core.c
> > > create mode 100644 include/hw/cpu/core.h
> > >
> > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > index 0954a18..942a4bb 100644
> > > --- a/hw/cpu/Makefile.objs
> > > +++ b/hw/cpu/Makefile.objs
> > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > +obj-y += core.o
> > >
> > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > new file mode 100644
> > > index 000..d8caf37
> > > --- /dev/null
> > > +++ b/hw/cpu/core.c
> > > @@ -0,0 +1,44 @@
> > > +/*
> > > + * CPU core abstract device
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "hw/cpu/core.h"
> > > +
> > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > +{
> > > +CPUCore *core = CPU_CORE(obj);
> > > +
> > > +return g_strdup(core->slot);
> > > +}
> > > +
> > > +static void core_prop_set_slot(Object *obj, const char *val, Error
> > > **errp)
> > > +{
> > > +CPUCore *core = CPU_CORE(obj);
> > > +
> > > +core->slot = g_strdup(val);
> > > +}
> > > +
> > > +static void cpu_core_instance_init(Object *obj)
> > > +{
> > > +object_property_add_str(obj, "slot", core_prop_get_slot,
> > > core_prop_set_slot,
> > > +NULL);
> > > +}
> > > +
> > > +static const TypeInfo cpu_core_type_info = {
> > > +.name = TYPE_CPU_CORE,
> > > +.parent = TYPE_DEVICE,
> > > +.abstract = true,
> > > +.instance_size = sizeof(CPUCore),
> > > +.instance_init = cpu_core_instance_init,
> > > +};
> > > +
> > > +static void cpu_core_register_types(void)
> > > +{
> > > +type_register_static(&cpu_core_type_info);
> > > +}
> > > +
> > > +type_init(cpu_core_register_types)
> > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > new file mode 100644
> > > index 000..2daa724
> > > --- /dev/null
> > > +++ b/include/hw/cpu/core.h
> > > @@ -0,0 +1,30 @@
> > > +/*
> > > + * CPU core abstract device
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#ifndef HW_CPU_CORE_H
> > > +#define HW_CPU_CORE_H
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "hw/qdev.h"
> > > +
> > > +#define TYPE_CPU_CORE "cpu-core"
> > > +
> > > +#define CPU_CORE(obj) \
> > > +OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > +
> > > +typedef struct CPUCore {
> > > +/*< private >*/
> > > +DeviceState parent_obj;
> > > +
> > > +/*< public >*/
> > > +char *slot;
> > > +} CPUCore;
> > > +
> > > +#define CPU_CORE_SLOT_PROP "slot"
> > as it's generic property I'd rename to 'core' so it would fit all users
>
> Ok. Also note that this is a string property which is associated with the
> link name (string) that we created from machine object to this core. I think
> it would be ideal if this becomes an interger property in which case it
> becomes easier to feed the core location into your CPUSlotProperties.core.
agreed, it should be core number.
> > on top of that I'd add numeric 'threads' property to base class so
> > all derived cores would inherit it.
> >
> > Then as easy integration with -smp threads=x, a machine could push
> > a global variable 'cpu-core.threads=[smp_threads]' which would
> > make every created cpu-core object to have threads set
> > at instance_init() time (device_init).
> >
> > That way user won't have to specify 'threads=y' for every
> > device_add spapr-core,core=x
> > as it will be taken from global property 'cpu-core.threads'
> > but if user wishes he/she still could override global by explicitly
> > providing thread property at device_add time:
> > device_add spapr-core,core=x,threads=y
> >
> > wrt this series it would mean, instead of creating threads in property
> > setter, delaying threads creation to core.realize() time,
> > but since realize is allowed to fail it should be fine do so.
>
> Ok that would suit us as there are two properties on which thread creation
> is dependent
Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation
On Fri, 4 Mar 2016 12:50:05 +0100
David Hildenbrand wrote:
> > On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> > >
> > > > > cpu_exec_init(cs, &err);
> > > > > if (err != NULL) {
> > > > > error_propagate(errp, err);
> > > > > return;
> > > > > }
> > > > > +scc->next_cpu_id = cs->cpu_index + 1;
> > > >
> > > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of
> > > > arch_id
> > > > for you. If it is just going to be monotonically increasing like
> > > > cs->cpu_index,
> > > > couldn't you just use cs->cpu_index instead of introducing additional
> > > > IDs ?
> > > >
> > > > Note that cpu_exec_init(cs, &err) returns with the next available
> > > > cpu_index
> > > > which can be compared against max_cpus directly.
> > > >
> > > > Regards,
> > > > Bharata.
> > >
> > > I don't think that we should mix the id setting and cpu_index for now.
> > >
> > > We can't simply set cpu_index before the device is realized. That logic
> > > belongs to cpu_exec_init().
> >
> > Yes, I see that, but apart from the following obvious uses of the id
> > property from realizefn, are there other uses ?
> >
> > 1 Checking against max_cpus
> > cpu_index can be used for this.
> >
> > 2 Checking if cpu with such an id exists
> > cpu_exec_init() would never return with an in-use index. Hence cpu_index
> > can be used here too given that you don't define ->get_arch_id()
> >
> > 3 Checking if the id is the next expected one
> > cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
> > always have the next available id to be used in cpu_index.
> >
> > Did I miss any other use other than these which makes it necessary to have
> > an explicit id property here ?
>
> After all the discussions about
> -device-add s390-cpu,id=XX
>
> As substitute/addition in the future for hotplug it is the straightforward
> approach to allow setting the id as property. Nobody knows what crazy new
> hotplug method we will come up with. But doing it the device way with
> properties
> cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add
> id=XX).
with device_add 'id' is not a vcpu concept but and arbitrary user supplied
string
property owned by Device. But since s390 matches current x86 thread based model
it could be migrated to device_add the same way, for example:
device_add s390-cpu,thread=XX
>
> So I'd like to avoid reworking everything again, to realize later that we
> want it as a property and rewriting it once again.
for s390, the thing about not rewriting everything once again could be
replacing places where cpu_index is used with CpuClass.arch_id().
arch_id() defaults to cpu_index but you can later override it with
your own id (whatever s390 uses for identifying cpus on baremetal)
so switching to device_add won't break anything.
>
> David
>
Re: [Qemu-devel] [PATCH v4 RFC 00/17] qcow2: persistent dirty bitmaps
On 02/17/2016 10:28 AM, Vladimir Sementsov-Ogievskiy wrote: > This series add persistent dirty bitmaps feature to qcow2. > Specification is in docs/spec/qcow2.txt (not merged yet, see > [PATCH v10] spec: add qcow2 bitmaps extension specification) > > This series are based on Fam's > [PATCH v2 00/13] Dirty bitmap changes for migration/persistence work > (meta bitmaps not used, and only hbitmap_deserialize_finish from > serialization) > > This also needs some preparation patches, most of them are in my > bitmap-migration series. I've not listed them here to keep things > simpler, this is RFC any way. > > v4: > > Previous version was posted more than five months ago, so I will not > carefully list all changes. > > What should be noted: > - some changes are made to sutisfy last version of specification >- removed staff, related to possibility of saving bitmaps for one > disk in the other qcow2. > - to make bitmap store/load zero-copy, I've moved load/store code to >HBitmap - this is new patch 01. >so, bdrv_dirty_bitmap_serialize_part and friends are not needed, >only hbitmap_deserialize_finish, to repair bitmap consistency after >loading its last level. > - two patches added about AUTO and EXTRA_DATA_COMPATIBLE flags > - some fixes made after John's comments on v3 > > Vladimir Sementsov-Ogievskiy (17): > hbitmap: load/store > qcow2: Bitmaps extension: structs and consts > qcow2-dirty-bitmap: read dirty bitmap directory > qcow2-dirty-bitmap: add qcow2_bitmap_load() > qcow2-dirty-bitmap: add qcow2_bitmap_store() > qcow2: add dirty bitmaps extension > qcow2-dirty-bitmap: add qcow2_bitmap_load_check() > block: store persistent dirty bitmaps > block: add bdrv_load_dirty_bitmap() > qcow2-dirty-bitmap: add autoclear bit > qemu: command line option for dirty bitmaps > qcow2-dirty-bitmap: add IN_USE flag > qcow2-dirty-bitmaps: disallow stroing bitmap to other bs > iotests: add VM.test_launcn() > iotests: test internal persistent dirty bitmap > qcow2-dirty-bitmap: add AUTO flag > qcow2-dirty-bitmap: add EXTRA_DATA_COMPATIBLE flag > > block.c | 3 + > block/Makefile.objs | 2 +- > block/dirty-bitmap.c | 101 + > block/qcow2-dirty-bitmap.c| 839 > ++ > block/qcow2.c | 105 +- > block/qcow2.h | 59 +++ > blockdev.c| 36 ++ > include/block/block_int.h | 9 + > include/block/dirty-bitmap.h | 21 ++ > include/qemu/hbitmap.h| 12 + > include/sysemu/blockdev.h | 1 + > include/sysemu/sysemu.h | 1 + > qemu-options.hx | 35 ++ > tests/qemu-iotests/160| 112 ++ > tests/qemu-iotests/160.out| 21 ++ > tests/qemu-iotests/group | 1 + > tests/qemu-iotests/iotests.py | 25 ++ > util/hbitmap.c| 182 + > vl.c | 78 > 19 files changed, 1640 insertions(+), 3 deletions(-) > create mode 100644 block/qcow2-dirty-bitmap.c > create mode 100755 tests/qemu-iotests/160 > create mode 100644 tests/qemu-iotests/160.out > In your prerequisite patches, "iotests-add-default-node-name" breaks qemu iotests 055 and 118. Didn't look hard enough to find out why, yet.
Re: [Qemu-devel] [PATCH v15 8/9] Implement new driver for block replication
On Fri, Feb 05, 2016 at 12:18:07PM +0800, Changlong Xie wrote:
> +static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> +{
> +Error *local_err = NULL;
> +int ret;
> +
> +if (!s->secondary_disk->bs->job) {
> +error_setg(errp, "Backup job was cancelled unexpectedly");
> +return;
> +}
Do you have test cases for the replication driver?
This error code path seems like something that should be exercised to
make sure it works.
Basic start/checkpoint/stop and checks that the active/secondary/hidden
disks contain the expected data are also important.
Probably the easiest way to do this would be a file called
tests/test-replication.c that calls start/checkpoint/stop and read/write
functions directly instead of running a guest.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PULL 00/12] QAPI patches for 2016-03-04
On 4 March 2016 at 16:45, Markus Armbruster wrote: > The following changes since commit 3c0f12df65da872d5fbccae469f2cb21ed1c03b7: > > Merge remote-tracking branch > 'remotes/pmaydell/tags/pull-target-arm-20160304' into staging (2016-03-04 > 11:46:32 +) > > are available in the git repository at: > > git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2016-03-04 > > for you to fetch changes up to 984ebcbab466a05beccc5bfcd7eff4ce7efc48d5: > > qapi: Drop useless 'data' member of unions (2016-03-04 17:27:50 +0100) > > > QAPI patches for 2016-03-04 > > Hi; no build issues, but most of the patches in this pull seem to be missing your sign-off as maintainer -- could you respin, please? thanks -- PMM
Re: [Qemu-devel] [PATCH v15 8/9] Implement new driver for block replication
On Fri, Feb 05, 2016 at 12:18:07PM +0800, Changlong Xie wrote:
> +static void replication_start(ReplicationState *rs, ReplicationMode mode,
> + Error **errp)
> +{
> +BlockDriverState *bs = rs->opaque;
> +BDRVReplicationState *s = bs->opaque;
> +int64_t active_length, hidden_length, disk_length;
> +AioContext *aio_context;
> +Error *local_err = NULL;
> +
> +if (s->replication_state != BLOCK_REPLICATION_NONE) {
Dereferencing bs and s is not thread-safe since the AioContext isn't
acquired here. Unless you have other locking rules, the default is that
everything in BlockDriverState and bs->opaque must be accessed with the
AioContext acquired.
Please apply this comment to the rest of the patch series, too.
Functions that are not part of BlockDriver are probably called outside
the AioContext and must acquire it before they are allowed to access
anything else in bs.
> +s->top_bs = get_top_bs(bs);
> +if (!s->top_bs) {
> +error_setg(errp, "Cannot get the top block driver state to do"
> + " internal backup");
> +return;
> +}
Traversing the BDS graph using the parent pointer is not safe - you
cannot stash the parent pointer because it changes if a parent node is
inserted/removed.
I suggest passing the drive name as an argument so that bdrv_lookup_bs()
can be used when installing the op blocker. Then the BdrvChild.parent
pointer patch and get_top_bs() can be deleted.
Jeff Cody is currently working on a new op blocker system. Hopefully
this system will allow you to install blockers on bs instead of on the
drive's top BDS. But let's not worry about that for now and just use
the drive name as an argument.
> +/*
> + * Must protect backup target if backup job was stopped/cancelled
> + * unexpectedly
> + */
> +bdrv_ref(s->hidden_disk->bs);
Where is the matching bdrv_unref() call?
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v9 7/7] s390x/cpu: Allow hotplug of CPUs
Implement cpu hotplug routine and add the machine hook.
Signed-off-by: Matthew Rosato
Reviewed-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 10 ++
target-s390x/cpu.c | 7 +++
2 files changed, 17 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7fc1879..a84375b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -186,6 +186,15 @@ static HotplugHandler
*s390_get_hotplug_handler(MachineState *machine,
return NULL;
}
+static void s390_hot_add_cpu(const int64_t id, Error **errp)
+{
+MachineState *machine = MACHINE(qdev_get_machine());
+Error *err = NULL;
+
+s390x_new_cpu(machine->cpu_model, id, &err);
+error_propagate(errp, err);
+}
+
static void ccw_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -194,6 +203,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void
*data)
mc->init = ccw_init;
mc->reset = s390_machine_reset;
+mc->hot_add_cpu = s390_hot_add_cpu;
mc->block_default_type = IF_VIRTIO;
mc->no_cdrom = 1;
mc->no_floppy = 1;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 3ae38fa..1cbf703 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -34,6 +34,7 @@
#ifndef CONFIG_USER_ONLY
#include "sysemu/arch_init.h"
#include "sysemu/sysemu.h"
+#include "hw/s390x/sclp.h"
#endif
#define CR0_RESET 0xE0UL
@@ -240,6 +241,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
scc->parent_realize(dev, &err);
+#if !defined(CONFIG_USER_ONLY)
+if (dev->hotplugged) {
+raise_irq_cpu_hotplug();
+}
+#endif
+
out:
error_propagate(errp, err);
}
--
1.9.1
[Qemu-devel] [PATCH v9 5/7] s390x/cpu: Add CPU property links
Link each CPUState as property machine/cpu[n] during initialization.
Add a hotplug handler to s390-virtio-ccw machine and set the
state during plug.
Signed-off-by: Matthew Rosato
Reviewed-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 34 ++
hw/s390x/s390-virtio.c | 17 -
2 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b05ed8b..7fc1879 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -156,10 +156,41 @@ static void ccw_init(MachineState *machine)
gtod_save, gtod_load, kvm_state);
}
+static void s390_cpu_plug(HotplugHandler *hotplug_dev,
+DeviceState *dev, Error **errp)
+{
+gchar *name;
+S390CPU *cpu = S390_CPU(dev);
+CPUState *cs = CPU(dev);
+
+name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
+ errp);
+g_free(name);
+}
+
+static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+s390_cpu_plug(hotplug_dev, dev, errp);
+}
+}
+
+static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
+DeviceState *dev)
+{
+if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+return HOTPLUG_HANDLER(machine);
+}
+return NULL;
+}
+
static void ccw_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
NMIClass *nc = NMI_CLASS(oc);
+HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
mc->init = ccw_init;
mc->reset = s390_machine_reset;
@@ -171,6 +202,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void
*data)
mc->no_sdcard = 1;
mc->use_sclp = 1;
mc->max_cpus = 255;
+mc->get_hotplug_handler = s390_get_hotplug_handler;
+hc->plug = s390_machine_device_plug;
nc->nmi_monitor_handler = s390_nmi;
}
@@ -232,6 +265,7 @@ static const TypeInfo ccw_machine_info = {
.class_init= ccw_machine_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_NMI },
+{ TYPE_HOTPLUG_HANDLER},
{ }
},
};
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 57c3c88..f00d6b4 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -97,6 +97,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
void s390_init_cpus(MachineState *machine)
{
int i;
+gchar *name;
if (machine->cpu_model == NULL) {
machine->cpu_model = "host";
@@ -104,12 +105,18 @@ void s390_init_cpus(MachineState *machine)
cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
-for (i = 0; i < smp_cpus; i++) {
-S390CPU *cpu;
-
-cpu = cpu_s390x_init(machine->cpu_model);
+for (i = 0; i < max_cpus; i++) {
+name = g_strdup_printf("cpu[%i]", i);
+object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
+ (Object **) &cpu_states[i],
+ object_property_allow_set_link,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ &error_abort);
+g_free(name);
+}
-cpu_states[i] = cpu;
+for (i = 0; i < smp_cpus; i++) {
+cpu_s390x_init(machine->cpu_model);
}
}
--
1.9.1
[Qemu-devel] [PATCH v9 6/7] s390x/cpu: Add error handling to cpu creation
Check for and propogate errors during s390 cpu creation.
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-virtio.c | 2 +-
target-s390x/cpu-qom.h | 1 +
target-s390x/cpu.c | 73 +++---
target-s390x/cpu.h | 2 ++
target-s390x/helper.c | 41 ++--
5 files changed, 112 insertions(+), 7 deletions(-)
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index f00d6b4..4ea9040 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
}
for (i = 0; i < smp_cpus; i++) {
-cpu_s390x_init(machine->cpu_model);
+s390x_new_cpu(machine->cpu_model, i, &error_fatal);
}
}
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 56d82f2..1c90933 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -68,6 +68,7 @@ typedef struct S390CPU {
/*< public >*/
CPUS390XState env;
+int64_t id;
/* needed for live migration */
void *irqstate;
uint32_t irqstate_saved_size;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 76c8eaf..3ae38fa 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -30,8 +30,10 @@
#include "qemu/error-report.h"
#include "hw/hw.h"
#include "trace.h"
+#include "qapi/visitor.h"
#ifndef CONFIG_USER_ONLY
#include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
#endif
#define CR0_RESET 0xE0UL
@@ -199,16 +201,35 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
CPUS390XState *env = &cpu->env;
Error *err = NULL;
+#if !defined(CONFIG_USER_ONLY)
+if (cpu->id >= max_cpus) {
+error_setg(&err, "Unable to add CPU: %" PRIi64
+ ", max allowed: %d", cpu->id, max_cpus - 1);
+goto out;
+}
+#endif
+if (cpu_exists(cpu->id)) {
+error_setg(&err, "Unable to add CPU: %" PRIi64
+ ", it already exists", cpu->id);
+goto out;
+}
+if (cpu->id != scc->next_cpu_id) {
+error_setg(&err, "Unable to add CPU: %" PRIi64
+ ", The next available id is %" PRIi64, cpu->id,
+ scc->next_cpu_id);
+goto out;
+}
+
cpu_exec_init(cs, &err);
if (err != NULL) {
-error_propagate(errp, err);
-return;
+goto out;
}
+scc->next_cpu_id++;
#if !defined(CONFIG_USER_ONLY)
qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
#endif
-env->cpu_num = scc->next_cpu_id++;
+env->cpu_num = cpu->id;
s390_cpu_gdb_init(cs);
qemu_init_vcpu(cs);
#if !defined(CONFIG_USER_ONLY)
@@ -217,7 +238,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
cpu_reset(cs);
#endif
-scc->parent_realize(dev, errp);
+scc->parent_realize(dev, &err);
+
+out:
+error_propagate(errp, err);
+}
+
+static void s390x_cpu_get_id(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+S390CPU *cpu = S390_CPU(obj);
+int64_t value = cpu->id;
+
+visit_type_int(v, name, &value, errp);
+}
+
+static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+S390CPU *cpu = S390_CPU(obj);
+DeviceState *dev = DEVICE(obj);
+const int64_t min = 0;
+const int64_t max = UINT32_MAX;
+Error *err = NULL;
+int64_t value;
+
+if (dev->realized) {
+error_setg(errp, "Attempt to set property '%s' on '%s' after "
+ "it was realized", name, object_get_typename(obj));
+return;
+}
+
+visit_type_int(v, name, &value, &err);
+if (err) {
+error_propagate(errp, err);
+return;
+}
+if (value < min || value > max) {
+error_setg(errp, "Property %s.%s doesn't take value %" PRId64
+ " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
+ object_get_typename(obj), name, value, min, max);
+return;
+}
+cpu->id = value;
}
static void s390_cpu_initfn(Object *obj)
@@ -233,6 +296,8 @@ static void s390_cpu_initfn(Object *obj)
cs->env_ptr = env;
cs->halted = 1;
cs->exception_index = EXCP_HLT;
+object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
+s390x_cpu_set_id, NULL, NULL, NULL);
#if !defined(CONFIG_USER_ONLY)
qemu_get_timedate(&tm, 0);
env->tod_offset = TOD_UNIX_EPOCH +
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 49c8415..6d97c08 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t
code, uint32_t ilen);
#endif
S390CPU *cpu_s390x_init(const char *cpu_model);
+S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
+S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
void s390x_translate_init(void);
int cpu_s390x_exec(CPUState *
[Qemu-devel] [PATCH v9 2/7] s390x/cpu: Set initial CPU state in common routine
Both initial and hotplugged CPUs need to set the same initial
state.
Signed-off-by: Matthew Rosato
Reviewed-by: David Hildenbrand
---
hw/s390x/s390-virtio.c | 4
target-s390x/cpu.c | 2 ++
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index d40d0dc..c501a48 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -105,14 +105,10 @@ void s390_init_cpus(MachineState *machine)
for (i = 0; i < smp_cpus; i++) {
S390CPU *cpu;
-CPUState *cs;
cpu = cpu_s390x_init(machine->cpu_model);
-cs = CPU(cpu);
ipi_states[i] = cpu;
-cs->halted = 1;
-cs->exception_index = EXCP_HLT;
}
}
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 73a910d..603c2a1 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -219,6 +219,8 @@ static void s390_cpu_initfn(Object *obj)
#endif
cs->env_ptr = env;
+cs->halted = 1;
+cs->exception_index = EXCP_HLT;
cpu_exec_init(cs, &error_abort);
#if !defined(CONFIG_USER_ONLY)
qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
--
1.9.1
[Qemu-devel] [PATCH v9 3/7] s390x/cpu: Get rid of side effects when creating a vcpu
In preparation for hotplug, defer some CPU initialization
until the device is actually being realized, including
cpu_exec_init.
Signed-off-by: Matthew Rosato
Reviewed-by: David Hildenbrand
---
target-s390x/cpu-qom.h | 2 ++
target-s390x/cpu.c | 20 +++-
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 029a44a..56d82f2 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -47,6 +47,8 @@ typedef struct S390CPUClass {
CPUClass parent_class;
/*< public >*/
+int64_t next_cpu_id;
+
DeviceRealize parent_realize;
void (*parent_reset)(CPUState *cpu);
void (*load_normal)(CPUState *cpu);
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 603c2a1..76c8eaf 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -195,7 +195,20 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
{
CPUState *cs = CPU(dev);
S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+S390CPU *cpu = S390_CPU(dev);
+CPUS390XState *env = &cpu->env;
+Error *err = NULL;
+
+cpu_exec_init(cs, &err);
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+#if !defined(CONFIG_USER_ONLY)
+qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
+#endif
+env->cpu_num = scc->next_cpu_id++;
s390_cpu_gdb_init(cs);
qemu_init_vcpu(cs);
#if !defined(CONFIG_USER_ONLY)
@@ -213,7 +226,6 @@ static void s390_cpu_initfn(Object *obj)
S390CPU *cpu = S390_CPU(obj);
CPUS390XState *env = &cpu->env;
static bool inited;
-static int cpu_num = 0;
#if !defined(CONFIG_USER_ONLY)
struct tm tm;
#endif
@@ -221,9 +233,7 @@ static void s390_cpu_initfn(Object *obj)
cs->env_ptr = env;
cs->halted = 1;
cs->exception_index = EXCP_HLT;
-cpu_exec_init(cs, &error_abort);
#if !defined(CONFIG_USER_ONLY)
-qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
qemu_get_timedate(&tm, 0);
env->tod_offset = TOD_UNIX_EPOCH +
(time2tod(mktimegm(&tm)) * 10ULL);
@@ -232,7 +242,6 @@ static void s390_cpu_initfn(Object *obj)
env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
#endif
-env->cpu_num = cpu_num++;
if (tcg_enabled() && !inited) {
inited = true;
@@ -339,6 +348,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
CPUClass *cc = CPU_CLASS(scc);
DeviceClass *dc = DEVICE_CLASS(oc);
+scc->next_cpu_id = 0;
scc->parent_realize = dc->realize;
dc->realize = s390_cpu_realizefn;
@@ -371,7 +381,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
cc->gdb_arch_name = s390_gdb_arch_name;
/*
- * Reason: s390_cpu_initfn() calls cpu_exec_init(), which saves
+ * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
* the object in cpus -> dangling pointer after final
* object_unref().
*/
--
1.9.1
[Qemu-devel] [PATCH v9 4/7] s390x/cpu: Tolerate max_cpus
Once hotplug is enabled, interrupts may come in for CPUs
with an address > smp_cpus. Allocate for this and allow
search routines to look beyond smp_cpus.
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-virtio.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index c501a48..57c3c88 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -58,15 +58,16 @@
#define S390_TOD_CLOCK_VALUE_MISSING0x00
#define S390_TOD_CLOCK_VALUE_PRESENT0x01
-static S390CPU **ipi_states;
+static S390CPU **cpu_states;
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
{
-if (cpu_addr >= smp_cpus) {
+if (cpu_addr >= max_cpus) {
return NULL;
}
-return ipi_states[cpu_addr];
+/* Fast lookup via CPU ID */
+return cpu_states[cpu_addr];
}
void s390_init_ipl_dev(const char *kernel_filename,
@@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
machine->cpu_model = "host";
}
-ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
+cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
for (i = 0; i < smp_cpus; i++) {
S390CPU *cpu;
cpu = cpu_s390x_init(machine->cpu_model);
-ipi_states[i] = cpu;
+cpu_states[i] = cpu;
}
}
--
1.9.1
[Qemu-devel] [PATCH v9 1/7] s390x/cpu: Cleanup init in preparation for hotplug
Ensure a valid cpu_model is set upfront by setting the
default value directly into the MachineState when none is
specified. This is needed to ensure hotplugged CPUs share
the same cpu_model.
Signed-off-by: Matthew Rosato
Reviewed-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 2 +-
hw/s390x/s390-virtio.c | 8
hw/s390x/s390-virtio.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 89f5d0d..b05ed8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
virtio_ccw_register_hcalls();
/* init CPUs */
-s390_init_cpus(machine->cpu_model);
+s390_init_cpus(machine);
if (kvm_enabled()) {
kvm_s390_enable_css_support(s390_cpu_addr2state(0));
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 8e533ae..d40d0dc 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -93,12 +93,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
qdev_init_nofail(dev);
}
-void s390_init_cpus(const char *cpu_model)
+void s390_init_cpus(MachineState *machine)
{
int i;
-if (cpu_model == NULL) {
-cpu_model = "host";
+if (machine->cpu_model == NULL) {
+machine->cpu_model = "host";
}
ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
@@ -107,7 +107,7 @@ void s390_init_cpus(const char *cpu_model)
S390CPU *cpu;
CPUState *cs;
-cpu = cpu_s390x_init(cpu_model);
+cpu = cpu_s390x_init(machine->cpu_model);
cs = CPU(cpu);
ipi_states[i] = cpu;
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index eebce8e..ffd014c 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -19,7 +19,7 @@
typedef int (*s390_virtio_fn)(const uint64_t *args);
void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
-void s390_init_cpus(const char *cpu_model);
+void s390_init_cpus(MachineState *machine);
void s390_init_ipl_dev(const char *kernel_filename,
const char *kernel_cmdline,
const char *initrd_filename,
--
1.9.1
[Qemu-devel] [PATCH v9 0/7] Allow hotplug of s390 CPUs
Changes from v8->v9: * Patch 4: fix bug introduced during split, only init smp_cpus (David) * Patch 6: * nit change to error_propagate handling in realizefn (David) * scc->next_cpu_id++ (David) * s/s390_cpu_set_id/s390x_cpu_set_id/ (David) * s/s390_new_cpu/s390x_new_cpu/ (David) * Put id getter back in (David) * Move linux-user next_cpu_id into s390_cpux_init (David) * Patch 7: nit change to error_propagate handling (David) ** As discussed in the KVM call, we will go ahead with cpu_add for s390x to get cpu hotplug functionality in s390x now, until architectures that require a more robust hotplug interface settle on a design. To configure a guest with 2 CPUs online at boot and 4 maximum: qemu -smp 2,maxcpus=4 Or, when using libvirt: ... 4 ... To subsequently hotplug a CPU: Issue 'cpu-add ' from qemu monitor, or use virsh setvcpus --count , where is the total number of desired guest CPUs. At this point, the guest must bring the CPU online for use -- This can be achieved via "echo 1 > /sys/devices/system/cpu/cpuX/online" or via a management tool like cpuplugd. This patch set is based on work previously done by Jason Herne. Matthew Rosato (7): s390x/cpu: Cleanup init in preparation for hotplug s390x/cpu: Set initial CPU state in common routine s390x/cpu: Get rid of side effects when creating a vcpu s390x/cpu: Tolerate max_cpus s390x/cpu: Add CPU property links s390x/cpu: Add error handling to cpu creation s390x/cpu: Allow hotplug of CPUs hw/s390x/s390-virtio-ccw.c | 46 +- hw/s390x/s390-virtio.c | 36 + hw/s390x/s390-virtio.h | 2 +- target-s390x/cpu-qom.h | 3 ++ target-s390x/cpu.c | 96 +++--- target-s390x/cpu.h | 2 + target-s390x/helper.c | 41 +++- 7 files changed, 200 insertions(+), 26 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH v2][Outreachy Round 12]
Replaced g_malloc() with g_new() as it would detect multiplication overflow if
nb_fields ever oversize.
There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.
This replacement was suggested as part of the bite-sized tasks.
Signed-off-by: Sarah Khan
---
Changes in v2 :Make commit message clearer
Replace g_malloc() with g_new()
---
thunk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/thunk.c b/thunk.c
index bddabae..6237702 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
argtype *types)
for(i = 0;i < 2; i++) {
offset = 0;
max_align = 1;
-se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
+se->field_offsets[i] = g_new(int,nb_fields);
type_ptr = se->field_types;
for(j = 0;j < nb_fields; j++) {
size = thunk_type_size(type_ptr, i);
--
1.9.1
Re: [Qemu-devel] [PATCH v2 0/3] add missing virtio aliases
Sascha Silbe writes: > Split out from the series fixing qemu-iotest 140 on s390x [1]. > > This series makes crafting qemu command lines that work across > multiple (virtio-supporting) platforms at lot easier. > > Changes since the original series: > - included patch to improve the error message when the target device > class of an alias doesn't exist > - added comment about keeping alias table sorted > - covering all non-abstract virtio device classes rather than just > those already implemented on s390x > > Sascha Silbe (3): > qdev-monitor: improve error message when alias device is unavailable > qdev-monitor: sort alias table by typename > qdev-monitor: add missing aliases for virtio device classes > > qdev-monitor.c | 42 ++ > 1 file changed, 34 insertions(+), 8 deletions(-) > > [1] mid:1455023713-104799-1-git-send-email-si...@linux.vnet.ibm.com > "[PATCH 0/3] qemu-iotests: fix 140 on s390x" by Sascha Silbe > on 2016-02-09 Applied to my monitor-next branch, will hold the queue for a couple of days to give others a chance to review, thanks!
[Qemu-devel] [PULL 05/12] qapi-visit: Expose visit_type_FOO_members()
From: Eric Blake
Dan Berrange reported a case where he needs to work with a
QCryptoBlockOptions union type using the OptsVisitor, but only
visit one of the branches of that type (the discriminator is not
visited directly, but learned externally). When things were
boxed, it was easy: just visit the variant directly, which took
care of both allocating the variant and visiting its members, then
store that pointer in the union type. But now that things are
unboxed, we need a way to visit the members without allocation,
done by exposing visit_type_FOO_members() to the user.
Before the patch, we had quite a bit of code associated with
object_members_seen to make sure that a declaration of the helper
was in scope before any use of the function. But now that the
helper is public and declared in the header, the .c file no
longer needs to worry about topological sorting (the helper is
always in scope), which leads to some nice cleanups.
Signed-off-by: Eric Blake
Message-Id: <1457021813-10704-4-git-send-email-ebl...@redhat.com>
---
scripts/qapi-visit.py | 33 +++--
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1e52f76..a712e9a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
from qapi import *
import re
-# visit_type_FOO_members() is always emitted; track if a forward declaration
-# or implementation has already been output.
-object_members_seen = set()
-
def gen_visit_decl(name, scalar=False):
c_type = c_name(name) + ' *'
@@ -30,37 +26,23 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
%(c_type)sobj, Error **
c_name=c_name(name), c_type=c_type)
-def gen_visit_members_decl(typ):
-if typ.name in object_members_seen:
-return ''
-object_members_seen.add(typ.name)
+def gen_visit_members_decl(name):
return mcgen('''
-static void visit_type_%(c_type)s_members(Visitor *v, %(c_type)s *obj, Error
**errp);
+void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
''',
- c_type=typ.c_name())
+ c_name=c_name(name))
def gen_visit_object_members(name, base, members, variants):
-ret = ''
+ret = mcgen('''
-if base:
-ret += gen_visit_members_decl(base)
-if variants:
-for var in variants.variants:
-# Ugly special case for simple union TODO get rid of it
-if not var.simple_union_type():
-ret += gen_visit_members_decl(var.type)
-
-object_members_seen.add(name)
-ret += mcgen('''
-
-static void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error
**errp)
+void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
{
Error *err = NULL;
''',
- c_name=c_name(name))
+c_name=c_name(name))
if base:
ret += mcgen('''
@@ -173,8 +155,6 @@ def gen_visit_alternate(name, variants):
for var in variants.variants:
if var.type.alternate_qtype() == 'QTYPE_QINT':
promote_int = 'false'
-if isinstance(var.type, QAPISchemaObjectType):
-ret += gen_visit_members_decl(var.type)
ret += mcgen('''
@@ -316,6 +296,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.defn += defn
def visit_object_type(self, name, info, base, members, variants):
+self.decl += gen_visit_members_decl(name)
self.decl += gen_visit_decl(name)
self.defn += gen_visit_object(name, base, members, variants)
--
2.4.3
[Qemu-devel] [PULL 11/12] chardev: Drop useless ChardevDummy type
From: Eric Blake
Commit d0d7708b made ChardevDummy be an empty wrapper type around
ChardevCommon. But there is no technical reason for this indirection,
so simplify the code by directly using the base type.
Also change the fallback assignment to assign u.null rather than
u.data, since a future patch will remove the data member of the C
struct generated for QAPI unions.
Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrange
Message-Id: <1457106160-23614-1-git-send-email-ebl...@redhat.com>
---
backends/baum.c| 2 +-
backends/msmouse.c | 2 +-
qapi-schema.json | 15 ++-
qemu-char.c| 8
4 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 374562a..c11320e 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -567,7 +567,7 @@ static CharDriverState *chr_baum_init(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.braille);
+ChardevCommon *common = backend->u.braille;
BaumDriverState *baum;
CharDriverState *chr;
brlapi_handle_t *handle;
diff --git a/backends/msmouse.c b/backends/msmouse.c
index 9a82efd..5e1833c 100644
--- a/backends/msmouse.c
+++ b/backends/msmouse.c
@@ -68,7 +68,7 @@ static CharDriverState *qemu_chr_open_msmouse(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.msmouse);
+ChardevCommon *common = backend->u.msmouse;
CharDriverState *chr;
chr = qemu_chr_alloc(common, errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index 42fd61b..362c9d8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3323,23 +3323,20 @@
#
# Since: 1.4 (testdev since 2.2)
##
-{ 'struct': 'ChardevDummy', 'data': { },
- 'base': 'ChardevCommon' }
-
{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
'serial' : 'ChardevHostdev',
'parallel': 'ChardevHostdev',
'pipe' : 'ChardevHostdev',
'socket' : 'ChardevSocket',
'udp': 'ChardevUdp',
- 'pty': 'ChardevDummy',
- 'null' : 'ChardevDummy',
+ 'pty': 'ChardevCommon',
+ 'null' : 'ChardevCommon',
'mux': 'ChardevMux',
- 'msmouse': 'ChardevDummy',
- 'braille': 'ChardevDummy',
- 'testdev': 'ChardevDummy',
+ 'msmouse': 'ChardevCommon',
+ 'braille': 'ChardevCommon',
+ 'testdev': 'ChardevCommon',
'stdio' : 'ChardevStdio',
- 'console': 'ChardevDummy',
+ 'console': 'ChardevCommon',
'spicevmc' : 'ChardevSpiceChannel',
'spiceport' : 'ChardevSpicePort',
'vc' : 'ChardevVC',
diff --git a/qemu-char.c b/qemu-char.c
index af31102..e0147f3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -420,7 +420,7 @@ static CharDriverState *qemu_chr_open_null(const char *id,
Error **errp)
{
CharDriverState *chr;
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.null);
+ChardevCommon *common = backend->u.null;
chr = qemu_chr_alloc(common, errp);
if (!chr) {
@@ -1366,7 +1366,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
PtyCharDriver *s;
int master_fd, slave_fd;
char pty_name[PATH_MAX];
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.pty);
+ChardevCommon *common = backend->u.pty;
master_fd = qemu_openpty_raw(&slave_fd, pty_name);
if (master_fd < 0) {
@@ -2183,7 +2183,7 @@ static CharDriverState *qemu_chr_open_win_con(const char
*id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console);
+ChardevCommon *common = backend->u.console;
return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE),
common, errp);
}
@@ -3817,7 +3817,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
} else {
ChardevCommon *cc = g_new0(ChardevCommon, 1);
qemu
[Qemu-devel] [PULL 08/12] util: Shorten references into SocketAddress
From: Eric Blake
An upcoming patch will alter how simple unions, like SocketAddress,
are laid out, which will impact all lines of the form 'addr->u.XXX'
(expanding it to the longer 'addr->u.XXX.data'). For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
a SocketAddress. Also, take advantage of some C99 initialization where
it makes sense (simplifying g_new0() to g_new()).
Signed-off-by: Eric Blake
Message-Id: <1457021813-10704-7-git-send-email-ebl...@redhat.com>
---
block/nbd.c| 14 ++--
qemu-char.c| 49 --
qemu-nbd.c | 9
tests/test-io-channel-socket.c | 34 ++---
ui/vnc.c | 39 +
util/qemu-sockets.c| 11 +-
6 files changed, 88 insertions(+), 68 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index db57b49..9f333c9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -204,18 +204,20 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict
*options, char **export,
saddr = g_new0(SocketAddress, 1);
if (qdict_haskey(options, "path")) {
+UnixSocketAddress *q_unix;
saddr->type = SOCKET_ADDRESS_KIND_UNIX;
-saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
-saddr->u.q_unix->path = g_strdup(qdict_get_str(options, "path"));
+q_unix = saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+q_unix->path = g_strdup(qdict_get_str(options, "path"));
qdict_del(options, "path");
} else {
+InetSocketAddress *inet;
saddr->type = SOCKET_ADDRESS_KIND_INET;
-saddr->u.inet = g_new0(InetSocketAddress, 1);
-saddr->u.inet->host = g_strdup(qdict_get_str(options, "host"));
+inet = saddr->u.inet = g_new0(InetSocketAddress, 1);
+inet->host = g_strdup(qdict_get_str(options, "host"));
if (!qdict_get_try_str(options, "port")) {
-saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
} else {
-saddr->u.inet->port = g_strdup(qdict_get_str(options, "port"));
+inet->port = g_strdup(qdict_get_str(options, "port"));
}
qdict_del(options, "host");
qdict_del(options, "port");
diff --git a/qemu-char.c b/qemu-char.c
index 5ea1d34..af31102 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3659,20 +3659,23 @@ static void qemu_chr_parse_socket(QemuOpts *opts,
ChardevBackend *backend,
addr = g_new0(SocketAddress, 1);
if (path) {
+UnixSocketAddress *q_unix;
addr->type = SOCKET_ADDRESS_KIND_UNIX;
-addr->u.q_unix = g_new0(UnixSocketAddress, 1);
-addr->u.q_unix->path = g_strdup(path);
+q_unix = addr->u.q_unix = g_new0(UnixSocketAddress, 1);
+q_unix->path = g_strdup(path);
} else {
addr->type = SOCKET_ADDRESS_KIND_INET;
-addr->u.inet = g_new0(InetSocketAddress, 1);
-addr->u.inet->host = g_strdup(host);
-addr->u.inet->port = g_strdup(port);
-addr->u.inet->has_to = qemu_opt_get(opts, "to");
-addr->u.inet->to = qemu_opt_get_number(opts, "to", 0);
-addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
-addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
-addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
-addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+addr->u.inet = g_new(InetSocketAddress, 1);
+*addr->u.inet = (InetSocketAddress) {
+.host = g_strdup(host),
+.port = g_strdup(port),
+.has_to = qemu_opt_get(opts, "to"),
+.to = qemu_opt_get_number(opts, "to", 0),
+.has_ipv4 = qemu_opt_get(opts, "ipv4"),
+.ipv4 = qemu_opt_get_bool(opts, "ipv4", 0),
+.has_ipv6 = qemu_opt_get(opts, "ipv6"),
+.ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
+};
}
sock->addr = addr;
}
@@ -3711,22 +3714,26 @@ static void qemu_chr_parse_udp(QemuOpts *opts,
ChardevBackend *backend,
addr = g_new0(SocketAddress, 1);
addr->type = SOCKET_ADDRESS_KIND_INET;
-addr->u.inet = g_new0(InetSocketAddress, 1);
-addr->u.inet->host = g_strdup(host);
-addr->u.inet->port = g_strdup(port);
-addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
-addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
-addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
-addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+addr->u.inet = g_new(InetSocketAddress, 1);
+*addr->u.inet = (InetSocketAddress) {
+.host = g_strdup(host),
+.port = g_strdup(port),
+.has_ipv4 = qemu_opt_get(opts, "ipv4"),
+.ipv4 =
[Qemu-devel] [PULL 12/12] qapi: Drop useless 'data' member of unions
From: Eric Blake
We started moving away from the use of the 'void *data' member
in the C union corresponding to a QAPI union back in commit
544a373; recent commits have gotten rid of other uses. Now
that it is completely unused, we can remove the member itself
as well as the FIXME comment. Update the testsuite to drop the
negative test union-clash-data.
Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrange
Message-Id: <1457021813-10704-11-git-send-email-ebl...@redhat.com>
---
scripts/qapi-types.py | 9 -
tests/Makefile | 1 -
tests/qapi-schema/union-clash-data.err | 0
tests/qapi-schema/union-clash-data.exit | 1 -
tests/qapi-schema/union-clash-data.json | 7 ---
tests/qapi-schema/union-clash-data.out | 9 -
6 files changed, 27 deletions(-)
delete mode 100644 tests/qapi-schema/union-clash-data.err
delete mode 100644 tests/qapi-schema/union-clash-data.exit
delete mode 100644 tests/qapi-schema/union-clash-data.json
delete mode 100644 tests/qapi-schema/union-clash-data.out
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 19d1fff..0306a88 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,17 +116,8 @@ static inline %(base)s *qapi_%(c_name)s_base(const
%(c_name)s *obj)
def gen_variants(variants):
-# FIXME: What purpose does data serve, besides preventing a union that
-# has a branch named 'data'? We use it in qapi-visit.py to decide
-# whether to bypass the switch statement if visiting the discriminator
-# failed; but since we 0-initialize structs, and cannot tell what
-# branch of the union is in use if the discriminator is invalid, there
-# should not be any data leaks even without a data pointer. Or, if
-# 'data' is merely added to guarantee we don't have an empty union,
-# shouldn't we enforce that at .json parse time?
ret = mcgen('''
union { /* union tag is @%(c_name)s */
-void *data;
''',
c_name=c_name(variants.tag_member.name))
diff --git a/tests/Makefile b/tests/Makefile
index 04e34b5..cd4bbd4 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -358,7 +358,6 @@ qapi-schema += unicode-str.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-branch-case.json
qapi-schema += union-clash-branches.json
-qapi-schema += union-clash-data.json
qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
qapi-schema += union-optional-branch.json
diff --git a/tests/qapi-schema/union-clash-data.err
b/tests/qapi-schema/union-clash-data.err
deleted file mode 100644
index e69de29..000
diff --git a/tests/qapi-schema/union-clash-data.exit
b/tests/qapi-schema/union-clash-data.exit
deleted file mode 100644
index 573541a..000
--- a/tests/qapi-schema/union-clash-data.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/union-clash-data.json
b/tests/qapi-schema/union-clash-data.json
deleted file mode 100644
index 7308e69..000
--- a/tests/qapi-schema/union-clash-data.json
+++ /dev/null
@@ -1,7 +0,0 @@
-# Union branch 'data'
-# FIXME: this parses, but then fails to compile due to a duplicate 'data'
-# (one from the branch name, another as a filler to avoid an empty union).
-# we should either detect the collision at parse time, or change the
-# generated struct to allow this to compile.
-{ 'union': 'TestUnion',
- 'data': { 'data': 'int' } }
diff --git a/tests/qapi-schema/union-clash-data.out
b/tests/qapi-schema/union-clash-data.out
deleted file mode 100644
index f5752f4..000
--- a/tests/qapi-schema/union-clash-data.out
+++ /dev/null
@@ -1,9 +0,0 @@
-object :empty
-object :obj-int-wrapper
-member data: int optional=False
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat',
'qbool']
-prefix QTYPE
-object TestUnion
-member type: TestUnionKind optional=False
-case data: :obj-int-wrapper
-enum TestUnionKind ['data']
--
2.4.3
[Qemu-devel] [PULL 03/12] qapi: Rename 'fields' to 'members' in generator
From: Eric Blake
C types and JSON objects don't have fields, but members. We
shouldn't gratuitously invent terminology. This patch is a
strict renaming of generator code internals (including testsuite
comments), before later patches rename C interfaces.
No change to generated code with this patch.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
Message-Id: <1457021813-10704-2-git-send-email-ebl...@redhat.com>
---
scripts/qapi-commands.py| 4 ++--
scripts/qapi-event.py | 4 ++--
scripts/qapi-types.py | 8
scripts/qapi-visit.py | 28 ++--
scripts/qapi.py | 20 ++--
tests/qapi-schema/qapi-schema-test.json | 2 +-
6 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f831621..f44e01f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -111,7 +111,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
v = qmp_input_get_visitor(qiv);
''')
-ret += gen_visit_fields(arg_type.members, skiperr=dealloc)
+ret += gen_visit_members(arg_type.members, skiperr=dealloc)
if dealloc:
ret += mcgen('''
@@ -175,7 +175,7 @@ def gen_marshal(name, arg_type, ret_type):
ret += gen_marshal_input_visit(arg_type)
ret += gen_call(name, arg_type, ret_type)
-# 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
+# 'goto out' produced by gen_marshal_input_visit->gen_visit_members()
# for each arg_type member, and by gen_call() for ret_type
if (arg_type and arg_type.members) or ret_type:
ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 544ae12..fb579dd 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -67,8 +67,8 @@ def gen_event_send(name, arg_type):
''',
name=name)
ret += gen_err_check()
-ret += gen_visit_fields(arg_type.members, need_cast=True,
-label='out_obj')
+ret += gen_visit_members(arg_type.members, need_cast=True,
+ label='out_obj')
ret += mcgen('''
out_obj:
visit_end_struct(v, err ? NULL : &err);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index eac90d2..8858d29 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -38,7 +38,7 @@ struct %(c_name)s {
c_name=c_name(name), c_type=element_type.c_type())
-def gen_struct_fields(members):
+def gen_struct_members(members):
ret = ''
for memb in members:
if memb.optional:
@@ -77,16 +77,16 @@ struct %(c_name)s {
/* Members inherited from %(c_name)s: */
''',
c_name=base.c_name())
-ret += gen_struct_fields(base.members)
+ret += gen_struct_members(base.members)
ret += mcgen('''
/* Own members: */
''')
-ret += gen_struct_fields(members)
+ret += gen_struct_members(members)
if variants:
ret += gen_variants(variants)
-# Make sure that all structs have at least one field; this avoids
+# Make sure that all structs have at least one member; this avoids
# potential issues with attempting to malloc space for zero-length
# structs in C, and also incompatibility with C++ (where an empty
# struct is size 1).
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2308268..b21d3ef 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,9 +15,9 @@
from qapi import *
import re
-# visit_type_FOO_fields() is always emitted; track if a forward declaration
+# visit_type_FOO_members() is always emitted; track if a forward declaration
# or implementation has already been output.
-struct_fields_seen = set()
+object_members_seen = set()
def gen_visit_decl(name, scalar=False):
@@ -30,10 +30,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
%(c_type)sobj, Error **
c_name=c_name(name), c_type=c_type)
-def gen_visit_fields_decl(typ):
-if typ.name in struct_fields_seen:
+def gen_visit_members_decl(typ):
+if typ.name in object_members_seen:
return ''
-struct_fields_seen.add(typ.name)
+object_members_seen.add(typ.name)
return mcgen('''
static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error
**errp);
@@ -41,18 +41,18 @@ static void visit_type_%(c_type)s_fields(Visitor *v,
%(c_type)s *obj, Error **er
c_type=typ.c_name())
-def gen_visit_struct_fields(name, base, members, variants):
+def gen_visit_object_members(name, base, members, variants):
ret = ''
if base:
-ret += gen_visit_fields_decl(base)
+ret += gen_visit_members_decl(base)
if variants:
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
if n
[Qemu-devel] [PULL 09/12] ui: Shorten references into InputEvent
From: Eric Blake
An upcoming patch will alter how simple unions, like InputEvent, are
laid out, which will impact all lines of the form 'evt->u.XXX'
(expanding it to the longer 'evt->u.XXX.data'). For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
an InputEvent.
There was one instance in hid.c:hid_pointer_event() where the code
was referring to evt->u.rel inside the case label where evt->u.abs
is the correct name; thankfully, both members of the union have the
same type, so it happened to work, but it is now cleaner.
Signed-off-by: Eric Blake
Message-Id: <1457021813-10704-8-git-send-email-ebl...@redhat.com>
---
hw/char/escc.c | 12 +-
hw/input/hid.c | 36 +-
hw/input/ps2.c | 27 ++-
hw/input/virtio-input-hid.c | 33 ---
replay/replay-input.c | 31 --
ui/input-legacy.c | 25 -
ui/input.c | 54 ++---
7 files changed, 129 insertions(+), 89 deletions(-)
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 98a1c21..c7a24ac 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -842,14 +842,16 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
{
ChannelState *s = (ChannelState *)dev;
int qcode, keycode;
+InputKeyEvent *key;
assert(evt->type == INPUT_EVENT_KIND_KEY);
-qcode = qemu_input_key_value_to_qcode(evt->u.key->key);
+key = evt->u.key;
+qcode = qemu_input_key_value_to_qcode(key->key);
trace_escc_sunkbd_event_in(qcode, QKeyCode_lookup[qcode],
- evt->u.key->down);
+ key->down);
if (qcode == Q_KEY_CODE_CAPS_LOCK) {
-if (evt->u.key->down) {
+if (key->down) {
s->caps_lock_mode ^= 1;
if (s->caps_lock_mode == 2) {
return; /* Drop second press */
@@ -863,7 +865,7 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
}
if (qcode == Q_KEY_CODE_NUM_LOCK) {
-if (evt->u.key->down) {
+if (key->down) {
s->num_lock_mode ^= 1;
if (s->num_lock_mode == 2) {
return; /* Drop second press */
@@ -877,7 +879,7 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
}
keycode = qcode_to_keycode[qcode];
-if (!evt->u.key->down) {
+if (!key->down) {
keycode |= 0x80;
}
trace_escc_sunkbd_event_out(keycode);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 81a85fb..41a9387 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -116,37 +116,42 @@ static void hid_pointer_event(DeviceState *dev,
QemuConsole *src,
};
HIDState *hs = (HIDState *)dev;
HIDPointerEvent *e;
+InputMoveEvent *move;
+InputBtnEvent *btn;
assert(hs->n < QUEUE_LENGTH);
e = &hs->ptr.queue[(hs->head + hs->n) & QUEUE_MASK];
switch (evt->type) {
case INPUT_EVENT_KIND_REL:
-if (evt->u.rel->axis == INPUT_AXIS_X) {
-e->xdx += evt->u.rel->value;
-} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
-e->ydy += evt->u.rel->value;
+move = evt->u.rel;
+if (move->axis == INPUT_AXIS_X) {
+e->xdx += move->value;
+} else if (move->axis == INPUT_AXIS_Y) {
+e->ydy += move->value;
}
break;
case INPUT_EVENT_KIND_ABS:
-if (evt->u.rel->axis == INPUT_AXIS_X) {
-e->xdx = evt->u.rel->value;
-} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
-e->ydy = evt->u.rel->value;
+move = evt->u.abs;
+if (move->axis == INPUT_AXIS_X) {
+e->xdx = move->value;
+} else if (move->axis == INPUT_AXIS_Y) {
+e->ydy = move->value;
}
break;
case INPUT_EVENT_KIND_BTN:
-if (evt->u.btn->down) {
-e->buttons_state |= bmap[evt->u.btn->button];
-if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
+btn = evt->u.btn;
+if (btn->down) {
+e->buttons_state |= bmap[btn->button];
+if (btn->button == INPUT_BUTTON_WHEEL_UP) {
e->dz--;
-} else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+} else if (btn->button == INPUT_BUTTON_WHEEL_DOWN) {
e->dz++;
}
} else {
-e->buttons_state &= ~bmap[evt->u.btn->button];
+e->buttons_state &= ~bmap[btn->button];
}
break;
@@ -223,9 +228,10 @@ static void hid_keyboard_event(DeviceState *dev,
QemuConsole *src,
HIDState *hs = (HIDState *)dev;
int scancodes[3], i, count;
int slot;
+InputKeyEvent *ke
[Qemu-devel] [PULL 00/12] QAPI patches for 2016-03-04
The following changes since commit 3c0f12df65da872d5fbccae469f2cb21ed1c03b7: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160304' into staging (2016-03-04 11:46:32 +) are available in the git repository at: git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2016-03-04 for you to fetch changes up to 984ebcbab466a05beccc5bfcd7eff4ce7efc48d5: qapi: Drop useless 'data' member of unions (2016-03-04 17:27:50 +0100) QAPI patches for 2016-03-04 Daniel P. Berrange (1): qmp-shell: fix pretty printing of JSON responses Eric Blake (11): qapi-dealloc: Reduce use outside of generated code qapi: Rename 'fields' to 'members' in generator qapi: Rename 'fields' to 'members' in generated C code qapi-visit: Expose visit_type_FOO_members() qapi: Update docs to match recent generator changes chardev: Shorten references into ChardevBackend util: Shorten references into SocketAddress ui: Shorten references into InputEvent qapi: Avoid use of 'data' member of QAPI unions chardev: Drop useless ChardevDummy type qapi: Drop useless 'data' member of unions backends/baum.c | 2 +- backends/msmouse.c | 2 +- block/nbd.c | 14 +- blockdev.c | 31 +-- docs/qapi-code-gen.txt | 364 +--- docs/qmp-spec.txt | 4 +- hw/acpi/core.c | 11 +- hw/char/escc.c | 12 +- hw/input/hid.c | 36 ++-- hw/input/ps2.c | 27 ++- hw/input/virtio-input-hid.c | 33 +-- net/net.c | 31 +-- numa.c | 9 +- qapi-schema.json| 15 +- qemu-char.c | 179 +--- qemu-nbd.c | 9 +- replay/replay-input.c | 31 +-- scripts/qapi-commands.py| 4 +- scripts/qapi-event.py | 4 +- scripts/qapi-types.py | 19 +- scripts/qapi-visit.py | 51 ++--- scripts/qapi.py | 20 +- scripts/qmp/qmp-shell | 23 +- tests/Makefile | 1 - tests/qapi-schema/qapi-schema-test.json | 2 +- tests/qapi-schema/union-clash-data.err | 0 tests/qapi-schema/union-clash-data.exit | 1 - tests/qapi-schema/union-clash-data.json | 7 - tests/qapi-schema/union-clash-data.out | 9 - tests/test-io-channel-socket.c | 34 +-- tests/test-opts-visitor.c | 10 +- ui/input-legacy.c | 25 ++- ui/input.c | 56 ++--- ui/vnc.c| 39 ++-- util/qemu-sockets.c | 11 +- 35 files changed, 564 insertions(+), 562 deletions(-) delete mode 100644 tests/qapi-schema/union-clash-data.err delete mode 100644 tests/qapi-schema/union-clash-data.exit delete mode 100644 tests/qapi-schema/union-clash-data.json delete mode 100644 tests/qapi-schema/union-clash-data.out -- 2.4.3
[Qemu-devel] [PULL 02/12] qapi-dealloc: Reduce use outside of generated code
From: Eric Blake
No need to roll our own use of the dealloc visitors when we can
just directly use the qapi_free_FOO() functions that do what we
want in one line.
In net.c, inline net_visit() into its remaining lone caller.
After this patch, test-visitor-serialization.c is the only
non-generated file that needs to use a dealloc visitor, because
it is testing low level aspects of the visitor interface.
Signed-off-by: Eric Blake
Message-Id: <1456262075-3311-2-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
hw/acpi/core.c| 11 +--
net/net.c | 31 ++-
numa.c| 9 +
tests/test-opts-visitor.c | 10 +-
4 files changed, 13 insertions(+), 48 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 3a14e90..3d9e5c4 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -26,7 +26,6 @@
#include "hw/nvram/fw_cfg.h"
#include "qemu/config-file.h"
#include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
#include "qapi-visit.h"
#include "qapi-event.h"
@@ -297,15 +296,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
out:
g_free(blob);
g_strfreev(pathnames);
-
-if (hdrs != NULL) {
-QapiDeallocVisitor *dv;
-
-dv = qapi_dealloc_visitor_new();
-visit_type_AcpiTableOptions(qapi_dealloc_get_visitor(dv), NULL, &hdrs,
-NULL);
-qapi_dealloc_visitor_cleanup(dv);
-}
+qapi_free_AcpiTableOptions(hdrs);
error_propagate(errp, err);
}
diff --git a/net/net.c b/net/net.c
index aebf753..b0c832e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -42,7 +42,6 @@
#include "qemu/main-loop.h"
#include "qapi-visit.h"
#include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
#include "sysemu/sysemu.h"
#include "net/filter.h"
#include "qapi/string-output-visitor.h"
@@ -1043,38 +1042,28 @@ static int net_client_init1(const void *object, int
is_netdev, Error **errp)
}
-static void net_visit(Visitor *v, int is_netdev, void **object, Error **errp)
-{
-if (is_netdev) {
-visit_type_Netdev(v, NULL, (Netdev **)object, errp);
-} else {
-visit_type_NetLegacy(v, NULL, (NetLegacy **)object, errp);
-}
-}
-
-
int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
{
void *object = NULL;
Error *err = NULL;
int ret = -1;
+OptsVisitor *ov = opts_visitor_new(opts);
+Visitor *v = opts_get_visitor(ov);
-{
-OptsVisitor *ov = opts_visitor_new(opts);
-
-net_visit(opts_get_visitor(ov), is_netdev, &object, &err);
-opts_visitor_cleanup(ov);
+if (is_netdev) {
+visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
+} else {
+visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
}
if (!err) {
ret = net_client_init1(object, is_netdev, &err);
}
-if (object) {
-QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
-
-net_visit(qapi_dealloc_get_visitor(dv), is_netdev, &object, NULL);
-qapi_dealloc_visitor_cleanup(dv);
+if (is_netdev) {
+qapi_free_Netdev(object);
+} else {
+qapi_free_NetLegacy(object);
}
error_propagate(errp, err);
diff --git a/numa.c b/numa.c
index 4c4f7f5..da27bf8 100644
--- a/numa.c
+++ b/numa.c
@@ -31,7 +31,6 @@
#include "include/exec/cpu-common.h" /* for RAM_ADDR_FMT */
#include "qapi-visit.h"
#include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
#include "hw/boards.h"
#include "sysemu/hostmem.h"
#include "qmp-commands.h"
@@ -243,13 +242,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error
**errp)
error:
error_report_err(err);
-
-if (object) {
-QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
-visit_type_NumaOptions(qapi_dealloc_get_visitor(dv), NULL, &object,
- NULL);
-qapi_dealloc_visitor_cleanup(dv);
-}
+qapi_free_NumaOptions(object);
return -1;
}
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index b7acf7d..297a02d 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -17,7 +17,6 @@
#include "qemu/option.h" /* qemu_opts_parse() */
#include "qapi/opts-visitor.h"/* opts_visitor_new() */
#include "test-qapi-visit.h" /* visit_type_UserDefOptions() */
-#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
static QemuOptsList userdef_opts = {
.name = "userdef",
@@ -55,14 +54,7 @@ setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
static void
teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
{
-if (f->userdef != NULL) {
-QapiDeallocVisitor *dv;
-
-dv = qapi_dealloc_visitor_new();
-visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), NULL,
- &f->userdef, NULL);
-qapi_dealloc_visitor_cleanup(dv);
-
[Qemu-devel] [PULL 01/12] qmp-shell: fix pretty printing of JSON responses
From: "Daniel P. Berrange"
Pretty printing of JSON responses is important to be able to understand
large responses from query commands in particular. Unfortunately this
was broken during the addition of the verbose flag in
commit 1ceca07e48ead0dd2e41576c81d40e6a91cafefd
Author: John Snow
Date: Wed Apr 29 15:14:04 2015 -0400
scripts: qmp-shell: Add verbose flag
This is because that change turned the python data structure into a
formatted JSON string before the pretty print was given it. So we're
just pretty printing a string, which is a no-op.
The original pretty printer would output python objects.
(QEMU) query-chardev
{ u'return': [ { u'filename': u'vc',
u'frontend-open': False,
u'label': u'parallel0'},
{ u'filename': u'vc',
u'frontend-open': True,
u'label': u'serial0'},
{ u'filename': u'unix:/tmp/qemp,server',
u'frontend-open': True,
u'label': u'compat_monitor0'}]}
This fixes the problem by switching to outputting pretty formatted JSON
text instead. This has the added benefit that the pretty printed output
is now valid JSON text. Due to the way the verbose flag was handled, the
pretty printing now applies to the command sent, as well as its response:
(QEMU) query-chardev
{
"execute": "query-chardev",
"arguments": {}
}
{
"return": [
{
"frontend-open": false,
"label": "parallel0",
"filename": "vc"
},
{
"frontend-open": true,
"label": "serial0",
"filename": "vc"
},
{
"frontend-open": true,
"label": "compat_monitor0",
"filename": "unix:/tmp/qmp,server"
}
]
}
Signed-off-by: Daniel P. Berrange
Message-Id: <1456224706-1591-1-git-send-email-berra...@redhat.com>
Tested-by: Kashyap Chamarthy
Reviewed-by: John Snow
[Bonus fix: multiple -p now work]
Signed-off-by: Markus Armbruster
---
scripts/qmp/qmp-shell | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 7a402ed..0373b24 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -70,7 +70,6 @@ import json
import ast
import readline
import sys
-import pprint
class QMPCompleter(list):
def complete(self, text, state):
@@ -103,11 +102,11 @@ class FuzzyJSON(ast.NodeTransformer):
# TODO: QMPShell's interface is a bit ugly (eg. _fill_completion() and
# _execute_cmd()). Let's design a better one.
class QMPShell(qmp.QEMUMonitorProtocol):
-def __init__(self, address, pp=None):
+def __init__(self, address, pretty=False):
qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address))
self._greeting = None
self._completer = None
-self._pp = pp
+self._pretty = pretty
self._transmode = False
self._actions = list()
@@ -231,11 +230,11 @@ class QMPShell(qmp.QEMUMonitorProtocol):
return qmpcmd
def _print(self, qmp):
-jsobj = json.dumps(qmp)
-if self._pp is not None:
-self._pp.pprint(jsobj)
-else:
-print str(jsobj)
+indent = None
+if self._pretty:
+indent = 4
+jsobj = json.dumps(qmp, indent=indent)
+print str(jsobj)
def _execute_cmd(self, cmdline):
try:
@@ -377,7 +376,7 @@ def main():
addr = ''
qemu = None
hmp = False
-pp = None
+pretty = False
verbose = False
try:
@@ -387,9 +386,7 @@ def main():
fail_cmdline(arg)
hmp = True
elif arg == "-p":
-if pp is not None:
-fail_cmdline(arg)
-pp = pprint.PrettyPrinter(indent=4)
+pretty = True
elif arg == "-v":
verbose = True
else:
@@ -398,7 +395,7 @@ def main():
if hmp:
qemu = HMPShell(arg)
else:
-qemu = QMPShell(arg, pp)
+qemu = QMPShell(arg, pretty)
addr = arg
if qemu is None:
--
2.4.3
[Qemu-devel] [PULL 06/12] qapi: Update docs to match recent generator changes
From: Eric Blake Several commits have been changing the generator, but not updating the docs to match: - The implicit tag member is named "type", not "kind". Screwed up in commit 39a1815. - Commit 9f08c8ec made list types lazy, and thereby dropped UserDefOneList if nothing explicitly uses the list type. - Commit 51e72bc1 switched the parameter order with 'name' occurring earlier. - Commit e65d89bf changed the layout of UserDefOneList. - Prefer the term 'member' over 'field'. - We now expose visit_type_FOO_members() for objects. - etc. Rework the examples to show slightly more output (we don't want to show too much; that's what the testsuite is for), and regenerate the output to match all recent changes. Also, rearrange output to show .h files before .c (understanding the interface first often makes the implementation easier to follow). Reported-by: Marc-André Lureau Signed-off-by: Eric Blake Signed-off-by: Markus Armbruster Message-Id: <1457021813-10704-5-git-send-email-ebl...@redhat.com> --- docs/qapi-code-gen.txt | 364 ++--- docs/qmp-spec.txt | 4 +- 2 files changed, 192 insertions(+), 176 deletions(-) diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 999f3b9..e0b2ef1 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -1,7 +1,7 @@ = How to use the QAPI code generator = Copyright IBM Corp. 2011 -Copyright (C) 2012-2015 Red Hat, Inc. +Copyright (C) 2012-2016 Red Hat, Inc. This work is licensed under the terms of the GNU GPL, version 2 or later. See the COPYING file in the top-level directory. @@ -52,7 +52,7 @@ schema. The documentation is delimited between two lines of ##, then the first line names the expression, an optional overview is provided, then individual documentation about each member of 'data' is provided, and finally, a 'Since: x.y.z' tag lists the release that introduced -the expression. Optional fields are tagged with the phrase +the expression. Optional members are tagged with the phrase '#optional', often with their default value; and extensions added after the expression was first released are also given a '(since x.y.z)' comment. For example: @@ -108,15 +108,15 @@ user-defined type names, while built-in types are lowercase. Type definitions should not end in 'Kind', as this namespace is used for creating implicit C enums for visiting union types, or in 'List', as this namespace is used for creating array types. Command names, -and field names within a type, should be all lower case with words +and member names within a type, should be all lower case with words separated by a hyphen. However, some existing older commands and complex types use underscore; when extending such expressions, consistency is preferred over blindly avoiding underscore. Event -names should be ALL_CAPS with words separated by underscore. Field +names should be ALL_CAPS with words separated by underscore. Member names cannot start with 'has-' or 'has_', as this is reserved for -tracking optional fields. +tracking optional members. -Any name (command, event, type, field, or enum value) beginning with +Any name (command, event, type, member, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed incompatibly in a future release. All names must begin with a letter, and contain only ASCII letters, digits, dash, and underscore. There @@ -127,7 +127,7 @@ the vendor), even if the rest of the name uses dash (example: __com.redhat_drive-mirror). Names beginning with 'q_' are reserved for the generator: QMP names that resemble C keywords or other problematic strings will be munged in C to use this prefix. For -example, a field named "default" in qapi becomes "q_default" in the +example, a member named "default" in qapi becomes "q_default" in the generated C code. In the rest of this document, usage lines are given for each @@ -217,17 +217,18 @@ and must continue to work). On output structures (only mentioned in the 'returns' side of a command), changing from mandatory to optional is in general unsafe (older clients may be -expecting the field, and could crash if it is missing), although it can be done -if the only way that the optional argument will be omitted is when it is -triggered by the presence of a new input flag to the command that older clients -don't know to send. Changing from optional to mandatory is safe. +expecting the member, and could crash if it is missing), although it +can be done if the only way that the optional argument will be omitted +is when it is triggered by the presence of a new input flag to the +command that older clients don't know to send. Changing from optional +to mandatory is safe. A structure that is used in both input and output of various commands must consider the backwards compatibility constraints of both directions of use. A struct definition can specify another struct as its base. -In this c
[Qemu-devel] [PULL 07/12] chardev: Shorten references into ChardevBackend
From: Eric Blake
An upcoming patch will alter how simple unions, like ChardevBackend,
are laid out, which will impact all lines of the form 'backend->u.XXX'
(expanding it to the longer 'backend->u.XXX.data'). For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
a ChardevBackend. It doesn't hurt that this also makes the code more
consistent: some clients touched here already had a temporary variable
but weren't using it.
Signed-off-by: Eric Blake
Reviewed-By: Daniel P. Berrange
Message-Id: <1457021813-10704-6-git-send-email-ebl...@redhat.com>
---
qemu-char.c | 122
1 file changed, 66 insertions(+), 56 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index fc8ffda..5ea1d34 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -724,7 +724,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
ChardevMux *mux = backend->u.mux;
CharDriverState *chr, *drv;
MuxDriver *d;
-ChardevCommon *common = qapi_ChardevMux_base(backend->u.mux);
+ChardevCommon *common = qapi_ChardevMux_base(mux);
drv = qemu_chr_find(mux->chardev);
if (drv == NULL) {
@@ -1043,7 +1043,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
char *filename_in;
char *filename_out;
const char *filename = opts->device;
-ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
+ChardevCommon *common = qapi_ChardevHostdev_base(opts);
filename_in = g_strdup_printf("%s.in", filename);
@@ -1123,7 +1123,7 @@ static CharDriverState *qemu_chr_open_stdio(const char
*id,
ChardevStdio *opts = backend->u.stdio;
CharDriverState *chr;
struct sigaction act;
-ChardevCommon *common = qapi_ChardevStdio_base(backend->u.stdio);
+ChardevCommon *common = qapi_ChardevStdio_base(opts);
if (is_daemonized()) {
error_setg(errp, "cannot use stdio with -daemonize");
@@ -2141,7 +2141,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
const char *filename = opts->device;
CharDriverState *chr;
WinCharState *s;
-ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
+ChardevCommon *common = qapi_ChardevHostdev_base(opts);
chr = qemu_chr_alloc(common, errp);
if (!chr) {
@@ -3216,7 +3216,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const char
*id,
Error **errp)
{
ChardevRingbuf *opts = backend->u.ringbuf;
-ChardevCommon *common = qapi_ChardevRingbuf_base(backend->u.ringbuf);
+ChardevCommon *common = qapi_ChardevRingbuf_base(opts);
CharDriverState *chr;
RingBufCharDriver *d;
@@ -3506,26 +3506,29 @@ static void qemu_chr_parse_file_out(QemuOpts *opts,
ChardevBackend *backend,
Error **errp)
{
const char *path = qemu_opt_get(opts, "path");
+ChardevFile *file;
if (path == NULL) {
error_setg(errp, "chardev: file: no filename given");
return;
}
-backend->u.file = g_new0(ChardevFile, 1);
-qemu_chr_parse_common(opts, qapi_ChardevFile_base(backend->u.file));
-backend->u.file->out = g_strdup(path);
+file = backend->u.file = g_new0(ChardevFile, 1);
+qemu_chr_parse_common(opts, qapi_ChardevFile_base(file));
+file->out = g_strdup(path);
-backend->u.file->has_append = true;
-backend->u.file->append = qemu_opt_get_bool(opts, "append", false);
+file->has_append = true;
+file->append = qemu_opt_get_bool(opts, "append", false);
}
static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
{
-backend->u.stdio = g_new0(ChardevStdio, 1);
-qemu_chr_parse_common(opts, qapi_ChardevStdio_base(backend->u.stdio));
-backend->u.stdio->has_signal = true;
-backend->u.stdio->signal = qemu_opt_get_bool(opts, "signal", true);
+ChardevStdio *stdio;
+
+stdio = backend->u.stdio = g_new0(ChardevStdio, 1);
+qemu_chr_parse_common(opts, qapi_ChardevStdio_base(stdio));
+stdio->has_signal = true;
+stdio->signal = qemu_opt_get_bool(opts, "signal", true);
}
#ifdef HAVE_CHARDEV_SERIAL
@@ -3533,14 +3536,15 @@ static void qemu_chr_parse_serial(QemuOpts *opts,
ChardevBackend *backend,
Error **errp)
{
const char *device = qemu_opt_get(opts, "path");
+ChardevHostdev *serial;
if (device == NULL) {
error_setg(errp, "chardev: serial/tty: no device path given");
return;
}
-backend->u.serial = g_new0(ChardevHostdev, 1);
-qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(backend->u.serial));
-backend->u.serial->device = g_strdup(device);
+serial = backend->u.serial = g_new0(ChardevHostdev, 1);
+qemu_chr_parse_common(opts,
[Qemu-devel] [PULL 10/12] qapi: Avoid use of 'data' member of QAPI unions
From: Eric Blake
QAPI code generators currently create a 'void *data' member as
part of the anonymous union embedded in the C struct corresponding
to a QAPI union. However, directly assigning to this member of
the union feels a bit fishy, when we can assign to another member
of the struct instead.
Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrange
Message-Id: <1457021813-10704-9-git-send-email-ebl...@redhat.com>
---
blockdev.c | 31 +--
ui/input.c | 2 +-
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d4bc435..0f20c65 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1202,15 +1202,11 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
}
}
-static void blockdev_do_action(TransactionActionKind type, void *data,
- Error **errp)
+static void blockdev_do_action(TransactionAction *action, Error **errp)
{
-TransactionAction action;
TransactionActionList list;
-action.type = type;
-action.u.data = data;
-list.value = &action;
+list.value = action;
list.next = NULL;
qmp_transaction(&list, false, NULL, errp);
}
@@ -1236,8 +1232,11 @@ void qmp_blockdev_snapshot_sync(bool has_device, const
char *device,
.has_mode = has_mode,
.mode = mode,
};
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
- &snapshot, errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
+.u.blockdev_snapshot_sync = &snapshot,
+};
+blockdev_do_action(&action, errp);
}
void qmp_blockdev_snapshot(const char *node, const char *overlay,
@@ -1247,9 +1246,11 @@ void qmp_blockdev_snapshot(const char *node, const char
*overlay,
.node = (char *) node,
.overlay = (char *) overlay
};
-
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
- &snapshot_data, errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+.u.blockdev_snapshot = &snapshot_data,
+};
+blockdev_do_action(&action, errp);
}
void qmp_blockdev_snapshot_internal_sync(const char *device,
@@ -1260,9 +1261,11 @@ void qmp_blockdev_snapshot_internal_sync(const char
*device,
.device = (char *) device,
.name = (char *) name
};
-
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
- &snapshot, errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+.u.blockdev_snapshot_internal_sync = &snapshot,
+};
+blockdev_do_action(&action, errp);
}
SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
diff --git a/ui/input.c b/ui/input.c
index 13ee117..b035f86 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -470,7 +470,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
InputMoveEvent *move = g_new0(InputMoveEvent, 1);
evt->type = kind;
-evt->u.data = move;
+evt->u.rel = move; /* evt->u.rel is the same as evt->u.abs */
move->axis = axis;
move->value = value;
return evt;
--
2.4.3
[Qemu-devel] [PULL 04/12] qapi: Rename 'fields' to 'members' in generated C code
From: Eric Blake
C types and JSON objects don't have fields, but members. We
shouldn't gratuitously invent terminology. This patch is a
strict renaming of static genarated functions, plus the naming
of the dummy filler member for empty structs, before the next
patch exposes some of that naming to the rest of the code base.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
Message-Id: <1457021813-10704-3-git-send-email-ebl...@redhat.com>
---
scripts/qapi-types.py | 2 +-
scripts/qapi-visit.py | 12 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 8858d29..19d1fff 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -92,7 +92,7 @@ struct %(c_name)s {
# struct is size 1).
if not (base and base.members) and not members and not variants:
ret += mcgen('''
-char qapi_dummy_field_for_empty_struct;
+char qapi_dummy_for_empty_struct;
''')
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b21d3ef..1e52f76 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -36,7 +36,7 @@ def gen_visit_members_decl(typ):
object_members_seen.add(typ.name)
return mcgen('''
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error
**errp);
+static void visit_type_%(c_type)s_members(Visitor *v, %(c_type)s *obj, Error
**errp);
''',
c_type=typ.c_name())
@@ -55,7 +55,7 @@ def gen_visit_object_members(name, base, members, variants):
object_members_seen.add(name)
ret += mcgen('''
-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error
**errp)
+static void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error
**errp)
{
Error *err = NULL;
@@ -64,7 +64,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s *obj, Error **er
if base:
ret += mcgen('''
-visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, &err);
+visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err);
''',
c_type=base.c_name())
ret += gen_err_check()
@@ -94,7 +94,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s *obj, Error **er
c_name=c_name(var.name))
else:
ret += mcgen('''
-visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err);
+visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
@@ -202,7 +202,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
%(c_name)s **obj, Error
if (err) {
break;
}
-visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err);
+visit_type_%(c_type)s_members(v, &(*obj)->u.%(c_name)s, &err);
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
@@ -254,7 +254,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
%(c_name)s **obj, Error
if (!*obj) {
goto out_obj;
}
-visit_type_%(c_name)s_fields(v, *obj, &err);
+visit_type_%(c_name)s_members(v, *obj, &err);
error_propagate(errp, err);
err = NULL;
out_obj:
--
2.4.3
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 4 March 2016 at 16:35, Eric Blake wrote:
> On 03/04/2016 09:06 AM, Peter Maydell wrote:
>
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name,
const argtype *types)
for(i = 0;i < 2; i++) {
offset = 0;
max_align = 1;
-se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
type_ptr = se->field_types;
for(j = 0;j < nb_fields; j++) {
size = thunk_type_size(type_ptr, i);
>>>
>>> Where is the corresponding free()? g_malloc() must be paired with
>>> g_free(), so you need to convert both places at once.
>>
>> There is no corresponding free(). thunk_register_struct() is called
>> only at startup from the linux-user code in order to populate the
>> struct_entries array; this data structure then remains live for
>> the entire lifetime of the program and is automatically freed when
>> QEMU exits.
>
> Fair enough. However, g_new(int, nb_fields) is probably a bit nicer
> than g_malloc() (in that it would detect multiplication overflow if
> nb_fields were ever oversized).
Yes, good idea.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 00/10] easier unboxed visits/qapi implicit types
Eric Blake writes: > This is patches 1-9 of v2: > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06079.html > > the rest of that series needs a bit more work on commit messages, > so I'll post it later as v4. > > Depends on Markus' qapi-next branch. Applied to my qapi-next branch with the fixed up PATCH 09, thanks! [...]
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 03/04/2016 09:06 AM, Peter Maydell wrote:
>>> +++ b/thunk.c
>>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name,
>>> const argtype *types)
>>> for(i = 0;i < 2; i++) {
>>> offset = 0;
>>> max_align = 1;
>>> -se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>>> +se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>> type_ptr = se->field_types;
>>> for(j = 0;j < nb_fields; j++) {
>>> size = thunk_type_size(type_ptr, i);
>>
>> Where is the corresponding free()? g_malloc() must be paired with
>> g_free(), so you need to convert both places at once.
>
> There is no corresponding free(). thunk_register_struct() is called
> only at startup from the linux-user code in order to populate the
> struct_entries array; this data structure then remains live for
> the entire lifetime of the program and is automatically freed when
> QEMU exits.
Fair enough. However, g_new(int, nb_fields) is probably a bit nicer
than g_malloc() (in that it would detect multiplication overflow if
nb_fields were ever oversized).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
Ok. Will complete the required.
Thanks,
Sarah
On 4 March 2016 at 15:44, Eric Blake wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in
bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name,
const argtype *types)
>> for(i = 0;i < 2; i++) {
>> offset = 0;
>> max_align = 1;
>> -se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>> type_ptr = se->field_types;
>> for(j = 0;j < nb_fields; j++) {
>> size = thunk_type_size(type_ptr, i);
>
> Where is the corresponding free()? g_malloc() must be paired with
> g_free(), so you need to convert both places at once.
There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.
This is worth mentioning in the commit message, but the code is
correct I think:
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
On 04/03/2016 15:26, Li, Liang Z wrote: >> > >> > The memory usage will keep increasing due to ever growing caches, etc, so >> > you'll be left with very little free memory fairly soon. >> > > I don't think so. > Roman is right. For example, here I am looking at a 64 GB (physical) machine which was booted about 30 minutes ago, and which is running disk-heavy workloads (installing VMs). Since I have started writing this email (2 minutes?), the amount of free memory has already gone down from 37 GB to 33 GB. I expect that by the time I have finished running the workload, in two hours, it will not have any free memory. Paolo
Re: [Qemu-devel] [Qemu-block] [PATCH v15 7/9] Introduce new APIs to do replication operation
On Fri, Feb 05, 2016 at 12:18:06PM +0800, Changlong Xie wrote:
> diff --git a/replication.h b/replication.h
> new file mode 100644
> index 000..faea649
> --- /dev/null
> +++ b/replication.h
> @@ -0,0 +1,53 @@
> +/*
> + * Replication filter
> + *
> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> + * Copyright (c) 2016 Intel Corporation
> + * Copyright (c) 2016 FUJITSU LIMITED
> + *
> + * Author:
> + * Wen Congyang
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef REPLICATION_H
> +#define REPLICATION_H
> +
> +#include "sysemu/sysemu.h"
> +
> +typedef struct ReplicationOps ReplicationOps;
> +typedef struct ReplicationState ReplicationState;
> +typedef void (*Start)(ReplicationState *rs, ReplicationMode mode, Error
> **errp);
> +typedef void (*Stop)(ReplicationState *rs, bool failover, Error **errp);
> +typedef void (*Checkpoint)(ReplicationState *rs, Error **errp);
> +typedef void (*GetError)(ReplicationState *rs, Error **errp);
These typedefs are likely to collide with system headers. Please prefix
the name.
That said, I don't think Start, Stop, Checkpoint, GetError are
necessary. Just declare the prototypes in struct ReplicationOps. No
user passes single function pointers, they always pass ReplicationOps.
Therefore it's not necessary to declare types for these functions at
all.
> +
> +struct ReplicationState {
> +void *opaque;
> +ReplicationOps *ops;
> +QLIST_ENTRY(ReplicationState) node;
> +};
> +
> +struct ReplicationOps{
> +Start start;
> +Checkpoint checkpoint;
> +GetError get_error;
> +Stop stop;
> +};
Please add doc comments that explain the semantics of these functions.
For examples, see include/qom/object.h. This documentation should allow
someone implementing a new replication listener to understand the
constraints and the purpose of these functions.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 4 March 2016 at 15:44, Eric Blake wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in
>> bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
>> argtype *types)
>> for(i = 0;i < 2; i++) {
>> offset = 0;
>> max_align = 1;
>> -se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>> type_ptr = se->field_types;
>> for(j = 0;j < nb_fields; j++) {
>> size = thunk_type_size(type_ptr, i);
>
> Where is the corresponding free()? g_malloc() must be paired with
> g_free(), so you need to convert both places at once.
There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.
This is worth mentioning in the commit message, but the code is
correct I think:
Reviewed-by: Peter Maydell
thanks
-- PMM
[Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation
There is a bug in ARM address translation regime with a long-descriptor
format. On the descriptor reading its address is formed from an index
which is a part of the input address. And on the first iteration this index
is incorrectly masked with 'grainsize' mask. But it can be wider according
to pseudo-code.
On the other hand on the iterations other than first the descriptor address
is formed from the previous level descriptor by masking with 'descaddrmask'
value. It always clears just 12 lower bits, but it must clear 'grainsize'
lower bits instead according to pseudo-code.
The patch fixes both cases.
Signed-off-by: Sergey Sorokin
---
target-arm/helper.c | 29 ++---
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index dec8e8b..b5f289c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7243,7 +7243,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
target_ulong address,
uint32_t tg;
uint64_t ttbr;
int ttbr_select;
-hwaddr descaddr, descmask;
+hwaddr descaddr, indexmask, indexmask_grainsize;
uint32_t tableattrs;
target_ulong page_size;
uint32_t attrs;
@@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env,
target_ulong address,
}
}
-/* Clear the vaddr bits which aren't part of the within-region address,
- * so that we don't have to special case things when calculating the
- * first descriptor address.
- */
-if (va_size != inputsize) {
-address &= (1ULL << inputsize) - 1;
-}
-
-descmask = (1ULL << (stride + 3)) - 1;
+indexmask_grainsize = (1ULL << (stride + 3)) - 1;
+indexmask = (1ULL << (inputsize - (stride * (4 - level - 1;
/* Now we can extract the actual base address from the TTBR */
descaddr = extract64(ttbr, 0, 48);
-descaddr &= ~((1ULL << (inputsize - (stride * (4 - level - 1);
+descaddr &= ~indexmask;
-/* The address field in the descriptor goes up to bit 39 for ARMv7
- * but up to bit 47 for ARMv8.
+/* The address field in the descriptor goes up to bit 39 for AArch32
+ * but up to bit 47 for AArch64.
*/
-if (arm_feature(env, ARM_FEATURE_V8)) {
-descaddrmask = 0xf000ULL;
-} else {
-descaddrmask = 0xfff000ULL;
-}
+descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
+ ~indexmask_grainsize;
/* Secure accesses start with the page table in secure memory and
* can be downgraded to non-secure at any step. Non-secure accesses
@@ -7464,7 +7454,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
target_ulong address,
uint64_t descriptor;
bool nstable;
-descaddr |= (address >> (stride * (4 - level))) & descmask;
+descaddr |= (address >> (stride * (4 - level))) & indexmask;
descaddr &= ~7ULL;
nstable = extract32(tableattrs, 4, 1);
descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
@@ -7487,6 +7477,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
target_ulong address,
*/
tableattrs |= extract64(descriptor, 59, 5);
level++;
+indexmask = indexmask_grainsize;
continue;
}
/* Block entry at level 1 or 2, or page entry at level 3.
--
1.9.3
[Qemu-devel] [PATCH v6 3/4] nvdimm acpi: let qemu handle _DSM method
If dsm memory is successfully patched, we let qemu fully emulate
the dsm method
This patch saves _DSM input parameters into dsm memory, tell dsm
memory address to QEMU, then fetch the result from the dsm memory
Signed-off-by: Xiao Guangrong
---
hw/acpi/nvdimm.c | 120 ---
1 file changed, 115 insertions(+), 5 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 90032e5..19c2642 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -372,6 +372,24 @@ static void nvdimm_build_nfit(GSList *device_list, GArray
*table_offsets,
g_array_free(structures, true);
}
+struct NvdimmDsmIn {
+uint32_t handle;
+uint32_t revision;
+uint32_t function;
+/* the remaining size in the page is used by arg3. */
+union {
+uint8_t arg3[0];
+};
+} QEMU_PACKED;
+typedef struct NvdimmDsmIn NvdimmDsmIn;
+
+struct NvdimmDsmOut {
+/* the size of buffer filled by QEMU. */
+uint32_t len;
+uint8_t data[0];
+} QEMU_PACKED;
+typedef struct NvdimmDsmOut NvdimmDsmOut;
+
static uint64_t
nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
{
@@ -411,11 +429,18 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state,
MemoryRegion *io,
static void nvdimm_build_common_dsm(Aml *dev)
{
-Aml *method, *ifctx, *function;
+Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
uint8_t byte_list[1];
-method = aml_method(NVDIMM_COMMON_DSM, 4, AML_NOTSERIALIZED);
+method = aml_method(NVDIMM_COMMON_DSM, 4, AML_SERIALIZED);
function = aml_arg(2);
+dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
+
+/*
+ * do not support any method if DSM memory address has not been
+ * patched.
+ */
+unpatched = aml_if(aml_equal(dsm_mem, aml_int(0x0)));
/*
* function 0 is called to inquire what functions are supported by
@@ -424,12 +449,38 @@ static void nvdimm_build_common_dsm(Aml *dev)
ifctx = aml_if(aml_equal(function, aml_int(0)));
byte_list[0] = 0 /* No function Supported */;
aml_append(ifctx, aml_return(aml_buffer(1, byte_list)));
-aml_append(method, ifctx);
+aml_append(unpatched, ifctx);
/* No function is supported yet. */
byte_list[0] = 1 /* Not Supported */;
-aml_append(method, aml_return(aml_buffer(1, byte_list)));
+aml_append(unpatched, aml_return(aml_buffer(1, byte_list)));
+aml_append(method, unpatched);
+/*
+ * The HDLE indicates the DSM function is issued from which device,
+ * it is not used at this time as no function is supported yet.
+ * Currently we make it always be 0 for all the devices and will set
+ * the appropriate value once real function is implemented.
+ */
+aml_append(method, aml_store(aml_int(0x0), aml_name("HDLE")));
+aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
+aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
+
+/*
+ * tell QEMU about the real address of DSM memory, then QEMU
+ * gets the control and fills the result in DSM memory.
+ */
+aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
+
+result_size = aml_local(1);
+aml_append(method, aml_store(aml_name("RLEN"), result_size));
+aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
+ result_size));
+aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
+result_size, "OBUF"));
+aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
+ aml_arg(6)));
+aml_append(method, aml_return(aml_arg(6)));
aml_append(dev, method);
}
@@ -472,7 +523,7 @@ static void nvdimm_build_nvdimm_devices(GSList
*device_list, Aml *root_dev)
static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
GArray *table_data, GArray *linker)
{
-Aml *ssdt, *sb_scope, *dev;
+Aml *ssdt, *sb_scope, *dev, *field;
int mem_addr_offset, nvdimm_ssdt;
acpi_add_table(table_offsets, table_data);
@@ -497,6 +548,65 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray
*table_offsets,
*/
aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
+/* map DSM memory and IO into ACPI namespace. */
+aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
+ aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
+ aml_name(NVDIMM_ACPI_MEM_ADDR), TARGET_PAGE_SIZE));
+
+/*
+ * DSM notifier:
+ * NTFI: write the address of DSM memory and notify QEMU to emulate
+ * the access.
+ *
+ * It is the IO port so that accessing them will cause VM-exit, the
+ * control will be transferred to QEMU.
+ */
+field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+aml_append(field, aml_nam
[Qemu-devel] [PATCH v6 1/4] nvdimm acpi: initialize the resource used by NVDIMM ACPI
32 bits IO port starting from 0x0a18 in guest is reserved for NVDIMM
ACPI emulation. The table, NVDIMM_DSM_MEM_FILE, will be patched into
NVDIMM ACPI binary code
OSPM uses this port to tell QEMU the final address of the DSM memory
and notify QEMU to emulate the DSM method
Signed-off-by: Xiao Guangrong
---
hw/acpi/Makefile.objs | 2 +-
hw/acpi/nvdimm.c| 35 +++
hw/i386/acpi-build.c| 10 +-
hw/i386/pc.c| 6 +++---
hw/i386/pc_piix.c | 5 +
hw/i386/pc_q35.c| 8 +++-
include/hw/i386/pc.h| 4 +++-
include/hw/mem/nvdimm.h | 24 +++-
8 files changed, 78 insertions(+), 16 deletions(-)
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index f3ade9a..faee86c 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o cpu_hotplug_acpi_table.o
common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
memory_hotplug_acpi_table.o
-common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
+obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
common-obj-$(CONFIG_ACPI) += acpi_interface.o
common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
common-obj-$(CONFIG_ACPI) += aml-build.o
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 49ee68e..8568b20 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -29,6 +29,7 @@
#include "qemu/osdep.h"
#include "hw/acpi/acpi.h"
#include "hw/acpi/aml-build.h"
+#include "hw/nvram/fw_cfg.h"
#include "hw/mem/nvdimm.h"
static int nvdimm_plugged_device_list(Object *obj, void *opaque)
@@ -370,6 +371,40 @@ static void nvdimm_build_nfit(GSList *device_list, GArray
*table_offsets,
g_array_free(structures, true);
}
+static uint64_t
+nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
+{
+return 0;
+}
+
+static void
+nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+}
+
+static const MemoryRegionOps nvdimm_dsm_ops = {
+.read = nvdimm_dsm_read,
+.write = nvdimm_dsm_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 4,
+.max_access_size = 4,
+},
+};
+
+void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
+FWCfgState *fw_cfg, Object *owner)
+{
+memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
+ "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
+memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
+
+state->dsm_mem = g_array_new(false, true /* clear */, 1);
+acpi_data_push(state->dsm_mem, TARGET_PAGE_SIZE);
+fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
+state->dsm_mem->len);
+}
+
#define NVDIMM_COMMON_DSM "NCAL"
static void nvdimm_build_common_dsm(Aml *dev)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f26ac9b..738985a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -39,7 +39,6 @@
#include "hw/isa/isa.h"
#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
-#include "hw/mem/nvdimm.h"
#include "sysemu/tpm.h"
#include "hw/acpi/tpm.h"
#include "sysemu/tpm_backend.h"
@@ -2624,13 +2623,6 @@ static bool acpi_has_iommu(void)
return intel_iommu && !ambiguous;
}
-static bool acpi_has_nvdimm(void)
-{
-PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
-
-return pcms->nvdimm;
-}
-
static
void acpi_build(AcpiBuildTables *tables)
{
@@ -2715,7 +2707,7 @@ void acpi_build(AcpiBuildTables *tables)
build_dmar_q35(tables_blob, tables->linker);
}
-if (acpi_has_nvdimm()) {
+if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d7fea61..d44490a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1854,14 +1854,14 @@ static bool pc_machine_get_nvdimm(Object *obj, Error
**errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
-return pcms->nvdimm;
+return pcms->acpi_nvdimm_state.is_enabled;
}
static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
-pcms->nvdimm = value;
+pcms->acpi_nvdimm_state.is_enabled = value;
}
static void pc_machine_initfn(Object *obj)
@@ -1900,7 +1900,7 @@ static void pc_machine_initfn(Object *obj)
&error_abort);
/* nvdimm is disabled on default. */
-pcms->nvdimm = false;
+pcms->acpi_nvdimm_state.is_enabled = false;
object_property_add_bool(obj, PC_MACHINE_NVDIMM, pc_machine_get_nvdimm,
pc_machine_set_nvdimm, &error_abort);
}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6f8c2cd..6a69b23 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -27
[Qemu-devel] [PATCH v6 2/4] nvdimm acpi: introduce patched dsm memory
The dsm memory is used to save the input parameters and store
the dsm result which is filled by QEMU.
The address of dsm memory is decided by bios and patched into
int32 object named "MEMA"
Signed-off-by: Xiao Guangrong
---
hw/acpi/nvdimm.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 8568b20..90032e5 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -29,6 +29,7 @@
#include "qemu/osdep.h"
#include "hw/acpi/acpi.h"
#include "hw/acpi/aml-build.h"
+#include "hw/acpi/bios-linker-loader.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/mem/nvdimm.h"
@@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state,
MemoryRegion *io,
}
#define NVDIMM_COMMON_DSM "NCAL"
+#define NVDIMM_ACPI_MEM_ADDR "MEMA"
static void nvdimm_build_common_dsm(Aml *dev)
{
@@ -471,6 +473,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray
*table_offsets,
GArray *table_data, GArray *linker)
{
Aml *ssdt, *sb_scope, *dev;
+int mem_addr_offset, nvdimm_ssdt;
acpi_add_table(table_offsets, table_data);
@@ -500,13 +503,24 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray
*table_offsets,
nvdimm_build_nvdimm_devices(device_list, dev);
aml_append(sb_scope, dev);
-
aml_append(ssdt, sb_scope);
+
+nvdimm_ssdt = table_data->len;
+
/* copy AML table into ACPI tables blob and patch header there */
g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+mem_addr_offset = build_append_named_dword(table_data,
+ NVDIMM_ACPI_MEM_ADDR);
+
+bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
+ false /* high memory */);
+bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+ NVDIMM_DSM_MEM_FILE, table_data,
+ table_data->data + mem_addr_offset,
+ sizeof(uint32_t));
build_header(linker, table_data,
-(void *)(table_data->data + table_data->len - ssdt->buf->len),
-"SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
+(void *)(table_data->data + nvdimm_ssdt),
+"SSDT", table_data->len - nvdimm_ssdt, 1, NULL, "NVDIMM");
free_aml_allocator();
}
--
1.8.3.1
[Qemu-devel] [PATCH v6 4/4] nvdimm acpi: emulate dsm method
Emulate dsm method after IO VM-exit
Currently, we only introduce the framework and no function is actually
supported
Signed-off-by: Xiao Guangrong
---
hw/acpi/nvdimm.c| 56 +
include/hw/mem/nvdimm.h | 8 +++
2 files changed, 64 insertions(+)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 19c2642..d9048c0 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -390,15 +390,71 @@ struct NvdimmDsmOut {
} QEMU_PACKED;
typedef struct NvdimmDsmOut NvdimmDsmOut;
+struct NvdimmDsmFunc0Out {
+/* the size of buffer filled by QEMU. */
+ uint32_t len;
+ uint32_t supported_func;
+} QEMU_PACKED;
+typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;
+
+struct NvdimmDsmFuncNoPayloadOut {
+/* the size of buffer filled by QEMU. */
+ uint32_t len;
+ uint32_t func_ret_status;
+} QEMU_PACKED;
+typedef struct NvdimmDsmFuncNoPayloadOut NvdimmDsmFuncNoPayloadOut;
+
static uint64_t
nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
{
+nvdimm_debug("BUG: we never read _DSM IO Port.\n");
return 0;
}
static void
nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
+NvdimmDsmIn *in;
+hwaddr dsm_mem_addr = val;
+
+nvdimm_debug("dsm memory address %#lx.\n", dsm_mem_addr);
+
+/*
+ * The DSM memory is mapped to guest address space so an evil guest
+ * can change its content while we are doing DSM emulation. Avoid
+ * this by copying DSM memory to QEMU local memory.
+ */
+in = g_malloc(TARGET_PAGE_SIZE);
+cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
+
+le32_to_cpus(&in->revision);
+le32_to_cpus(&in->function);
+le32_to_cpus(&in->handle);
+
+nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
+ in->handle, in->function);
+
+/*
+ * function 0 is called to inquire which functions are supported by
+ * OSPM
+ */
+if (in->function == 0) {
+NvdimmDsmFunc0Out func0 = {
+.len = cpu_to_le32(sizeof(func0)),
+ /* No function supported other than function 0 */
+.supported_func = cpu_to_le32(0),
+};
+cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);
+} else {
+/* No function except function 0 is supported yet. */
+NvdimmDsmFuncNoPayloadOut out = {
+.len = cpu_to_le32(sizeof(out)),
+.func_ret_status = cpu_to_le32(1) /* Not Supported */,
+};
+cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
+}
+
+g_free(in);
}
static const MemoryRegionOps nvdimm_dsm_ops = {
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 26d5b78..517de9c 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,6 +25,14 @@
#include "hw/mem/pc-dimm.h"
+#define NVDIMM_DEBUG 0
+#define nvdimm_debug(fmt, ...)\
+do { \
+if (NVDIMM_DEBUG) { \
+fprintf(stderr, "nvdimm: " fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
#define TYPE_NVDIMM "nvdimm"
#define NVDIMM_DSM_MEM_FILE "etc/acpi/nvdimm-mem"
--
1.8.3.1
[Qemu-devel] [PATCH v6 0/4] NVDIMM ACPI: introduce the framework of QEMU emulated DSM
This patchset is against commit f6547bcd9a1b6 (fw-cfg: support writeable blobs) on pci branch of Michael's git tree and can be found at: https://github.com/xiaogr/qemu.git nvdimm-acpi-v6 Michael, two suggestions from you was not followed in this version, one is using ssdt->buf->len to indicate the payload of SSDT and another is dropping extra () when we build ARG3. I have replied it in the old thread to explain why we can not. Sorry again for i did not spot it when i reply your mail in the first time. Changelog in v6: This version addresses the comments from Michael: - move the comments belong near field in AcpiNVDIMMState - improve the comment to explain why HDLE is set to 0 for all the devices - fix some coding styles and improve other comments - drop the _CRS as it is not designed for reserved memory Changelog in v5: Thanks to Michael's review, the changes in this version are: - use nvdimm_debug() instead of fprintf() on the path triggered by read access of IO port - introduce the data struct to represent output information to drop the open-code - improve the comment to better explain the fact that currently no function other than function 0 is supported Changelog in v4: - drop the unnecessary assert() in aml_concatenate() based on Igor's suggestion - introduce build_append_named_dword() and use it to simplify the code as Michael's suggestion Changelog in v3: Changes addressing Michael's comment: - rebase the patchset against current code Changes addressing Igor's comment: - rename the parameters of aml_create_field() to reflect the ACPI spec - fix the issue that the @target operand can not be optional in aml_concatenate() that is also cleaned up by using build_opcode_2arg_dst() Others: - separate the test patches to the single set and will be posted on later These changes are based on Igor's comments: - drop ssdt.rev2 support as the memory address allocated by BIOS/OVMF are always 32 bits - support to test NVDIMM tables (NFIT and NVDIMM SSDT) - add _CRS to report its operation region - make AML APIs change be the separated patches This is the second part of vNVDIMM implementation which implements the BIOS patched dsm memory and introduces the framework that allows QEMU to emulate DSM method Thanks to Michael's idea, we do not reserve any memory for NVDIMM ACPI, instead we let BIOS allocate the memory and patch the address to the offset we want IO port is still enabled as it plays as the way to notify QEMU and pass the patched dsm memory address, so that IO port region, 0x0a18 - 0xa20, is reserved and it is divided into two 32 bits ports and used to pass the low 32 bits and high 32 bits of dsm memory address to QEMU Thanks Igor's idea, this patchset also extends DSDT/SSDT to revision 2 to apply 64 bit operations, in order to keeping compatibility, old version (<= 2.5) still uses revision 1. Since 64 bit operations breaks old guests (such as windows XP), we should keep the 64 bits stuff in the private place where common ACPI operation does not touch it Xiao Guangrong (4): nvdimm acpi: initialize the resource used by NVDIMM ACPI nvdimm acpi: introduce patched dsm memory nvdimm acpi: let qemu handle _DSM method nvdimm acpi: emulate dsm method hw/acpi/Makefile.objs | 2 +- hw/acpi/nvdimm.c| 231 ++-- hw/i386/acpi-build.c| 10 +-- hw/i386/pc.c| 6 +- hw/i386/pc_piix.c | 5 ++ hw/i386/pc_q35.c| 8 +- include/hw/i386/pc.h| 4 +- include/hw/mem/nvdimm.h | 32 ++- 8 files changed, 274 insertions(+), 24 deletions(-) -- 1.8.3.1
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 03/04/2016 08:49 AM, Eric Blake wrote: > Oh, I see you DID include a S-o-b, but in the subject line of the patch > instead of the commit body. Which still needs work. You need a > one-line summary as the subject, not your S-o-b, so the commit message > should look more like: > > thunk: Replace malloc with g_malloc() > > This replacement was suggested as part of the bite-sized tasks. > > Signed-off-by: Sarah Khan Oh, and I'd be remiss if I did not add: This looks like your first contribution. Welcome to the QEMU community, and hope you enjoy it here! Keep in mind that email is a bad medium for expressing emotion, and that reviewers tend to point out flaws without looking for the good; but in reality, we are grateful when someone steps up to help with the bite-sized tasks. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCHv9 0/10] slirp: Adding IPv6 support to Qemu -net user mode
On 2016-03-04 09:41, Thomas Huth wrote: > On 22.02.2016 20:28, Samuel Thibault wrote: >> Hello, >> >> This is another respin of IPv6 in Qemu -net user mode. >> >> These patches add ICMPv6, NDP, make UDP and TCP compatible with IPv6, and add >> TFTP over IPv6. > > *ping* > > Jan, Jason, > > could you please have a look at this series? Would be cool to include it > for 2.6 so that we'd finally have IPv6 support in Slirp, too. > As far as I could see, the patches look fine now - there was just one > rather cosmetic issue left in patch 9/10, but it should also be ok in > the current shape already, I think, so IMHO no need for a respin. As indicated before, we need someone else than me to manage the slirp system. I'm out of bandwidth for this task, sorry. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
> > > > > > Only detect the unmapped/zero mapped pages is not enough.
> > > Consider
> > > > > the
> > > > > > situation like case 2, it can't achieve the same result.
> > > > >
> > > > > Your case 2 doesn't exist in the real world. If people could
> > > > > stop their main memory consumer in the guest prior to migration
> > > > > they wouldn't need live migration at all.
> > > >
> > > > The case 2 is just a simplified scenario, not a real case.
> > > > As long as the guest's memory usage does not keep increasing, or
> > > > not always run out, it can be covered by the case 2.
> > >
> > > The memory usage will keep increasing due to ever growing caches,
> > > etc, so you'll be left with very little free memory fairly soon.
> > >
> >
> > I don't think so.
>
> Here's my laptop:
> KiB Mem : 16048560 total, 8574956 free, 3360532 used, 4113072 buff/cache
>
> But here's a server:
> KiB Mem: 32892768 total, 20092812 used, 12799956 free, 368704 buffers
>
> What is the difference? A ton of tiny daemons not doing anything, staying
> resident in memory.
>
> > > > > I tend to think you can safely assume there's no free memory in
> > > > > the guest, so there's little point optimizing for it.
> > > >
> > > > If this is true, we should not inflate the balloon either.
> > >
> > > We certainly should if there's "available" memory, i.e. not free but
> > > cheap to reclaim.
> > >
> >
> > What's your mean by "available" memory? if they are not free, I don't think
> it's cheap.
>
> clean pages are cheap to drop as they don't have to be written.
> whether they will be ever be used is another matter.
>
> > > > > OTOH it makes perfect sense optimizing for the unmapped memory
> > > > > that's made up, in particular, by the ballon, and consider
> > > > > inflating the balloon right before migration unless you already
> > > > > maintain it at the optimal size for other reasons (like e.g. a
> > > > > global resource manager
> > > optimizing the VM density).
> > > > >
> > > >
> > > > Yes, I believe the current balloon works and it's simple. Do you
> > > > take the
> > > performance impact for consideration?
> > > > For and 8G guest, it takes about 5s to inflating the balloon. But
> > > > it only takes 20ms to traverse the free_list and construct the
> > > > free pages
> > > bitmap.
> > >
> > > I don't have any feeling of how important the difference is. And if
> > > the limiting factor for balloon inflation speed is the granularity
> > > of communication it may be worth optimizing that, because quick
> > > balloon reaction may be important in certain resource management
> scenarios.
> > >
> > > > By inflating the balloon, all the guest's pages are still be
> > > > processed (zero
> > > page checking).
> > >
> > > Not sure what you mean. If you describe the current state of
> > > affairs that's exactly the suggested optimization point: skip unmapped
> pages.
> > >
> >
> > You'd better check the live migration code.
>
> What's there to check in migration code?
> Here's the extent of what balloon does on output:
>
>
> while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4)
> {
> ram_addr_t pa;
> ram_addr_t addr;
> int p = virtio_ldl_p(vdev, &pfn);
>
> pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> offset += 4;
>
> /* FIXME: remove get_system_memory(), but how? */
> section = memory_region_find(get_system_memory(), pa, 1);
> if (!int128_nz(section.size) || !memory_region_is_ram(section.mr))
> continue;
>
>
> trace_virtio_balloon_handle_output(memory_region_name(section.mr),
>pa);
> /* Using memory_region_get_ram_ptr is bending the rules a bit, but
>should be OK because we only want a single page. */
> addr = section.offset_within_region;
> balloon_page(memory_region_get_ram_ptr(section.mr) + addr,
> !!(vq == s->dvq));
> memory_region_unref(section.mr);
> }
>
> so all that happens when we get a page is balloon_page.
> and
>
> static void balloon_page(void *addr, int deflate) { #if defined(__linux__)
> if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
> kvm_has_sync_mmu())) {
> qemu_madvise(addr, TARGET_PAGE_SIZE,
> deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
> }
> #endif
> }
>
>
> Do you see anything that tracks pages to help migration skip the ballooned
> memory? I don't.
>
No. And it's exactly what I mean. The ballooned memory is still processed during
live migration without skipping. The live migration code is in migration/ram.c.
>
> > > > The only advantage of ' inflating the balloon before live
> > > > migration' is simple,
> > > nothing more.
> > >
> > > That's a big advantage. Another one is that it does something
> > > useful in real-
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 03/04/2016 08:44 AM, Eric Blake wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in
>> bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
>> argtype *types)
>> for(i = 0;i < 2; i++) {
>> offset = 0;
>> max_align = 1;
>> -se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>> type_ptr = se->field_types;
>> for(j = 0;j < nb_fields; j++) {
>> size = thunk_type_size(type_ptr, i);
>
> Where is the corresponding free()? g_malloc() must be paired with
> g_free(), so you need to convert both places at once.
>
> Also, your patch is missing a Signed-off-by designation; without that,
> we can't accept it.
Oh, I see you DID include a S-o-b, but in the subject line of the patch
instead of the commit body. Which still needs work. You need a
one-line summary as the subject, not your S-o-b, so the commit message
should look more like:
thunk: Replace malloc with g_malloc()
This replacement was suggested as part of the bite-sized tasks.
Signed-off-by: Sarah Khan
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
On 03/04/2016 07:53 AM, Sarah Khan wrote:
> This patch replaces malloc() with g_malloc() as stated to be done in
> bitesized task
>
> diff --git a/thunk.c b/thunk.c
> index f057d86..bddabae 100644
> --- a/thunk.c
> +++ b/thunk.c
> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
> argtype *types)
> for(i = 0;i < 2; i++) {
> offset = 0;
> max_align = 1;
> -se->field_offsets[i] = malloc(nb_fields * sizeof(int));
> +se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
> type_ptr = se->field_types;
> for(j = 0;j < nb_fields; j++) {
> size = thunk_type_size(type_ptr, i);
Where is the corresponding free()? g_malloc() must be paired with
g_free(), so you need to convert both places at once.
Also, your patch is missing a Signed-off-by designation; without that,
we can't accept it. More hints at:
http://wiki.qemu.org/Contribute/SubmitAPatch
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1#n297
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3.5 09/10] chardev: Drop useless ChardevDummy type
Commit d0d7708b made ChardevDummy be an empty wrapper type around
ChardevCommon. But there is no technical reason for this indirection,
so simplify the code by directly using the base type.
Also change the fallback assignment to assign u.null rather than
u.data, since a future patch will remove the data member of the C
struct generated for QAPI unions.
Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrange
---
v3.5: one-liner fix to mingw compile
v3: no change
v2: add R-b
---
backends/baum.c| 2 +-
backends/msmouse.c | 2 +-
qemu-char.c| 8
qapi-schema.json | 15 ++-
4 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 374562a..c11320e 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -567,7 +567,7 @@ static CharDriverState *chr_baum_init(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.braille);
+ChardevCommon *common = backend->u.braille;
BaumDriverState *baum;
CharDriverState *chr;
brlapi_handle_t *handle;
diff --git a/backends/msmouse.c b/backends/msmouse.c
index 9a82efd..5e1833c 100644
--- a/backends/msmouse.c
+++ b/backends/msmouse.c
@@ -68,7 +68,7 @@ static CharDriverState *qemu_chr_open_msmouse(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.msmouse);
+ChardevCommon *common = backend->u.msmouse;
CharDriverState *chr;
chr = qemu_chr_alloc(common, errp);
diff --git a/qemu-char.c b/qemu-char.c
index af31102..e0147f3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -420,7 +420,7 @@ static CharDriverState *qemu_chr_open_null(const char *id,
Error **errp)
{
CharDriverState *chr;
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.null);
+ChardevCommon *common = backend->u.null;
chr = qemu_chr_alloc(common, errp);
if (!chr) {
@@ -1366,7 +1366,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
PtyCharDriver *s;
int master_fd, slave_fd;
char pty_name[PATH_MAX];
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.pty);
+ChardevCommon *common = backend->u.pty;
master_fd = qemu_openpty_raw(&slave_fd, pty_name);
if (master_fd < 0) {
@@ -2183,7 +2183,7 @@ static CharDriverState *qemu_chr_open_win_con(const char
*id,
ChardevReturn *ret,
Error **errp)
{
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console);
+ChardevCommon *common = backend->u.console;
return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE),
common, errp);
}
@@ -3817,7 +3817,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
} else {
ChardevCommon *cc = g_new0(ChardevCommon, 1);
qemu_chr_parse_common(opts, cc);
-backend->u.data = cc;
+backend->u.null = cc; /* Any ChardevCommon member would work */
}
ret = qmp_chardev_add(bid ? bid : id, backend, errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index 42fd61b..362c9d8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3323,23 +3323,20 @@
#
# Since: 1.4 (testdev since 2.2)
##
-{ 'struct': 'ChardevDummy', 'data': { },
- 'base': 'ChardevCommon' }
-
{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
'serial' : 'ChardevHostdev',
'parallel': 'ChardevHostdev',
'pipe' : 'ChardevHostdev',
'socket' : 'ChardevSocket',
'udp': 'ChardevUdp',
- 'pty': 'ChardevDummy',
- 'null' : 'ChardevDummy',
+ 'pty': 'ChardevCommon',
+ 'null' : 'ChardevCommon',
'mux': 'ChardevMux',
- 'msmouse': 'ChardevDummy',
- 'braille': 'ChardevDummy',
- 'testdev': 'ChardevDummy',
+ 'msmouse': 'ChardevCommon',
+ 'braille': 'ChardevCommon',
+ 'testdev': 'ChardevCommon',
'stdio' : 'ChardevStdio',
- 'console': 'ChardevDummy',
+ 'console': 'ChardevCommon',
Re: [Qemu-devel] [PATCH v5 2/5] nvdimm acpi: introduce patched dsm memory
On 03/03/2016 09:12 PM, Michael S. Tsirkin wrote: /* copy AML table into ACPI tables blob and patch header there */ g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len); +mem_addr_offset = build_append_named_dword(table_data, + NVDIMM_ACPI_MEM_ADDR); + +bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE, + false /* high memory */); +bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, + NVDIMM_DSM_MEM_FILE, table_data, + table_data->data + mem_addr_offset, + sizeof(uint32_t)); build_header(linker, table_data, -(void *)(table_data->data + table_data->len - ssdt->buf->len), -"SSDT", ssdt->buf->len, 1, NULL, "NVDIMM"); +(void *)(table_data->data + nvdimm_ssdt), +"SSDT", table_data->len - nvdimm_ssdt, 1, NULL, "NVDIMM"); free_aml_allocator(); I prefer ssdt->buf->len to table_data->len - nvdimm_ssdt. Pls fix by a follow-up patch unless there is a respin. Ah, we can not do that as the NVDIMM_ACPI_MEM_ADDR is appended in the table which is not taken into account in ssdt. Sorry, i just spotted it when i was addressing all your comments in the new version. :(
Re: [Qemu-devel] [PATCH v3 09/10] chardev: Drop useless ChardevDummy type
On 03/04/2016 07:40 AM, Markus Armbruster wrote: > Eric Blake writes: > >> Commit d0d7708b made ChardevDummy be an empty wrapper type around >> ChardevCommon. But there is no technical reason for this indirection, >> so simplify the code by directly using the base type. >> >> Also change the fallback assignment to assign u.null rather than >> u.data, since a future patch will remove the data member of the C >> struct generated for QAPI unions. >> >> Signed-off-by: Eric Blake >> Reviewed-by: Daniel P. Berrange > > Doesn't compile with MinGW: > > qemu/qemu-char.c: In function 'qemu_chr_open_win_con': > qemu/qemu-char.c:2186:29: warning: implicit declaration of function > 'qapi_ChardevDummy_base' [-Wimplicit-function-declaration] > ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console); > ^ The curse of not compiling all the backends, and failing to grep for ChardevDummy. Replacement patch coming up soon. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults
On 4 March 2016 at 15:03, Sergey Sorokin wrote: > Do we really need to rearrange the code? > In pseudo-code the level is set explicitly as well, in case of a fault. That's why when I originally brought the point up I said "but I'm not sure it's worth rearranging that code"... If you want to neaten things up so that we pick the initial level for faults (0 or 1 depending on va_size) in one place, then you need to rearrange the code. If you don't want to rearrange that bit of code you can leave it all as it is, but you don't get the neat "only one place makes this decision" structure. You could argue either way and I don't much mind which we do. (Incidentally the pseudocode is not always a good guide for the structure of code; sometimes it makes clear choices, but sometimes it makes confusing choices.) thanks -- PMM
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
> > Maybe I am not clear enough.
> >
> > I mean if we inflate balloon before live migration, for a 8GB guest, it
> > takes
> about 5 Seconds for the inflating operation to finish.
>
> And these 5 seconds are spent where?
>
The time is spent on allocating the pages and send the allocated pages pfns to
QEMU
through virtio.
> > For the PV solution, there is no need to inflate balloon before live
> > migration, the only cost is to traversing the free_list to construct
> > the free pages bitmap, and it takes about 20ms for a 8GB idle guest( less if
> there is less free pages), passing the free pages info to host will take
> about
> extra 3ms.
> >
> >
> > Liang
>
> So now let's please stop talking about solutions at a high level and discuss
> the
> interface changes you make in detail.
> What makes it faster? Better host/guest interface? No need to go through
> buddy allocator within guest? Less interrupts? Something else?
>
I assume you are familiar with the current virtio-balloon and how it works.
The new interface is very simple, send a request to the virtio-balloon driver,
The virtio-driver will travers the '&zone->free_area[order].free_list[t])' to
construct a 'free_page_bitmap', and then the driver will send the content
of 'free_page_bitmap' back to QEMU. That all the new interface does and
there are no ' alloc_page' related affairs, so it's faster.
Some code snippet:
--
+static void mark_free_pages_bitmap(struct zone *zone,
+unsigned long *free_page_bitmap, unsigned long pfn_gap) {
+ unsigned long pfn, flags, i;
+ unsigned int order, t;
+ struct list_head *curr;
+
+ if (zone_is_empty(zone))
+ return;
+
+ spin_lock_irqsave(&zone->lock, flags);
+
+ for_each_migratetype_order(order, t) {
+ list_for_each(curr, &zone->free_area[order].free_list[t]) {
+
+ pfn = page_to_pfn(list_entry(curr, struct page, lru));
+ for (i = 0; i < (1UL << order); i++) {
+ if ((pfn + i) >= PFN_4G)
+ set_bit_le(pfn + i - pfn_gap,
+ free_page_bitmap);
+ else
+ set_bit_le(pfn + i, free_page_bitmap);
+ }
+ }
+ }
+
+ spin_unlock_irqrestore(&zone->lock, flags); }
Sorry for my poor English and expression, if you still can't understand,
you could glance at the patch, total about 400 lines.
>
> > > --
> > > MST
Re: [Qemu-devel] [PATCH v2 1/2] trace: include CPU index in trace_memory_region_*()
On Wed, Mar 02, 2016 at 12:12:54PM -0800, Hollis Blanchard wrote:
> Knowing which CPU performed an action is essential for understanding SMP guest
> behavior.
>
> However, cpu_physical_memory_rw() may be executed by a machine init function,
> before any VCPUs are running, when there is no CPU running ('current_cpu' is
> NULL). In this case, store -1 in the trace record as the CPU index. Trace
> analysis tools may need to be aware of this special case.
>
> Signed-off-by: Hollis Blanchard
> ---
> v2: use get_cpu_index() helper function
> ---
> memory.c | 32
> trace-events | 8
> 2 files changed, 24 insertions(+), 16 deletions(-)
Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 3/5] nvdimm acpi: let qemu handle _DSM method
On 03/03/2016 09:23 PM, Michael S. Tsirkin wrote:
+ *
+ * They are RAM mapping on host so that these accesses never cause
+ * VM-EXIT.
+ */
+field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+aml_append(field, aml_named_field("HDLE",
+ sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
+aml_append(field, aml_named_field("REVS",
+ sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
+aml_append(field, aml_named_field("FUNC",
+ sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
+aml_append(field, aml_named_field("ARG3",
+ (TARGET_PAGE_SIZE - offsetof(NvdimmDsmIn, arg3)) *
+ BITS_PER_BYTE));
drop the extra () here please, and align BITS_PER_BYTE with
TARGET_PAGE_SIZE.
Michael, the () is necessary here, as it is calculating the number of bits:
(PAGE_SIZE - offset_of_arg3) * BITS_PER_BYTE
Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults
03.03.2016, 19:54, "Peter Maydell" : > On 3 March 2016 at 16:37, Sergey Fedorov wrote: >> On 03.03.2016 17:55, Peter Maydell wrote: >>> Yes, this patch is definitely fixing a bug; I'm just mentioning that other >>> code path because it seems to be the result of previously fixing the bug >>> for a particular special case... >> >> Ah, right, I think I understand you :) So we'd better remove these lines: >> >> /* AArch64 reports these as level 0 faults. >> * AArch32 reports these as level 1 faults. >> */ >> level = va_size == 64 ? 0 : 1; >> fault_type = translation_fault; > > Those lines come after some code which has set level to something > else, so you'd need to rearrange that code a bit so it didn't > set level before it had determined that there wasn't a fault. > > thanks > -- PMM Do we really need to rearrange the code? In pseudo-code the level is set explicitly as well, in case of a fault.
Re: [Qemu-devel] [PATCH v5 0/4] Extend TPM support with a QEMU-external
Hi All, I was wondering if there was any additional traction on this patch? I'm toward performing automated testing against the swtpm package and it would be ideal to not need to patch and recompile QEMU on all of my test systems. Thanks, Trevor -- Trevor Vaughan Vice President, Onyx Point, Inc (410) 541-6699 -- This account not approved for unencrypted proprietary information --
[Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan
This patch replaces malloc() with g_malloc() as stated to be done in bitesized
task
diff --git a/thunk.c b/thunk.c
index f057d86..bddabae 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
argtype *types)
for(i = 0;i < 2; i++) {
offset = 0;
max_align = 1;
-se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
type_ptr = se->field_types;
for(j = 0;j < nb_fields; j++) {
size = thunk_type_size(type_ptr, i);
---
thunk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/thunk.c b/thunk.c
index f057d86..bddabae 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const
argtype *types)
for(i = 0;i < 2; i++) {
offset = 0;
max_align = 1;
-se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
type_ptr = se->field_types;
for(j = 0;j < nb_fields; j++) {
size = thunk_type_size(type_ptr, i);
--
1.9.1
Re: [Qemu-devel] [PATCH v2 0/3] add missing virtio aliases
"Michael S. Tsirkin" writes: > On Thu, Feb 18, 2016 at 10:44:11PM +0100, Sascha Silbe wrote: >> Split out from the series fixing qemu-iotest 140 on s390x [1]. >> >> This series makes crafting qemu command lines that work across >> multiple (virtio-supporting) platforms at lot easier. >> >> Changes since the original series: >> - included patch to improve the error message when the target device >> class of an alias doesn't exist >> - added comment about keeping alias table sorted >> - covering all non-abstract virtio device classes rather than just >> those already implemented on s390x >> >> Sascha Silbe (3): >> qdev-monitor: improve error message when alias device is unavailable >> qdev-monitor: sort alias table by typename >> qdev-monitor: add missing aliases for virtio device classes >> >> qdev-monitor.c | 42 ++ >> 1 file changed, 34 insertions(+), 8 deletions(-) >> >> [1] mid:1455023713-104799-1-git-send-email-si...@linux.vnet.ibm.com >> "[PATCH 0/3] qemu-iotests: fix 140 on s390x" by Sascha Silbe >> on 2016-02-09 > > I don't see issues here: > > Reviewed-by: Michael S. Tsirkin > > You should also Cc lcapitul...@redhat.com and ebl...@redhat.com. Cc'ing them now. > Markus, will you take this series? qdev-monitor falls in the crack between monitor (maintained by me) and qdev (no maintainer, which is a bloody shame). I can take it through my tree.
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
On Fri, Mar 04, 2016 at 02:26:49PM +, Li, Liang Z wrote:
> > Subject: Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration
> > optimization
> >
> > On Fri, Mar 04, 2016 at 09:08:44AM +, Li, Liang Z wrote:
> > > > On Fri, Mar 04, 2016 at 01:52:53AM +, Li, Liang Z wrote:
> > > > > > I wonder if it would be possible to avoid the kernel changes
> > > > > > by parsing /proc/self/pagemap - if that can be used to detect
> > > > > > unmapped/zero mapped pages in the guest ram, would it achieve
> > > > > > the
> > > > same result?
> > > > >
> > > > > Only detect the unmapped/zero mapped pages is not enough.
> > Consider
> > > > the
> > > > > situation like case 2, it can't achieve the same result.
> > > >
> > > > Your case 2 doesn't exist in the real world. If people could stop
> > > > their main memory consumer in the guest prior to migration they
> > > > wouldn't need live migration at all.
> > >
> > > The case 2 is just a simplified scenario, not a real case.
> > > As long as the guest's memory usage does not keep increasing, or not
> > > always run out, it can be covered by the case 2.
> >
> > The memory usage will keep increasing due to ever growing caches, etc, so
> > you'll be left with very little free memory fairly soon.
> >
>
> I don't think so.
Here's my laptop:
KiB Mem : 16048560 total, 8574956 free, 3360532 used, 4113072 buff/cache
But here's a server:
KiB Mem: 32892768 total, 20092812 used, 12799956 free, 368704 buffers
What is the difference? A ton of tiny daemons not doing anything,
staying resident in memory.
> > > > I tend to think you can safely assume there's no free memory in the
> > > > guest, so there's little point optimizing for it.
> > >
> > > If this is true, we should not inflate the balloon either.
> >
> > We certainly should if there's "available" memory, i.e. not free but cheap
> > to
> > reclaim.
> >
>
> What's your mean by "available" memory? if they are not free, I don't think
> it's cheap.
clean pages are cheap to drop as they don't have to be written.
whether they will be ever be used is another matter.
> > > > OTOH it makes perfect sense optimizing for the unmapped memory
> > > > that's made up, in particular, by the ballon, and consider inflating
> > > > the balloon right before migration unless you already maintain it at
> > > > the optimal size for other reasons (like e.g. a global resource manager
> > optimizing the VM density).
> > > >
> > >
> > > Yes, I believe the current balloon works and it's simple. Do you take the
> > performance impact for consideration?
> > > For and 8G guest, it takes about 5s to inflating the balloon. But it
> > > only takes 20ms to traverse the free_list and construct the free pages
> > bitmap.
> >
> > I don't have any feeling of how important the difference is. And if the
> > limiting factor for balloon inflation speed is the granularity of
> > communication
> > it may be worth optimizing that, because quick balloon reaction may be
> > important in certain resource management scenarios.
> >
> > > By inflating the balloon, all the guest's pages are still be processed
> > > (zero
> > page checking).
> >
> > Not sure what you mean. If you describe the current state of affairs that's
> > exactly the suggested optimization point: skip unmapped pages.
> >
>
> You'd better check the live migration code.
What's there to check in migration code?
Here's the extent of what balloon does on output:
while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
ram_addr_t pa;
ram_addr_t addr;
int p = virtio_ldl_p(vdev, &pfn);
pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
offset += 4;
/* FIXME: remove get_system_memory(), but how? */
section = memory_region_find(get_system_memory(), pa, 1);
if (!int128_nz(section.size) || !memory_region_is_ram(section.mr))
continue;
trace_virtio_balloon_handle_output(memory_region_name(section.mr),
pa);
/* Using memory_region_get_ram_ptr is bending the rules a bit, but
should be OK because we only want a single page. */
addr = section.offset_within_region;
balloon_page(memory_region_get_ram_ptr(section.mr) + addr,
!!(vq == s->dvq));
memory_region_unref(section.mr);
}
so all that happens when we get a page is balloon_page.
and
static void balloon_page(void *addr, int deflate)
{
#if defined(__linux__)
if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
kvm_has_sync_mmu())) {
qemu_madvise(addr, TARGET_PAGE_SIZE,
deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
}
#endif
}
Do you see anything that tracks pages to help migration skip
the ballooned memory? I don't.
> > > The only advantage of ' inflatin
Re: [Qemu-devel] [PATCH v3 09/10] chardev: Drop useless ChardevDummy type
Eric Blake writes: > Commit d0d7708b made ChardevDummy be an empty wrapper type around > ChardevCommon. But there is no technical reason for this indirection, > so simplify the code by directly using the base type. > > Also change the fallback assignment to assign u.null rather than > u.data, since a future patch will remove the data member of the C > struct generated for QAPI unions. > > Signed-off-by: Eric Blake > Reviewed-by: Daniel P. Berrange Doesn't compile with MinGW: qemu/qemu-char.c: In function 'qemu_chr_open_win_con': qemu/qemu-char.c:2186:29: warning: implicit declaration of function 'qapi_ChardevDummy_base' [-Wimplicit-function-declaration] ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console); ^ qemu/qemu-char.c:2186:5: warning: nested extern declaration of 'qapi_ChardevDummy_base' [-Wnested-externs] ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console); ^ qemu/qemu-char.c:2186:29: warning: initialization makes pointer from integer without a cast [-Wint-conversion] ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console); ^
Re: [Qemu-devel] [PATCH v2 19/19] qapi: Make c_type() more OO-like
On 03/03/2016 06:29 AM, Markus Armbruster wrote: > Eric Blake writes: > >> QAPISchemaType.c_type() was a bit awkward. Rather than having two >> optional orthogonal boolean flags that should never both be true, >> and where all callers pass a compile-time constant, provide three >> different method names that can be overridden as needed, and where >> the caller just uses the right variant. It requires slightly more >> Python, but is arguably easier to read. >> >> + >> +# Use of a type in a struct declaration >> +def c_unboxed_type(self): >> +return self.c_type() > > This one I find bit problematic. We only ever box object types, but > those we box almost everywhere. This method gets used in the select > places where we don't box them (currently just one). I'm afraid "used > in a struct declaration" isn't a good definition. > > I initially suggested to define c_unboxed_type() only for > QAPISchemaObjectType because there boxed vs. unboxed makes sense. > However, your code would like to call it for arbitrary types. qapi-types.py is calling it for arbitrary types thanks to QAPI alternates. If I didn't put it here, then the code there needs to special-case for isinstance(type, QAPISchemaObjectType) before calling .c_unboxed_type(), and fall back to .c_type() otherwise. > > Perhaps we can cure my belly-ache with nothing more than carefully > drafted function contracts. Let me try. > > # Return the C type for common use. > # For the types we commonly box, this is a pointer type. > def c_type(self): > > # Return the C type to be used in a parameter list. > def c_param_type(self): > > # Return the C type to be used where we suppress boxing. > def c_unboxed_type(self): Sure, that helps. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 18/19] qapi: Use anonymous base in CpuInfo
On 03/03/2016 06:08 AM, Markus Armbruster wrote: > Eric Blake writes: > >> Now that the generator supports it, we might as well use an >> anonymous base rather than breaking out a single-use CpuInfoBase >> structure. >> >> Signed-off-by: Eric Blake > > Again, introspection value remains the same. > > Patch looks good. > > Should we eliminate more base types? At this point, we only have one other: BlockdevOptions. Yes, I'll add it to the list of cleanups. I also have patches posted in the "subset F" series that converts 'netdev_add' to be introspectible, and which also uses the anonymous base. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 16/19] qapi: Allow anonymous base for flat union
On 03/03/2016 06:04 AM, Markus Armbruster wrote:
> Eric Blake writes:
>
>> Rather than requiring all flat unions to explicitly create
>> a separate base struct, we can allow the qapi schema to specify
>> the common members via an inline dictionary. This is similar to
>> how commands can specify an inline anonymous type for its 'data',
>
> Suggest to end the sentence here, and then...
>
>> and matches the fact that our code base already has several
>> flat unions that had to create a separate base type that is used
>> nowhere but in the union.
>
> "We already have several struct types that only exist to serve as a
> single flat union's base. The next commits will clean them up."
>
> Replace "them" by "some" if you don't clean them all up.
>
> It's a nice step towards having a variant record type in the schema
> language similar to what we have in introspection.
>
>> @@ -63,7 +62,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
>> *obj, Error **errp)
>> c_name=c_name(name))
>>
>> if base:
>> -ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
>> +ret += gen_visit_members_call(base, 'qapi_%s_base(obj)' %
>> c_name(name),
>
> I started at this for several minutes until I could guess what's going
> on here.
>
> The old code works fine when the type isn't implicit.
>
> When it is, it fails the assertion in base.c_name(), even though
> gen_visit_members_call() is not going to use its value.
>
> You hack around it by passing 'qapi_NAME_base(obj)' instead.
>
> If NAME isn't implicit, the function exists, and does the same as the
> expression it replaces.
>
> If NAME is implicit, the function doesn't exist, but
> gen_visit_members_call() doesn't care, because it doesn't use the
> argument then.
>
> Ugh! More evidence that we better not munge the two cases together into
> one function.
Even with my v4 work towards exposing implicit types as a concrete
struct, I'm still not creating qapi_NAME_base(obj) for objects with an
implicit type. But '(_obj_FOO_base *)FOO' works well for a base with a
concrete implicit base type.
>> @@ -354,7 +355,7 @@ code generator can ensure that branches exist for all
>> values of the
>> enum (although the order of the keys need not match the declaration of
>> the enum). In the resulting generated C data types, a flat union is
>> represented as a struct with the base member fields included directly,
>> -and then a union of structures for each branch of the struct.
>> +and then a union of pointers to structures for each branch of the struct.
>
> Uh, that became wrong in commit 544a373 already, didn't it?
>
> Is that a bug in PATCH 3 then?
Yes, and fixed up accordingly in my v3 respin. (I think it was some
rebase conflicts that I resolved incorrectly at some point).
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -75,14 +75,10 @@
>>'base': 'UserDefZero',
>>'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>>
>> -{ 'struct': 'UserDefUnionBase2',
>> - 'base': 'UserDefZero',
>> - 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
>> -
>> # this variant of UserDefFlatUnion defaults to a union that uses fields with
>> # allocated types to test corner cases in the cleanup/dealloc visitor
>> { 'union': 'UserDefFlatUnion2',
>> - 'base': 'UserDefUnionBase2',
>> + 'base': { 'string': 'str', 'enum1': 'QEnumTwo' },
>
> You lost member 'integer' from the base's base. Harmless (I think), but
> visible when you compare generated output.
Easy enough to keep.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx
A small update on this. I have a working implementation of the "halted state" mechanism for waiting all the pending flushes to be completed. However, the way I'm going back to the cpus.c loop (the while(1) in qemu_tcg_cpu_thread_fn) is a bit convoluted. In the case of the TLB ops that always end the TB, a simple cpu_exit() allows me to go back to the main loop. I think in this case we can also use the cpu_loop_exit(), though making the code a bit more complicated since the PC would require some adjustments. I wanted then to apply the same "halted state" to the LoadLink helper, since also this one might cause some flush requests. In this case, we can not just call cpu_loop_exit() in that the guest code would miss the returned value. Forcing the LDREX instruction to also end the TB through an empty 'is_jmp' condition did the trick allowing once again to use cpu_exit(). Is there another better solution? Thank you, alvise On Mon, Feb 29, 2016 at 3:18 PM, alvise rigo wrote: > I see the risk. I will come back with something and let you know. > > Thank you, > alvise > > On Mon, Feb 29, 2016 at 3:06 PM, Paolo Bonzini > wrote: > > > > > > On 29/02/2016 15:02, alvise rigo wrote: > >> > Yeah, that's the other approach -- really split the things that can > >> > be async and do real "wait for completion" at points which must > >> > synchronize. (Needs a little care since DMB is not the only such > point.) > >> > An initial implementation that does an immediate wait-for-completion > >> > is probably simpler to review though, and add the real asynchrony > >> > later. And either way you need an API for the target to wait for > >> > completion. > >> OK, so basically being sure that the target CPU performs the flush > >> before executing the next TB is not enough. We need a sort of feedback > >> that the flush has been done before emulating the next guest > >> instruction. Did I get it right? > > > > That risks getting deadlocks if CPU A asks B to flush the TLB and vice > > versa. Using a halted state means that the VCPU thread goes through the > > cpus.c loop and can for example service other CPUs' TLB flush requests. > > > > Paolo >
Re: [Qemu-devel] [PATCH v2 15/19] qapi-visit: Move error check into gen_visit_members_call()
On 03/03/2016 04:56 AM, Markus Armbruster wrote:
> Eric Blake writes:
>
>> When first introduced, neither branch of gen_visit_members_call()
>> would output a goto. But now that the implicit struct visit
>> always ends with a goto, we should do the same for regular
>> struct visits, so that callers don't have to worry about whether
>> they are creating two identical goto's in a row.
>>
>> Generated code gets slightly larger; if desired, we could patch
>> qapi.py:gen_visit_members() to have a mode where it skips the
>> final goto and leave it up to the callers when to use that mode,
>> but that adds more maintenance burden when the compiler should
>> be smart enough to not bloat the .o file just because the .c
>> file got larger.
>>
>> Signed-off-by: Eric Blake
>>
>> +++ b/scripts/qapi-visit.py
>> @@ -48,6 +48,7 @@ def gen_visit_members_call(typ, direct_name,
>> implicit_name=None):
>assert isinstance(typ, QAPISchemaObjectType)
>if typ.is_empty():
>pass
>elif typ.is_implicit():
>assert implicit_name
>assert not typ.variants
>ret += gen_visit_members(typ.members, prefix=implicit_name)
>
> This is the goto-generating part mentioned in the commit message.
>
>> @@ -95,9 +95,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
>> *obj, Error **errp)
>> }
>> ''')
>>
>> -# 'goto out' produced for base, by gen_visit_members() for each member,
>> -# and if variants were present
>> -if base or members or variants:
>> +# 'goto out' produced for non-empty base, by gen_visit_members() for
>> +# each member, and if variants were present
>> +if (base and not base.is_empty()) or members or variants:
>> ret += mcgen('''
>>
>> out:
>
> Uh, sure this hunk belongs to this patch?
Unfortunately, yeah - because the empty base case doesn't generate a
goto. I'm leaning more and more towards not bothering to special case
empty types on the next round.
I've already started playing with making type.c_name() work on implicit
types - it generates names like '_obj_Foo_wrapper', which won't collide
(because we reserved leading underscore for our own use), but also
doesn't quite match conventional naming conventions - but as long as it
is used only in generated code, it isn't that bad. And with an implicit
type directly laid out, I can then just blindly call
visit_type_FOO_members(), even for implicit base or variant. The v4
spin of the second half of this series is looking promising...
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization
> Subject: Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration > optimization > > On Fri, Mar 04, 2016 at 09:08:44AM +, Li, Liang Z wrote: > > > On Fri, Mar 04, 2016 at 01:52:53AM +, Li, Liang Z wrote: > > > > > I wonder if it would be possible to avoid the kernel changes > > > > > by parsing /proc/self/pagemap - if that can be used to detect > > > > > unmapped/zero mapped pages in the guest ram, would it achieve > > > > > the > > > same result? > > > > > > > > Only detect the unmapped/zero mapped pages is not enough. > Consider > > > the > > > > situation like case 2, it can't achieve the same result. > > > > > > Your case 2 doesn't exist in the real world. If people could stop > > > their main memory consumer in the guest prior to migration they > > > wouldn't need live migration at all. > > > > The case 2 is just a simplified scenario, not a real case. > > As long as the guest's memory usage does not keep increasing, or not > > always run out, it can be covered by the case 2. > > The memory usage will keep increasing due to ever growing caches, etc, so > you'll be left with very little free memory fairly soon. > I don't think so. > > > I tend to think you can safely assume there's no free memory in the > > > guest, so there's little point optimizing for it. > > > > If this is true, we should not inflate the balloon either. > > We certainly should if there's "available" memory, i.e. not free but cheap to > reclaim. > What's your mean by "available" memory? if they are not free, I don't think it's cheap. > > > OTOH it makes perfect sense optimizing for the unmapped memory > > > that's made up, in particular, by the ballon, and consider inflating > > > the balloon right before migration unless you already maintain it at > > > the optimal size for other reasons (like e.g. a global resource manager > optimizing the VM density). > > > > > > > Yes, I believe the current balloon works and it's simple. Do you take the > performance impact for consideration? > > For and 8G guest, it takes about 5s to inflating the balloon. But it > > only takes 20ms to traverse the free_list and construct the free pages > bitmap. > > I don't have any feeling of how important the difference is. And if the > limiting factor for balloon inflation speed is the granularity of > communication > it may be worth optimizing that, because quick balloon reaction may be > important in certain resource management scenarios. > > > By inflating the balloon, all the guest's pages are still be processed (zero > page checking). > > Not sure what you mean. If you describe the current state of affairs that's > exactly the suggested optimization point: skip unmapped pages. > You'd better check the live migration code. > > The only advantage of ' inflating the balloon before live migration' is > > simple, > nothing more. > > That's a big advantage. Another one is that it does something useful in real- > world scenarios. > I don't think the heave performance impaction is something useful in real world scenarios. Liang > Roman.
Re: [Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus
On 03/04/2016 03:05 AM, David Hildenbrand wrote:
>> Once hotplug is enabled, interrupts may come in for CPUs
>> with an address > smp_cpus. Allocate for this and allow
>> search routines to look beyond smp_cpus.
>>
>> Signed-off-by: Matthew Rosato
>> ---
>> hw/s390x/s390-virtio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index c501a48..90bc58a 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -58,15 +58,16 @@
>> #define S390_TOD_CLOCK_VALUE_MISSING0x00
>> #define S390_TOD_CLOCK_VALUE_PRESENT0x01
>>
>> -static S390CPU **ipi_states;
>> +static S390CPU **cpu_states;
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> -if (cpu_addr >= smp_cpus) {
>> +if (cpu_addr >= max_cpus) {
>> return NULL;
>> }
>>
>> -return ipi_states[cpu_addr];
>> +/* Fast lookup via CPU ID */
>> +return cpu_states[cpu_addr];
>> }
>>
>> void s390_init_ipl_dev(const char *kernel_filename,
>> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>> machine->cpu_model = "host";
>> }
>>
>> -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>> +cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>>
>> -for (i = 0; i < smp_cpus; i++) {
>> +for (i = 0; i < max_cpus; i++) {
>> S390CPU *cpu;
>>
>> cpu = cpu_s390x_init(machine->cpu_model);
>>
>> -ipi_states[i] = cpu;
>> +cpu_states[i] = cpu;
>
> This looks wrong (creating all cpus). But the net patch fixes it again.
>
Ouch. Definitely wrong, error introduced during patch split. We
allocate for max_cpus, but should only create smp_cpus cpus during init.
> Can you make this patch a simple rename patch and move the max_cpu stuff into
> the next patch if this makes sense?
>
> Or simply set the cpu_state for everything above smp_cpus to zero in this
> patch.
This is done via the gmalloc0. If I hadn't messed up this loop, the net
result would be the ability to handle > smp_cpus, but nobody will ever
create one (yet).
Matt
Re: [Qemu-devel] [PULL 00/30] target-arm queue
On 4 March 2016 at 11:41, Peter Maydell wrote: > Here's the target-arm queue: fairly large with a roundup of lots > of patches that hit the list at or just before the softfreeze > deadline. Most notable thing in here is Peter/Paolo's bigendian > and SETEND support patchset. > > There are still some patchsets on list that I haven't got to > reviewing yet (eg last set of raspi patches, imx6) which I hope > to get to early next week and into a pullreq next week sometime. > > thanks > -- PMM > > The following changes since commit 2d3b7c0164e1b9287304bc70dd6ed071ba3e8dfc: > > Merge remote-tracking branch 'remotes/amit-virtio-rng/tags/rng-for-2.6-1' > into staging (2016-03-03 13:13:36 +) > > are available in the git repository at: > > > git://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20160304 > > for you to fetch changes up to ba63cf47a93041137a94e86b7d0cd87fc896949b: > > target-arm: Only trap SRS from S-EL1 if specified mode is MON (2016-03-04 > 11:30:22 +) > > > target-arm queue: > * Correct handling of writes to CPSR from gdbstub in user mode > * virt: lift maximum RAM limit to 255GB > * sdhci: implement reset > * virt: if booting in Secure mode, provide secure-only RAM, make first >flash device secure-only, and assume the EL3 boot rom will handle PSCI > * bcm2835: use explicit endianness accessors rather than ldl/stl_phys > * support big-endian in system mode for ARM > * implement SETEND instruction > * arm_gic: implement the GICv2 GICC_DIR register > * fix SRS bug: only trap from S-EL1 to EL3 if specified mode is Mon > Applied, thanks. -- PMM
Re: [Qemu-devel] [Nbd] [RFC 1/1] nbd (specification): add NBD_CMD_WRITE_ZEROES command
On 04/03/2016 10:54, Kevin Wolf wrote: >>> - pls write the following amount of zeroes in either way (even calling >>> > >write directly), i.e. ensure that the data is zeroed and the space on >>> > >the file system is allocated for that. >> > >> > IOW, you *don't* want to have a sparse file in that case? Or do I >> > misunderstand things here? > I think what we're looking for is more like "zero out this area, feel > free to use whatever method is most efficient to achieve that". > > So if the server knows that the backing store supports an efficient way > to write zeros (e.g. FALLOC_FL_ZERO_RANGE), it will use that. Otherwise, > if TRIM works and we know that the result is zeroed space instead of > undefined contents, the server is free to use it. And if even that > fails, it just falls back to an explicit write of a zeroed buffer. > > If we want, we can give the client a little more control about whether > or not discarding in the process is allowed (or maybe even preferred). > qemu's interface for writing zeros has a BDRV_REQ_MAY_UNMAP flag, for > example. NBD-wise, I think the TRIM command is good as it is, and NBD_CMD_WRITE_ZEROES should be added like Den is doing. It also makes sense to use trimming to implement NBD_CMD_WRITE_ZEROES, but it should be explicitly requested by the user. For this, my suggestion is that NBD_CMD_WRITE_ZEROES should have an NBD_FLAG_TRY_TRIM flag in bit 16. If specified, the backend can use a zero-writing mechanism that trims, _but_ it must ensure that the bytes read as zero. If it cannot ensure that, it must not trim and it should instead do a full write. This is similar to the SCSI command WRITE SAME (when the command payload is all zeroes). Like Kevin said, it also happens to map nicely to the QEMU block device layer. Paolo
Re: [Qemu-devel] [RFC] host and guest kernel trace merging
On Fri, 4 Mar 2016 11:19:33 + Stefan Hajnoczi wrote: > > 2. Build a trace-cmd-server > > > > Which does the following: > > > > 1. Receive trace-cmd record commands from a guest, > > to be performed in the host > > Sometimes the opposite is desirable: the host controls tracing inside > the guest. Any thoughts on this use case? My idea for a trace-cmd server, is to have a --client operation, for running on the guest. trace-cmd server --client The connection will be some socket, either network or something directly attached to the host. Then on the host, we can have trace-cmd server --connect Where the server will create a connection to the guest. And then, you could run on the host: trace-cmd record --connect And this will start recording host events, and then connect to the local server that connects to the guest(s) and that will start tracing on the guest as well. Then events on the guest will be passed to the host server. Something like this is my idea. We can work out the details on the best way to get things working. We may be able to eliminate the host server middle man. But I envision that we need a trace-cmd server running on the guest to start off the commands. The problem I have with the guest server, and something that we may be able to fix later on, but should always keep it in the back of our minds, is the security issue. For this to work, the guest server needs to run as root. It will have an open socket (network or to host), that will enable tracing on the guest. There needs to be some sort of verification on that connection to prevent anyone from connecting to it. In the protocol for the connection between guest and host, I'll currently add a "security" feature, that will allow the guest to tell whomever is connecting to it, what type of security feature it wants. For now it will be TRACE_CMD_NO_SECURITY. But that will have to change in the future. -- Steve > > > 2. Read the TSC offset for vCPUs being traced > > > > 3. Read the CPU frequency for --ts2secs > > > > 4. Receive the guest.dat from the guest and store it together > > with the host's > > > > I'd suggest the default communication between guest and > > host be via networking. Then we add vsock support when it's > > available.
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers
On Fri, Mar 04, 2016 at 01:57:05PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote: > >> Plugging an MSI-capable device into an MSI-incapable board works just > >> fine, both for physical and for virtual hardware. What doesn't work is > >> plugging an MSI-capable device into an MSI-capable board with *broken* > >> MSI support. > >> > >> As a convenience feature, we summarily and silently remove a device's > >> MSI capability when we detect such a broken board. At least that's what > >> the commit message you quoted claims. > > > > And this makes sense, right? > > Yes, except for the part where we ignore the user's explicit orders, > and, to a lesser degree, for the part where we create variants of > physical devices that don't exist physically. > > >> In reality, we remove it not just for broken boards, but even for > >> MSI-incapable boards. > >> > >> I take issue with "summarily and silently", and "even for MSI-incapable > >> boards". > >> > >> When multiple variants of a device exist, and the user didn't ask for a > >> specific one, then picking the one that works best with the board is > >> just fine. > >> > >> It's absolutely not fine when the user did ask for a specific one. When > >> I ask for msi=on, I mean it. If it can't work with this board, tell me. > >> But silently ignoring my orders is a bug. > > > > I agree. msi is not the only case either. We really need - for any boolean > > flag - a way to figure out whether it was set by user. > > With that in place we could fix it. > > QMP provides that directly as "optional bool", but qdev properties do > "optional" diffently, and when you see the default value, you don't know > whether it comes from the user or not. > > Another solution is an on/off/auto type. We got it already in the QAPI > schema, as OnOffAuto, and my recent "[PATCH 32/38] qdev: New > DEFINE_PROP_ON_OFF_AUTO" makes it available as qdev property. With > default set to auto, we should be set. Should we somehow change all on/off properties to on/off/auto? > > However, almost no one uses the msi/msi-x flag - we introduced > > them only for one reason - for backwards compatibility. > > The fact that each time we need a compatibility flag > > we also expose a new user interface is very unfortunate. > > IMO it was a design mistake made a long time ago and it won't > > be easy to fix now. > > I agree there's no easy fix, but we can try to find a non-easy one. > > For backward compatibility, we need to configure some device models for > certain machine types. We use the only object configuration mechanism > we have: properties. The properties we use for compatibility are all > exposed to the user. > > We could easily provide a flag to mark properties private, and only > accept non-private properties at external interfaces. This should help > stopping growth of the problem, but it's not an easy fix for reducing > it, because making a property private retroactively is problematic. We > could have a flag to mark them deprecated instead, warn on use, remove > them from documentation, and perhaps drop them a couple of releases > later. Sounds good. > > And for the above reason I personally do not intend to > > spend time designing a specific hack just for the msi > > property. > > > >> It's fine to have emulations of MSI-capable boards where MSI doesn't yet > >> work. Even if that means we have to reject MSI-capable devices. > > > > I don't know what does reject mean here. Removing msi capability? > > In that case I agree. > > By "reject" I mean "reject the user's order to plug in an MSI-capable > device." I don't believe anyone tweaks these properties in practice. However, I have to wonder. Assume that you have supplied a device with 10 properties. QEMU can not support them. At your suggesion, device is rejected. How does user know which property to tweak? Try all values for them all? > >> It's absolutely not fine to reject them for MSI-incapable boards, where > >> they'd work just fine. > > > > I think that as long as users did not ask for msi explicitly, > > and board is msi incapable, it does not matter much whether > > device has msi capability or not - guest will not try > > to use it anyway. > > If the device comes in MSI-capable and MSI-incapable variants, and the > user didn't specify a variant, then picking one that fits the board is > fine. > > If the device does not come in variants (and many physical devices > don't), then rejecting it just because it's MSI-capable and the board > isn't is not fine. > > To fix, we'd have to limit what is now !msi_supported to the boards that > are nominally MSI-capable, except our emulation doesn't work, i.e. do > exactly what the commit message promised. I rather think it's academic. MSI is a performance optimization, and is optional for guests anyway. It's hard to see when may users absolutely require it. > The PCI core en
Re: [Qemu-devel] [PATCH v8 0/7] Allow hotplug of s390 CPUs
On Thu, 3 Mar 2016 16:30:26 -0500 Matthew Rosato wrote: > Matthew Rosato (7): > s390x/cpu: Cleanup init in preparation for hotplug > s390x/cpu: Set initial CPU state in common routine > s390x/cpu: Get rid of side effects when creating a vcpu > s390x/cpu: Tolerate max_cpus > s390x/cpu: Add CPU property links > s390x/cpu: Add error handling to cpu creation > s390x/cpu: Allow hotplug of CPUs > > hw/s390x/s390-virtio-ccw.c | 49 ++- > hw/s390x/s390-virtio.c | 36 +++- > hw/s390x/s390-virtio.h | 2 +- > target-s390x/cpu-qom.h | 3 ++ > target-s390x/cpu.c | 83 > +++--- > target-s390x/cpu.h | 2 ++ > target-s390x/helper.c | 42 +-- > 7 files changed, 192 insertions(+), 25 deletions(-) If I followed the discussion here correctly, there aren't any non-minor comments left, are there? If so, I'd be happy to apply a v9 to my branch so we can finally get this into 2.6.
Re: [Qemu-devel] [PATCH v2 0/3] add missing virtio aliases
On Thu, Feb 18, 2016 at 10:44:11PM +0100, Sascha Silbe wrote: > Split out from the series fixing qemu-iotest 140 on s390x [1]. > > This series makes crafting qemu command lines that work across > multiple (virtio-supporting) platforms at lot easier. > > Changes since the original series: > - included patch to improve the error message when the target device > class of an alias doesn't exist > - added comment about keeping alias table sorted > - covering all non-abstract virtio device classes rather than just > those already implemented on s390x > > Sascha Silbe (3): > qdev-monitor: improve error message when alias device is unavailable > qdev-monitor: sort alias table by typename > qdev-monitor: add missing aliases for virtio device classes > > qdev-monitor.c | 42 ++ > 1 file changed, 34 insertions(+), 8 deletions(-) > > [1] mid:1455023713-104799-1-git-send-email-si...@linux.vnet.ibm.com > "[PATCH 0/3] qemu-iotests: fix 140 on s390x" by Sascha Silbe > on 2016-02-09 I don't see issues here: Reviewed-by: Michael S. Tsirkin You should also Cc lcapitul...@redhat.com and ebl...@redhat.com. Markus, will you take this series? > -- > 2.1.4
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers
"Michael S. Tsirkin" writes: > On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote: >> Plugging an MSI-capable device into an MSI-incapable board works just >> fine, both for physical and for virtual hardware. What doesn't work is >> plugging an MSI-capable device into an MSI-capable board with *broken* >> MSI support. >> >> As a convenience feature, we summarily and silently remove a device's >> MSI capability when we detect such a broken board. At least that's what >> the commit message you quoted claims. > > And this makes sense, right? Yes, except for the part where we ignore the user's explicit orders, and, to a lesser degree, for the part where we create variants of physical devices that don't exist physically. >> In reality, we remove it not just for broken boards, but even for >> MSI-incapable boards. >> >> I take issue with "summarily and silently", and "even for MSI-incapable >> boards". >> >> When multiple variants of a device exist, and the user didn't ask for a >> specific one, then picking the one that works best with the board is >> just fine. >> >> It's absolutely not fine when the user did ask for a specific one. When >> I ask for msi=on, I mean it. If it can't work with this board, tell me. >> But silently ignoring my orders is a bug. > > I agree. msi is not the only case either. We really need - for any boolean > flag - a way to figure out whether it was set by user. > With that in place we could fix it. QMP provides that directly as "optional bool", but qdev properties do "optional" diffently, and when you see the default value, you don't know whether it comes from the user or not. Another solution is an on/off/auto type. We got it already in the QAPI schema, as OnOffAuto, and my recent "[PATCH 32/38] qdev: New DEFINE_PROP_ON_OFF_AUTO" makes it available as qdev property. With default set to auto, we should be set. > However, almost no one uses the msi/msi-x flag - we introduced > them only for one reason - for backwards compatibility. > The fact that each time we need a compatibility flag > we also expose a new user interface is very unfortunate. > IMO it was a design mistake made a long time ago and it won't > be easy to fix now. I agree there's no easy fix, but we can try to find a non-easy one. For backward compatibility, we need to configure some device models for certain machine types. We use the only object configuration mechanism we have: properties. The properties we use for compatibility are all exposed to the user. We could easily provide a flag to mark properties private, and only accept non-private properties at external interfaces. This should help stopping growth of the problem, but it's not an easy fix for reducing it, because making a property private retroactively is problematic. We could have a flag to mark them deprecated instead, warn on use, remove them from documentation, and perhaps drop them a couple of releases later. > And for the above reason I personally do not intend to > spend time designing a specific hack just for the msi > property. > >> It's fine to have emulations of MSI-capable boards where MSI doesn't yet >> work. Even if that means we have to reject MSI-capable devices. > > I don't know what does reject mean here. Removing msi capability? > In that case I agree. By "reject" I mean "reject the user's order to plug in an MSI-capable device." >> It's absolutely not fine to reject them for MSI-incapable boards, where >> they'd work just fine. > > I think that as long as users did not ask for msi explicitly, > and board is msi incapable, it does not matter much whether > device has msi capability or not - guest will not try > to use it anyway. If the device comes in MSI-capable and MSI-incapable variants, and the user didn't specify a variant, then picking one that fits the board is fine. If the device does not come in variants (and many physical devices don't), then rejecting it just because it's MSI-capable and the board isn't is not fine. To fix, we'd have to limit what is now !msi_supported to the boards that are nominally MSI-capable, except our emulation doesn't work, i.e. do exactly what the commit message promised. The PCI core encourages creating both variants, and most (but not all) device models do, but: >> Furthermore, I doubt the wisdom of creating virtual devices that don't >> exist physically just to have something that works in our broken boards. >> If the physical device exists in MSI-capable and MSI-incapable variants, >> emulating both is fine. But if it only ever existed MSI-capable, having >> the PCI core(!) drop the MSI capability is a questionable idea. I >> suspect that the need for this dubious hack would be much smaller if we >> didn't foolishly treat every MSI-incapable board as broken MSI-capable >> board. >> >> If you conclude that cleaning up this carelessly made mess is not worth >> the bother now, then at least explain the mess in the code, please. >> It's not obvious, an
Re: [Qemu-devel] [PATCH v2 0/3] add missing virtio aliases
Sascha Silbe writes: [...] > This series makes crafting qemu command lines that work across > multiple (virtio-supporting) platforms at lot easier. [...] Ping. Conny has already given her Reviewed-By; Markus gave it for a previous version of the third patch. Are there any other opinions on this series? It was posted in mid-February (i.e. before soft freeze) and I'd like it to land in qemu 2.6. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 4/7] target-i386: Dump illegal opcodes with -d unimp
On 03/03/2016 20:06, Richard Henderson wrote: > On 03/03/2016 02:08 AM, Paolo Bonzini wrote: >>> Do you want LOG_UNIMP or LOG_GUEST_ERROR? >> >> I would actually use LOG_IN_ASM. As you noticed, guests sometimes use >> illegal opcodes; another example is Xen's hypercall interface. >> >> On 03/03/2016 07:57, Hervé Poussineau wrote: >>> This patch is not quiet on some operating systems: >>> OS/2: >>> ILLOPC: 000172e1: 0f a6 >>> >>> Windows XP: >>> ILLOPC: 00020d1a: c4 c4 >>> >>> And very verbose in Windows 3.11, Windows 9x: >>> ILLOPC: 000ffb17: 63 >>> ILLOPC: 000ffb17: 63 >>> >>> Is it normal? >> >> Yes, it is. As usual, Raymond Chen explains what's going on: >> >> https://blogs.msdn.microsoft.com/oldnewthing/20041215-00/?p=37003 > > Wow. That's... interesting. It's actually even more interesting (the explanation is in the book) if you notice that 0xffb17 is in the middle of the BIOS. Indeed Windows 95 first locates a single 0x63 in the BIOS (so that it's ROM and no one can write a different byte). Then the 32-bit code can use a system service that allocates a callback from 16-bit MS-DOS. That service gets a 32-bit address for the 32-bit code and returns a real-mode address to be used in 16-bit code. The kick is that all the real-mode addresses point to that single 0x63 that was found in ROM. For example in the case above the real-mode addresses could be FFB1:07, FFB0:17, FFAF:27, etc. The illegal opcode exception handler looks at the segment to figure out which 32-bit address to jump to. There are also cases where the ARPL is patched into existing code (like a breakpoint) to trap that code to 32-bit. But this one using the ROM is much cooler. Paolo
[Qemu-devel] [PATCH V3 3/3] tests/test-filter-redirector: Add unit test for filter-redirector
In this unit test,we will test the filter redirector function. Case 1, tx traffic flow: qemu side | test side | +-+| +---+ | backend <---+ sock0 | +++| +---+ | | +v+ +---+ | | rd0+->+chardev| | +-+ +---+---+ | | | +-+ | | | rd1<--+ | +++| | | +v+| +---+ | rd2+--->sock1 | +-+| +---+ + Start qemu with: "-netdev socket,id=qtest-bn0,fd=%d " "-device rtl8139,netdev=qtest-bn0,id=qtest-e0 " "-chardev socket,id=redirector0,path=%s,server,nowait " "-chardev socket,id=redirector1,path=%s,server,nowait " "-chardev socket,id=redirector2,path=%s,nowait " "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0," "queue=tx,outdev=redirector0 " "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0," "queue=tx,indev=redirector2 " "-object filter-redirector,id=qtest-f2,netdev=qtest-bn0," "queue=tx,outdev=redirector1 " -- Case 2, rx traffic flow qemu side | test side | +-+| +---+ | backend +---> sock1 | +^+| +---+ | | +++ +---+ | | rd0+<-+chardev| | +-+ +---+---+ | ^ | +-+ | | | rd1+--+ | +^+| | | +++| +---+ | rd2<---+sock0 | +-+| +---+ Start qemu with: "-netdev socket,id=qtest-bn0,fd=%d " "-device rtl8139,netdev=qtest-bn0,id=qtest-e0 " "-chardev socket,id=redirector0,path=%s,server,nowait " "-chardev socket,id=redirector1,path=%s,server,nowait " "-chardev socket,id=redirector2,path=%s,nowait " "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0," "queue=rx,outdev=redirector0 " "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0," "queue=rx,indev=redirector2 " "-object filter-redirector,id=qtest-f2,netdev=qtest-bn0," "queue=rx,outdev=redirector1 " Signed-off-by: Zhang Chen Signed-off-by: Wen Congyang --- tests/.gitignore | 1 + tests/Makefile | 2 + tests/test-filter-redirector.c | 214 + 3 files changed, 217 insertions(+) create mode 100644 tests/test-filter-redirector.c diff --git a/tests/.gitignore b/tests/.gitignore index 10df017..5069d5d 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -64,5 +64,6 @@ test-x86-cpuid test-xbzrle test-netfilter test-filter-mirror +test-filter-redirector *-test qapi-schema/*.test.* diff --git a/tests/Makefile b/tests/Makefile index e56c514..275c4cc 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -213,6 +213,7 @@ check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXE endif check-qtest-i386-y += tests/test-netfilter$(EXESUF) check-qtest-i386-y += tests/test-filter-mirror$(EXESUF) +check-qtest-i386-y += tests/test-filter-redirector$(EXESUF) check-qtest-x86_64-y = $(check-qtest-i386-y) gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) @@ -565,6 +566,7 @@ tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y) tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y) tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y) tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y) +tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o $(qtest-obj-y) tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y) tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c new file mode 100644 index 000..6074cd7 --- /dev/null +++ b/tests/test-filter-redirector.c @@ -0,0 +1,214 @@ +/* + * QTest testcase for filter-redirector + * + * Copyright (c) 2016 FUJITSU LIMITED + * Author: Zhang Chen + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * later. See the COPYING file in the top-level directory. + * + * Case 1, tx traffic flow: + * + * qemu side | test side + *| + * +-+| +---+ + * | backend <---+ sock0 | + * +++| +---+ + * | | + * +v+ +---+ | + * | rd0+->+chardev| | + * +-+ +---+---+ | + * | | + * +-+ | | + * | rd1<--+ | + * +++| + * | | + * +v+| +---+ + * | rd2+--->sock1 | + * +-
[Qemu-devel] [PATCH V3 0/3] Introduce filter-redirector
Filter-redirector is a netfilter plugin. It gives qemu the ability to redirect net packet. redirector can redirect filter's net packet to outdev. and redirect indev's packet to filter. filter + | | redirector | +--+ || | || | || | indev +---+ +--> outdev || | || | || | +--+ | | v filter v3: -Address Jason's comments. v2: - Address Jason's comments. - Add filter-traffic.h to reuse parts of the codes - Add unit test case v1: initial patch. Zhang Chen (3): net/filter-mirror: Change filter_mirror_send interface net/filter-mirror:Add filter-redirector func tests/test-filter-redirector: Add unit test for filter-redirector net/filter-mirror.c| 221 - qemu-options.hx| 8 ++ tests/.gitignore | 1 + tests/Makefile | 2 + tests/test-filter-redirector.c | 214 +++ vl.c | 3 +- 6 files changed, 443 insertions(+), 6 deletions(-) create mode 100644 tests/test-filter-redirector.c -- 1.9.1
[Qemu-devel] [PATCH V3 2/3] net/filter-mirror:Add filter-redirector func
Filter-redirector is a netfilter plugin.
It gives qemu the ability to redirect net packet.
redirector can redirect filter's net packet to outdev.
and redirect indev's packet to filter.
filter
+
|
|
redirector |
+--+
|| |
|| |
|| |
indev +---+ +--> outdev
|| |
|| |
|| |
+--+
|
|
v
filter
usage:
-netdev user,id=hn0
-chardev socket,id=s0,host=ip_primary,port=X,server,nowait
-chardev socket,id=s1,host=ip_primary,port=Y,server,nowait
-filter-redirector,id=r0,netdev=hn0,queue=tx/rx/all,indev=s0,outdev=s1
Signed-off-by: Zhang Chen
Signed-off-by: Wen Congyang
---
net/filter-mirror.c | 211
qemu-options.hx | 8 ++
vl.c| 3 +-
3 files changed, 221 insertions(+), 1 deletion(-)
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index 4ff7619..d137168 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -25,11 +25,19 @@
#define FILTER_MIRROR(obj) \
OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
+#define FILTER_REDIRECTOR(obj) \
+OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_REDIRECTOR)
+
#define TYPE_FILTER_MIRROR "filter-mirror"
+#define TYPE_FILTER_REDIRECTOR "filter-redirector"
+#define REDIRECT_HEADER_LEN sizeof(uint32_t)
typedef struct MirrorState {
NetFilterState parent_obj;
+NetQueue *incoming_queue;
+char *indev;
char *outdev;
+CharDriverState *chr_in;
CharDriverState *chr_out;
} MirrorState;
@@ -67,6 +75,68 @@ err:
return ret < 0 ? ret : -EIO;
}
+static int redirector_chr_can_read(void *opaque)
+{
+return REDIRECT_HEADER_LEN;
+}
+
+static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
+{
+NetFilterState *nf = opaque;
+MirrorState *s = FILTER_REDIRECTOR(nf);
+uint32_t len;
+int ret = 0;
+uint8_t *recv_buf;
+
+memcpy(&len, buf, size);
+if (size < REDIRECT_HEADER_LEN) {
+ret = qemu_chr_fe_read_all(s->chr_in, ((uint8_t *)&len) + size,
+REDIRECT_HEADER_LEN - size);
+if (ret != (REDIRECT_HEADER_LEN - size)) {
+error_report("filter-redirector recv size failed");
+return;
+}
+}
+
+len = ntohl(len);
+
+if (len > 0 && len < NET_BUFSIZE) {
+recv_buf = g_malloc(len);
+ret = qemu_chr_fe_read_all(s->chr_in, recv_buf, len);
+if (ret != len) {
+error_report("filter-redirector recv buf failed");
+g_free(recv_buf);
+return;
+}
+
+if (nf->direction == NET_FILTER_DIRECTION_ALL ||
+nf->direction == NET_FILTER_DIRECTION_TX) {
+ret = qemu_net_queue_send(s->incoming_queue, nf->netdev,
+0, (const uint8_t *)recv_buf, len, NULL);
+
+if (ret < 0) {
+/*
+ *FIXME: just report an error, let NET_FILTER_DIRECTION_RX
have chance to pass
+ * to next filter ?
+ */
+error_report("filter-redirector out to guest failed");
+}
+}
+
+if (nf->direction == NET_FILTER_DIRECTION_ALL ||
+nf->direction == NET_FILTER_DIRECTION_RX) {
+struct iovec iov = {
+.iov_base = recv_buf,
+.iov_len = sizeof(recv_buf)
+};
+qemu_netfilter_pass_to_next(nf->netdev->peer, 0, &iov, 1, nf);
+}
+g_free(recv_buf);
+} else {
+error_report("filter-redirector recv len failed");
+}
+}
+
static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
NetClientState *sender,
unsigned flags,
@@ -89,6 +159,27 @@ static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
return 0;
}
+static ssize_t filter_redirector_receive_iov(NetFilterState *nf,
+ NetClientState *sender,
+ unsigned flags,
+ const struct iovec *iov,
+ int iovcnt,
+ NetPacketSent *sent_cb)
+{
+MirrorState *s = FILTER_REDIRECTOR(nf);
+int ret;
+
+if (s->chr_out) {
+ret = filter_mirror_send(s->chr_out, iov, iovcnt);
+if (ret) {
+error_report("filter_mirror_send failed(%s)", strerror(-ret));
+}
+return iov_size(iov, iovcnt);
+} else {
+return 0;
+}
+}
+
static void filter_mirror_cleanup(NetFilterState *nf)
[Qemu-devel] [PATCH V3 1/3] net/filter-mirror: Change filter_mirror_send interface
Change filter_mirror_send interface to make it easier
to used by other filter
Signed-off-by: Zhang Chen
Signed-off-by: Wen Congyang
---
net/filter-mirror.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index ee13d94..4ff7619 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -33,11 +33,10 @@ typedef struct MirrorState {
CharDriverState *chr_out;
} MirrorState;
-static int filter_mirror_send(NetFilterState *nf,
+static int filter_mirror_send(CharDriverState *chr_out,
const struct iovec *iov,
int iovcnt)
{
-MirrorState *s = FILTER_MIRROR(nf);
int ret = 0;
ssize_t size = 0;
uint32_t len = 0;
@@ -49,14 +48,14 @@ static int filter_mirror_send(NetFilterState *nf,
}
len = htonl(size);
-ret = qemu_chr_fe_write_all(s->chr_out, (uint8_t *)&len, sizeof(len));
+ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)&len, sizeof(len));
if (ret != sizeof(len)) {
goto err;
}
buf = g_malloc(size);
iov_to_buf(iov, iovcnt, 0, buf, size);
-ret = qemu_chr_fe_write_all(s->chr_out, (uint8_t *)buf, size);
+ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)buf, size);
g_free(buf);
if (ret != size) {
goto err;
@@ -75,9 +74,10 @@ static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
int iovcnt,
NetPacketSent *sent_cb)
{
+MirrorState *s = FILTER_MIRROR(nf);
int ret;
-ret = filter_mirror_send(nf, iov, iovcnt);
+ret = filter_mirror_send(s->chr_out, iov, iovcnt);
if (ret) {
error_report("filter_mirror_send failed(%s)", strerror(-ret));
}
--
1.9.1
[Qemu-devel] [PULL 19/30] target-arm: pass DisasContext to gen_aa32_ld*/st*
From: Paolo Bonzini
We'll need the DisasContext in the next patch to retrieve the
desired endianness, so pass it as a whole to gen_aa32_ld*/st*.
Unfortunately we cannot let those functions call get_mem_index,
because of user-mode load/store instructions.
Signed-off-by: Paolo Bonzini
[ PC changes:
* Fix long lines
]
Reviewed-by: Peter Maydell
Signed-off-by: Peter Crosthwaite
Signed-off-by: Peter Maydell
---
target-arm/translate.c | 270 ++---
1 file changed, 142 insertions(+), 128 deletions(-)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index b52bbc3..41c3c7f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -921,23 +921,27 @@ static inline void store_reg_from_load(DisasContext *s,
int reg, TCGv_i32 var)
#if TARGET_LONG_BITS == 32
#define DO_GEN_LD(SUFF, OPC) \
-static inline void gen_aa32_ld##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
+static inline void gen_aa32_ld##SUFF(DisasContext *s, TCGv_i32 val, \
+ TCGv_i32 addr, int index) \
{\
tcg_gen_qemu_ld_i32(val, addr, index, (OPC));\
}
#define DO_GEN_ST(SUFF, OPC) \
-static inline void gen_aa32_st##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
+static inline void gen_aa32_st##SUFF(DisasContext *s, TCGv_i32 val, \
+ TCGv_i32 addr, int index) \
{\
tcg_gen_qemu_st_i32(val, addr, index, (OPC));\
}
-static inline void gen_aa32_ld64(TCGv_i64 val, TCGv_i32 addr, int index)
+static inline void gen_aa32_ld64(DisasContext *s, TCGv_i64 val,
+ TCGv_i32 addr, int index)
{
tcg_gen_qemu_ld_i64(val, addr, index, MO_TEQ);
}
-static inline void gen_aa32_st64(TCGv_i64 val, TCGv_i32 addr, int index)
+static inline void gen_aa32_st64(DisasContext *s, TCGv_i64 val,
+ TCGv_i32 addr, int index)
{
tcg_gen_qemu_st_i64(val, addr, index, MO_TEQ);
}
@@ -945,7 +949,8 @@ static inline void gen_aa32_st64(TCGv_i64 val, TCGv_i32
addr, int index)
#else
#define DO_GEN_LD(SUFF, OPC) \
-static inline void gen_aa32_ld##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
+static inline void gen_aa32_ld##SUFF(DisasContext *s, TCGv_i32 val, \
+ TCGv_i32 addr, int index) \
{\
TCGv addr64 = tcg_temp_new();\
tcg_gen_extu_i32_i64(addr64, addr); \
@@ -954,7 +959,8 @@ static inline void gen_aa32_ld##SUFF(TCGv_i32 val, TCGv_i32
addr, int index) \
}
#define DO_GEN_ST(SUFF, OPC) \
-static inline void gen_aa32_st##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
+static inline void gen_aa32_st##SUFF(DisasContext *s, TCGv_i32 val, \
+ TCGv_i32 addr, int index) \
{\
TCGv addr64 = tcg_temp_new();\
tcg_gen_extu_i32_i64(addr64, addr); \
@@ -962,7 +968,8 @@ static inline void gen_aa32_st##SUFF(TCGv_i32 val, TCGv_i32
addr, int index) \
tcg_temp_free(addr64); \
}
-static inline void gen_aa32_ld64(TCGv_i64 val, TCGv_i32 addr, int index)
+static inline void gen_aa32_ld64(DisasContext *s, TCGv_i64 val,
+ TCGv_i32 addr, int index)
{
TCGv addr64 = tcg_temp_new();
tcg_gen_extu_i32_i64(addr64, addr);
@@ -970,7 +977,8 @@ static inline void gen_aa32_ld64(TCGv_i64 val, TCGv_i32
addr, int index)
tcg_temp_free(addr64);
}
-static inline void gen_aa32_st64(TCGv_i64 val, TCGv_i32 addr, int index)
+static inline void gen_aa32_st64(DisasContext *s, TCGv_i64 val,
+ TCGv_i32 addr, int index)
{
TCGv addr64 = tcg_temp_new();
tcg_gen_extu_i32_i64(addr64, addr);
@@ -1285,18 +1293,18 @@ VFP_GEN_FIX(ulto, )
static inline void gen_vfp_ld(DisasContext *s, int dp, TCGv_i32 addr)
{
if (dp) {
-gen_aa32_ld64(cpu_F0d, addr, get_mem_index(s));
+gen_aa32_ld64(s, cpu_F0d, addr, get_mem_index(s));
} else {
-gen_aa32_ld32u(cpu_F0s, addr, get_mem_index(s));
+gen_aa32_ld32u(s, cpu_F0s, addr, get_mem_index(s));
}
}
static inline void gen_vfp_st(DisasContext *s, int dp, TCGv_i32 addr)
{
if (dp) {
-gen_aa32_st64(cpu_F0d, addr, get_mem_index(s));
+gen_aa32_st64(s, cpu_F0d,
Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation
> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> >
> > > > cpu_exec_init(cs, &err);
> > > > if (err != NULL) {
> > > > error_propagate(errp, err);
> > > > return;
> > > > }
> > > > +scc->next_cpu_id = cs->cpu_index + 1;
> > >
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of
> > > arch_id
> > > for you. If it is just going to be monotonically increasing like
> > > cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs
> > > ?
> > >
> > > Note that cpu_exec_init(cs, &err) returns with the next available
> > > cpu_index
> > > which can be compared against max_cpus directly.
> > >
> > > Regards,
> > > Bharata.
> >
> > I don't think that we should mix the id setting and cpu_index for now.
> >
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().
>
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
>
> 1 Checking against max_cpus
> cpu_index can be used for this.
>
> 2 Checking if cpu with such an id exists
> cpu_exec_init() would never return with an in-use index. Hence cpu_index
> can be used here too given that you don't define ->get_arch_id()
>
> 3 Checking if the id is the next expected one
> cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
> always have the next available id to be used in cpu_index.
>
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?
After all the discussions about
-device-add s390-cpu,id=XX
As substitute/addition in the future for hotplug it is the straightforward
approach to allow setting the id as property. Nobody knows what crazy new
hotplug method we will come up with. But doing it the device way with properties
cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).
So I'd like to avoid reworking everything again, to realize later that we
want it as a property and rewriting it once again.
David
