Re: [Qemu-devel] [PATCH V5 1/4] Implement sync modes for drive-backup.

[Ian Main]

On Fri, Jul 19, 2013 at 04:11:13PM -0600, Eric Blake wrote:
> On 07/19/2013 03:49 PM, Ian Main wrote:
> >>> +++ b/qapi-schema.json
> >>> @@ -1807,6 +1807,10 @@
> >>>  # @format: #optional the format of the new destination, default is to
> >>>  #  probe if @mode is 'existing', else the format of the source
> >>>  #
> >>> +# @sync: what parts of the disk image should be copied to the destination
> >>> +#(all the disk, only the sectors allocated in the topmost image, 
> >>> or
> >>> +#only new I/O).
> >>> +#
> >>>  # @mode: #optional whether and how QEMU should create a new image, 
> >>> default is
> >>>  #'absolute-paths'.
> >>>  #
> >>
> >> This hunk is still wrong.  And we still haven't answered the
> >> meta-question on whether @format should be mandatory (which I'd rather
> >> fix that first, then rebase this on top of that).
> > 
> > How is it wrong when it cleanly applies?  You want it separated into
> > another patch?
> 
> It cleanly applied to the wrong place (drive-mirror), no thanks to git's
> automatic fuzzing.  It is _no longer necessary_ - DriveBackup already
> has this information.  I want it dropped completely.

I gotcha sorry I'm slow.

Ian

Re: [Qemu-devel] [RFC PATCH v1 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting

[Peter Crosthwaite]

Ping!

If theres not objections to the change pattern id like to proceed with the full
change.

Regards,
Peter

On Fri, Jul 12, 2013 at 2:29 PM,   wrote:
> From: Peter Crosthwaite 
>
> There are a mix of usages of the qemu_fdt_* API calls, some which
> wish to assert and abort QEMU on failure and some of which wish to do
> their own error handling. The latter in more correct, but the former
> is in the majority and more pragmatic, so facilitate both schemes by
> creating asserting and non asserting variants. This patch does this
> for qemu fdt_setprop and its wrapping users:
>
>  * qemu_fdt_setprop
>  * qemu_fdt_setprop_u64
>  * qemu_fdt_setprop_cells
>
> Error reporting is stylistically udpated to use Error ** instead of
> integer return codes and exit(1).
>
> Users of these APIs that ignore the return code are converted to using
> the _assert variants. Users that check the return code are converted to
> use Error ** for their error detection paths.
>
> Signed-off-by: Peter Crosthwaite 
> ---
> If this is ok, I will apply the change pattern to the entire
> qemu_fdt API
>
>  device_tree.c| 35 
>  hw/arm/boot.c|  7 ++--
>  hw/ppc/e500.c| 66 +++---
>  hw/ppc/e500plat.c|  6 +--
>  hw/ppc/mpc8544ds.c   |  6 +--
>  hw/ppc/ppc440_bamboo.c   |  8 ++--
>  include/sysemu/device_tree.h | 97 
> +---
>  7 files changed, 166 insertions(+), 59 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 048b8e1..ca2a88d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -4,8 +4,10 @@
>   * interface.
>   *
>   * Copyright 2008 IBM Corporation.
> + * Copyright 2013 Xilinx Inc.
>   * Authors: Jerone Young 
>   *  Hollis Blanchard 
> + *  Peter Crosthwaite 
>   *
>   * This work is licensed under the GNU GPL license version 2 or later.
>   *
> @@ -126,19 +128,25 @@ static int findnode_nofail(void *fdt, const char 
> *node_path)
>  return offset;
>  }
>
> -int qemu_fdt_setprop(void *fdt, const char *node_path,
> - const char *property, const void *val, int size)
> +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
> +  const void *val, int size, Error **errp)
>  {
>  int r;
>
>  r = fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, val, 
> size);
>  if (r < 0) {
> -fprintf(stderr, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> -property, fdt_strerror(r));
> -exit(1);
> +error_setg(errp, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> +   property, fdt_strerror(r));
>  }
> +}
>
> -return r;
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> + const char *property, const void *val, int size)
> +{
> +Error *errp = NULL;
> +
> +qemu_fdt_setprop(fdt, node_path, property, val, size, &errp);
> +assert_no_error(errp);
>  }
>
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> @@ -156,11 +164,20 @@ int qemu_fdt_setprop_cell(void *fdt, const char 
> *node_path,
>  return r;
>  }
>
> -int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> - const char *property, uint64_t val)
> +void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> +  const char *property, uint64_t val, Error **errp)
>  {
>  val = cpu_to_be64(val);
> -return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val));
> +qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val), errp);
> +}
> +
> +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
> + const char *property, uint64_t val)
> +{
> +Error *errp = NULL;
> +
> +qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp);
> +assert_no_error(errp);
>  }
>
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 2768b2b..9164bb9 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -233,6 +233,7 @@ static int load_dtb(hwaddr addr, const struct 
> arm_boot_info *binfo)
>  char *filename;
>  int size, rc;
>  uint32_t acells, scells, hival;
> +Error *errp = NULL;
>
>  filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>  if (!filename) {
> @@ -276,9 +277,9 @@ static int load_dtb(hwaddr addr, const struct 
> arm_boot_info *binfo)
>  goto fail;
>  }
>
> -rc = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> -  mem_reg_propsize * sizeof(uint32_t));
> -if (rc < 0) {
> +qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> + mem_reg_propsize * sizeof(uint32_t), &errp);
> +if (errp) {
>  fprintf(stderr, "couldn't set /memory/reg\n");
>  goto fail;
>  }
> d

Re: [Qemu-devel] [PATCH 4/4] exec: fix incorrect assumptions in memory_access_size

[Luiz Capitulino]

On Fri, 19 Jul 2013 22:07:58 +0200
Paolo Bonzini  wrote:

> access_size_min can be 1 because erroneous accesses must not crash
> QEMU, they should trigger exceptions in the guest or just return
> garbage (depending on the CPU).  I am not sure I understand the
> comment: placing a 4-byte field at the last byte of a region
> makes no sense (unless impl.unaligned is true), and that is
> why memory.c:access_with_adjusted_size does not bother with
> minimums larger than the remaining length.
> 
> access_size_max can be mr->ops->valid.max_access_size because memory.c
> can and will still break accesses bigger than
> mr->ops->impl.max_access_size.
> 
> Reported-by: Markus Armbruster 
> Tested-by: Markus Armbruster 
> Signed-off-by: Paolo Bonzini 

Yeah, works for me now:

Tested-by: Luiz Capitulino 

> ---
>  exec.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d312bb4..c8658c6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1898,14 +1898,10 @@ static inline bool 
> memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
> -unsigned access_size_min = mr->ops->impl.min_access_size;
> -unsigned access_size_max = mr->ops->impl.max_access_size;
> +unsigned access_size_max = mr->ops->valid.max_access_size;
>  
>  /* Regions are assumed to support 1-4 byte accesses unless
> otherwise specified.  */
> -if (access_size_min == 0) {
> -access_size_min = 1;
> -}
>  if (access_size_max == 0) {
>  access_size_max = 4;
>  }
> @@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, 
> unsigned l, hwaddr addr)
>  if (l > access_size_max) {
>  l = access_size_max;
>  }
> -/* ??? The users of this function are wrong, not supporting minimums 
> larger
> -   than the remaining length.  C.f. memory.c:access_with_adjusted_size.  
> */
> -assert(l >= access_size_min);
>  
>  return l;
>  }

[Qemu-devel] [PATCH V7 06/13] monitor: avoid direct use of global variable *mon_cmds

[Wenchao Xia]

New member *cmd_table is added in structure Monitor to avoid direct usage of
*mon_cmds. Now monitor have an associated command table, when global variable
*info_cmds is also discarded, structure Monitor would gain full control about
how to deal with user input.

Signed-off-by: Wenchao Xia 
---
 monitor.c |   14 +-
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3146820..8c95167 100644
--- a/monitor.c
+++ b/monitor.c
@@ -195,6 +195,7 @@ struct Monitor {
 CPUState *mon_cpu;
 BlockDriverCompletionFunc *password_completion_cb;
 void *password_opaque;
+mon_cmd_t *cmd_table;
 QError *error;
 QLIST_HEAD(,mon_fd_t) fds;
 QLIST_ENTRY(Monitor) entry;
@@ -757,7 +758,7 @@ static void help_cmd(Monitor *mon, const char *name)
 if (name && !strcmp(name, "info")) {
 help_cmd_dump(mon, info_cmds, "info ", NULL);
 } else {
-help_cmd_dump(mon, mon_cmds, "", name);
+help_cmd_dump(mon, mon->cmd_table, "", name);
 if (name && !strcmp(name, "log")) {
 const QEMULogItem *item;
 monitor_printf(mon, "Log items (comma separated):\n");
@@ -3980,7 +3981,7 @@ static void handle_user_command(Monitor *mon, const char 
*cmdline)
 
 qdict = qdict_new();
 
-cmd = monitor_parse_command(mon, cmdline, 0, mon_cmds, qdict);
+cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
 if (!cmd)
 goto out;
 
@@ -4169,12 +4170,12 @@ static void monitor_find_completion(Monitor *mon,
 else
 cmdname = args[0];
 readline_set_completion_index(mon->rs, strlen(cmdname));
-for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
+for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
 cmd_completion(mon, cmdname, cmd->name);
 }
 } else {
 /* find the command */
-for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
+for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
 if (compare_cmd(args[0], cmd->name)) {
 break;
 }
@@ -4225,7 +4226,7 @@ static void monitor_find_completion(Monitor *mon,
 }
 } else if (!strcmp(cmd->name, "help|?")) {
 readline_set_completion_index(mon->rs, strlen(str));
-for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
+for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
 cmd_completion(mon, str, cmd->name);
 }
 }
@@ -4788,6 +4789,9 @@ void monitor_init(CharDriverState *chr, int flags)
 if (!default_mon || (flags & MONITOR_IS_DEFAULT))
 default_mon = mon;
 
+/* Use *mon_cmds by default. */
+mon->cmd_table = mon_cmds;
+
 sortcmdlist();
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V7 08/13] monitor: refine parse_cmdline()

[Wenchao Xia]

Since this function will be used by help_cmd() later, so improve
it to make it more generic and easier to use. free_cmdline_args()
is added to as paired function to free the result.

One change of this function is that, when the valid args in input
exceed the limit of MAX_ARGS, it fails now, instead of return with
MAX_ARGS of parsed args in old code. This should not impact much
since it is rare that user input many args in monitor's "help" and
auto complete scenario.

Signed-off-by: Wenchao Xia 
---
 monitor.c |   51 ---
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 177eb85..b68e145 100644
--- a/monitor.c
+++ b/monitor.c
@@ -809,9 +809,33 @@ static int get_str(char *buf, int buf_size, const char 
**pp)
 
 #define MAX_ARGS 16
 
-/* NOTE: this parser is an approximate form of the real command parser */
-static void parse_cmdline(const char *cmdline,
-  int *pnb_args, char **args)
+static void free_cmdline_args(char **args, int nb_args)
+{
+int i;
+
+assert(nb_args <= MAX_ARGS);
+
+for (i = 0; i < nb_args; i++) {
+g_free(args[i]);
+}
+
+}
+
+/*
+ * Parse the command line to get valid args.
+ * @cmdline: command line to be parsed.
+ * @pnb_args: location to store the number of args, must NOT be NULL.
+ * @args: location to store the args, which should be freed by caller, must
+ *NOT be NULL.
+ *
+ * Returns 0 on success, negative on failure.
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ *   of args have a limit of MAX_ARGS. If cmdline contains more, it will
+ *   return with failure.
+ */
+static int parse_cmdline(const char *cmdline,
+ int *pnb_args, char **args)
 {
 const char *p;
 int nb_args, ret;
@@ -827,16 +851,21 @@ static void parse_cmdline(const char *cmdline,
 break;
 }
 if (nb_args >= MAX_ARGS) {
-break;
+goto fail;
 }
 ret = get_str(buf, sizeof(buf), &p);
-args[nb_args] = g_strdup(buf);
-nb_args++;
 if (ret < 0) {
-break;
+goto fail;
 }
+args[nb_args] = g_strdup(buf);
+nb_args++;
 }
 *pnb_args = nb_args;
+return 0;
+
+ fail:
+free_cmdline_args(args, nb_args);
+return -1;
 }
 
 static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
@@ -4152,7 +4181,9 @@ static void monitor_find_completion(Monitor *mon,
 const mon_cmd_t *cmd;
 MonitorBlockComplete mbs;
 
-parse_cmdline(cmdline, &nb_args, args);
+if (parse_cmdline(cmdline, &nb_args, args) < 0) {
+return;
+}
 #ifdef DEBUG_COMPLETION
 for (i = 0; i < nb_args; i++) {
 monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
@@ -4242,9 +4273,7 @@ static void monitor_find_completion(Monitor *mon,
 }
 
 cleanup:
-for (i = 0; i < nb_args; i++) {
-g_free(args[i]);
-}
+free_cmdline_args(args, nb_args);
 }
 
 static int monitor_can_read(void *opaque)
-- 
1.7.1

[Qemu-devel] [PATCH V7 05/13] monitor: avoid use of global *cur_mon in readline_completion()

[Wenchao Xia]

Now all completion functions do not use *cur_mon any more, instead
they use rs->mon. In short, structure ReadLineState decide where
the complete action would be taken now.

Tested with the case that qemu have two telnet monitors, auto
completion function works normal.

Signed-off-by: Wenchao Xia 
---
 readline.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/readline.c b/readline.c
index c91b324..abf27dd 100644
--- a/readline.c
+++ b/readline.c
@@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int 
index)
 
 static void readline_completion(ReadLineState *rs)
 {
-Monitor *mon = cur_mon;
 int len, i, j, max_width, nb_cols, max_prefix;
 char *cmdline;
 
@@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
 if (len > 0 && rs->completions[0][len - 1] != '/')
 readline_insert_char(rs, ' ');
 } else {
-monitor_printf(mon, "\n");
+monitor_printf(rs->mon, "\n");
 max_width = 0;
 max_prefix = 0;
 for(i = 0; i < rs->nb_completions; i++) {
-- 
1.7.1

[Qemu-devel] [PATCH V7 04/13] monitor: avoid use of global *cur_mon in monitor_find_completion()

[Wenchao Xia]

Parameter *mon is added, and local variable *mon added in previous patch
is removed. The caller readline_completion(), pass rs->mon as value, which
should be initialized in readline_init() called by monitor_init().

Signed-off-by: Wenchao Xia 
---
 include/monitor/readline.h |3 ++-
 monitor.c  |   18 +-
 readline.c |2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/monitor/readline.h b/include/monitor/readline.h
index fc9806e..0faf6e1 100644
--- a/include/monitor/readline.h
+++ b/include/monitor/readline.h
@@ -8,7 +8,8 @@
 #define READLINE_MAX_COMPLETIONS 256
 
 typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
-typedef void ReadLineCompletionFunc(const char *cmdline);
+typedef void ReadLineCompletionFunc(Monitor *mon,
+const char *cmdline);
 
 typedef struct ReadLineState {
 char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
diff --git a/monitor.c b/monitor.c
index 10ebabc..3146820 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4136,20 +4136,20 @@ static const char *next_arg_type(const char *typestr)
 return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(const char *cmdline)
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
 {
 const char *cmdname;
 char *args[MAX_ARGS];
 int nb_args, i, len;
 const char *ptype, *str;
 const mon_cmd_t *cmd;
-Monitor *mon = cur_mon;
 MonitorBlockComplete mbs;
 
 parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
-for(i = 0; i < nb_args; i++) {
-monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
+for (i = 0; i < nb_args; i++) {
+monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
 }
 #endif
 
@@ -4168,7 +4168,7 @@ static void monitor_find_completion(const char *cmdline)
 cmdname = "";
 else
 cmdname = args[0];
-readline_set_completion_index(cur_mon->rs, strlen(cmdname));
+readline_set_completion_index(mon->rs, strlen(cmdname));
 for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
 cmd_completion(mon, cmdname, cmd->name);
 }
@@ -4198,7 +4198,7 @@ static void monitor_find_completion(const char *cmdline)
 switch(*ptype) {
 case 'F':
 /* file completion */
-readline_set_completion_index(cur_mon->rs, strlen(str));
+readline_set_completion_index(mon->rs, strlen(str));
 file_completion(mon, str);
 break;
 case 'B':
@@ -4211,7 +4211,7 @@ static void monitor_find_completion(const char *cmdline)
 case 's':
 /* XXX: more generic ? */
 if (!strcmp(cmd->name, "info")) {
-readline_set_completion_index(cur_mon->rs, strlen(str));
+readline_set_completion_index(mon->rs, strlen(str));
 for(cmd = info_cmds; cmd->name != NULL; cmd++) {
 cmd_completion(mon, str, cmd->name);
 }
@@ -4219,12 +4219,12 @@ static void monitor_find_completion(const char *cmdline)
 char *sep = strrchr(str, '-');
 if (sep)
 str = sep + 1;
-readline_set_completion_index(cur_mon->rs, strlen(str));
+readline_set_completion_index(mon->rs, strlen(str));
 for (i = 0; i < Q_KEY_CODE_MAX; i++) {
 cmd_completion(mon, str, QKeyCode_lookup[i]);
 }
 } else if (!strcmp(cmd->name, "help|?")) {
-readline_set_completion_index(cur_mon->rs, strlen(str));
+readline_set_completion_index(mon->rs, strlen(str));
 for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
 cmd_completion(mon, str, cmd->name);
 }
diff --git a/readline.c b/readline.c
index 1c0f7ee..c91b324 100644
--- a/readline.c
+++ b/readline.c
@@ -285,7 +285,7 @@ static void readline_completion(ReadLineState *rs)
 cmdline = g_malloc(rs->cmd_buf_index + 1);
 memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
 cmdline[rs->cmd_buf_index] = '\0';
-rs->completion_finder(cmdline);
+rs->completion_finder(rs->mon, cmdline);
 g_free(cmdline);
 
 /* no completion found */
-- 
1.7.1

[Qemu-devel] [PATCH V7 03/13] monitor: avoid use of global *cur_mon in block_completion_it()

[Wenchao Xia]

Signed-off-by: Wenchao Xia 
---
 monitor.c |   18 ++
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 0f92bca..10ebabc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4086,14 +4086,21 @@ static void file_completion(Monitor *mon, const char 
*input)
 closedir(ffs);
 }
 
+typedef struct MonitorBlockComplete {
+Monitor *mon;
+const char *input;
+} MonitorBlockComplete;
+
 static void block_completion_it(void *opaque, BlockDriverState *bs)
 {
 const char *name = bdrv_get_device_name(bs);
-const char *input = opaque;
+MonitorBlockComplete *mbc = opaque;
+Monitor *mon = mbc->mon;
+const char *input = mbc->input;
 
 if (input[0] == '\0' ||
 !strncmp(name, (char *)input, strlen(input))) {
-readline_add_completion(cur_mon->rs, name);
+readline_add_completion(mon->rs, name);
 }
 }
 
@@ -4137,6 +4144,7 @@ static void monitor_find_completion(const char *cmdline)
 const char *ptype, *str;
 const mon_cmd_t *cmd;
 Monitor *mon = cur_mon;
+MonitorBlockComplete mbs;
 
 parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
@@ -4195,8 +4203,10 @@ static void monitor_find_completion(const char *cmdline)
 break;
 case 'B':
 /* block device name completion */
-readline_set_completion_index(cur_mon->rs, strlen(str));
-bdrv_iterate(block_completion_it, (void *)str);
+mbs.mon = mon;
+mbs.input = str;
+readline_set_completion_index(mon->rs, strlen(str));
+bdrv_iterate(block_completion_it, &mbs);
 break;
 case 's':
 /* XXX: more generic ? */
-- 
1.7.1

[Qemu-devel] [PATCH V7 13/13] monitor: improve auto complete of "help" for single command in sub group

[Wenchao Xia]

Now special case "help *" in auto completion can work with sub commands,
such as "help info u*".

Signed-off-by: Wenchao Xia 
---
 monitor.c |6 ++
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index ea3ed96..95fa918 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4314,10 +4314,8 @@ static void monitor_find_completion_by_table(Monitor 
*mon,
 cmd_completion(mon, str, QKeyCode_lookup[i]);
 }
 } else if (!strcmp(cmd->name, "help|?")) {
-readline_set_completion_index(mon->rs, strlen(str));
-for (cmd = cmd_table; cmd->name != NULL; cmd++) {
-cmd_completion(mon, str, cmd->name);
-}
+monitor_find_completion_by_table(mon, cmd_table,
+ &args[1], nb_args - 1);
 }
 break;
 default:
-- 
1.7.1

[Qemu-devel] [PATCH V7 10/13] monitor: refine monitor_find_completion()

[Wenchao Xia]

In order to support sub command in auto completion, an reentrant function
is needed, so monitor_find_completion() is splitted into two parts. The
first part does parsing of user input which need to be done only once,
the second part does the auto completion job according to the parsing
result, which contains the necessary code to support sub command and
works as the reentrant function. The global "info_cmds" is still used
in second part, which will be replaced by sub command code later.

Signed-off-by: Wenchao Xia 
---
 monitor.c |   65 
 1 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5aa65d6..b6e7b64 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4214,34 +4214,17 @@ static const char *next_arg_type(const char *typestr)
 return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(Monitor *mon,
-const char *cmdline)
+static void monitor_find_completion_by_table(Monitor *mon,
+ const mon_cmd_t *cmd_table,
+ char **args,
+ int nb_args)
 {
 const char *cmdname;
-char *args[MAX_ARGS];
-int nb_args, i, len;
+int i;
 const char *ptype, *str;
 const mon_cmd_t *cmd;
 MonitorBlockComplete mbs;
 
-if (parse_cmdline(cmdline, &nb_args, args) < 0) {
-return;
-}
-#ifdef DEBUG_COMPLETION
-for (i = 0; i < nb_args; i++) {
-monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
-}
-#endif
-
-/* if the line ends with a space, it means we want to complete the
-   next arg */
-len = strlen(cmdline);
-if (len > 0 && qemu_isspace(cmdline[len - 1])) {
-if (nb_args >= MAX_ARGS) {
-goto cleanup;
-}
-args[nb_args++] = g_strdup("");
-}
 if (nb_args <= 1) {
 /* command completion */
 if (nb_args == 0)
@@ -4249,18 +4232,18 @@ static void monitor_find_completion(Monitor *mon,
 else
 cmdname = args[0];
 readline_set_completion_index(mon->rs, strlen(cmdname));
-for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+for (cmd = cmd_table; cmd->name != NULL; cmd++) {
 cmd_completion(mon, cmdname, cmd->name);
 }
 } else {
 /* find the command */
-for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+for (cmd = cmd_table; cmd->name != NULL; cmd++) {
 if (compare_cmd(args[0], cmd->name)) {
 break;
 }
 }
 if (!cmd->name) {
-goto cleanup;
+return;
 }
 
 ptype = next_arg_type(cmd->args_type);
@@ -4305,7 +4288,7 @@ static void monitor_find_completion(Monitor *mon,
 }
 } else if (!strcmp(cmd->name, "help|?")) {
 readline_set_completion_index(mon->rs, strlen(str));
-for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+for (cmd = cmd_table; cmd->name != NULL; cmd++) {
 cmd_completion(mon, str, cmd->name);
 }
 }
@@ -4314,6 +4297,36 @@ static void monitor_find_completion(Monitor *mon,
 break;
 }
 }
+}
+
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
+{
+char *args[MAX_ARGS];
+int nb_args, len;
+
+/* 1. parse the cmdline */
+if (parse_cmdline(cmdline, &nb_args, args) < 0) {
+return;
+}
+#ifdef DEBUG_COMPLETION
+for (i = 0; i < nb_args; i++) {
+monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
+}
+#endif
+
+/* if the line ends with a space, it means we want to complete the
+   next arg */
+len = strlen(cmdline);
+if (len > 0 && qemu_isspace(cmdline[len - 1])) {
+if (nb_args >= MAX_ARGS) {
+goto cleanup;
+}
+args[nb_args++] = g_strdup("");
+}
+
+/* 2. auto complete according to args */
+monitor_find_completion_by_table(mon, mon->cmd_table, args, nb_args);
 
 cleanup:
 free_cmdline_args(args, nb_args);
-- 
1.7.1

[Qemu-devel] [PATCH V7 12/13] monitor: allow "help" show message for single command in sub group

[Wenchao Xia]

A new parameter type 'S' is introduced to allow user input any string.
"help info block" works normal now.

Signed-off-by: Wenchao Xia 
---
 hmp-commands.hx |2 +-
 monitor.c   |   27 +++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..c161fe9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -11,7 +11,7 @@ ETEXI
 
 {
 .name   = "help|?",
-.args_type  = "name:s?",
+.args_type  = "name:S?",
 .params = "[cmd]",
 .help   = "show the help",
 .mhandler.cmd = do_help_cmd,
diff --git a/monitor.c b/monitor.c
index 4430aa3..ea3ed96 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
  * 'F'  filename
  * 'B'  block device name
  * 's'  string (accept optional quote)
+ * 'S'  it just appends the rest of the string (accept optional quote)
  * 'O'  option string of the form NAME=VALUE,...
  *  parsed according to QemuOptsList given by its name
  *  Example: 'device:O' uses qemu_device_opts.
@@ -4031,6 +4032,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
*mon,
 }
 }
 break;
+case 'S':
+{
+/* package all remaining string */
+int len;
+
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*typestr == '?') {
+typestr++;
+if (*p == '\0') {
+/* no remaining string: NULL argument */
+break;
+}
+}
+len = strlen(p);
+if (len <= 0) {
+monitor_printf(mon, "%s: string expected\n",
+   cmdname);
+break;
+}
+qdict_put(qdict, key, qstring_from_str(p));
+p += len;
+}
+break;
 default:
 bad_type:
 monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
@@ -4278,6 +4304,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
 bdrv_iterate(block_completion_it, &mbs);
 break;
 case 's':
+case 'S':
 if (!strcmp(cmd->name, "sendkey")) {
 char *sep = strrchr(str, '-');
 if (sep)
-- 
1.7.1

[Qemu-devel] [PATCH V7 01/13] monitor: avoid use of global *cur_mon in cmd_completion()

[Wenchao Xia]

A new local variable *mon is added in monitor_find_completion()
to make compile pass, which will be removed later in
convertion patch for monitor_find_completion().

Signed-off-by: Wenchao Xia 
---
 monitor.c |   13 +++--
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6db2ba4..b88c02b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4004,7 +4004,7 @@ out:
 QDECREF(qdict);
 }
 
-static void cmd_completion(const char *name, const char *list)
+static void cmd_completion(Monitor *mon, const char *name, const char *list)
 {
 const char *p, *pstart;
 char cmd[128];
@@ -4022,7 +4022,7 @@ static void cmd_completion(const char *name, const char 
*list)
 memcpy(cmd, pstart, len);
 cmd[len] = '\0';
 if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
-readline_add_completion(cur_mon->rs, cmd);
+readline_add_completion(mon->rs, cmd);
 }
 if (*p == '\0')
 break;
@@ -4136,6 +4136,7 @@ static void monitor_find_completion(const char *cmdline)
 int nb_args, i, len;
 const char *ptype, *str;
 const mon_cmd_t *cmd;
+Monitor *mon = cur_mon;
 
 parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
@@ -4161,7 +4162,7 @@ static void monitor_find_completion(const char *cmdline)
 cmdname = args[0];
 readline_set_completion_index(cur_mon->rs, strlen(cmdname));
 for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
-cmd_completion(cmdname, cmd->name);
+cmd_completion(mon, cmdname, cmd->name);
 }
 } else {
 /* find the command */
@@ -4202,7 +4203,7 @@ static void monitor_find_completion(const char *cmdline)
 if (!strcmp(cmd->name, "info")) {
 readline_set_completion_index(cur_mon->rs, strlen(str));
 for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-cmd_completion(str, cmd->name);
+cmd_completion(mon, str, cmd->name);
 }
 } else if (!strcmp(cmd->name, "sendkey")) {
 char *sep = strrchr(str, '-');
@@ -4210,12 +4211,12 @@ static void monitor_find_completion(const char *cmdline)
 str = sep + 1;
 readline_set_completion_index(cur_mon->rs, strlen(str));
 for (i = 0; i < Q_KEY_CODE_MAX; i++) {
-cmd_completion(str, QKeyCode_lookup[i]);
+cmd_completion(mon, str, QKeyCode_lookup[i]);
 }
 } else if (!strcmp(cmd->name, "help|?")) {
 readline_set_completion_index(cur_mon->rs, strlen(str));
 for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
-cmd_completion(str, cmd->name);
+cmd_completion(mon, str, cmd->name);
 }
 }
 break;
-- 
1.7.1

[Qemu-devel] [PATCH V7 00/13] monitor: support sub command group in auto completion and help

[Wenchao Xia]

This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, "info" is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.

Patch 5 replaced cur_mon with rs->mon, it is safe because:
monitor_init() calls readline_init() which initialize mon->rs, result is
mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, which make
monitor_read() function take *mon as its opaque. Later, when user input,
monitor_read() is called, where cur_mon is set to *mon by "cur_mon = opaque".
If qemu's monitors run in one thread, then later in readline_handle_byte()
and readline_comletion(), cur_mon is actually equal to rs->mon, in another
word, it points to the monitor instance, so it is safe to replace *cur_mon
in those functions.

Thanks for Luiz's reviewing.

V2:
  General:
  To discard *info_comds more graceful, help related function is modified to 
support
sub command too.
  Patch 6/7 are added to improve help related functions.
  Patch 5: not directly return to make sure args are freed.

  Address Luiz's comments:
  Split patch into small series.
  struct mon_cmd_t was not moved into header file, instead mon_cmnd_t 
*cmd_table is
added as a member in struct Monitor.
  5/7: drop original code comments for "info" in monitor_find_completion().

v3:
  5/7: add parameter **args_cmdline in parse_cmdline() to tell next valid
parameter's position. This fix the issue in case command length in input is not
equal to its name's length such as "help|?", and the case input start with
space such as "  s". 
  7/7: better commit message.

v4:
  Address Eric's comments:
  1/7, 2/7, 4/7: better commit title and message.
  1/7 remove useless (char *) in old code, add space around "for ()" in old 
code.
  3/7: separate code moving patch before usage.
  4/7: add space around "for ()" in old code, add min(nb_args, MAX_ARGS) in free
to make code stronger.

v5:
  4/7: use "a < b ? a : b" instead of macro min.

v6:
  Address Luiz's comments:
  1/13 ~ 5/13: splitted small patches.
  5/13: added commit message about the correctness of replacing of cur_mon and
test result.
  6/13: better comments in code.
  7/13: added commit message about the reason of code moving.
  8/13: new patch to improve parse_cmdline(), since it is a more generic
function now.
  9/13: reworked the commit message, better commentes in code, use
free_cmdline_args() in clean. It is a bit hard to split this patch into
smaller meaning ful ones, so kepted this patch as a relative larger one,
with better commit message.
  12/13: put case 'S' with case 's' in monitor_find_completion_by_table().
moved this patch ahead of patch 13/13.
  13/13: this patch is moved behind patch 12/13.

  Generic change:
  10/13: splitted patch which moved out the reentrant part into a separate
function, make review easier. This also avoided re-parsing the command line
which does in previous version.
  11/13: splitted patch, which simply remove usage of info_cmds and support
sub command by re-enter the function.

v7:
  5/13: moved the comments why the change is safe, to cover-letter.
  8/13: use assert in free_cmdline_args(), fail when args in input exceed
the limit in parse_cmdline().

Wenchao Xia (13):
  1 monitor: avoid use of global *cur_mon in cmd_completion()
  2 monitor: avoid use of global *cur_mon in file_completion()
  3 monitor: avoid use of global *cur_mon in block_completion_it()
  4 monitor: avoid use of global *cur_mon in monitor_find_completion()
  5 monitor: avoid use of global *cur_mon in readline_completion()
  6 monitor: avoid direct use of global variable *mon_cmds
  7 monitor: code move for parse_cmdline()
  8 monitor: refine parse_cmdline()
  9 monitor: support sub command in help
  10 monitor: refine monitor_find_completion()
  11 monitor: support sub command in auto completion
  12 monitor: allow "help" show message for single command in sub group
  13 monitor: improve auto complete of "help" for single command in sub group

 hmp-commands.hx|2 +-
 include/monitor/readline.h |3 +-
 monitor.c  |  438 
 readline.c |5 +-
 4 files changed, 289 insertions(+), 159 deletions(-)

[Qemu-devel] [PATCH V7 07/13] monitor: code move for parse_cmdline()

[Wenchao Xia]

help_cmd() need this function later, so move it. get_str() is called by
parse_cmdline() so it is moved also. Some code style error reported by
check script, is also fixed.

Signed-off-by: Wenchao Xia 
---
 monitor.c |  191 +++--
 1 files changed, 98 insertions(+), 93 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8c95167..177eb85 100644
--- a/monitor.c
+++ b/monitor.c
@@ -741,6 +741,104 @@ static int compare_cmd(const char *name, const char *list)
 return 0;
 }
 
+static int get_str(char *buf, int buf_size, const char **pp)
+{
+const char *p;
+char *q;
+int c;
+
+q = buf;
+p = *pp;
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*p == '\0') {
+fail:
+*q = '\0';
+*pp = p;
+return -1;
+}
+if (*p == '\"') {
+p++;
+while (*p != '\0' && *p != '\"') {
+if (*p == '\\') {
+p++;
+c = *p++;
+switch (c) {
+case 'n':
+c = '\n';
+break;
+case 'r':
+c = '\r';
+break;
+case '\\':
+case '\'':
+case '\"':
+break;
+default:
+qemu_printf("unsupported escape code: '\\%c'\n", c);
+goto fail;
+}
+if ((q - buf) < buf_size - 1) {
+*q++ = c;
+}
+} else {
+if ((q - buf) < buf_size - 1) {
+*q++ = *p;
+}
+p++;
+}
+}
+if (*p != '\"') {
+qemu_printf("unterminated string\n");
+goto fail;
+}
+p++;
+} else {
+while (*p != '\0' && !qemu_isspace(*p)) {
+if ((q - buf) < buf_size - 1) {
+*q++ = *p;
+}
+p++;
+}
+}
+*q = '\0';
+*pp = p;
+return 0;
+}
+
+#define MAX_ARGS 16
+
+/* NOTE: this parser is an approximate form of the real command parser */
+static void parse_cmdline(const char *cmdline,
+  int *pnb_args, char **args)
+{
+const char *p;
+int nb_args, ret;
+char buf[1024];
+
+p = cmdline;
+nb_args = 0;
+for (;;) {
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*p == '\0') {
+break;
+}
+if (nb_args >= MAX_ARGS) {
+break;
+}
+ret = get_str(buf, sizeof(buf), &p);
+args[nb_args] = g_strdup(buf);
+nb_args++;
+if (ret < 0) {
+break;
+}
+}
+*pnb_args = nb_args;
+}
+
 static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
   const char *prefix, const char *name)
 {
@@ -3417,71 +3515,6 @@ static int get_double(Monitor *mon, double *pval, const 
char **pp)
 return 0;
 }
 
-static int get_str(char *buf, int buf_size, const char **pp)
-{
-const char *p;
-char *q;
-int c;
-
-q = buf;
-p = *pp;
-while (qemu_isspace(*p))
-p++;
-if (*p == '\0') {
-fail:
-*q = '\0';
-*pp = p;
-return -1;
-}
-if (*p == '\"') {
-p++;
-while (*p != '\0' && *p != '\"') {
-if (*p == '\\') {
-p++;
-c = *p++;
-switch(c) {
-case 'n':
-c = '\n';
-break;
-case 'r':
-c = '\r';
-break;
-case '\\':
-case '\'':
-case '\"':
-break;
-default:
-qemu_printf("unsupported escape code: '\\%c'\n", c);
-goto fail;
-}
-if ((q - buf) < buf_size - 1) {
-*q++ = c;
-}
-} else {
-if ((q - buf) < buf_size - 1) {
-*q++ = *p;
-}
-p++;
-}
-}
-if (*p != '\"') {
-qemu_printf("unterminated string\n");
-goto fail;
-}
-p++;
-} else {
-while (*p != '\0' && !qemu_isspace(*p)) {
-if ((q - buf) < buf_size - 1) {
-*q++ = *p;
-}
-p++;
-}
-}
-*q = '\0';
-*pp = p;
-return 0;
-}
-
 /*
  * Store the command-name in cmdname, and return a pointer to
  * the remaining of the command string.
@@ -3538,8 +3571,6 @@ static char *key_get_info(const char *type, char **key)
 static int default_fmt_format = 'x';
 static int default_fmt_size = 4;
 
-#define MAX_ARGS 16
-
 static int is_valid_option(const char *c, const char *typestr)
 {
 char option[3];
@@ -4105,32 +4136,6 @@ static void block

[Qemu-devel] [PATCH V7 11/13] monitor: support sub command in auto completion

[Wenchao Xia]

This patch allow auto completion work normal for sub command case,
"info block [DEVICE]" can auto complete now, by re-enter the completion
function. In original code "info" is treated as a special case, now it
is treated as a sub command group, global variable info_cmds is not used
any more.

"help" command is still treated as a special case, since it is not a sub
command group but want to auto complete command in root command table.

Signed-off-by: Wenchao Xia 
---
 monitor.c |   14 +++---
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index b6e7b64..4430aa3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4246,6 +4246,12 @@ static void monitor_find_completion_by_table(Monitor 
*mon,
 return;
 }
 
+if (cmd->sub_table) {
+/* do the job again */
+return monitor_find_completion_by_table(mon, cmd->sub_table,
+&args[1], nb_args - 1);
+}
+
 ptype = next_arg_type(cmd->args_type);
 for(i = 0; i < nb_args - 2; i++) {
 if (*ptype != '\0') {
@@ -4272,13 +4278,7 @@ static void monitor_find_completion_by_table(Monitor 
*mon,
 bdrv_iterate(block_completion_it, &mbs);
 break;
 case 's':
-/* XXX: more generic ? */
-if (!strcmp(cmd->name, "info")) {
-readline_set_completion_index(mon->rs, strlen(str));
-for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-cmd_completion(mon, str, cmd->name);
-}
-} else if (!strcmp(cmd->name, "sendkey")) {
+if (!strcmp(cmd->name, "sendkey")) {
 char *sep = strrchr(str, '-');
 if (sep)
 str = sep + 1;
-- 
1.7.1

[Qemu-devel] [PATCH V7 02/13] monitor: avoid use of global *cur_mon in file_completion()

[Wenchao Xia]

Signed-off-by: Wenchao Xia 
---
 monitor.c |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index b88c02b..0f92bca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4030,7 +4030,7 @@ static void cmd_completion(Monitor *mon, const char 
*name, const char *list)
 }
 }
 
-static void file_completion(const char *input)
+static void file_completion(Monitor *mon, const char *input)
 {
 DIR *ffs;
 struct dirent *d;
@@ -4053,7 +4053,7 @@ static void file_completion(const char *input)
 pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
 }
 #ifdef DEBUG_COMPLETION
-monitor_printf(cur_mon, "input='%s' path='%s' prefix='%s'\n",
+monitor_printf(mon, "input='%s' path='%s' prefix='%s'\n",
input, path, file_prefix);
 #endif
 ffs = opendir(path);
@@ -4080,7 +4080,7 @@ static void file_completion(const char *input)
 if (stat(file, &sb) == 0 && S_ISDIR(sb.st_mode)) {
 pstrcat(file, sizeof(file), "/");
 }
-readline_add_completion(cur_mon->rs, file);
+readline_add_completion(mon->rs, file);
 }
 }
 closedir(ffs);
@@ -4191,7 +4191,7 @@ static void monitor_find_completion(const char *cmdline)
 case 'F':
 /* file completion */
 readline_set_completion_index(cur_mon->rs, strlen(str));
-file_completion(str);
+file_completion(mon, str);
 break;
 case 'B':
 /* block device name completion */
-- 
1.7.1

[Qemu-devel] [PATCH V7 09/13] monitor: support sub command in help

[Wenchao Xia]

The old code in help_cmd() use global 'info_cmds' and treat it as a
special case. Actually 'info_cmds' is an sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() need to change its work
mechanism to support sub command and not treat it as a special case
any more.

To support sub command, help_cmd() will first parse the input and then call
help_cmd_dump(), which works as an reentrant function. When it mets sub
command, it simply re-enter the function again. Since help dumping need to
know whole input to printf full help message include prefix, for example,
"help info block" need to printf prefix "info", so help_cmd_dump() takes all
args from input and extra parameter arg_index to identify the progress.
Another function help_cmd_dump_one() is introduced to printf the prefix
and command's help message.

Now help support sub command, so later if another sub command group is
added in any depth, help will automatically work for it. Still "help info
block" will show error since command parser reject additional parameter,
which can be improved later. "log" is still treated as a special case.

Signed-off-by: Wenchao Xia 
---
 monitor.c |   63 +++-
 1 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index b68e145..5aa65d6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -868,33 +868,76 @@ static int parse_cmdline(const char *cmdline,
 return -1;
 }
 
+static void help_cmd_dump_one(Monitor *mon,
+  const mon_cmd_t *cmd,
+  char **prefix_args,
+  int prefix_args_nb)
+{
+int i;
+
+for (i = 0; i < prefix_args_nb; i++) {
+monitor_printf(mon, "%s ", prefix_args[i]);
+}
+monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
+}
+
+/* @args[@arg_index] is the valid command need to find in @cmds */
 static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
-  const char *prefix, const char *name)
+  char **args, int nb_args, int arg_index)
 {
 const mon_cmd_t *cmd;
 
-for(cmd = cmds; cmd->name != NULL; cmd++) {
-if (!name || !strcmp(name, cmd->name))
-monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name,
-   cmd->params, cmd->help);
+/* No valid arg need to compare with, dump all in *cmds */
+if (arg_index >= nb_args) {
+for (cmd = cmds; cmd->name != NULL; cmd++) {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+return;
+}
+
+/* Find one entry to dump */
+for (cmd = cmds; cmd->name != NULL; cmd++) {
+if (compare_cmd(args[arg_index], cmd->name)) {
+if (cmd->sub_table) {
+/* continue with next arg */
+help_cmd_dump(mon, cmd->sub_table,
+  args, nb_args, arg_index + 1);
+} else {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+break;
+}
 }
 }
 
 static void help_cmd(Monitor *mon, const char *name)
 {
-if (name && !strcmp(name, "info")) {
-help_cmd_dump(mon, info_cmds, "info ", NULL);
-} else {
-help_cmd_dump(mon, mon->cmd_table, "", name);
-if (name && !strcmp(name, "log")) {
+char *args[MAX_ARGS];
+int nb_args = 0;
+
+/* 1. parse user input */
+if (name) {
+/* special case for log, directly dump and return */
+if (!strcmp(name, "log")) {
 const QEMULogItem *item;
 monitor_printf(mon, "Log items (comma separated):\n");
 monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
 for (item = qemu_log_items; item->mask != 0; item++) {
 monitor_printf(mon, "%-10s %s\n", item->name, item->help);
 }
+return;
+}
+
+if (parse_cmdline(name, &nb_args, args) < 0) {
+goto cleanup;
 }
 }
+
+/* 2. dump the contents according to parsed args */
+help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
+
+cleanup:
+free_cmdline_args(args, nb_args);
 }
 
 static void do_help_cmd(Monitor *mon, const QDict *qdict)
-- 
1.7.1

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

On 07/20/2013 10:55 AM, Alexey Kardashevskiy wrote:
> On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
>> Ok. So.
>>
>> What broke is...
>> I could try explaining but backtraces are lot better :)
>>
>> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
>> we had a workaround in spapr_io_ops), now it works so double swap happens
>> and everything gets broken.
>>
>> If we talk about VGA (in powerpc, it is all about powerpc), I guess
>> memory_region_iorange_write() will go through mr->ops->old_portio branch
>> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
>> fine here. I do not understand yet why it works on mac99 though, too late
>> here :)
> 
> 
> I understood. VGA does not work for mac99 either with this command line:
> ./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
> So it works for pseries only because of parity bug in spapr_io_ops.


oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops
does the swapping in this case and portio_ops does not swap (in upstream).

Oh. Ah. Uh.

Adding cc:Benh...


> So the right fix is to get rid of spapr_io_ops and every other hack like
> that and to add byte swapping to every "if (mr->ops->old_portio)" branch
> (should fix VGA and all other old_portio users). Current byte swapping in
> memory regions seems to be right.
> 
> I would try fixing it but since all my patches were terrible shit so far, I
> won't risk :)
> 
> 
> 
>> h_logical_store is a hypercall for system firmware to do cache inhibited
>> read/write.
>>
>>
>> This is with the patch applied (git checkout  b40acf9):
>>
>>
>> #0  virtqueue_init (vq=0x11014ac0) at
>> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
>> #1  0x10371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
>> addr=0xd0fb000)
>> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
>> #2  0x102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
>> val=0xd0fb)
>> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
>> #3  0x10202f08 in virtio_pci_config_write (opaque=0x11019580,
>> addr=0x8, val=0xd0fb, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
>> #4  0x1037e220 in memory_region_write_accessor (opaque=0x11019c78,
>> addr=0x8, value=0x1f0edc00,
>> size=0x4, shift=0x0, mask=0x) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #5  0x1037e36c in access_with_adjusted_size (addr=0x8,
>> value=0x1f0edc00, size=0x4,
>> access_size_min=0x1, access_size_max=0x4, access=
>> @0x1069df40: 0x1037e164 , 
>> opaque=0x11019c78)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #6  0x10380b5c in memory_region_dispatch_write (mr=0x11019c78,
>> addr=0x8, data=0xd0fb, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
>> #7  0x10383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
>> val=0xfbd0, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
>> #8  0x102e2fdc in address_space_rw (as=0x10ef4350
>> , addr=0x48,
>> buf=0x1f0edde0 "", len=0x4, is_write=0x1) at
>> /home/alexey/pcipassthru/qemu-impreza/exec.c:1918
>> #9  0x102e33c8 in address_space_write (as=0x10ef4350
>> , addr=0x48,
>> buf=0x1f0edde0 "", len=0x4) at
>> /home/alexey/pcipassthru/qemu-impreza/exec.c:1969
>> #10 0x10375754 in cpu_outl (addr=0x48, val=0xfbd0)
>> at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
>> #11 0x10358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
>> data=0xfbd0, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
>> #12 0x1037e220 in memory_region_write_accessor (opaque=0x110191f8,
>> addr=0x48, value=0x1f0ee060,
>> size=0x4, shift=0x0, mask=0x) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #13 0x1037e36c in access_with_adjusted_size (addr=0x48,
>> value=0x1f0ee060, size=0x4,
>> access_size_min=0x1, access_size_max=0x4, access=
>> @0x1069df40: 0x1037e164 , 
>> opaque=0x110191f8)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #14 0x10380b5c in memory_region_dispatch_write (mr=0x110191f8,
>> addr=0x48, data=0xfbd0, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
>> #15 0x10383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
>> val=0xd0fb, size=0x4)
>> at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
>> #16 0x102e47ac in stl_phys_internal (addr=0x1008048,
>> val=0xd0fb, endian=
>> DEVICE_NATIVE_ENDIAN) at 
>> /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
>> #17 0x102e48a8 in stl_phys (addr=0x1008048, val=0xd0fb)
>> at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
>> #18 0x10354f1c in h_logical_store (cpu=0x1f0f0010,
>> spapr=0x10fe9510, opcode=0x40,
>> args=0x1ffd003

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
> Ok. So.
> 
> What broke is...
> I could try explaining but backtraces are lot better :)
> 
> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
> we had a workaround in spapr_io_ops), now it works so double swap happens
> and everything gets broken.
> 
> If we talk about VGA (in powerpc, it is all about powerpc), I guess
> memory_region_iorange_write() will go through mr->ops->old_portio branch
> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
> fine here. I do not understand yet why it works on mac99 though, too late
> here :)


I understood. VGA does not work for mac99 either with this command line:
./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
So it works for pseries only because of parity bug in spapr_io_ops.

So the right fix is to get rid of spapr_io_ops and every other hack like
that and to add byte swapping to every "if (mr->ops->old_portio)" branch
(should fix VGA and all other old_portio users). Current byte swapping in
memory regions seems to be right.

I would try fixing it but since all my patches were terrible shit so far, I
won't risk :)



> h_logical_store is a hypercall for system firmware to do cache inhibited
> read/write.
> 
> 
> This is with the patch applied (git checkout  b40acf9):
> 
> 
> #0  virtqueue_init (vq=0x11014ac0) at
> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
> #1  0x10371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
> addr=0xd0fb000)
> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
> #2  0x102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
> val=0xd0fb)
> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
> #3  0x10202f08 in virtio_pci_config_write (opaque=0x11019580,
> addr=0x8, val=0xd0fb, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
> #4  0x1037e220 in memory_region_write_accessor (opaque=0x11019c78,
> addr=0x8, value=0x1f0edc00,
> size=0x4, shift=0x0, mask=0x) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #5  0x1037e36c in access_with_adjusted_size (addr=0x8,
> value=0x1f0edc00, size=0x4,
> access_size_min=0x1, access_size_max=0x4, access=
> @0x1069df40: 0x1037e164 , opaque=0x11019c78)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #6  0x10380b5c in memory_region_dispatch_write (mr=0x11019c78,
> addr=0x8, data=0xd0fb, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
> #7  0x10383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
> val=0xfbd0, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
> #8  0x102e2fdc in address_space_rw (as=0x10ef4350
> , addr=0x48,
> buf=0x1f0edde0 "", len=0x4, is_write=0x1) at
> /home/alexey/pcipassthru/qemu-impreza/exec.c:1918
> #9  0x102e33c8 in address_space_write (as=0x10ef4350
> , addr=0x48,
> buf=0x1f0edde0 "", len=0x4) at
> /home/alexey/pcipassthru/qemu-impreza/exec.c:1969
> #10 0x10375754 in cpu_outl (addr=0x48, val=0xfbd0)
> at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
> #11 0x10358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
> data=0xfbd0, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
> #12 0x1037e220 in memory_region_write_accessor (opaque=0x110191f8,
> addr=0x48, value=0x1f0ee060,
> size=0x4, shift=0x0, mask=0x) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #13 0x1037e36c in access_with_adjusted_size (addr=0x48,
> value=0x1f0ee060, size=0x4,
> access_size_min=0x1, access_size_max=0x4, access=
> @0x1069df40: 0x1037e164 , opaque=0x110191f8)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #14 0x10380b5c in memory_region_dispatch_write (mr=0x110191f8,
> addr=0x48, data=0xfbd0, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
> #15 0x10383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
> val=0xd0fb, size=0x4)
> at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
> #16 0x102e47ac in stl_phys_internal (addr=0x1008048,
> val=0xd0fb, endian=
> DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
> #17 0x102e48a8 in stl_phys (addr=0x1008048, val=0xd0fb)
> at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
> #18 0x10354f1c in h_logical_store (cpu=0x1f0f0010,
> spapr=0x10fe9510, opcode=0x40,
> args=0x1ffd0030) at
> /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
> 
> 
> 
> This is without this patch (i.e. git checkout  b40acf9^ ):
> 
> #0  virtqueue_init (vq=0x11014ac0) at
> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
> #1  0x103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
> addr=0xffe2000)
> at /home/a

Re: [Qemu-devel] [PATCH V5 3/8] qmp: add internal snapshot support in qmp_transaction

[Wenchao Xia]

于 2013-7-19 18:13, Kevin Wolf 写道:

Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben:

于 2013-7-18 20:22, Kevin Wolf 写道:

Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:

Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot.

Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case. Request with
empty name will be rejected.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia 
---
  blockdev.c   |  117 ++
  qapi-schema.json |   18 -
  qmp-commands.hx  |   34 
  3 files changed, 160 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b3a57e0..6554768 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,118 @@ struct BlkTransactionState {
  QSIMPLEQ_ENTRY(BlkTransactionState) entry;
  };

+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+BlkTransactionState common;
+BlockDriverState *bs;
+QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+  Error **errp)
+{
+const char *device;
+const char *name;
+BlockDriverState *bs;
+QEMUSnapshotInfo sn, *sn1;
+bool ret;
+qemu_timeval tv;
+BlockdevSnapshotInternal *internal;
+InternalSnapshotState *state;
+int ret1;
+
+g_assert(common->action->kind ==
+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+internal = common->action->blockdev_snapshot_internal_sync;
+state = DO_UPCAST(InternalSnapshotState, common, common);
+
+/* 1. parse input */
+device = internal->device;
+name = internal->name;
+
+/* 2. check for validation */
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+if (!bdrv_is_inserted(bs)) {
+error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+return;
+}
+
+if (bdrv_is_read_only(bs)) {
+error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+return;
+}
+
+if (!bdrv_can_snapshot(bs)) {
+error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+  bs->drv->format_name, device, "internal snapshot");
+return;
+}
+
+if (!strlen(name)) {
+error_setg(errp, "Name is empty on device '%s'", device);


Name is empty. This has nothing to do with the device.


   OK.


+return;
+}
+
+/* check whether a snapshot with name exist */
+ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
+if (error_is_set(errp)) {
+return;
+}
+if (ret) {


Save one line with '} else if {' ?


   will change, thanks.


+error_setg(errp,
+   "Snapshot with name '%s' already exists on device '%s'",
+   name, device);
+return;
+}
+
+/* 3. take the snapshot */
+sn1 = &state->sn;


do_savevm() has a memset() to clear all of sn1 before it starts filling
it in. Should we do the same here? For example sn1.vm_state_size looks
undefined.


   I think state->sn is set to zero by
qmp_transaction():
state = g_malloc0(ops->instance_size);


Oh, yes. I was confused by the fact that there is a local sn, which
isn't related to sn1 at all. Perhaps some renaming to make things
clearer?

Kevin


  sure, will rename local sn to sn_old.

--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v2 00/17] Add qemu-img subcommand to dump file metadata

[Wenchao Xia]

于 2013-7-19 20:14, Paolo Bonzini 写道:

Il 19/07/2013 12:48, Wenchao Xia ha scritto:

   I think dump of allocated info in qemu-img in this series, can do
dirty change tracking job for backing chain snapshot now, qemu's QMP
interface is not very needed.
   Only thing not perfect, is that it talks with string.


It uses JSON, actually.


A library would be better, but I am not selling libqblock:),


Oh you should!  Are you going back to it?  It was very close to
mergeable state.


  Thanks for your positive support, it is encouraging to me.
I'll check about the block's status in qemu and go for this library
later, with a discuss with more detail such about thread, co-routine,
code organization, etc.


Paolo




--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Sören Brinkmann]

On Sat, Jul 20, 2013 at 12:20:48AM +0100, Peter Maydell wrote:
> On 19 July 2013 18:53, Sören Brinkmann  wrote:
> > On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
> >> On 19 July 2013 18:39, Sören Brinkmann  wrote:
> >> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> >> >> On 8 July 2013 23:40, Soren Brinkmann  
> >> >> wrote:
> >> >> > +
> >> >> > +if (ep) {
> >> >> > +*ep = hdr->ih_ep;
> >> >> > +}
> >> >>
> >> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> >> >> but it makes sense for consistency with the other pointer
> >> >> argument handling.)
> >> >>
> >> >> > +
> >> >> > +/* TODO: Check CPU type.  */
> >> >> > +if (is_linux) {
> >> >> > +if (hdr->ih_os == IH_OS_LINUX) {
> >> >> > +*is_linux = 1;
> >> >> > +} else {
> >> >> > +*is_linux = 0;
> >> >> > +}
> >> >> > +}
> >> >> > +
> >> >> > +break;
> >> >> > +case IH_TYPE_RAMDISK:
> >> >> > +address = *loadaddr;
> >> >>
> >> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
> >> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> >> > The thing is in case of a ramdisk, it's a mandatory input argument which 
> >> > has
> >> > to be provided, whereas for the kernel, it's an optional output value.
> >> > And other than the load_ramdisk() and load_kernel() routines nobody is
> >> > supposed to call this function directly, IMHO. And I think plausibility
> >> > checks should be done in those routines when possible. And
> >> > load_ramdisk() ensures that the loadaddr pointer is valid.
> >>
> >> Well, by that argument you shouldn't introduce the "allow
> >> ep to be NULL" change...
> > I didn't introduce it, that is the current state for loading a kernel.
> 
> Current code:
> *ep = hdr->ih_ep;
> 
> Your patch:
> +if (ep) {
> +*ep = hdr->ih_ep;
> +}
> 
> On reflection I feel like this is making a mountain out of
> a molehill, though, so:
I'm sorry you're right. I was looking at 'loadaddr' and 'is_linux'. Looking
at ep, I wonder why I changed it, too.
Maybe I wanted to make it a little more robust against wrongly calling
load_ramdisk() with a kernel image. In my original patch that would have
gone through and resulted in a seg-fault because of a missing NULL
check, I think. And it would have been easily triggered just by mixing
up the '-initrd' and '-kernel' command line parameters.
But in v2, I added your proposed header type checking. So, this error
scenario would be recognized properly and the check for 'ep' is
obsolete.

Up to you, leaving as is and have consistent NULL checking on all
pointer arguments, or changing this back to what it was?

Sören

Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Peter Maydell]

On 19 July 2013 18:53, Sören Brinkmann  wrote:
> On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
>> On 19 July 2013 18:39, Sören Brinkmann  wrote:
>> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
>> >> On 8 July 2013 23:40, Soren Brinkmann  wrote:
>> >> > +
>> >> > +if (ep) {
>> >> > +*ep = hdr->ih_ep;
>> >> > +}
>> >>
>> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
>> >> but it makes sense for consistency with the other pointer
>> >> argument handling.)
>> >>
>> >> > +
>> >> > +/* TODO: Check CPU type.  */
>> >> > +if (is_linux) {
>> >> > +if (hdr->ih_os == IH_OS_LINUX) {
>> >> > +*is_linux = 1;
>> >> > +} else {
>> >> > +*is_linux = 0;
>> >> > +}
>> >> > +}
>> >> > +
>> >> > +break;
>> >> > +case IH_TYPE_RAMDISK:
>> >> > +address = *loadaddr;
>> >>
>> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
>> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
>> > The thing is in case of a ramdisk, it's a mandatory input argument which 
>> > has
>> > to be provided, whereas for the kernel, it's an optional output value.
>> > And other than the load_ramdisk() and load_kernel() routines nobody is
>> > supposed to call this function directly, IMHO. And I think plausibility
>> > checks should be done in those routines when possible. And
>> > load_ramdisk() ensures that the loadaddr pointer is valid.
>>
>> Well, by that argument you shouldn't introduce the "allow
>> ep to be NULL" change...
> I didn't introduce it, that is the current state for loading a kernel.

Current code:
*ep = hdr->ih_ep;

Your patch:
+if (ep) {
+*ep = hdr->ih_ep;
+}

On reflection I feel like this is making a mountain out of
a molehill, though, so:

Reviewed-by: Peter Maydell 

-- PMM

Re: [Qemu-devel] [PATCH V5 1/4] Implement sync modes for drive-backup.

[Ian Main]

On Fri, Jul 19, 2013 at 01:41:10PM -0600, Eric Blake wrote:
> On 07/19/2013 11:03 AM, Ian Main wrote:
> > This patch adds sync-modes to the drive-backup interface and
> > implements the FULL, NONE and TOP modes of synchronization.
> > 
> 
> > Signed-off-by: Ian Main 
> > ---
> >  block/backup.c| 91 
> > +++
> >  blockdev.c| 25 -
> >  include/block/block_int.h |  4 ++-
> >  qapi-schema.json  |  4 +++
> >  qmp-commands.hx   |  1 +
> >  5 files changed, 86 insertions(+), 39 deletions(-)
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -1807,6 +1807,10 @@
> >  # @format: #optional the format of the new destination, default is to
> >  #  probe if @mode is 'existing', else the format of the source
> >  #
> > +# @sync: what parts of the disk image should be copied to the destination
> > +#(all the disk, only the sectors allocated in the topmost image, or
> > +#only new I/O).
> > +#
> >  # @mode: #optional whether and how QEMU should create a new image, default 
> > is
> >  #'absolute-paths'.
> >  #
> 
> This hunk is still wrong.  And we still haven't answered the
> meta-question on whether @format should be mandatory (which I'd rather
> fix that first, then rebase this on top of that).

How is it wrong when it cleanly applies?  You want it separated into
another patch?

Ian

Re: [Qemu-devel] [PATCH v2 14/17] raw-posix: return get_block_status data and flags

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  block/raw-posix.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 

> +++ b/block/raw-posix.c
> @@ -1089,7 +1089,7 @@ static int64_t coroutine_fn 
> raw_co_get_block_status(BlockDriverState *bs,
>  int nb_sectors, int *pnum)
>  {
>  off_t start, data, hole;
> -int ret;
> +int64_t ret;
>  
>  ret = fd_open(bs);
>  if (ret < 0) {
> @@ -1097,6 +1097,7 @@ static int64_t coroutine_fn 
> raw_co_get_block_status(BlockDriverState *bs,
>  }
>  
>  start = sector_num * BDRV_SECTOR_SIZE;
> +ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;

off_t is a signed type; if you are compiling on a platform with 32-bit
off_t, is it possible that you will get unintended sign extension for
values of 'start' between 2 and 4 GB?  Or are such files already
impossible to open?  [Or do we intentionally require off_t be 64-bits on
all platforms we care about?]

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PULL 0/4] Memory API fixes for soft freeze

[Paolo Bonzini]

Anthony,

The following changes since commit 6453a3a69488196f26d12654c6b148446abdf3d6:

  Merge remote-tracking branch 'quintela/migration.next' into staging 
(2013-07-15 14:49:16 -0500)

are available in the git repository at:

  git://github.com/bonzini/qemu.git iommu-for-anthony

for you to fetch changes up to e1622f4b15391bd44eb0f99a244fdf19a20fd981:

  exec: fix incorrect assumptions in memory_access_size (2013-07-18 06:03:25 
+0200)


Jan Kiszka (1):
  memory: Return -1 again on reads from unsigned regions

Paolo Bonzini (2):
  memory: actually set the owner
  exec: fix incorrect assumptions in memory_access_size

Peter Maydell (1):
  exec.c: Pass correct pointer type to qemu_ram_ptr_length

 exec.c   | 11 ++-
 memory.c |  3 +--
 2 files changed, 3 insertions(+), 11 deletions(-)
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/4] exec: fix incorrect assumptions in memory_access_size

[Paolo Bonzini]

access_size_min can be 1 because erroneous accesses must not crash
QEMU, they should trigger exceptions in the guest or just return
garbage (depending on the CPU).  I am not sure I understand the
comment: placing a 4-byte field at the last byte of a region
makes no sense (unless impl.unaligned is true), and that is
why memory.c:access_with_adjusted_size does not bother with
minimums larger than the remaining length.

access_size_max can be mr->ops->valid.max_access_size because memory.c
can and will still break accesses bigger than
mr->ops->impl.max_access_size.

Reported-by: Markus Armbruster 
Tested-by: Markus Armbruster 
Signed-off-by: Paolo Bonzini 
---
 exec.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index d312bb4..c8658c6 100644
--- a/exec.c
+++ b/exec.c
@@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion 
*mr, bool is_write)
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-unsigned access_size_min = mr->ops->impl.min_access_size;
-unsigned access_size_max = mr->ops->impl.max_access_size;
+unsigned access_size_max = mr->ops->valid.max_access_size;
 
 /* Regions are assumed to support 1-4 byte accesses unless
otherwise specified.  */
-if (access_size_min == 0) {
-access_size_min = 1;
-}
 if (access_size_max == 0) {
 access_size_max = 4;
 }
@@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned 
l, hwaddr addr)
 if (l > access_size_max) {
 l = access_size_max;
 }
-/* ??? The users of this function are wrong, not supporting minimums larger
-   than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
-assert(l >= access_size_min);
 
 return l;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 16/17] block: add default get_block_status implementation for protocols

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Protocols return raw data, so you can assume the offsets to pass
> through unchanged.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 2/4] memory: actually set the owner

[Paolo Bonzini]

Brown paper bag for me.  Originally commit 803c0816 came before commit
2c9b15c.  When the order was inverted, I left in the NULL initialization
of mr->owner.

Reviewed-by: Hu Tao 
Signed-off-by: Paolo Bonzini 
---
 memory.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/memory.c b/memory.c
index c8f9a2b..9938b6b 100644
--- a/memory.c
+++ b/memory.c
@@ -805,7 +805,6 @@ void memory_region_init(MemoryRegion *mr,
 mr->owner = owner;
 mr->iommu_ops = NULL;
 mr->parent = NULL;
-mr->owner = NULL;
 mr->size = int128_make64(size);
 if (size == UINT64_MAX) {
 mr->size = int128_2_64();
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP

[Eric Blake]

On 07/17/2013 02:36 PM, Luiz Capitulino wrote:
>> We need to parse all commands json definition, and generated a
>> dynamical tree, QMP infrastructure will convert the tree to
>> json string and return to QMP client.
>>
>> So here I defined a 'DataObject' type in qapi-schema.json,
>> it's used to describe the dynamical dictionary/list/string.
> 
> I skimmed over the patch and made some comments, but my impression
> is that this is getting too complex... Why did we move from letting
> mngt query type by type (last version) to this version which returns
> all commands and their input types? Does this satisfy libvirt needs?

Libvirt can query once, and then browse through the (giant) reply as
many times as needed to drill down to a full definition.  The ability to
filter by passing in a name to look up is a bonus, but not mandatory.

I'm also working on a reply to the full patch.

> You return only commands. That is, types are returned as part of the
> command input. ErrorClass can't be queried then? I'm not judging, only
> observing.
> 
> Eric, this is good enough for libvirt?

It might be sufficient (after all, you can't use a type except by
passing it as part of a command [argument or return] or event [which we
still don't have converted into qapi...]).  In one respect, it means
that if we want to know if an optional field was added for command
'foo', we start by querying command foo; then regardless of what type
names were used, we will see that in the results for the command.  On
the other hand, we've been arguing that qapi type names are part of the
API, and that we shouldn't arbitrarily change type names (particularly
not once introspection is in place).  Thus, if I can query for the
contents of type 'FooDict', that shaves a step from querying the
structure of command 'foo' that uses the type 'FooDict'.

In other words, it will "work" for libvirt, but it may not be optimal.

-- 
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 2/2] full introspection support for QMP

[Eric Blake]

On 07/16/2013 04:37 AM, Amos Kong wrote:
> Introduces new monitor command to query QMP schema information,
> the return data is a dynamical and nested dict/list, it contains

s/dynamical/dynamic/

> the useful metadata to help management to check feature support,
> QMP commands detail, etc.
> 
> I added a document for QMP introspection support.
> (docs/qmp-full-introspection.txt)

Yay - docs make a world of difference.

> 
> We need to parse all commands json definition, and generated a

mixing tense ("need" present, "generated" past); for commit messages,
present tense is generally appropriate.  Thus:

s/generated/generate/

> dynamical tree, QMP infrastructure will convert the tree to

s/dynamical/dynamic/

> json string and return to QMP client.
> 
> So here I defined a 'DataObject' type in qapi-schema.json,
> it's used to describe the dynamical dictionary/list/string.

s/dynamical/dynamic/

> 
> { 'type': 'DataObject',
>   'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> 
> Not all the keys in data will be used.
>  # List: type
>  # Dict: key, type
>  # nested List: type, data
>  # nested Dict: key, type, data

I wonder if we can take advantage of Kevin's work on unions to make this
MUCH easier to use:

https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html

{ 'enum': 'DataObjectType',
  'data': [ 'command', 'struct', 'union', 'enum' ] }
# extend to add 'event' later

{ 'type': 'DataObjectBase',
  'data': { 'name': 'str' } }

{ 'union': 'DataObjectMemberType',
  'discriminator': {},
  'data': { 'reference': 'str',
'inline': 'DataObject' } }

{ 'type': DataObjectMember',
  'data': { 'name': 'str', 'type': 'DataObjectMemberType',
'*optional': 'bool', '*recursive': 'bool' } }

{ 'type': 'DataObjectStruct',
  'data': { 'data': [ 'DataObjectMember' ] } }
{ 'type': 'DataObjectEnum',
  'data': { 'data': [ 'str' ] } }
{ 'type': 'DataObjectUnion',
  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
'*discriminator': 'str' } }
{ 'type': 'DataObjectCommand',
  'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }

{ 'union': 'DataObject',
  'base': 'DataObjectBase',
  'discriminator': 'type',
  'data': {
'struct': 'DataObjectStruct',
'enum': 'DataObjectEnum',
'union': 'DataObjectUnion',
'command': 'DataObjectCommand',
'array': {} }

> 
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
> 
> The following content gives an example of query-tpm-types:
> 
>  ## Define example in qapi-schema.json:
> 
>  { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
>  { 'command': 'query-tpm-types', 'returns': ['TpmType'] }

It might be more useful to have a (made-up) example that shows as many
details as possible - a command that takes arguments and has a return
type will exercise more of the code paths than a query command with only
a return.

> 
>  ## Returned description:
> 
>  {
>  "name": "query-tpm-types",
>  "type": "Command",
>  "returns": [
>  {
>  "type": "TpmType",
>  "data": [
>  {
>  "type": "passthrough"
>  }
>  ]

I need a way to know the difference between a TpmType returned directly
in a dict, vs. a return containing an array of TpmType.

>  }
>  ]
>  },

Thus, using the discriminated union I set out above, I would return
something like:
{ "name": "TpmType", "type": "enum",
  "data": [ "passthrough" ] },
{ "name": "query-tpm-types", "type": "command",
  "data": [ ],
  "returns": { "name": "TpmType", "type": "array" } }

> 
> 'TpmType' is a defined type, it will be extended in returned
> description. [ 'passthrough' ] is a list, so 'type' and 'data'
> will be used.
> 
> TODO:
> We can add events definations to qapi-schema.json by another

s/definations/definitions/

> patch, then event can also be queried.
> 
> Introduce another command 'query-qga-schema' to query QGA schema
> information, it's easy to add this support with current current
> patch.

Such a command would be part of the QGA interface, not a QMP command.
But yes, it is needed, and the ideal patch series from you will include
that patch as part of the series.  But I don't mind waiting until we get
the interface nailed down before you actually implement the QGA repeat
of the interface.

> 
> Signed-off-by: Amos Kong 
> ---
>  docs/qmp-full-introspection.txt | 143 +++
>  qapi-schema.json|  69 +
>  qmp-commands.hx |  39 +
>  qmp.c   | 307 
> 
>  4 files changed, 558 insertions(+)
>  create mode 100644 docs/qmp-full-introspection.txt
> 
> diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> new file mode 100644
> index 000..cc0fb80
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,143 @@
> += full introspection support for QMP

[Qemu-devel] [PATCH 3/4] memory: Return -1 again on reads from unsigned regions

[Paolo Bonzini]

From: Jan Kiszka 

This restore the behavior prior to b018ddf633 which accidentally changed
the return code to 0. Specifically guests probing for register existence
were affected by this.

Signed-off-by: Jan Kiszka 
Signed-off-by: Paolo Bonzini 
---
 memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index 9938b6b..34a088e 100644
--- a/memory.c
+++ b/memory.c
@@ -840,7 +840,7 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr 
addr,
 if (current_cpu != NULL) {
 cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
 }
-return 0;
+return -1ULL;
 }
 
 static void unassigned_mem_write(void *opaque, hwaddr addr,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup.

[Ian Main]

On Thu, Jul 18, 2013 at 01:54:45PM -0600, Eric Blake wrote:
> On 07/18/2013 01:06 PM, Ian Main wrote:
> > On Thu, Jul 18, 2013 at 11:19:43AM -0600, Eric Blake wrote:
> >> On 07/17/2013 02:04 PM, Ian Main wrote:
> >>> This patch adds sync-modes to the drive-backup interface and
> >>> implements the FULL, NONE and TOP modes of synchronization.
> >>>
> 
> >>> @@ -1807,6 +1807,10 @@
> >>>  # @format: #optional the format of the new destination, default is to
> >>>  #  probe if @mode is 'existing', else the format of the source
> >>>  #
> >>> +# @sync: what parts of the disk image should be copied to the destination
> >>> +#(all the disk, only the sectors allocated in the topmost image, 
> >>> or
> >>> +#only new I/O).
> >>> +#
> >>>  # @mode: #optional whether and how QEMU should create a new image, 
> >>> default is
> >>>  #'absolute-paths'.
> >>
> >> This hunk looks bogus; the 'DriveBackup' type already documents @sync as
> >> of commit b53169e, which makes me suspect this hunk got misapplied
> >> during rebasing.
> > 
> > Did you check that?  I know there was one bit of documentation missing
> > that I fixed here.  I also just did a clean rebase (git am) to
> > kevin/block and it all applied fine.
> 
> Yes, it _applied_ fine, because of git's insistence on applying hunks
> regardless of line fuzzing.  It probably applied to 'drive-mirror',
> since I see this context in the section for 'drive-mirror' when reading
> the unpatched file at current qemu.git head:
> 
> > # @format: #optional the format of the new destination, default is to
> > #  probe if @mode is 'existing', else the format of the source
> > #
> > # @mode: #optional whether and how QEMU should create a new image, default 
> > is
> > #'absolute-paths'.
> > #
> 
> Hence, my argument that you DON'T want this hunk.

Ah, yes you are right.  I'll fix it next rev along with other things
mentioned.

Ian

Re: [Qemu-devel] [PATCH V5 1/4] Implement sync modes for drive-backup.

[Eric Blake]

On 07/19/2013 11:03 AM, Ian Main wrote:
> This patch adds sync-modes to the drive-backup interface and
> implements the FULL, NONE and TOP modes of synchronization.
> 

> Signed-off-by: Ian Main 
> ---
>  block/backup.c| 91 
> +++
>  blockdev.c| 25 -
>  include/block/block_int.h |  4 ++-
>  qapi-schema.json  |  4 +++
>  qmp-commands.hx   |  1 +
>  5 files changed, 86 insertions(+), 39 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -1807,6 +1807,10 @@
>  # @format: #optional the format of the new destination, default is to
>  #  probe if @mode is 'existing', else the format of the source
>  #
> +# @sync: what parts of the disk image should be copied to the destination
> +#(all the disk, only the sectors allocated in the topmost image, or
> +#only new I/O).
> +#
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #'absolute-paths'.
>  #

This hunk is still wrong.  And we still haven't answered the
meta-question on whether @format should be mandatory (which I'd rather
fix that first, then rebase this on top of that).

-- 
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] memory: Fix zero-sized memory region print

[Paolo Bonzini]

Il 19/07/2013 20:42, Alex Williamson ha scritto:
> if mr->size == 0, then
> 
> int128_get64(int128_sub(mr->size, int128_make64(1))) => assert(!a.hi)
> 
> Also, use int128_one().
> 
> Signed-off-by: Alex Williamson 

Reviewed-by: Paolo Bonzini 

> ---
>  memory.c |8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index c8f9a2b..6e17c21 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1788,7 +1788,9 @@ static void mtree_print_mr(fprintf_function mon_printf, 
> void *f,
> "-" TARGET_FMT_plx "\n",
> base + mr->addr,
> base + mr->addr
> -   + (hwaddr)int128_get64(int128_sub(mr->size, 
> int128_make64(1))),
> +   + (int128_nz(mr->size) ?
> +  (hwaddr)int128_get64(int128_sub(mr->size,
> +  int128_one())) : 0),
> mr->priority,
> mr->romd_mode ? 'R' : '-',
> !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
> @@ -1803,7 +1805,9 @@ static void mtree_print_mr(fprintf_function mon_printf, 
> void *f,
> TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %c%c): 
> %s\n",
> base + mr->addr,
> base + mr->addr
> -   + (hwaddr)int128_get64(int128_sub(mr->size, 
> int128_make64(1))),
> +   + (int128_nz(mr->size) ?
> +  (hwaddr)int128_get64(int128_sub(mr->size,
> +  int128_one())) : 0),
> mr->priority,
> mr->romd_mode ? 'R' : '-',
> !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
> 

Re: [Qemu-devel] [PATCH V5 1/4] Implement sync modes for drive-backup.

[Eric Blake]

On 07/19/2013 03:49 PM, Ian Main wrote:
>>> +++ b/qapi-schema.json
>>> @@ -1807,6 +1807,10 @@
>>>  # @format: #optional the format of the new destination, default is to
>>>  #  probe if @mode is 'existing', else the format of the source
>>>  #
>>> +# @sync: what parts of the disk image should be copied to the destination
>>> +#(all the disk, only the sectors allocated in the topmost image, or
>>> +#only new I/O).
>>> +#
>>>  # @mode: #optional whether and how QEMU should create a new image, default 
>>> is
>>>  #'absolute-paths'.
>>>  #
>>
>> This hunk is still wrong.  And we still haven't answered the
>> meta-question on whether @format should be mandatory (which I'd rather
>> fix that first, then rebase this on top of that).
> 
> How is it wrong when it cleanly applies?  You want it separated into
> another patch?

It cleanly applied to the wrong place (drive-mirror), no thanks to git's
automatic fuzzing.  It is _no longer necessary_ - DriveBackup already
has this information.  I want it dropped completely.

-- 
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 15/17] raw-posix: detect XFS unwritten extents

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  block/raw-posix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d011cfd..1b41ea3 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1128,6 +1128,9 @@ static int64_t coroutine_fn 
> raw_co_get_block_status(BlockDriverState *bs,
>  } else {
>  data = f.fe.fe_logical;
>  hole = f.fe.fe_logical + f.fe.fe_length;
> +if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) {
> +ret |= BDRV_BLOCK_ZERO;
> +}
>  }

Reviewed-by: Eric Blake 

-- 
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 17/17] block: look for zero blocks in bs->file

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 1/4] exec.c: Pass correct pointer type to qemu_ram_ptr_length

[Paolo Bonzini]

From: Peter Maydell 

Commit e3127ae0 introduced a problem where we're passing a
hwaddr* to qemu_ram_ptr_length() but it wants a ram_addr_t*;
this will cause problems on 32 bit hosts and in any case
provokes a clang warning on MacOSX:

  CCarm-softmmu/exec.o
exec.c:2164:46: warning: incompatible pointer types passing 'hwaddr *'
(aka 'unsigned long long *') to parameter of type 'ram_addr_t *'
(aka 'unsigned long *')
[-Wincompatible-pointer-types]
return qemu_ram_ptr_length(raddr + base, plen);
 ^~~~
exec.c:1392:63: note: passing argument to parameter 'size' here
static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
  ^

Since this function is only used in one place, change its
prototype to pass a hwaddr* rather than a ram_addr_t*,
rather than contorting the calling code to get the type right.

Signed-off-by: Peter Maydell 
Tested-by: Riku Voipio 
Tested-by: Peter Crosthwaite 
Signed-off-by: Paolo Bonzini 
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index c99a883..d312bb4 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ static void *qemu_safe_ram_ptr(ram_addr_t addr)
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
  * but takes a size argument */
-static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
+static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
 {
 if (*size == 0) {
 return NULL;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup.

[Paolo Bonzini]

Il 19/07/2013 03:13, Fam Zheng ha scritto:
>>> > > Also set target->backing_file and target->backing_format here? Paolo?
>> > 
>> > I don't think so, it is temporary while the job runs so that the NBD
>> > server can already return the actual data.
> OK.
> 
> For NBD export, it's also going to have a name, to be used with
> nbd_server_add. So should we call bdrv_set_in_use() here to protect
> target from being used elsewhere?

I think it'd still be safe, but it's probably a good idea anyway.

Paolo

Re: [Qemu-devel] BUG: make check -> ERROR:tests/boot-order-test.c:43:test_a_boot_order

[Markus Armbruster]

Alex Bligh  writes:

> Using current master: 24943978cbe79634a9a8b02a20efb25b29b3ab49
>
> 'make check' gives:
>
> ERROR:tests/boot-order-test.c:43:test_a_boot_order: assertion failed
> (actual == expected_boot): (0x1230 == 0x)
> GTester: last random seed: R02Se371669dcb0a3274fa9c170e22654334
>
> I'm on x86_64
>
> I can't help but note the last commit is:
>
> commit 24943978cbe79634a9a8b02a20efb25b29b3ab49
> Author: Markus Armbruster 
> Date:   Wed Jun 26 15:52:23 2013 +0200
>
>boot-order-test: Add tests for Sun4u
>
> Obviously that should make no difference for x86_64. I've copied
> the author just in case.

Same series also added PC tests.

The tests pass for me.  Anything unusual in your environment?

Did you rebuild from scratch, or just ran make after git-pull?  Our
build process still isn't 100% reliable...

Re: [Qemu-devel] [RFC 0/3] Determinitic behaviour with icount.

[Frederic Konrad]

On 18/07/2013 18:35, Paolo Bonzini wrote:

Il 18/07/2013 18:31, Frederic Konrad ha scritto:

On 18/07/2013 17:35, Paolo Bonzini wrote:

Il 18/07/2013 17:06, Peter Maydell ha scritto:

On 18 July 2013 16:02,   wrote:

As I said in the last email, we have issues with determinism with
icount.
We are wondering if determinism is really ensured with icount?

My opinion is that it *should* be deterministic but it would
be unsurprising if the determinism had got broken along the way.

First of all, it can only be deterministic if the guest satisfies (at
least) all the following condition:

1) only uses timer that QEMU bases on vm_clock (which means that you
should use "-rtc clock=vm"---sorry Fred, didn't think about this in the
previous answer);

Oops sorry, I didn't mentioned that, but we used rtc clock=vm for our
tests.

2) never does any network operation nor any asynchronous disk I/O
operation

3) never halts the VCPU waiting for an interrupt


Point 1 is obvious.


To explain points 2, let's consider what happens if a block device uses
synchronous vs. asynchronous I/O.

With synchronous I/O, each block device operation will complete
immediately.  All clocks are stalled during the operation.

With asynchronous I/O, each block device operation will be done while
the CPU is running.  If the CPU is polling a completion flag, the number
of instructions executed (thus icount) depends on how long it takes to
do I/O.

So I suppose this can happen even if there are any network card or block
device.

We probably need to disable it until we finally save and replay IO, to
get this thing working.

Are you aware of the work that was done on fault tolerance (Kemari)?
Orit is working on resurrecting it.

Paolo


No, but I will take a look that can be really usefull for IO.

Thanks,
Fred

[Qemu-devel] [PATCH] memory: Fix zero-sized memory region print

[Alex Williamson]

if mr->size == 0, then

int128_get64(int128_sub(mr->size, int128_make64(1))) => assert(!a.hi)

Also, use int128_one().

Signed-off-by: Alex Williamson 
---
 memory.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index c8f9a2b..6e17c21 100644
--- a/memory.c
+++ b/memory.c
@@ -1788,7 +1788,9 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
"-" TARGET_FMT_plx "\n",
base + mr->addr,
base + mr->addr
-   + (hwaddr)int128_get64(int128_sub(mr->size, 
int128_make64(1))),
+   + (int128_nz(mr->size) ?
+  (hwaddr)int128_get64(int128_sub(mr->size,
+  int128_one())) : 0),
mr->priority,
mr->romd_mode ? 'R' : '-',
!mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
@@ -1803,7 +1805,9 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %c%c): %s\n",
base + mr->addr,
base + mr->addr
-   + (hwaddr)int128_get64(int128_sub(mr->size, 
int128_make64(1))),
+   + (int128_nz(mr->size) ?
+  (hwaddr)int128_get64(int128_sub(mr->size,
+  int128_one())) : 0),
mr->priority,
mr->romd_mode ? 'R' : '-',
!mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'

[Qemu-devel] [PATCH V5 3/4] Add backing drive while performing backup.

[Ian Main]

This patch adds the original source drive as a backing drive to our target
image so that the target image will appear complete during backup.  This
is especially useful for SYNC_MODE_NONE as it allows export via NBD to
have a complete point-in-time snapshot available for export.

Signed-off-by: Ian Main 
---
 block/backup.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index 68abd23..e086e76 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -323,6 +323,10 @@ static void coroutine_fn backup_run(void *opaque)
 
 hbitmap_free(job->bitmap);
 
+/* Set the target backing drive back to NULL before calling delete or
+ * it will also delete the underlying drive. */
+target->backing_hd = NULL;
+
 bdrv_iostatus_disable(target);
 bdrv_delete(target);
 
@@ -362,6 +366,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 return;
 }
 
+/* Manually set the backing hd to be the backup source drive so
+ * that all reads done while we are backing up will be passed
+ * on to the original source drive.  This allows reading from the
+ * image while the backup is in progress, or in the case of
+ * SYNC_MODE_NONE allows a complete image to be present for export.
+ * Note that we do this for all modes including SYNC_MODE_TOP as
+ * even then it allows on-the-fly reading. */
+target->backing_hd = bs;
+
 job->on_source_error = on_source_error;
 job->on_target_error = on_target_error;
 job->target = target;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] aes: Remove unused code (NDEBUG, u16)

[Stefan Weil]

Am 29.06.2013 17:10, schrieb Stefan Weil:
> The current code includes assert.h very early (from qemu-common.h),
> so the definition of NDEBUG was without any effect.
>
> In the initial version from 2004, NDEBUG was used to disable the assertions.
> Those assertions are not in time critical code, so it is no longer
> reasonable to disable them and the definition of NDEBUG can be removed.
>
> Type u16 is also unused and therefore does not need a type definition.
>
> Signed-off-by: Stefan Weil 
> ---
>  util/aes.c |5 -
>  1 file changed, 5 deletions(-)
>
> diff --git a/util/aes.c b/util/aes.c
> index 91e97fa..4b4d88e 100644
> --- a/util/aes.c
> +++ b/util/aes.c
> @@ -30,12 +30,7 @@
>  #include "qemu-common.h"
>  #include "qemu/aes.h"
>  
> -#ifndef NDEBUG
> -#define NDEBUG
> -#endif
> -
>  typedef uint32_t u32;
> -typedef uint16_t u16;
>  typedef uint8_t u8;
>  
>  /* This controls loop-unrolling in aes_core.c */


Please apply this patch to the qemu-trivial queue,
and maybe this one, too: http://patchwork.ozlabs.org/patch/257416/.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH] PPC: dbdma: macio: Fix format specifiers (build regression)

[Stefan Weil]

Am 19.07.2013 00:40, schrieb Alexander Graf:
> On 18.07.2013, at 21:49, Stefan Weil wrote:
>
>> Am 16.07.2013 07:54, schrieb Stefan Weil:
>>> Am 12.07.2013 18:48, schrieb Stefan Weil:
 Fix a number of warnings for 32 bit builds (tested on MingW and Linux):

  CChw/ide/macio.o
 qemu/hw/ide/macio.c: In function 'pmac_ide_atapi_transfer_cb':
 qemu/hw/ide/macio.c:134:9: error: format '%lx' expects argument of type 
 'long unsigned int', but argument 3 has type 'hwaddr' [-Werror=format]
 qemu/hw/ide/macio.c: In function 'pmac_ide_transfer_cb':
 qemu/hw/ide/macio.c:215:5: error: format '%ld' expects argument of type 
 'long int', but argument 5 has type 'int64_t' [-Werror=format]
 qemu/hw/ide/macio.c:222:9: error: format '%lx' expects argument of type 
 'long unsigned int', but argument 3 has type 'hwaddr' [-Werror=format]
 qemu/hw/ide/macio.c:264:9: error: format '%lx' expects argument of type 
 'long unsigned int', but argument 3 has type 'hwaddr' [-Werror=format]
 cc1: all warnings being treated as errors
 make: *** [hw/ide/macio.o] Error 1

 Signed-off-by: Stefan Weil 
 ---



 Hi Anthony,

 the patch fixes a build regression which was introduced today.
 Could you please apply it without waiting for the next pull requests?

 Thanks,
 Stefan

[...]

> Acked-by: Alexander Graf  I assume this can go through
> the trivial tree? Or directly get applied by Anthony? Alex 


I still think that build regressions should be fixed by a direct
maintainer commit instead of waiting for a pull request,
but obviously this does not happen here (why?).

So hopefully the fix will be committed via qemu-trivial at least.
See http://patchwork.ozlabs.org/patch/258774/ for the full patch.

Stefan

Re: [Qemu-devel] [PATCH v2 13/17] block: use bdrv_has_zero_init to return BDRV_BLOCK_ZERO

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Alternatively, this could use a "discard zeroes data" flag returned
> by bdrv_get_info.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Commit 23326164ae (exec: Support 64-bit op...) triggers assertion

[Richard Henderson]

On 07/19/2013 10:28 AM, Luiz Capitulino wrote:
> Hi,
> 
> Reproducer:
> 
> # ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native \
>   -enable-kvm -m 1G -monitor stdio  -usb -usbdevice host:1.43
> QEMU 1.5.50 monitor - type 'help' for more information
> (qemu) qemu-qmp: 
> /home/lcapitulino/work/src/upstream/qmp-unstable/exec.c:1927: 
> memory_access_size: Assertion `l >= access_size_min' failed.
> 
> I have an USB stick on 1.43, although I did manage to get this without
> USB at all (forgot how, though).
> 
> Bisect says the bug was introduced by:
> 
> commit 23326164ae6fe8d94b7eff123e03f97ca6978d33
> Author: Richard Henderson 
> Date:   Mon Jul 8 14:55:59 2013 -0700
> 
> exec: Support 64-bit operations in address_space_rw
> 

Yes, we've already discussed it and agreed upon a solution.
I guess it hasn't been applied yet?


r~

[Qemu-devel] [PATCH V5 1/4] Implement sync modes for drive-backup.

[Ian Main]

This patch adds sync-modes to the drive-backup interface and
implements the FULL, NONE and TOP modes of synchronization.

FULL performs as before copying the entire contents of the drive
while preserving the point-in-time using CoW.
NONE only copies new writes to the target drive.
TOP copies changes to the topmost drive image and preserves the
point-in-time using CoW.

For sync mode TOP are creating a new target image using the same backing
file as the original disk image.  Then any new data that has been laid
on top of it since creation is copied in the main backup_run() loop.
There is an extra check in the 'TOP' case so that we don't bother to copy
all the data of the backing file as it already exists in the target.
This is where the bdrv_co_is_allocated() is used to determine if the
data exists in the topmost layer or below.

Also any new data being written is intercepted via the write_notifier
hook which ends up calling backup_do_cow() to copy old data out before
it gets overwritten.

For mode 'NONE' we create the new target image and only copy in the
original data from the disk image starting from the time the call was
made.  This preserves the point in time data by only copying the parts
that are *going to change* to the target image.  This way we can
reconstruct the final image by checking to see if the given block exists
in the new target image first, and if it does not, you can get it from
the original image.  This is basically an optimization allowing you to
do point-in-time snapshots with low overhead vs the 'FULL' version.

Since there is no old data to copy out the loop in backup_run() for the
NONE case just calls qemu_coroutine_yield() which only wakes up after
an event (usually cancel in this case).  The rest is handled by the
before_write notifier which again calls backup_do_cow() to write out
the old data so it can be preserved.

Signed-off-by: Ian Main 
---
 block/backup.c| 91 +++
 blockdev.c| 25 -
 include/block/block_int.h |  4 ++-
 qapi-schema.json  |  4 +++
 qmp-commands.hx   |  1 +
 5 files changed, 86 insertions(+), 39 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 16105d4..68abd23 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,7 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
 BlockJob common;
 BlockDriverState *target;
+MirrorSyncMode sync_mode;
 RateLimit limit;
 BlockdevOnError on_source_error;
 BlockdevOnError on_target_error;
@@ -247,40 +248,69 @@ static void coroutine_fn backup_run(void *opaque)
 
 bdrv_add_before_write_notifier(bs, &before_write);
 
-for (; start < end; start++) {
-bool error_is_read;
-
-if (block_job_is_cancelled(&job->common)) {
-break;
+if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
+while (!block_job_is_cancelled(&job->common)) {
+/* Yield until the job is cancelled.  We just let our before_write
+ * notify callback service CoW requests. */
+job->common.busy = false;
+qemu_coroutine_yield();
+job->common.busy = true;
 }
+} else {
+/* Both FULL and TOP SYNC_MODE's require copying.. */
+for (; start < end; start++) {
+bool error_is_read;
 
-/* we need to yield so that qemu_aio_flush() returns.
- * (without, VM does not reboot)
- */
-if (job->common.speed) {
-uint64_t delay_ns = ratelimit_calculate_delay(
-&job->limit, job->sectors_read);
-job->sectors_read = 0;
-block_job_sleep_ns(&job->common, rt_clock, delay_ns);
-} else {
-block_job_sleep_ns(&job->common, rt_clock, 0);
-}
+if (block_job_is_cancelled(&job->common)) {
+break;
+}
 
-if (block_job_is_cancelled(&job->common)) {
-break;
-}
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(
+&job->limit, job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, rt_clock, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, rt_clock, 0);
+}
 
-ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
-BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
-if (ret < 0) {
-/* Depending on error action, fail now or retry cluster */
-BlockErrorAction action =
-backup_error_action(job, error_is_read, -ret);
-if (action == BDRV_ACTION_REPORT) {
+if (block_job_is_cancelled(&job->common)) {
 break;
-} else {

Re: [Qemu-devel] [PATCH v2 17/17] block: look for zero blocks in bs->file

[Paolo Bonzini]

Il 19/07/2013 09:33, Stefan Hajnoczi ha scritto:
> On Tue, Jul 16, 2013 at 06:29:28PM +0200, Paolo Bonzini wrote:
>> diff --git a/block.c b/block.c
>> index 557ce29..2d7d71f 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2977,7 +2977,7 @@ static int64_t coroutine_fn 
>> bdrv_co_get_block_status(BlockDriverState *bs,
>>   int nb_sectors, int 
>> *pnum)
>>  {
>>  int64_t n;
>> -int64_t ret;
>> +int64_t ret, ret2;
>>  
>>  if (sector_num >= bs->total_sectors) {
>>  *pnum = 0;
>> @@ -3003,6 +3003,14 @@ static int64_t coroutine_fn 
>> bdrv_co_get_block_status(BlockDriverState *bs,
>>  ret |= BDRV_BLOCK_ZERO;
>>  }
>>  
>> +if (bs->file &&
>> +(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>> +(ret & BDRV_BLOCK_OFFSET_VALID)) {
>> +ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
>> +*pnum, pnum);
>> +ret |= (ret2 & BDRV_BLOCK_ZERO);
>> +}
> 
> This patch breaks qemu-iotests 030 (image streaming).
> 
> The problem is that bdrv_co_get_block_status() uses bs->total_sectors
> directly instead of calling bdv_get_geometry()/bdrv_getlength().
> 
> With qcow2 the bs->file can grow on disk.  We don't update
> bs->total_sectors.
> 
> Then this patch calls bdrv_co_get_block_status(bs->file) where we fail
> with *pnum = 0, ret = 0 because bs->total_sectors suggests it is beyond
> the end of the file.
> 
> The result is that 030 goes into an infinite loop.
> 
> As a quick test I switched the direct bs->total_sectors accesses to
> bdrv_get_geometry() and it stopped hanging.  Perhaps the
> bs->total_sectors caching needs to be improved though.

Yes, fixing the caching also resolves the failure.  I'll send a v3 next
Monday or perhaps Sunday.

Paolo

[Qemu-devel] [PATCH V5 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

qcow2 supports backing files so it makes sense to default to qcow2
for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
breaks tests but that could be fixed if we wanted it.

Signed-off-by: Ian Main 
---
 blockdev.c   | 5 -
 qapi-schema.json | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 000dea6..a56ba08 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1462,7 +1462,10 @@ void qmp_drive_backup(const char *device, const char 
*target,
 }
 
 if (!has_format) {
-format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
+format = NULL;
+if (mode != NEW_IMAGE_MODE_EXISTING) {
+format = sync == MIRROR_SYNC_MODE_NONE ? "qcow2" : 
bs->drv->format_name;
+}
 }
 if (format) {
 drv = bdrv_find_format(format);
diff --git a/qapi-schema.json b/qapi-schema.json
index b3f6c2a..e2c86f9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1806,6 +1806,7 @@
 #
 # @format: #optional the format of the new destination, default is to
 #  probe if @mode is 'existing', else the format of the source
+#  drive.  If @sync is 'none' then the default is qcow2.
 #
 # @sync: what parts of the disk image should be copied to the destination
 #(all the disk, only the sectors allocated in the topmost image, or
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 12/17] qemu-img: add a "map" subcommand

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> This command dumps the metadata of an entire chain, in either tabular or JSON
> format.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: mention encrypted clusters, use PRId64
> 
>  qemu-img-cmds.hx |   6 ++
>  qemu-img.c   | 185 
> +++
>  2 files changed, 191 insertions(+)

Missing a change to qemu-img.texi to list the documentation required there.

It might also help to show a sample of the output in the commit message.

I didn't spot anything else beyond your other reviewers.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Commit 23326164ae (exec: Support 64-bit op...) triggers assertion

[Luiz Capitulino]

On Fri, 19 Jul 2013 12:06:42 -0700
Richard Henderson  wrote:

> On 07/19/2013 10:28 AM, Luiz Capitulino wrote:
> > Hi,
> > 
> > Reproducer:
> > 
> > # ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native \
> >   -enable-kvm -m 1G -monitor stdio  -usb -usbdevice host:1.43
> > QEMU 1.5.50 monitor - type 'help' for more information
> > (qemu) qemu-qmp: 
> > /home/lcapitulino/work/src/upstream/qmp-unstable/exec.c:1927: 
> > memory_access_size: Assertion `l >= access_size_min' failed.
> > 
> > I have an USB stick on 1.43, although I did manage to get this without
> > USB at all (forgot how, though).
> > 
> > Bisect says the bug was introduced by:
> > 
> > commit 23326164ae6fe8d94b7eff123e03f97ca6978d33
> > Author: Richard Henderson 
> > Date:   Mon Jul 8 14:55:59 2013 -0700
> > 
> > exec: Support 64-bit operations in address_space_rw
> > 
> 
> Yes, we've already discussed it and agreed upon a solution.
> I guess it hasn't been applied yet?

Well, I saw the problem against latest upstream (HEAD 24943978c).

[Qemu-devel] [PATCH V5 0/4] Implement sync modes for drive-backup.

[Ian Main]

This patch adds sync modes on top of the work that Stefan Hajnoczi has done.

These patches apply on kevin/block with
'[PATCH] block: add drive_backup HMP command' also applied.

Hopefully all is in order as this is my first QEMU patch.  Many thanks to
Stephan and Fam Zheng for their help.

V2:

- No longer poll, instead use qemu_coroutine_yield().
- Use bdrv_co_is_allocated().
- Much better SYNC_MODE_NONE test.

V3:

- A few style fixes.
- Better commit message explaining how TOP and NONE operate.
- Verified using checkpatch.pl.

V4:

- Add patch to use the source as a backing hd during backup.
- Add patch to default sync mode none to qcow2.

V5:

- Fix qcow2 patch.  Forgot to git add final version.

Ian Main (4):
  Implement sync modes for drive-backup.
  Add tests for sync modes 'TOP' and 'NONE'
  Add backing drive while performing backup.
  Change default to qcow2 for sync mode none.

 block/backup.c| 104 ++
 blockdev.c|  30 
 include/block/block_int.h |   4 +-
 qapi-schema.json  |   5 ++
 qmp-commands.hx   |   1 +
 tests/qemu-iotests/055|  67 +--
 tests/qemu-iotests/055.out|   4 +-
 tests/qemu-iotests/group  |   2 +-
 tests/qemu-iotests/iotests.py |   5 ++
 9 files changed, 175 insertions(+), 47 deletions(-)

--
1.8.1.4

[Qemu-devel] [PATCH V5 2/4] Add tests for sync modes 'TOP' and 'NONE'

[Ian Main]

This patch adds tests for sync modes top and none.

Signed-off-by: Ian Main 
---
 tests/qemu-iotests/055| 67 ---
 tests/qemu-iotests/055.out|  4 +--
 tests/qemu-iotests/group  |  2 +-
 tests/qemu-iotests/iotests.py |  5 
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c66f8db..06d1f77 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -23,8 +23,9 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_io, create_image
 
+backing_img = os.path.join(iotests.test_dir, 'backing.img')
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
 
@@ -34,7 +35,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 def setUp(self):
 # Write data to the image so we can compare later
 qemu_img('create', '-f', iotests.imgfmt, test_img, 
str(TestSingleDrive.image_len))
-qemu_io('-c', 'write -P0x5d 0 64k', test_img)
+qemu_io('-c', 'write -P0x41 0 512', test_img)
 qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
 qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
 qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
@@ -60,6 +61,22 @@ class TestSingleDrive(iotests.QMPTestCase):
 event = self.cancel_and_wait()
 self.assert_qmp(event, 'data/type', 'backup')
 
+def test_cancel_sync_none(self):
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('drive-backup', device='drive0',
+ sync='none', format='raw', target=target_img)
+self.assert_qmp(result, 'return', {})
+time.sleep(1)
+self.vm.hmp_qemu_io('drive0', 'write -P0x5e 0 512')
+self.vm.hmp_qemu_io('drive0', 'aio_flush')
+time.sleep(1)
+# Verify that the original contents exist in the target image.
+self.assertEqual(-1, qemu_io('-c', 'read -P0x41 0 512', 
target_img).find("verification failed"))
+
+event = self.cancel_and_wait()
+self.assert_qmp(event, 'data/type', 'backup')
+
 def test_pause(self):
 self.assert_no_active_block_jobs()
 
@@ -102,6 +119,47 @@ class TestSingleDrive(iotests.QMPTestCase):
  target=target_img, sync='full')
 self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+class TestSyncModeTop(iotests.QMPTestCase):
+image_len = 2 * 1024 * 1024 # MB
+
+def setUp(self):
+create_image(backing_img, TestSyncModeTop.image_len)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, test_img)
+qemu_io('-c', 'write -P0x5d 0 64k', test_img)
+qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
+qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
+self.vm = iotests.VM().add_drive(test_img)
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(test_img)
+os.remove(backing_img)
+try:
+os.remove(target_img)
+except OSError:
+pass
+
+def test_complete_top(self):
+self.assert_no_active_block_jobs()
+result = self.vm.qmp('drive-backup', device='drive0', sync='top',
+ target=target_img)
+self.assert_qmp(result, 'return', {})
+
+# Custom completed check as we are not copying all data.
+completed = False
+while not completed:
+for event in self.vm.get_qmp_events(wait=True):
+if event['event'] == 'BLOCK_JOB_COMPLETED':
+self.assert_qmp(event, 'data/device', 'drive0')
+self.assert_qmp_absent(event, 'data/error')
+completed = True
+
+self.assert_no_active_block_jobs()
+self.vm.shutdown()
+self.assertTrue(iotests.compare_images(test_img, target_img),
+'target image does not match source after backup')
+
 class TestSetSpeed(iotests.QMPTestCase):
 image_len = 80 * 1024 * 1024 # MB
 
@@ -127,7 +185,8 @@ class TestSetSpeed(iotests.QMPTestCase):
 self.assert_qmp(result, 'return[0]/device', 'drive0')
 self.assert_qmp(result, 'return[0]/speed', 0)
 
-result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 
1024 * 1024)
+result = self.vm.qmp('block-job-set-speed', device='drive0',
+ speed=8 * 1024 * 1024)
 self.assert_qmp(result, 'return', {})
 
 # Ensure the speed we set was accepted
@@ -285,4 +344,4 @@ class TestSingleTransaction(iotests.QMPTestCase):
 self.assert_no_active_block_jobs()
 
 if __name__ == '__main__':
-iotests.main(supported_fmts=['raw', 'qcow2'])
+iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index fa16b5c..96961e

Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Sören Brinkmann]

On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> On 8 July 2013 23:40, Soren Brinkmann  wrote:
> > +
> > +if (ep) {
> > +*ep = hdr->ih_ep;
> > +}
> 
> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> but it makes sense for consistency with the other pointer
> argument handling.)
> 
> > +
> > +/* TODO: Check CPU type.  */
> > +if (is_linux) {
> > +if (hdr->ih_os == IH_OS_LINUX) {
> > +*is_linux = 1;
> > +} else {
> > +*is_linux = 0;
> > +}
> > +}
> > +
> > +break;
> > +case IH_TYPE_RAMDISK:
> > +address = *loadaddr;
> 
> This is inconsistent -- for IH_TYPE_KERNEL we accept
> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
The thing is in case of a ramdisk, it's a mandatory input argument which has
to be provided, whereas for the kernel, it's an optional output value.
And other than the load_ramdisk() and load_kernel() routines nobody is
supposed to call this function directly, IMHO. And I think plausibility
checks should be done in those routines when possible. And
load_ramdisk() ensures that the loadaddr pointer is valid.

What would be your suggested change?

Soren

Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Sören Brinkmann]

On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
> On 19 July 2013 18:39, Sören Brinkmann  wrote:
> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> >> On 8 July 2013 23:40, Soren Brinkmann  wrote:
> >> > +
> >> > +if (ep) {
> >> > +*ep = hdr->ih_ep;
> >> > +}
> >>
> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> >> but it makes sense for consistency with the other pointer
> >> argument handling.)
> >>
> >> > +
> >> > +/* TODO: Check CPU type.  */
> >> > +if (is_linux) {
> >> > +if (hdr->ih_os == IH_OS_LINUX) {
> >> > +*is_linux = 1;
> >> > +} else {
> >> > +*is_linux = 0;
> >> > +}
> >> > +}
> >> > +
> >> > +break;
> >> > +case IH_TYPE_RAMDISK:
> >> > +address = *loadaddr;
> >>
> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> > The thing is in case of a ramdisk, it's a mandatory input argument which has
> > to be provided, whereas for the kernel, it's an optional output value.
> > And other than the load_ramdisk() and load_kernel() routines nobody is
> > supposed to call this function directly, IMHO. And I think plausibility
> > checks should be done in those routines when possible. And
> > load_ramdisk() ensures that the loadaddr pointer is valid.
> 
> Well, by that argument you shouldn't introduce the "allow
> ep to be NULL" change...
I didn't introduce it, that is the current state for loading a kernel.
I introduced making it mandatory for the ramdisk case.

> 
> > What would be your suggested change?
> 
> I don't mind as long as we're consistent one way or the other
> about whether non-optional pointer arguments are checked for
> NULL.
As I said, depending on whether we call this function to load a kernel
or ramdisk that argument is optional or mandatory.
When it's optional we do a NULL check. In the mandatory case the caller
ensures validity.

So, if leaving it as is, is not an option.
How about this:
Moving the NULL check to load_kernel() and passing an always valid
pointer to load_uboot_image() and removing the NULL check there?
Then load_kernel() can pass on this value or not depending on the
validity of the pointer it received.

Soren

Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Peter Maydell]

On 19 July 2013 18:39, Sören Brinkmann  wrote:
> On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
>> On 8 July 2013 23:40, Soren Brinkmann  wrote:
>> > +
>> > +if (ep) {
>> > +*ep = hdr->ih_ep;
>> > +}
>>
>> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
>> but it makes sense for consistency with the other pointer
>> argument handling.)
>>
>> > +
>> > +/* TODO: Check CPU type.  */
>> > +if (is_linux) {
>> > +if (hdr->ih_os == IH_OS_LINUX) {
>> > +*is_linux = 1;
>> > +} else {
>> > +*is_linux = 0;
>> > +}
>> > +}
>> > +
>> > +break;
>> > +case IH_TYPE_RAMDISK:
>> > +address = *loadaddr;
>>
>> This is inconsistent -- for IH_TYPE_KERNEL we accept
>> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> The thing is in case of a ramdisk, it's a mandatory input argument which has
> to be provided, whereas for the kernel, it's an optional output value.
> And other than the load_ramdisk() and load_kernel() routines nobody is
> supposed to call this function directly, IMHO. And I think plausibility
> checks should be done in those routines when possible. And
> load_ramdisk() ensures that the loadaddr pointer is valid.

Well, by that argument you shouldn't introduce the "allow
ep to be NULL" change...

> What would be your suggested change?

I don't mind as long as we're consistent one way or the other
about whether non-optional pointer arguments are checked for
NULL.

-- PMM

Re: [Qemu-devel] Commit 23326164ae (exec: Support 64-bit op...) triggers assertion

[Luiz Capitulino]

On Fri, 19 Jul 2013 13:28:52 -0400
Luiz Capitulino  wrote:

> Hi,
> 
> Reproducer:
> 
> # ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native \
>   -enable-kvm -m 1G -monitor stdio  -usb -usbdevice host:1.43
> QEMU 1.5.50 monitor - type 'help' for more information
> (qemu) qemu-qmp: 
> /home/lcapitulino/work/src/upstream/qmp-unstable/exec.c:1927: 
> memory_access_size: Assertion `l >= access_size_min' failed.
> 
> I have an USB stick on 1.43, although I did manage to get this without
> USB at all (forgot how, though).

It's possible to reproduce it passing just -usb (I guess I was wrong about
reproducing it w/o USB).

Re: [Qemu-devel] [PATCH] hw/9pfs: Fix potential memory leak and avoid reuse of freed memory

[Stefan Weil]

Am 04.07.2013 10:53, schrieb M. Mohan Kumar:
> Stefan Weil  writes:
>
>> The leak was reported by cppcheck.
>>
>> Function proxy_init also calls g_free for ctx->fs_root.
>> Avoid reuse of this memory by setting ctx->fs_root to NULL.
>>
>> Signed-off-by: Stefan Weil 
> Reviewed-by: M. Mohan Kumar 
>> ---
>>
>> Hi,
>>
>> I'm not sure whether ctx->fs_root should also be freed in the error case.
>> Please feel free to modify my patch if needed.
>>
>> Regards
>> Stefan Weil
>>
>>  hw/9pfs/virtio-9p-proxy.c |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
>> index 8ba2959..5f44bb7 100644
>> --- a/hw/9pfs/virtio-9p-proxy.c
>> +++ b/hw/9pfs/virtio-9p-proxy.c
>> @@ -1153,10 +1153,12 @@ static int proxy_init(FsContext *ctx)
>>  sock_id = atoi(ctx->fs_root);
>>  if (sock_id < 0) {
>>  fprintf(stderr, "socket descriptor not initialized\n");
>> +g_free(proxy);
>>  return -1;
>>  }
>>  }
>>  g_free(ctx->fs_root);
>> +ctx->fs_root = NULL;
>>
>>  proxy->in_iovec.iov_base  = g_malloc(PROXY_MAX_IO_SZ + PROXY_HDR_SZ);
>>  proxy->in_iovec.iov_len   = PROXY_MAX_IO_SZ + PROXY_HDR_SZ;
>> -- 
>> 1.7.10.4


Please add this patch to the qemu-trivial queue:
http://patchwork.ozlabs.org/patch/251666/

Thanks,
Stefan

Re: [Qemu-devel] [PATCH v2 11/17] block: return get_block_status data and flags for formats

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: handle extents that are stored in bs->file
> 
>  block/cow.c  |  8 +++-
>  block/qcow.c |  9 -
>  block/qcow2.c| 16 ++--
>  block/qed.c  | 35 ---
>  block/sheepdog.c |  2 +-
>  block/vdi.c  | 13 -
>  block/vmdk.c | 19 ++-
>  block/vvfat.c| 11 ++-
>  8 files changed, 94 insertions(+), 19 deletions(-)
> 
> +++ b/block/qcow.c
> @@ -410,7 +410,14 @@ static int64_t coroutine_fn 
> qcow_co_get_block_status(BlockDriverState *bs,
>  if (n > nb_sectors)
>  n = nb_sectors;
>  *pnum = n;
> -return (cluster_offset != 0);
> +if (!cluster_offset) {
> + return 0;

TAB damage.

> +++ b/block/qed.c
> @@ -652,16 +652,36 @@ static int bdrv_qed_create(const char *filename, 
> QEMUOptionParameter *options)
>  }
>  
>  typedef struct {
> +BlockDriverState *bs;
>  Coroutine *co;
> -int is_allocated;
> +uint64_t pos;
> +int64_t status;
>  int *pnum;
>  } QEDIsAllocatedCB;
>  
>  static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, 
> size_t len)
>  {
>  QEDIsAllocatedCB *cb = opaque;
> +BDRVQEDState *s = cb->bs->opaque;
>  *cb->pnum = len / BDRV_SECTOR_SIZE;
> -cb->is_allocated = (ret == QED_CLUSTER_FOUND || ret == QED_CLUSTER_ZERO);
> +switch (ret) {
> +case QED_CLUSTER_FOUND:
> +   offset |= qed_offset_into_cluster(s, cb->pos);
> + cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
> + break;

Even more mix of inconsistent use of TAB.

> +++ b/block/vdi.c
> @@ -479,12 +479,23 @@ static int64_t coroutine_fn 
> vdi_co_get_block_status(BlockDriverState *bs,
>  size_t sector_in_block = sector_num % s->block_sectors;
>  int n_sectors = s->block_sectors - sector_in_block;
>  uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]);
> +uint64_t offset;
> +int result;
> +
>  logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum);
>  if (n_sectors > nb_sectors) {
>  n_sectors = nb_sectors;
>  }
>  *pnum = n_sectors;
> -return VDI_IS_ALLOCATED(bmap_entry);
> +result = VDI_IS_ALLOCATED(bmap_entry);
> +if (!result) {
> + return 0;

On a roll with TABs.

As that's whitespace only,

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] BUG: make check -> ERROR:tests/boot-order-test.c:43:test_a_boot_order

[Alex Bligh]

Markus,

--On 19 July 2013 19:43:14 +0200 Markus Armbruster  
wrote:



The tests pass for me.  Anything unusual in your environment?

Did you rebuild from scratch, or just ran make after git-pull?  Our
build process still isn't 100% reliable...


You're quite correct - a make clean, and a long rebuild due to failing
to limit my configure options fixed this. Apologies.

--
Alex Bligh

Re: [Qemu-devel] [PATCH v2 10/17] block: define get_block_status return value

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-7

bits 3-8, actually

> are left for future extensions.
> 
> The return code is compatible with the old is_allocated API: returning
> just 0 or 1 (aka BDRV_BLOCK_DATA) will not cause any behavioral change
> in clients of is_allocated.  We will return more precise information
> in the next patches.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: improved comment
> 
>  block.c   |  7 +--
>  include/block/block.h | 26 ++
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 

> +++ b/include/block/block.h
> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>  
> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> + * BDRV_BLOCK_ZERO: sectors read as zero
> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> + *
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> + * bs->file where sector data can be read from as raw data.

And handy that the offset happens to be aligned to the current 512
sector size, so you can just mask without shifting to get an aligned
offset to the start of the sector.

> + *
> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> + *
> + * DATA ZERO OFFSET_VALID
> + *  ttt   sectors read as zero, bs->file is zero at offset
> + *  tft   sectors read as valid from bs->file at offset
> + *  ftt   sectors preallocated, read as zero, bs->file not
> + *necessarily zero at offset
> + *  fft   sectors preallocated but read from backing_hd,
> + *bs->file contains garbage at offset
> + *  ttf   sectors preallocated, read as zero, unknown offset
> + *  tff   sectors read from unknown file or offset
> + *  ftf   not allocated or unknown offset, read as zero
> + *  fff   not allocated or unknown offset, read from 
> backing_hd

Makes sense.

No further comments beyond other reviewers, if you want to add:
Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Commit 23326164ae (exec: Support 64-bit op...) triggers assertion

[Luiz Capitulino]

Hi,

Reproducer:

# ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native \
  -enable-kvm -m 1G -monitor stdio  -usb -usbdevice host:1.43
QEMU 1.5.50 monitor - type 'help' for more information
(qemu) qemu-qmp: /home/lcapitulino/work/src/upstream/qmp-unstable/exec.c:1927: 
memory_access_size: Assertion `l >= access_size_min' failed.

I have an USB stick on 1.43, although I did manage to get this without
USB at all (forgot how, though).

Bisect says the bug was introduced by:

commit 23326164ae6fe8d94b7eff123e03f97ca6978d33
Author: Richard Henderson 
Date:   Mon Jul 8 14:55:59 2013 -0700

exec: Support 64-bit operations in address_space_rw

[Qemu-devel] [PATCH] [RFC] aio/timers: Drop alarm timers; introduce QEMUClock to AioContext; run timers in aio_poll

[Alex Bligh]

[ This is a patch for RFC purposes only. It is compile tested on Linux x86_64 
only
and passes make check (or rather did before make check started dying in the
boot order test - different bug). I'd like to know whether I'm going in
the right direction ]

We no longer need alarm timers to trigger QEMUTimer as we'll be polling
them in aio_poll.

Remove static declaration from qemu_new_clock and introduce qemu_free_clock.

Maintain a list of QEMUClocks.

Introduce qemu_clock_deadline_ns and qemu_clock_deadine_all_ns which calculate 
how
long aio_poll etc. should wait, plus (for the time being) a conversion to 
milliseconds.

Make qemu_run_timers return a bool to indicate progress.

Add QEMUClock to AioContext.

Run timers attached to clock in aio_poll

Signed-off-by: Alex Bligh 
---
 aio-posix.c  |   16 +-
 aio-win32.c  |   20 +-
 async.c  |2 +
 include/block/aio.h  |5 +
 include/qemu/timer.h |   15 +-
 main-loop.c  |9 +-
 qemu-timer.c |  599 --
 vl.c |5 +-
 8 files changed, 150 insertions(+), 521 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index b68eccd..6401259 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -173,6 +173,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 {
 AioHandler *node;
 int ret;
+int timeout;
 bool busy, progress;
 
 progress = false;
@@ -195,6 +196,9 @@ bool aio_poll(AioContext *ctx, bool blocking)
 return true;
 }
 
+/* Run our timers */
+progress |= qemu_run_timers(ctx->clock);
+
 ctx->walking_handlers++;
 
 g_array_set_size(ctx->pollfds, 0);
@@ -232,9 +236,10 @@ bool aio_poll(AioContext *ctx, bool blocking)
 }
 
 /* wait until next event */
+timeout = qemu_timeout_ns_to_ms(qemu_clock_deadline_all_ns());
 ret = g_poll((GPollFD *)ctx->pollfds->data,
  ctx->pollfds->len,
- blocking ? -1 : 0);
+ blocking ? timeout : 0);
 
 /* if we have any readable fds, dispatch event */
 if (ret > 0) {
@@ -250,6 +255,15 @@ bool aio_poll(AioContext *ctx, bool blocking)
 }
 }
 
+if (blocking) {
+/* Run the timers a second time. We do this because otherwise aio_wait
+ * will not note progress - and will stop a drain early - if we have
+ * a timer that was not ready to run entering g_poll but is ready
+ * after g_poll. This will only do anything if a timer has expired.
+ */
+progress |= qemu_run_timers(ctx->clock);
+}
+
 assert(progress || busy);
 return true;
 }
diff --git a/aio-win32.c b/aio-win32.c
index 38723bf..68343ba 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -98,6 +98,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
 bool busy, progress;
 int count;
+int timeout;
 
 progress = false;
 
@@ -111,6 +112,9 @@ bool aio_poll(AioContext *ctx, bool blocking)
 progress = true;
 }
 
+/* Run timers */
+progress |= qemu_run_timers(ctx->clock);
+
 /*
  * Then dispatch any pending callbacks from the GSource.
  *
@@ -174,8 +178,11 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
 /* wait until next event */
 while (count > 0) {
-int timeout = blocking ? INFINITE : 0;
-int ret = WaitForMultipleObjects(count, events, FALSE, timeout);
+int ret;
+
+timeout = blocking ?
+qemu_timeout_ns_to_ms(qemu_clock_deadline_all_ns()) : 0;
+ret = WaitForMultipleObjects(count, events, FALSE, timeout);
 
 /* if we have any signaled events, dispatch event */
 if ((DWORD) (ret - WAIT_OBJECT_0) >= count) {
@@ -214,6 +221,15 @@ bool aio_poll(AioContext *ctx, bool blocking)
 events[ret - WAIT_OBJECT_0] = events[--count];
 }
 
+if (blocking) {
+/* Run the timers a second time. We do this because otherwise aio_wait
+ * will not note progress - and will stop a drain early - if we have
+ * a timer that was not ready to run entering g_poll but is ready
+ * after g_poll. This will only do anything if a timer has expired.
+ */
+progress |= qemu_run_timers(ctx->clock);
+}
+
 assert(progress || busy);
 return true;
 }
diff --git a/async.c b/async.c
index 90fe906..0d41431 100644
--- a/async.c
+++ b/async.c
@@ -177,6 +177,7 @@ aio_ctx_finalize(GSource *source)
 aio_set_event_notifier(ctx, &ctx->notifier, NULL, NULL);
 event_notifier_cleanup(&ctx->notifier);
 g_array_free(ctx->pollfds, TRUE);
+qemu_free_clock(ctx->clock);
 }
 
 static GSourceFuncs aio_source_funcs = {
@@ -215,6 +216,7 @@ AioContext *aio_context_new(void)
 aio_set_event_notifier(ctx, &ctx->notifier, 
(EventNotifierHandler *)
event_notifier_test_and_clear, NULL);
+ctx->clock = qemu_new_clock(QEMU_CLOCK_REALTIME);
 
 return ctx

[Qemu-devel] BUG: make check -> ERROR:tests/boot-order-test.c:43:test_a_boot_order

[Alex Bligh]

Using current master: 24943978cbe79634a9a8b02a20efb25b29b3ab49

'make check' gives:

ERROR:tests/boot-order-test.c:43:test_a_boot_order: assertion failed 
(actual == expected_boot): (0x1230 == 0x)

GTester: last random seed: R02Se371669dcb0a3274fa9c170e22654334

I'm on x86_64

I can't help but note the last commit is:

commit 24943978cbe79634a9a8b02a20efb25b29b3ab49
Author: Markus Armbruster 
Date:   Wed Jun 26 15:52:23 2013 +0200

   boot-order-test: Add tests for Sun4u

Obviously that should make no difference for x86_64. I've copied
the author just in case.

--
Alex Bligh

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC)

[Scott Wood]

On 07/19/2013 04:22:46 AM, Fabien Chouteau wrote:

On 07/18/2013 10:37 PM, Scott Wood wrote:
> On 07/18/2013 04:27:50 AM, Fabien Chouteau wrote:
>> The BD is full, we will have to put the rest of padding in the  
next one.

>
> What rest of padding?  I thought you said rx_padding was 2  
somehow?  If that were true, then it would be zero at the end.

>

Read my description again.


OK, I was confused by your answering "yes" to "wouldn't *size be 2  
here" -- I thought it was clear from the context that I was talking  
about at the time of the "if (*size == etsec->rx_padding)" statement.   
Maybe you thought I was talking about what would happen at that  
if-statement in the next descriptor?


In any case, it's a bit hard to follow this code.  rx_fcb_size is  
included in to_write, but not in *size.  rx_padding is included in  
*size, but not in to_write.  And it generally makes me nervous to see  
code that will go into an infinite loop (or other odd behavior) unless  
an exact-equality terminating condition is met, especially when it's as  
complicated as this, without an assertion to check for the bad case  
(even if it looks like it could never happen).


-Scott

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
we had a workaround in spapr_io_ops), now it works so double swap happens
and everything gets broken.

If we talk about VGA (in powerpc, it is all about powerpc), I guess
memory_region_iorange_write() will go through mr->ops->old_portio branch
and won't do any byte swapping (so spapr_io_ops will do the job), so we are
fine here. I do not understand yet why it works on mac99 though, too late
here :)

h_logical_store is a hypercall for system firmware to do cache inhibited
read/write.


This is with the patch applied (git checkout  b40acf9):


#0  virtqueue_init (vq=0x11014ac0) at
/home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1  0x10371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
addr=0xd0fb000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2  0x102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
val=0xd0fb)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3  0x10202f08 in virtio_pci_config_write (opaque=0x11019580,
addr=0x8, val=0xd0fb, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4  0x1037e220 in memory_region_write_accessor (opaque=0x11019c78,
addr=0x8, value=0x1f0edc00,
size=0x4, shift=0x0, mask=0x) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x1037e36c in access_with_adjusted_size (addr=0x8,
value=0x1f0edc00, size=0x4,
access_size_min=0x1, access_size_max=0x4, access=
@0x1069df40: 0x1037e164 , opaque=0x11019c78)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#6  0x10380b5c in memory_region_dispatch_write (mr=0x11019c78,
addr=0x8, data=0xd0fb, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#7  0x10383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
val=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#8  0x102e2fdc in address_space_rw (as=0x10ef4350
, addr=0x48,
buf=0x1f0edde0 "", len=0x4, is_write=0x1) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1918
#9  0x102e33c8 in address_space_write (as=0x10ef4350
, addr=0x48,
buf=0x1f0edde0 "", len=0x4) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1969
#10 0x10375754 in cpu_outl (addr=0x48, val=0xfbd0)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
#11 0x10358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#12 0x1037e220 in memory_region_write_accessor (opaque=0x110191f8,
addr=0x48, value=0x1f0ee060,
size=0x4, shift=0x0, mask=0x) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#13 0x1037e36c in access_with_adjusted_size (addr=0x48,
value=0x1f0ee060, size=0x4,
access_size_min=0x1, access_size_max=0x4, access=
@0x1069df40: 0x1037e164 , opaque=0x110191f8)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#14 0x10380b5c in memory_region_dispatch_write (mr=0x110191f8,
addr=0x48, data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#15 0x10383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
val=0xd0fb, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#16 0x102e47ac in stl_phys_internal (addr=0x1008048,
val=0xd0fb, endian=
DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
#17 0x102e48a8 in stl_phys (addr=0x1008048, val=0xd0fb)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
#18 0x10354f1c in h_logical_store (cpu=0x1f0f0010,
spapr=0x10fe9510, opcode=0x40,
args=0x1ffd0030) at
/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570



This is without this patch (i.e. git checkout  b40acf9^ ):

#0  virtqueue_init (vq=0x11014ac0) at
/home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1  0x103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
addr=0xffe2000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2  0x102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
val=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3  0x10202f08 in virtio_pci_config_write (opaque=0x11019580,
addr=0x8, val=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4  0x1037dca8 in memory_region_write_accessor (opaque=0x11019c78,
addr=0x8, value=0x1f0edca8,
size=0x4, shift=0x0, mask=0x) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x1037ddf4 in access_with_adjusted_size (addr=0x8,
value=0x1f0edca8, size=0x4,
access_size_min=0x1, access_size_max=0x4, access=
@0x1069def8: 0x1037dbec , opaque=0x1101

[Qemu-devel] [PATCH v3] linux-user: Handle compressed ISA encodings when processing MIPS exceptions

[Kwok Cheung Yeung]

Decode trap instructions during the handling of an EXCP_BREAK or EXCP_TRAP
according to the current ISA mode.

Signed-off-by: Kwok Cheung Yeung 
---
 linux-user/main.c | 46 +++---
 1 file changed, 43 insertions(+), 3 deletions(-)

v2->v3: Handle microMIPS and MIPS16e instructions when processing EXCP_BREAK.

diff --git a/linux-user/main.c b/linux-user/main.c
index 7f15d3d..b137216 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2348,7 +2348,31 @@ done_syscall:
 abi_ulong trap_instr;
 unsigned int code;
 
-ret = get_user_ual(trap_instr, env->active_tc.PC);
+if (env->hflags & MIPS_HFLAG_M16) {
+if (env->insn_flags & ASE_MICROMIPS) {
+/* microMIPS mode */
+abi_ulong instr[2];
+
+ret = get_user_u16(instr[0], env->active_tc.PC) ||
+  get_user_u16(instr[1], env->active_tc.PC + 2);
+
+trap_instr = (instr[0] << 16) | instr[1];
+} else {
+/* MIPS16e mode */
+ret = get_user_u16(trap_instr, env->active_tc.PC);
+if (ret != 0) {
+goto error;
+}
+code = (trap_instr >> 6) & 0x3f;
+if (do_break(env, &info, code) != 0) {
+goto error;
+}
+break;
+}
+} else {
+ret = get_user_ual(trap_instr, env->active_tc.PC);
+}
+
 if (ret != 0) {
 goto error;
 }
@@ -2372,14 +2396,30 @@ done_syscall:
 abi_ulong trap_instr;
 unsigned int code = 0;
 
-ret = get_user_ual(trap_instr, env->active_tc.PC);
+if (env->hflags & MIPS_HFLAG_M16) {
+/* microMIPS mode */
+abi_ulong instr[2];
+
+ret = get_user_u16(instr[0], env->active_tc.PC) ||
+  get_user_u16(instr[1], env->active_tc.PC + 2);
+
+trap_instr = (instr[0] << 16) | instr[1];
+} else {
+ret = get_user_ual(trap_instr, env->active_tc.PC);
+}
+
 if (ret != 0) {
 goto error;
 }
 
 /* The immediate versions don't provide a code.  */
 if (!(trap_instr & 0xFC00)) {
-code = ((trap_instr >> 6) & ((1 << 10) - 1));
+if (env->hflags & MIPS_HFLAG_M16) {
+/* microMIPS mode */
+code = ((trap_instr >> 12) & ((1 << 4) - 1));
+} else {
+code = ((trap_instr >> 6) & ((1 << 10) - 1));
+}
 }
 
 if (do_break(env, &info, code) != 0) {
-- 
1.8.3.3

Re: [Qemu-devel] [PATCH 0/3] dataplane: virtio-blk live migration with x-data-plane=on

[yinyin]

hi, stefan:
I use systemtap to test this patch,the migration will success. But I 
found the dataplane will start again after migration start. the 
virtio_blk_handle_output will start dataplane.


virtio_blk_data_plane_stop pid:29037 tid:29037
 0x6680fe : virtio_blk_data_plane_stop+0x0/0x232 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x667da2 : virtio_blk_data_plane_destroy+0x33/0x70 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x66a2e4 : virtio_blk_migration_state_changed+0x7c/0x12d 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x7740f4 : notifier_list_notify+0x59/0x79 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a5b16 : migrate_fd_connect+0xc6/0x104 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a3dba : tcp_wait_for_connect+0x6e/0x84 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x76c755 : wait_for_connect+0x170/0x19a 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a2e2c : qemu_iohandler_poll+0xec/0x188 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a3834 : main_loop_wait+0x92/0xc9 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x630083 : main_loop+0x5d/0x82 [/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x636954 : main+0x3666/0x369a [/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x37cf01ecdd [/lib64/libc-2.12.so+0x1ecdd/0x393000]
virtio_blk_data_plane_start pid:29037 tid:29037
 0x667ddf : virtio_blk_data_plane_start+0x0/0x31f 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x669802 : virtio_blk_handle_output+0x9c/0x118 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x6ac6ff : virtio_queue_notify_vq+0x92/0xa8 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x6ae128 : virtio_queue_host_notifier_read+0x50/0x66 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x6ae1bb : virtio_queue_set_host_notifier_fd_handler+0x7d/0x93 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x59cdf1 : virtio_pci_set_host_notifier_internal+0x132/0x157 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x59e98c : virtio_pci_set_host_notifier+0x73/0x8e 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x6682a8 : virtio_blk_data_plane_stop+0x1aa/0x232 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x667da2 : virtio_blk_data_plane_destroy+0x33/0x70 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x66a2e4 : virtio_blk_migration_state_changed+0x7c/0x12d 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x7740f4 : notifier_list_notify+0x59/0x79 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a5b16 : migrate_fd_connect+0xc6/0x104 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a3dba : tcp_wait_for_connect+0x6e/0x84 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x76c755 : wait_for_connect+0x170/0x19a 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a2e2c : qemu_iohandler_poll+0xec/0x188 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x5a3834 : main_loop_wait+0x92/0xc9 
[/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x630083 : main_loop+0x5d/0x82 [/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x636954 : main+0x3666/0x369a [/root/dataplane/qemu/x86_64-softmmu/qemu-kvm]
 0x37cf01ecdd [/lib64/libc-2.12.so+0x1ecdd/0x393000]


systemtap scripts:
probe 
process("/root/dataplane/qemu/x86_64-softmmu/qemu-kvm").function("virtio_blk_data_plane_start")
{
  printf("virtio_blk_data_plane_start pid:%d tid:%d\n",pid(),tid())
  print_ubacktrace();
}


probe 
process("/root/dataplane/qemu/x86_64-softmmu/qemu-kvm").function("virtio_blk_data_plane_stop")
{
  printf("virtio_blk_data_plane_stop pid:%d tid:%d\n",pid(),tid())
  print_ubacktrace();
}


Yin Yin
yin@cs2c.com.cn

[Qemu-devel] [PATCH v2] linux-user: Handle microMIPS encoding when processing trap exceptions

[Kwok Cheung Yeung]

Decode trap instructions during the handling of an EXCP_TRAP according to
the current ISA mode.

Signed-off-by: Kwok Cheung Yeung 
---
 linux-user/main.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

v2: Read microMIPS instructions sequentially as 16-bit values to avoid
endianess issues. Add braces to if statement to conform to formatting
standards.

diff --git a/linux-user/main.c b/linux-user/main.c
index 7f15d3d..7faa945 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2372,14 +2372,30 @@ done_syscall:
 abi_ulong trap_instr;
 unsigned int code = 0;
 
-ret = get_user_ual(trap_instr, env->active_tc.PC);
+if (env->hflags & MIPS_HFLAG_M16) {
+/* microMIPS mode */
+abi_ulong instr[2];
+
+ret = get_user_u16(instr[0], env->active_tc.PC) ||
+  get_user_u16(instr[1], env->active_tc.PC + 2);
+
+trap_instr = (instr[0] << 16) | instr[1];
+} else {
+ret = get_user_ual(trap_instr, env->active_tc.PC);
+}
+
 if (ret != 0) {
 goto error;
 }
 
 /* The immediate versions don't provide a code.  */
 if (!(trap_instr & 0xFC00)) {
-code = ((trap_instr >> 6) & ((1 << 10) - 1));
+if (env->hflags & MIPS_HFLAG_M16) {
+/* microMIPS mode */
+code = ((trap_instr >> 12) & ((1 << 4) - 1));
+} else {
+code = ((trap_instr >> 6) & ((1 << 10) - 1));
+}
 }
 
 if (do_break(env, &info, code) != 0) {
-- 
1.8.3.3

Re: [Qemu-devel] [PATCH v2] linux-user: Handle microMIPS encoding when processing trap exceptions

[Kwok Cheung Yeung]

On 19/07/2013 3:52 PM, Peter Maydell wrote:

On 19 July 2013 15:47, Kwok Cheung Yeung  wrote:

Decode trap instructions during the handling of an EXCP_TRAP according to
the current ISA mode.

Signed-off-by: Kwok Cheung Yeung 
---
  linux-user/main.c | 20 ++--
  1 file changed, 18 insertions(+), 2 deletions(-)

v2: Read microMIPS instructions sequentially as 16-bit values to avoid
 endianess issues. Add braces to if statement to conform to formatting
 standards.


This code looks OK but last time round I asked about
EXCP_BREAK -- why doesn't that also need to change?



This patch was intended to fix the handling of floating-point exceptions while 
running the GCC unit tests (gcc.c-torture/execute/20101011-1.c) on microMIPS, 
which only requires EXCP_TRAP to work properly. I'll post a version with 
EXCP_BREAK fixed shortly.


Thanks

Kwok

Re: [Qemu-devel] [PATCH v2] linux-user: Handle microMIPS encoding when processing trap exceptions

[Peter Maydell]

On 19 July 2013 15:47, Kwok Cheung Yeung  wrote:
> Decode trap instructions during the handling of an EXCP_TRAP according to
> the current ISA mode.
>
> Signed-off-by: Kwok Cheung Yeung 
> ---
>  linux-user/main.c | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> v2: Read microMIPS instructions sequentially as 16-bit values to avoid
> endianess issues. Add braces to if statement to conform to formatting
> standards.

This code looks OK but last time round I asked about
EXCP_BREAK -- why doesn't that also need to change?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Bug Fix:Segmentation fault when use usb-ehci device

[Peter Maydell]

On 19 July 2013 15:12, Andreas Färber  wrote:
> No, I don't. There were other segfault avoidance patches like yours over
> the past months - they're all fixing individual segfault symptoms.
> Question for Paolo is whether we want to continue to discover them one
> by one or whether to implement a fallback inside memory code if .read or
> .write is NULL.

I think that the correct behaviour is "if neither
.read nor .oldmmio.read[x] are set then behave as if
.valid.accepts returned false" (ie "device has not responded
to the read access, bus error").

That said, if we want to add do-nothing functions instead,
then (a) having memory.c provide a single set of nop functions
that devices can use would be nicer than lots of individual
nop functions and (b) a list to start with:

$ for f in $(find . -name '*.c'); do perl -e '$s = 0; while (<>) { if
(/MemoryRegionOps (.*) =/) { $n = $1; $s = 1; next; } next if $s == 0;
if (/\.read = /) { $s |= 2; } if (/\.write = /) { $s |= 4; } if (/;/)
{ print "$ARGV: $n: missing read\n" unless $s & 2; print "$ARGV: $n:
missing write\n" unless $s & 4; $s = 0; }}' $f; done
./memory.c: unassigned_mem_ops: missing read
./memory.c: unassigned_mem_ops: missing write
./exec.c: notdirty_mem_ops: missing read
./hw/pci-host/prep.c: PPC_intack_ops: missing write
./hw/ssi/xilinx_spips.c: lqspi_ops: missing write
./hw/arm/omap1.c: omap_pwt_ops: missing read
./hw/arm/musicpal.c: mv88w8618_wlan_ops: missing write
./hw/scsi/megasas.c: megasas_queue_ops: missing write
./hw/usb/hcd-ehci.c: ehci_mmio_caps_ops: missing write
./hw/usb/hcd-uhci.c: uhci_ioport_ops: missing read
./hw/intc/openpic_kvm.c: kvm_openpic_mem_ops: missing read
./hw/intc/openpic.c: openpic_glb_ops_le: missing read
./hw/intc/openpic.c: openpic_glb_ops_be: missing read
./hw/intc/openpic.c: openpic_tmr_ops_le: missing read
./hw/intc/openpic.c: openpic_tmr_ops_be: missing read
./hw/intc/openpic.c: openpic_cpu_ops_le: missing read
./hw/intc/openpic.c: openpic_cpu_ops_be: missing read
./hw/intc/openpic.c: openpic_src_ops_le: missing read
./hw/intc/openpic.c: openpic_src_ops_be: missing read
./hw/pci/msix.c: msix_pba_mmio_ops: missing write
./hw/xen/xen_platform.c: xen_pci_io_ops: missing read
./hw/misc/lm32_sys.c: sys_ops: missing read
./hw/misc/pc-testdev.c: test_irq_ops: missing read
./hw/misc/pc-testdev.c: test_flush_ops: missing read
./hw/misc/vfio.c: vfio_ati_3c3_quirk: missing write
./hw/misc/debugexit.c: debug_exit_ops: missing read
./hw/net/lan9118.c: *mem_ops: missing read
./hw/net/lan9118.c: *mem_ops: missing write
./hw/char/grlib_apbuart.c: grlib_apbuart_ops: missing read
./hw/char/grlib_apbuart.c: grlib_apbuart_ops: missing write
./hw/isa/pc87312.c: pc87312_io_ops: missing read
./hw/nvram/fw_cfg.c: fw_cfg_ctl_mem_ops: missing read

No doubt there are some false positives in there (eg fw_cfg.c
provides a valid function so we'll never try to do reads)
and it may miss some.

-- PMM

Re: [Qemu-devel] [PATCH] Bug Fix:Segmentation fault when use usb-ehci device

[Andreas Färber]

Am 19.07.2013 04:26, schrieb Mike Qiu:
> 于 2013/7/19 1:14, Andreas Färber 写道:
>> There's some typos in the commit message, but the change looks okay to
>> me - although there were discussions to catch this on the memory API
>> side of things instead.
> You mean this patch: see below:
> 
> exec: Support 64-bit operations in address_s

No, I don't. There were other segfault avoidance patches like yours over
the past months - they're all fixing individual segfault symptoms.
Question for Paolo is whether we want to continue to discover them one
by one or whether to implement a fallback inside memory code if .read or
.write is NULL.

Andreas

> 
> BTW, this bug has been opened before?
> 
> Thanks
> Mike
>>
>> Regards,
>> Andreas
>>
>>> Thanks
>>> Mike
>>> 2013/7/16 11:50, Mike Qiu wrote:
 For usb-ehci in qemu, its caps just has read() operation,
 the write() operation does not exist.

 This cause a Segmentation fault when use usb-ehci device in ppc64
 platform.

 here is gdb output:

 Program received signal SIGSEGV, Segmentation fault.
 [Switching to Thread 0x3fffa7fcef20 (LWP 6793)]
 0x103f5244 in memory_region_oldmmio_write_accessor
 (opaque=0x113e9e78, addr=9, value=0x3fffa7fce088,
 size=1, shift=0, mask=255) at /home/Mike/qemu-impreza/memory.c:384
 384  mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
 (gdb) p *mr->ops
 $1 = {read = @0x10716f68: 0x1020699c , write = 0,
endianness = DEVICE_LITTLE_ENDIAN, valid = {min_access_size = 1,
max_access_size = 4, unaligned = false, accepts = 0}, impl =
{min_access_size = 1, max_access_size = 1, unaligned = false},
old_mmio = {read = {0, 0, 0}, write = {0, 0, 0}}}

 Becasue function write() of mr->ops has not been implement, in
 function memory_region_dispatch_write(), it call
 oldmmio write accessor, but at the same time old_mmio still not
 been implement by default.

 That is the root cause of the Segmentation fault.

 To solve this problem, add empty function: ehci_caps_write()

 Signed-off-by: Mike Qiu 
 ---
  hw/usb/hcd-ehci.c |7 +++
  1 file changed, 7 insertions(+)

 diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
 index 67e4b24..6c8a439 100644
 --- a/hw/usb/hcd-ehci.c
 +++ b/hw/usb/hcd-ehci.c
 @@ -1072,6 +1072,12 @@ static void ehci_port_write(void *ptr, hwaddr addr,
  trace_usb_ehci_portsc_change(addr + s->portscbase, addr >> 2, 
 *portsc, old);
  }

 +static void ehci_caps_write(void *ptr, hwaddr addr, uint64_t val,
 +   unsigned size)
 +{
 +/* nothing */
 +}
 +
  static void ehci_opreg_write(void *ptr, hwaddr addr,
   uint64_t val, unsigned size)
  {
 @@ -2380,6 +2386,7 @@ static void ehci_frame_timer(void *opaque)

  static const MemoryRegionOps ehci_mmio_caps_ops = {
  .read = ehci_caps_read,
 +.write = ehci_caps_write,
  .valid.min_access_size = 1,
  .valid.max_access_size = 4,
  .impl.min_access_size = 1,
>>>
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH] gluster: Add image resize support

[Bharata B Rao]

From: Paolo Bonzini 

Implement .bdrv_truncate in GlusterFS block driver so that GlusterFS backend
can support image resizing.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Bharata B Rao 
Tested-by: Bharata B Rao 
---
 block/gluster.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 6de418c..645b7f1 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -493,6 +493,19 @@ out:
 return NULL;
 }
 
+static int qemu_gluster_truncate(BlockDriverState *bs, int64_t offset)
+{
+int ret;
+BDRVGlusterState *s = bs->opaque;
+
+ret = glfs_ftruncate(s->fd, offset);
+if (ret < 0) {
+return -errno;
+}
+
+return 0;
+}
+
 static BlockDriverAIOCB *qemu_gluster_aio_readv(BlockDriverState *bs,
 int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
 BlockDriverCompletionFunc *cb, void *opaque)
@@ -631,6 +644,7 @@ static BlockDriver bdrv_gluster = {
 .bdrv_create  = qemu_gluster_create,
 .bdrv_getlength   = qemu_gluster_getlength,
 .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size,
+.bdrv_truncate= qemu_gluster_truncate,
 .bdrv_aio_readv   = qemu_gluster_aio_readv,
 .bdrv_aio_writev  = qemu_gluster_aio_writev,
 .bdrv_aio_flush   = qemu_gluster_aio_flush,
@@ -650,6 +664,7 @@ static BlockDriver bdrv_gluster_tcp = {
 .bdrv_create  = qemu_gluster_create,
 .bdrv_getlength   = qemu_gluster_getlength,
 .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size,
+.bdrv_truncate= qemu_gluster_truncate,
 .bdrv_aio_readv   = qemu_gluster_aio_readv,
 .bdrv_aio_writev  = qemu_gluster_aio_writev,
 .bdrv_aio_flush   = qemu_gluster_aio_flush,
@@ -669,6 +684,7 @@ static BlockDriver bdrv_gluster_unix = {
 .bdrv_create  = qemu_gluster_create,
 .bdrv_getlength   = qemu_gluster_getlength,
 .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size,
+.bdrv_truncate= qemu_gluster_truncate,
 .bdrv_aio_readv   = qemu_gluster_aio_readv,
 .bdrv_aio_writev  = qemu_gluster_aio_writev,
 .bdrv_aio_flush   = qemu_gluster_aio_flush,
@@ -688,6 +704,7 @@ static BlockDriver bdrv_gluster_rdma = {
 .bdrv_create  = qemu_gluster_create,
 .bdrv_getlength   = qemu_gluster_getlength,
 .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size,
+.bdrv_truncate= qemu_gluster_truncate,
 .bdrv_aio_readv   = qemu_gluster_aio_readv,
 .bdrv_aio_writev  = qemu_gluster_aio_writev,
 .bdrv_aio_flush   = qemu_gluster_aio_flush,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[Peter Lieven]

On 19.07.2013 16:00, ronnie sahlberg wrote:

On Fri, Jul 19, 2013 at 6:49 AM, Peter Lieven  wrote:

On 19.07.2013 15:25, ronnie sahlberg wrote:

On Thu, Jul 18, 2013 at 11:08 PM, Peter Lieven  wrote:

On 19.07.2013 07:58, Paolo Bonzini wrote:

Il 18/07/2013 21:28, Peter Lieven ha scritto:

thanks for the details. I think to have optimal performance and best
change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Agreed about this.


as well as the write_zeroes_w_discard capability via the BDI

But why this?!?  It is _not_ needed.  All you need is to change the
default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is
nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the
-S
value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori.
This
has the benefit that combined with bdrv_get_block_status requests before
it
might
not need to touch large areas of the volume. Speaking for iSCSI its
likely
that
the user sets a fresh volume as the destination, but its not guaranteed.
With the Patch 4 there are only done a few get_block_status requests to
verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or
thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes
about
10-12
get_block_status requests to verify that it is completely unmapped. After
this
I am safe to set has_zero_init = 1 in qemu-img convert.

If I would convert a 1TB image to this target where lets say 50% are at
leat
15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write
same
requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame
exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send
one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send
writesame for ~667xOUG in each command and then it would "only" take
~100 writesames instead of ~35000.

So as long as you are sending in multiples of OUG you should be fine.

do I not have to take care of max_ws_size?

Yes you need to handle mas_ws_size   but I would imagine that on most
targets that max_ws_size >> OUG
I would be surprised if a target would set max_ws_size to just a single OUG.

for may target it is ;-(

maybe I would use max_ws_size for the write_zeroes_with unmap
thing. I would think it is a multiple of oug if it is specified.

Peter

Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[ronnie sahlberg]

On Fri, Jul 19, 2013 at 6:49 AM, Peter Lieven  wrote:
> On 19.07.2013 15:25, ronnie sahlberg wrote:
>>
>> On Thu, Jul 18, 2013 at 11:08 PM, Peter Lieven  wrote:
>>>
>>> On 19.07.2013 07:58, Paolo Bonzini wrote:

 Il 18/07/2013 21:28, Peter Lieven ha scritto:
>
> thanks for the details. I think to have optimal performance and best
> change for unmapping in qemu-img convert
> it might be best to export the OPTIMAL UNMAP GRANULARITY

 Agreed about this.

> as well as the write_zeroes_w_discard capability via the BDI

 But why this?!?  It is _not_ needed.  All you need is to change the
 default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is
 nonzero.
>>>
>>> 2 reasons:
>>> a) Does this guarantee that the requests are aligned to multiple of the
>>> -S
>>> value?
>>>
>>> b) If I this flag exists qemu-img convert can do the "zeroing" a priori.
>>> This
>>> has the benefit that combined with bdrv_get_block_status requests before
>>> it
>>> might
>>> not need to touch large areas of the volume. Speaking for iSCSI its
>>> likely
>>> that
>>> the user sets a fresh volume as the destination, but its not guaranteed.
>>> With the Patch 4 there are only done a few get_block_status requests to
>>> verify
>>> this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or
>>> thousands of writesame requests for possibly already unmapped data.
>>>
>>> To give an example. If I take my storage with an 1TB volume. It takes
>>> about
>>> 10-12
>>> get_block_status requests to verify that it is completely unmapped. After
>>> this
>>> I am safe to set has_zero_init = 1 in qemu-img convert.
>>>
>>> If I would convert a 1TB image to this target where lets say 50% are at
>>> leat
>>> 15MB
>>> zero blocks (15MB is the OUG of my storage) it would take ~35000 write
>>> same
>>> requests to achieve the same.
>>
>> I am not sure I am reading this right, but you dont have to writesame
>> exactly 1xOUG to get it to unmap.
>> nxOUG will work too,
>> So instead of sending one writesame for each OUG range, you can send
>> one writesame for every ~10G or so.
>> Say 10G is ~667 OUGs for your case, so you can send
>> writesame for ~667xOUG in each command and then it would "only" take
>> ~100 writesames instead of ~35000.
>>
>> So as long as you are sending in multiples of OUG you should be fine.
>
> do I not have to take care of max_ws_size?

Yes you need to handle mas_ws_size   but I would imagine that on most
targets that max_ws_size >> OUG
I would be surprised if a target would set max_ws_size to just a single OUG.


>
> Peter

Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[ronnie sahlberg]

On Thu, Jul 18, 2013 at 11:43 AM, Peter Lieven  wrote:
>
> Am 18.07.2013 um 16:35 schrieb Paolo Bonzini :
>
>> Il 18/07/2013 16:32, Peter Lieven ha scritto:
>
 (Mis)alignment and granularity can be handled later.  We can ignore them
 for now.  Later, if we decide the best way to support them is a flag,
 we'll add it.  Let's not put the cart before the horse.

 BTW, I expect alignment!=0 to be really, really rare.
>>> To explain my concerns:
>>>
>>> I know that my target has internal page size of 15MB. I will check what
>>> happens
>>> if I deallocate this 15MB in chunks of lets say 1MB. If the page gets
>>> unprovisioned
>>> after the last chunk is unmapped it would be fine :-)
>>
>> You're talking of granularity here, not (mis)alignment.
>
> you are right. for the target i am talking about this is 30720 512-byte 
> blocks for the granularity (pagesize) and 0 for the alignment.
> i will see what happens if I write same w/unmap the whole 30720 blocks in 
> smaller blocks ;-)

If you write in smaller than OUG chunks, whether or not the phusical
block becomes unmapped is I think implementation defined. A target is
allowed to make this unmapped but not required to.

For example if you do two writesame16 for your 30720 chunk :
1, writesame16 lba:0 tl:3
2, writesame16 lba:3 tl:720

then

SBC 4.7.3.4
===
A WRITE SAME command shall not cause an LBA to become unmapped if
unmapping that LBA creates a
case in which a subsequent read of that unmapped LBA mayis able to
return user data or protection
informationlogical block data that differs from the Data-Out Buffer
for that WRITE SAME command. The
protection information returned by a read of an unmapped LBA is set to
___h
(see 4.7.4.5).
===

during processing of the second writesame of the physical block, since
the physical block is now all zero once this writesame16 completes,  a
target may trigger the whole physical block to become unmapped,
eventhough this specific writesame16 was only for a fraction of the
block.
It is allowed to unmap the block, but not required to.


or it could do it at a later time. For example, a target is allowed to
have say a background scan-and-unmap process that goes through the
disk and unmaps all all-zero physical blocks :
I dont know if your target would do this. Would be neat to add this to STGT.

SBC 4.7.3.5 :
===
If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2)
is set to one, and a mapped LBA
references a logical block that contains:
a) user data with all bits set to zero; and
b) protection information, if any, set to ___h,
then the device server may transition that mapped LBA to anchored or
deallocated at any time.
===

I.e. a target is allowed at any time to automatically unmap physical
blocks as long as the block is all zero and lbprz is set.


> otherwise i will have to add support for honoring this values in qemu-img 
> convert
> as a follow up.
>
> Peter
>

Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[Peter Lieven]

On 19.07.2013 15:25, ronnie sahlberg wrote:

On Thu, Jul 18, 2013 at 11:08 PM, Peter Lieven  wrote:

On 19.07.2013 07:58, Paolo Bonzini wrote:

Il 18/07/2013 21:28, Peter Lieven ha scritto:

thanks for the details. I think to have optimal performance and best
change for unmapping in qemu-img convert
it might be best to export the OPTIMAL UNMAP GRANULARITY

Agreed about this.


as well as the write_zeroes_w_discard capability via the BDI

But why this?!?  It is _not_ needed.  All you need is to change the
default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is
nonzero.

2 reasons:
a) Does this guarantee that the requests are aligned to multiple of the -S
value?

b) If I this flag exists qemu-img convert can do the "zeroing" a priori.
This
has the benefit that combined with bdrv_get_block_status requests before it
might
not need to touch large areas of the volume. Speaking for iSCSI its likely
that
the user sets a fresh volume as the destination, but its not guaranteed.
With the Patch 4 there are only done a few get_block_status requests to
verify
this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or
thousands of writesame requests for possibly already unmapped data.

To give an example. If I take my storage with an 1TB volume. It takes about
10-12
get_block_status requests to verify that it is completely unmapped. After
this
I am safe to set has_zero_init = 1 in qemu-img convert.

If I would convert a 1TB image to this target where lets say 50% are at leat
15MB
zero blocks (15MB is the OUG of my storage) it would take ~35000 write same
requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame
exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send
one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send
writesame for ~667xOUG in each command and then it would "only" take
~100 writesames instead of ~35000.

So as long as you are sending in multiples of OUG you should be fine.

do I not have to take care of max_ws_size?

Peter

Re: [Qemu-devel] [PATCH v2 09/17] block: introduce bdrv_get_block_status API

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> For now, bdrv_get_block_status is just another name for bdrv_is_allocated.
> The next patches will add more flags.
> 
> This also touches all block drivers with a mostly mechanical rename.  The
> sole exception is cow; because it calls cow_co_is_allocated from the read
> code, we keep that function and make cow_co_get_block_status a wrapper.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: rebase after vmdk changes

Reviewed-by: Eric Blake 

> 
> @@ -370,7 +376,7 @@ static BlockDriver bdrv_cow = {
>  
>  .bdrv_read  = cow_co_read,
>  .bdrv_write = cow_co_write,
> -.bdrv_co_is_allocated   = cow_co_is_allocated,
> +.bdrv_co_get_block_status   = cow_co_get_block_status,

Is it worth realigning indentation now that you have a longer name?

> +++ b/block/qcow.c
> @@ -395,7 +395,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  return cluster_offset;
>  }
>  
> -static int coroutine_fn qcow_co_is_allocated(BlockDriverState *bs,
> +static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>  int64_t sector_num, int nb_sectors, int *pnum)

Is it worth fixing alignment while you touch this?

>  {
>  BDRVQcowState *s = bs->opaque;
> @@ -896,7 +896,7 @@ static BlockDriver bdrv_qcow = {
>  
>  .bdrv_co_readv  = qcow_co_readv,
>  .bdrv_co_writev = qcow_co_writev,
> -.bdrv_co_is_allocated   = qcow_co_is_allocated,
> +.bdrv_co_get_block_status   = qcow_co_get_block_status,

Another spot for realignment?

> +++ b/block/qcow2.c
> @@ -640,7 +640,7 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
>  return 0;
>  }
>  
> -static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
> +static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
>  int64_t sector_num, int nb_sectors, int *pnum)

alignment?

>  {
>  BDRVQcowState *s = bs->opaque;
> @@ -1784,7 +1784,7 @@ static BlockDriver bdrv_qcow2 = {
>  .bdrv_reopen_prepare  = qcow2_reopen_prepare,
>  .bdrv_create= qcow2_create,
>  .bdrv_has_zero_init = bdrv_has_zero_init_1,
> -.bdrv_co_is_allocated = qcow2_co_is_allocated,
> +.bdrv_co_get_block_status = qcow2_co_get_block_status,
>  .bdrv_set_key   = qcow2_set_key,
>  .bdrv_make_empty= qcow2_make_empty,
>  

wow, this is already an alignment mess before your change

> +++ b/block/qed.c
> @@ -667,7 +667,7 @@ static void qed_is_allocated_cb(void *opaque, int ret, 
> uint64_t offset, size_t l
>  }
>  }
>  
> -static int coroutine_fn bdrv_qed_co_is_allocated(BlockDriverState *bs,
> +static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState 
> *bs,
>   int64_t sector_num,
>   int nb_sectors, int *pnum)

alignment

> +++ b/block/raw-posix.c
> @@ -1084,7 +1084,7 @@ static int raw_create(const char *filename, 
> QEMUOptionParameter *options)
>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>   * beyond the end of the disk image it will be clamped.
>   */
> -static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
> +static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>  int64_t sector_num,
>  int nb_sectors, int *pnum)

and again

>  {
> @@ -1200,7 +1200,7 @@ static BlockDriver bdrv_file = {
>  .bdrv_close = raw_close,
>  .bdrv_create = raw_create,
>  .bdrv_has_zero_init = bdrv_has_zero_init_1,
> -.bdrv_co_is_allocated = raw_co_is_allocated,
> +.bdrv_co_get_block_status = raw_co_get_block_status,
>  
>  .bdrv_aio_readv = raw_aio_readv,
>  .bdrv_aio_writev = raw_aio_writev,

here, nothing was aligned, so you actually met status quo :)

But that raises a question of consistency between drivers...

> +++ b/block/raw.c
> @@ -35,11 +35,11 @@ static void raw_close(BlockDriverState *bs)
>  {
>  }
>  
> -static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
> +static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>  int64_t sector_num,
>  int nb_sectors, int *pnum)

alignment.  You get the picture; I'll quit pointing it out, since it
doesn't affect semantics.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Alexey Kardashevskiy]

On 07/19/2013 11:05 PM, Alexey Kardashevskiy wrote:
> On 07/19/2013 11:03 PM, Paolo Bonzini wrote:
>> Il 19/07/2013 14:58, Alexey Kardashevskiy ha scritto:
>>> On 07/19/2013 10:50 PM, Paolo Bonzini wrote:
 Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
> Nikunj A Dadhania  writes:
>> commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
>> Author: Paolo Bonzini 
>> Date:   Fri May 24 12:54:01 2013 +0200
>>
>> memory: add big endian support to access_with_adjusted_size
>> 
>> This will be used to split 8-byte access down to two four-byte 
>> accesses.
>> 
>> Reviewed-by: Richard Henderson 
>> Signed-off-by: Paolo Bonzini 
>>
>>
>> If I hack the above funniness in my USB EHCI driver, somewhere down the
>> qemu crashes at code introduced by this patch:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>> (gdb) bt
>> #0 0x in ?? ()
>> #1 0x557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, 
>> value=value@entry=0x7fffd5a86680, size=size@entry=1, 
>> access_size_min=, access_size_max=,
>> access=0x557a1f80 , 
>> opaque=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
>> #2 0x557a5ebb in memory_region_dispatch_write (size=1, data=0, 
>> addr=12, mr=0x567f8ab8) at 
>> /home/nikunj/work/power/code/qemu/memory.c:998
>>
>> Reverting this, I can safely boot using a usb-storage device put on ehci 
>> controller.
>
> Just reverting this patch does not help though, i will need to figure
> which all commits are bad.

 Hi Nikunj,

 can you try the attached patch?

 Alexey, with some luck it may even fix virtio-blk too.
>>>
>>>
>>> Heh. Bad luck. The behaviour has changed slightly but it still does not 
>>> work.
>>
>> How changed?
> 
> 
> See below. I am trying to debug :)


Fails here. io_mem_unassigned. Are you on any IRC?


(gdb) bt
#0  memory_region_access_valid (mr=0x10aee190 ,
addr=0xd0fb802, size=0x2,
is_write=0x0) at /home/alexey/pcipassthru/qemu-impreza/memory.c:931
#1  0x103838c0 in memory_region_dispatch_read (mr=0x10aee190
,
addr=0xd0fb802, pval=0x3fffdd30, size=0x2) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:962
#2  0x10387038 in io_mem_read (mr=0x10aee190 ,
addr=0xd0fb802,
pval=0x3fffdd30, size=0x2) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:1740
#3  0x102ebde0 in lduw_phys_internal (addr=0xd0fb802,
endian=DEVICE_NATIVE_ENDIAN)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2390
#4  0x102ebed8 in lduw_phys (addr=0xd0fb802)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2422
#5  0x1037387c in vring_avail_idx (vq=0x10c16e30)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:138
#6  0x1037429c in virtqueue_num_heads (vq=0x10c16e30, idx=0x0)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:285
#7  0x10374a74 in virtqueue_pop (vq=0x10c16e30, elem=0x10c34c08)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:441
#8  0x1030c1bc in virtio_blk_get_request (s=0x10c1c2f8)
at /home/alexey/pcipassthru/qemu-impreza/hw/block/virtio-blk.c:118
#9  0x1030cfb8 in virtio_blk_handle_output (vdev=0x10c1c2f8,
vq=0x10c16e30)
at /home/alexey/pcipassthru/qemu-impreza/hw/block/virtio-blk.c:411
#10 0x10375c48 in virtio_queue_notify_vq (vq=0x10c16e30)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:687
#11 0x1037776c in virtio_queue_host_notifier_read (n=0x10c16e80)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:1071
#12 0x1020fe74 in qemu_iohandler_poll (pollfds=0x10bb1a00, ret=0x2)
at /home/alexey/pcipassthru/qemu-impreza/iohandler.c:143
#13 0x10210c4c in main_loop_wait (nonblocking=0x0)
at /home/alexey/pcipassthru/qemu-impreza/main-loop.c:466
#14 0x102c97d4 in main_loop () at
/home/alexey/pcipassthru/qemu-impreza/vl.c:2090
#15 0x102d2c80 in main (argc=0x16, argv=0x31b8,
envp=0x3270)
at /home/alexey/pcipassthru/qemu-impreza/vl.c:4432



> 
> SLOF **
> QEMU Starting
>  Build Date = Apr 30 2013 14:04:00
>  FW Version = git-8cfdfc43f4c4c8c8
>  Press "s" to enter Open Firmware.
> 
> Populating /vdevice methods
> Populating /vdevice/nvram@7100
> 
> NVRAM: size=65536, fetch=200E, store=200F
> Populating /vdevice/vty@7101
> Populating /pci@8002000
>  Adapters on 08002000
>  00  (D) : 1af4 1001virtio [ block ]
> No NVRAM common partition, re-initializing...
> claim failed!
> Using default console: /vdevice/vty@7101
> 
>   Welcome to Open Firmware
> 
>   Copyright (c) 2004, 2011 IBM Corporation All rights reserved.
>   This program and the ac

Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[ronnie sahlberg]

On Thu, Jul 18, 2013 at 11:08 PM, Peter Lieven  wrote:
> On 19.07.2013 07:58, Paolo Bonzini wrote:
>>
>> Il 18/07/2013 21:28, Peter Lieven ha scritto:
>>>
>>> thanks for the details. I think to have optimal performance and best
>>> change for unmapping in qemu-img convert
>>> it might be best to export the OPTIMAL UNMAP GRANULARITY
>>
>> Agreed about this.
>>
>>> as well as the write_zeroes_w_discard capability via the BDI
>>
>> But why this?!?  It is _not_ needed.  All you need is to change the
>> default of the "-S" option to be the OPTIMAL UNMAP GRANULARITY if it is
>> nonzero.
>
> 2 reasons:
> a) Does this guarantee that the requests are aligned to multiple of the -S
> value?
>
> b) If I this flag exists qemu-img convert can do the "zeroing" a priori.
> This
> has the benefit that combined with bdrv_get_block_status requests before it
> might
> not need to touch large areas of the volume. Speaking for iSCSI its likely
> that
> the user sets a fresh volume as the destination, but its not guaranteed.
> With the Patch 4 there are only done a few get_block_status requests to
> verify
> this. If we just write zeroes with BDRV_MAY_UNMAP, we send hundreds or
> thousands of writesame requests for possibly already unmapped data.
>
> To give an example. If I take my storage with an 1TB volume. It takes about
> 10-12
> get_block_status requests to verify that it is completely unmapped. After
> this
> I am safe to set has_zero_init = 1 in qemu-img convert.
>
> If I would convert a 1TB image to this target where lets say 50% are at leat
> 15MB
> zero blocks (15MB is the OUG of my storage) it would take ~35000 write same
> requests to achieve the same.

I am not sure I am reading this right, but you dont have to writesame
exactly 1xOUG to get it to unmap.
nxOUG will work too,
So instead of sending one writesame for each OUG range, you can send
one writesame for every ~10G or so.
Say 10G is ~667 OUGs for your case, so you can send
writesame for ~667xOUG in each command and then it would "only" take
~100 writesames instead of ~35000.

So as long as you are sending in multiples of OUG you should be fine.


>
> Peter
>
>>
>> Paolo
>>
>>> and than zero
>>> out the whole device with bdrv_write_zeroes and the BDRV_MAY_DISCARD
>>> flag.
>>>
>>> the logic for this is already prepared in patch4 (topic of this email).
>>> except that
>>> i have to exchange bdrv_discard with bdrv_write_zeroes w/
>>> BDRV_MAY_DISCARD.
>>>
>>> what do you think?
>>>
>>>
>>>

 On Thu, Jul 18, 2013 at 11:43 AM, Peter Lieven  wrote:
>
> Am 18.07.2013 um 16:35 schrieb Paolo Bonzini :
>
>> Il 18/07/2013 16:32, Peter Lieven ha scritto:

 (Mis)alignment and granularity can be handled later.  We can ignore
 them
 for now.  Later, if we decide the best way to support them is a
 flag,
 we'll add it.  Let's not put the cart before the horse.

 BTW, I expect alignment!=0 to be really, really rare.
>>>
>>> To explain my concerns:
>>>
>>> I know that my target has internal page size of 15MB. I will check
>>> what
>>> happens
>>> if I deallocate this 15MB in chunks of lets say 1MB. If the page gets
>>> unprovisioned
>>> after the last chunk is unmapped it would be fine :-)
>>
>> You're talking of granularity here, not (mis)alignment.
>
> you are right. for the target i am talking about this is 30720 512-byte
> blocks for the granularity (pagesize) and 0 for the alignment.
> i will see what happens if I write same w/unmap the whole 30720 blocks
> in smaller blocks ;-) otherwise i will have to add support for honoring 
> this
> values in qemu-img convert
> as a follow up.
>
> Peter
>
>>>
>>>
>
>
> --
>
> Mit freundlichen Grüßen
>
> Peter Lieven
>
> ...
>
>   KAMP Netzwerkdienste GmbH
>   Vestische Str. 89-91 | 46117 Oberhausen
>   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
>   p...@kamp.de | http://www.kamp.de
>
>   Geschäftsführer: Heiner Lante | Michael Lante
>   Amtsgericht Duisburg | HRB Nr. 12154
>   USt-Id-Nr.: DE 120607556
>
> ...
>

Re: [Qemu-devel] [PATCH v2 08/17] block: make bdrv_has_zero_init return false for copy-on-write-images

[Paolo Bonzini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Il 19/07/2013 15:21, Eric Blake ha scritto:
> Question (more for my understanding, not that you need to change
> code): must we blindly return 0 in the presence of a backing file,
> or is it possible to recursively query the backing_hd's zero_init
> status, allowing us to return 1 iff all files in the chain support
> zero_init.

No, it's really blind.  bdrv_has_zero_init is like "was the image all
zeros at creation time", and the answer is no because the content of
the image used to be the same as the backing_hd.

Paolo
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR6T3bAAoJEBvWZb6bTYbyraEP/04nt2HxFsX+na7q/Ax7bGYn
Z0RvyP2zTukfv8dM9aJVjmQEvLRnp7ROQlSOXPbxF3gzr2nh67nb27Xi+vF0B3N0
fDQTnnUX/8Zz2jigolDCSSFp0ggDWRtVID+W/olfIQMAJI1PkRdt0VJQp9aeLGZV
7tae/Fl5AQtfZfAW6Ji4FZzJcmLDyM7HoveZxnAawGy9e17opKGFmKUmBNutew0C
LoKDkakWRrW/k4IzXB0TJOtQ5hOn5yoG16VjYG1YKblewRqaciFba62T+Z4ClJB0
TKJEr3fuGokI9mPoyPCpuZF0ZPEFFI26Mwocag8TpCS5IYJzJjC7RcGDW5CV4zzC
Lzr3fGeTTtEOCJfdF4GtqeSp2S+1QWuagLLr31cV1KpnuKVLItsQ95gDpZMtqjC0
FKLkF7g/BQrE/UgGzJ16YuF4h+Ct8+2Ihiz0BS6OD6ie2g9Ds/u1l/TCn/jfj+ID
PJhDUcDPynBqy+i1V475w+ZY7lLpE7oOg1HPJvQLqFgc6XkPTI75EttpWB01NOoO
vKfugwxKJBfLFCX/oWbTk04P69dysLJLeb/WedTrqWAdn/to0bhD1aKX2rV+aYx3
Rs+7DescTrGV+n6qcdx2TUEqswCyMrBoStBUwEGuM+S+EVFQ7KitAY83puNUO3zX
qAEpAOUWyqE0WXG3s7Ee
=ltDY
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 08/17] block: make bdrv_has_zero_init return false for copy-on-write-images

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> This helps implementing is_allocated on top of get_block_status.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c| 5 +
>  qemu-img.c | 9 +
>  2 files changed, 6 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake 

> +++ b/block.c
> @@ -2934,6 +2934,11 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>  {
>  assert(bs->drv);
>  
> +/* If BS is a copy on write image, it is initialized to
> +   the contents of the base image, which may not be zeroes.  */
> +if (bs->backing_hd) {
> +return 0;

Question (more for my understanding, not that you need to change code):
must we blindly return 0 in the presence of a backing file, or is it
possible to recursively query the backing_hd's zero_init status,
allowing us to return 1 iff all files in the chain support zero_init.

-- 
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 10/17] block: define get_block_status return value

[Peter Lieven]

On 19.07.2013 14:13, Paolo Bonzini wrote:

Il 19/07/2013 12:04, Peter Lieven ha scritto:

On 19.07.2013 11:58, Paolo Bonzini wrote:

Il 19/07/2013 08:48, Peter Lieven ha scritto:

-return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors,
pnum);
+return
+(ret & BDRV_BLOCK_DATA) ||
+((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));

i do also not understand the "((ret & BDRV_BLOCK_ZERO) &&
!bdrv_has_zero_init(bs))";
if a block is unallocated and reads as zero, but the device lacks zero
init, it is declared as allocated with this, isn't it?

Thinking more about it, I would say it is a bugfix.

If a block is unallocated and reads as zero, but the device lacks zero
init, the block contents have changed since the guest was created.  Thus
the guest might well be relying on the zero content of the block, and it
should be treated as allocated.

i was told that has_zero_init sole task is to report the state
of the device right after iscsi_create(). using it for r/w in qemu
might be a misuse.

Yes, and here I'm using it exactly for that task.  I'm saying "treat a
zero block as allocated if it wasn't zero right after _create()".

If it is zero and allocated the API should return only BDRV_BLOCK_DATA
and if it is zero and unallocated only BDRV_BLOCK_ZERO or not?

What I mean is the new API shouldn't change the behaviour of the old 
bdrv_is_allocated().
It would have returned

(ret & BDRV_BLOCK_DATA) regardless if BDRV_BLOCK_ZERO or not.


Peter

Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Alexey Kardashevskiy]

On 07/19/2013 11:03 PM, Paolo Bonzini wrote:
> Il 19/07/2013 14:58, Alexey Kardashevskiy ha scritto:
>> On 07/19/2013 10:50 PM, Paolo Bonzini wrote:
>>> Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
 Nikunj A Dadhania  writes:
> commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
> Author: Paolo Bonzini 
> Date:   Fri May 24 12:54:01 2013 +0200
>
> memory: add big endian support to access_with_adjusted_size
> 
> This will be used to split 8-byte access down to two four-byte 
> accesses.
> 
> Reviewed-by: Richard Henderson 
> Signed-off-by: Paolo Bonzini 
>
>
> If I hack the above funniness in my USB EHCI driver, somewhere down the
> qemu crashes at code introduced by this patch:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x in ?? ()
> (gdb) bt
> #0 0x in ?? ()
> #1 0x557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, 
> value=value@entry=0x7fffd5a86680, size=size@entry=1, 
> access_size_min=, access_size_max=,
> access=0x557a1f80 , 
> opaque=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
> #2 0x557a5ebb in memory_region_dispatch_write (size=1, data=0, 
> addr=12, mr=0x567f8ab8) at 
> /home/nikunj/work/power/code/qemu/memory.c:998
>
> Reverting this, I can safely boot using a usb-storage device put on ehci 
> controller.

 Just reverting this patch does not help though, i will need to figure
 which all commits are bad.
>>>
>>> Hi Nikunj,
>>>
>>> can you try the attached patch?
>>>
>>> Alexey, with some luck it may even fix virtio-blk too.
>>
>>
>> Heh. Bad luck. The behaviour has changed slightly but it still does not work.
> 
> How changed?


See below. I am trying to debug :)


SLOF **
QEMU Starting
 Build Date = Apr 30 2013 14:04:00
 FW Version = git-8cfdfc43f4c4c8c8
 Press "s" to enter Open Firmware.

Populating /vdevice methods
Populating /vdevice/nvram@7100

NVRAM: size=65536, fetch=200E, store=200F
Populating /vdevice/vty@7101
Populating /pci@8002000
 Adapters on 08002000
 00  (D) : 1af4 1001virtio [ block ]
No NVRAM common partition, re-initializing...
claim failed!
Using default console: /vdevice/vty@7101

  Welcome to Open Firmware

  Copyright (c) 2004, 2011 IBM Corporation All rights reserved.
  This program and the accompanying materials are made available
  under the terms of the BSD License available at
  http://www.opensource.org/licenses/bsd-license.php


Trying to load:  from: disk ... qemu-system-ppc64: Guest moved used index
from 0 to 65535



-- 
Alexey

Re: [Qemu-devel] [PATCH v2 07/17] qemu-img: always probe the input image for allocated sectors

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> qemu-img convert is assuming "that sectors which are unallocated in the input
> image are present in both the output's and input's base images", but it is
> only doing this if the output image is zero initialized.  And checking if
> the output image is zero initialized does not make much sense if the
> output image is copy-on-write.  Always do the test.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  qemu-img.c | 26 --
>  1 file changed, 12 insertions(+), 14 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Paolo Bonzini]

Il 19/07/2013 14:58, Alexey Kardashevskiy ha scritto:
> On 07/19/2013 10:50 PM, Paolo Bonzini wrote:
>> Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
>>> Nikunj A Dadhania  writes:
 commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
 Author: Paolo Bonzini 
 Date:   Fri May 24 12:54:01 2013 +0200

 memory: add big endian support to access_with_adjusted_size
 
 This will be used to split 8-byte access down to two four-byte 
 accesses.
 
 Reviewed-by: Richard Henderson 
 Signed-off-by: Paolo Bonzini 


 If I hack the above funniness in my USB EHCI driver, somewhere down the
 qemu crashes at code introduced by this patch:

 Program received signal SIGSEGV, Segmentation fault.
 0x in ?? ()
 (gdb) bt
 #0 0x in ?? ()
 #1 0x557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, 
 value=value@entry=0x7fffd5a86680, size=size@entry=1, 
 access_size_min=, access_size_max=,
 access=0x557a1f80 , 
 opaque=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
 #2 0x557a5ebb in memory_region_dispatch_write (size=1, data=0, 
 addr=12, mr=0x567f8ab8) at 
 /home/nikunj/work/power/code/qemu/memory.c:998

 Reverting this, I can safely boot using a usb-storage device put on ehci 
 controller.
>>>
>>> Just reverting this patch does not help though, i will need to figure
>>> which all commits are bad.
>>
>> Hi Nikunj,
>>
>> can you try the attached patch?
>>
>> Alexey, with some luck it may even fix virtio-blk too.
> 
> 
> Heh. Bad luck. The behaviour has changed slightly but it still does not work.

How changed?

Paolo

Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Alexey Kardashevskiy]

On 07/19/2013 10:50 PM, Paolo Bonzini wrote:
> Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
>> Nikunj A Dadhania  writes:
>>> commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
>>> Author: Paolo Bonzini 
>>> Date:   Fri May 24 12:54:01 2013 +0200
>>>
>>> memory: add big endian support to access_with_adjusted_size
>>> 
>>> This will be used to split 8-byte access down to two four-byte accesses.
>>> 
>>> Reviewed-by: Richard Henderson 
>>> Signed-off-by: Paolo Bonzini 
>>>
>>>
>>> If I hack the above funniness in my USB EHCI driver, somewhere down the
>>> qemu crashes at code introduced by this patch:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x in ?? ()
>>> (gdb) bt
>>> #0 0x in ?? ()
>>> #1 0x557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, 
>>> value=value@entry=0x7fffd5a86680, size=size@entry=1, 
>>> access_size_min=, access_size_max=,
>>> access=0x557a1f80 , 
>>> opaque=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
>>> #2 0x557a5ebb in memory_region_dispatch_write (size=1, data=0, 
>>> addr=12, mr=0x567f8ab8) at 
>>> /home/nikunj/work/power/code/qemu/memory.c:998
>>>
>>> Reverting this, I can safely boot using a usb-storage device put on ehci 
>>> controller.
>>
>> Just reverting this patch does not help though, i will need to figure
>> which all commits are bad.
> 
> Hi Nikunj,
> 
> can you try the attached patch?
> 
> Alexey, with some luck it may even fix virtio-blk too.


Heh. Bad luck. The behaviour has changed slightly but it still does not work.




-- 
Alexey

Re: [Qemu-devel] [PATCH v2 06/17] block: expect errors from bdrv_co_is_allocated

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Some bdrv_is_allocated callers do not expect errors, but the fallback
> in qcow2.c might make other callers trip on assertion failures or
> infinite loops.
> 
> Fix the callers to always look for errors.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: modify error message, add strerror(-ret)

> +++ b/qemu-img.c
> @@ -2073,6 +2073,11 @@ static int img_rebase(int argc, char **argv)
>  
>  /* If the cluster is allocated, we don't need to take action */
>  ret = bdrv_is_allocated(bs, sector, n, &n);
> +if (ret < 0) {
> +error_report("error while reading image metadata: %s",
> + strerror(-ret));

Hmm, we have error_setg_errno so that callers don't have to use
strerror(); is it time to introduce error_report_errno for the same
convenience factor?  But that's a side question that does not impact
this patch.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Paolo Bonzini]

Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
> Nikunj A Dadhania  writes:
>> commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
>> Author: Paolo Bonzini 
>> Date:   Fri May 24 12:54:01 2013 +0200
>>
>> memory: add big endian support to access_with_adjusted_size
>> 
>> This will be used to split 8-byte access down to two four-byte accesses.
>> 
>> Reviewed-by: Richard Henderson 
>> Signed-off-by: Paolo Bonzini 
>>
>>
>> If I hack the above funniness in my USB EHCI driver, somewhere down the
>> qemu crashes at code introduced by this patch:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>> (gdb) bt
>> #0 0x in ?? ()
>> #1 0x557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, 
>> value=value@entry=0x7fffd5a86680, size=size@entry=1, 
>> access_size_min=, access_size_max=,
>> access=0x557a1f80 , 
>> opaque=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
>> #2 0x557a5ebb in memory_region_dispatch_write (size=1, data=0, 
>> addr=12, mr=0x567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:998
>>
>> Reverting this, I can safely boot using a usb-storage device put on ehci 
>> controller.
> 
> Just reverting this patch does not help though, i will need to figure
> which all commits are bad.

Hi Nikunj,

can you try the attached patch?

Alexey, with some luck it may even fix virtio-blk too.

Paolo
diff --git a/exec.c b/exec.c
index c99a883..c8658c6 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ static void *qemu_safe_ram_ptr(ram_addr_t addr)
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
  * but takes a size argument */
-static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
+static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
 {
 if (*size == 0) {
 return NULL;
@@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-unsigned access_size_min = mr->ops->impl.min_access_size;
-unsigned access_size_max = mr->ops->impl.max_access_size;
+unsigned access_size_max = mr->ops->valid.max_access_size;
 
 /* Regions are assumed to support 1-4 byte accesses unless
otherwise specified.  */
-if (access_size_min == 0) {
-access_size_min = 1;
-}
 if (access_size_max == 0) {
 access_size_max = 4;
 }
@@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 if (l > access_size_max) {
 l = access_size_max;
 }
-/* ??? The users of this function are wrong, not supporting minimums larger
-   than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
-assert(l >= access_size_min);
 
 return l;
 }
diff --git a/memory.c b/memory.c
index c8f9a2b..a8db78c 100644
--- a/memory.c
+++ b/memory.c
@@ -339,65 +339,104 @@ static void flatview_simplify(FlatView *view)
 }
 }
 
-static void memory_region_oldmmio_read_accessor(void *opaque,
+static bool memory_region_big_endian(MemoryRegion *mr)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
+#else
+return mr->ops->endianness == DEVICE_BIG_ENDIAN;
+#endif
+}
+
+static bool memory_region_wrong_endianness(MemoryRegion *mr)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+return mr->ops->endianness == DEVICE_LITTLE_ENDIAN;
+#else
+return mr->ops->endianness == DEVICE_BIG_ENDIAN;
+#endif
+}
+
+static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size)
+{
+if (memory_region_wrong_endianness(mr)) {
+switch (size) {
+case 1:
+break;
+case 2:
+*data = bswap16(*data);
+break;
+case 4:
+*data = bswap32(*data);
+break;
+case 8:
+*data = bswap64(*data);
+break;
+default:
+abort();
+}
+}
+}
+
+static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 hwaddr addr,
 uint64_t *value,
 unsigned size,
 unsigned shift,
 uint64_t mask)
 {
-MemoryRegion *mr = opaque;
 uint64_t tmp;
 
 tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
+adjust_endianness(mr, &tmp, size);
 *value |= (tmp & mask) << shift;
 }
 
-static void memory_region_read_accessor(void *opaque,
+static void memory_region_read_accessor(MemoryRegion *mr,
 hwaddr addr,
 uint64_t *value,
 unsigned size,
 unsigned shift,
   

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:
> Hi!
> 
> This patch also breaks virtio on powerpc. I thought it was fixed
> (reverted?) in the master branch from qemu.org but it is still there. What
> did I miss?

It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.

Let me check if I can reproduce this, it looks like a endianness
problems reading virtio-blk config space.

Paolo

> Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> [many of those]
> 
> 
> 
> On 07/11/2013 10:29 PM, Alexander Graf wrote:
>>
>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>
>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
 Jan Kiszka a écrit :
> From: Jan Kiszka 
>
> The current ioport dispatcher is a complex beast, mostly due to the
> need to deal with old portio interface users. But we can overcome it
> without converting all portio users by embedding the required base
> address of a MemoryRegionPortio access into that data structure. That
> removes the need to have the additional MemoryRegionIORange structure
> in the loop on every access.
>
> To handle old portio memory ops, we simply install dispatching handlers
> for portio memory regions when registering them with the memory core.
> This removes the need for the old_portio field.
>
> We can drop the additional aliasing of ioport regions and also the
> special address space listener. cpu_in and cpu_out now simply call
> address_space_read/write. And we can concentrate portio handling in a
> single source file.
>
> Signed-off-by: Jan Kiszka 
> ---

 ...

> +
> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> +MemoryRegionPortioList *mrpio = opaque;
> +const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
> true);
> +
> +if (mrp) {
> +mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
> +} else if (size == 2) {
> +mrp = find_portio(mrpio, addr, 1, true);
> +assert(mrp);
> +mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
> +mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>> 8);
> +}
> +}
> +
> +static const MemoryRegionOps portio_ops = {
> +.read = portio_read,
> +.write = portio_write,
> +.valid.unaligned = true,
> +.impl.unaligned = true,
> +};
> +

 You need to mark these operations as DEVICE_LITTLE_ENDIAN.
 In portio_write above, you clearly assume that data is in LE format.
>>>
>>> Anything behind PIO is little endian, of course. Will add this.
>>
>> This patch breaks VGA on PPC as it is in master today.
>>
>>
>> Alex
>>
>>>

 This fixes PPC PReP emulation, which would otherwise be broken with this
 patchset.
>>>
>>> Thanks,
>>> Jan
>>>
>>>
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH v2 05/17] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Now that bdrv_is_allocated detects coroutine context, the two can
> use the same code.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c   | 46 --
>  block/commit.c|  6 +++---
>  block/mirror.c|  4 ++--
>  block/stream.c|  4 ++--
>  include/block/block.h |  4 
>  5 files changed, 11 insertions(+), 53 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
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 04/17] block: make bdrv_co_is_allocated static

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> bdrv_is_allocated can detect coroutine context and go through a fast
> path, similar to other block layer functions.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c   | 24 +++-
>  block/raw.c   |  2 +-
>  block/stream.c|  4 ++--
>  include/block/block.h |  2 --
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
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 03/17] cow: do not call bdrv_co_is_allocated

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> As we change bdrv_is_allocated to gather more information from bs and
> bs->file, it will become a bit slower.  It is still appropriate for online
> jobs, but not for reads/writes.  Call the internal function instead.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/cow.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

-- 
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 02/17] cow: make writes go at a less indecent speed

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Only sync once per write, rather than once per sector.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/cow.c | 19 ---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
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 01/17] cow: make reads go at a decent speed

[Eric Blake]

On 07/16/2013 10:29 AM, Paolo Bonzini wrote:
> Do not do two reads for each sector; load each sector of the bitmap
> and use bitmap operations to process it.
> 
> Writes are still dog slow!
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: use BDRV_SECTOR_SIZE, not 512 for bitmap array length
> 
>  block/cow.c | 54 --
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
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 1/2] qapi: change qapi to convert schema json

[Eric Blake]

On 07/16/2013 04:37 AM, Amos Kong wrote:
> QMP schema is defined in a json file, it will be parsed by
> qapi scripts and generate C files.
> 
> We want to return the schema information to management,
> this patch converts the json file to a string table in a
> C head file, then we can use the json content.
> 
> eg:
>   const char *const qmp_schema_table[] = {
> "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
> "{ 'command': 'query-name', 'returns': 'NameInfo' }",
> ...
>   }
> 
> Signed-off-by: Amos Kong 
> ---
>  Makefile |  5 -
>  scripts/qapi-commands.py |  2 +-
>  scripts/qapi-types.py| 47 ---
>  scripts/qapi-visit.py|  2 +-
>  scripts/qapi.py  |  4 +++-
>  5 files changed, 53 insertions(+), 7 deletions(-)

My python is weak, but I'll attempt a review anyway (how else do you
learn a new language? :)

> @@ -223,6 +223,9 @@ $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
>  qmp-commands.h qmp-marshal.c :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> +qmp-schema.h:\
> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> $(gen-out-type) -o "." -i "$@" < $<, "  GEN   $@")

Copy-and-paste, but any reason there is so much space around GEN?

> -exprs = parse_schema(sys.stdin)
> +exprs_all = parse_schema(sys.stdin)
> +
> +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */

Is it worth stating what file this was generated from?  If I open a
generated file to try and make an edit, I like to have it tell me what
the real source file is.

> +
> +/*
> + * Schema json string table converted from qapi-schema.json

Well, this is close, but as we are asking you to do multiple
qapi-schema.json files (one for qemu's QMP monitor, one for qemu-ga), a
relative path to the file within the overall qemu.git might be nicer.

> + *
> + * Copyright (c) 2013 Red Hat, Inc.

This copyright won't auto-update as years change.  Should it?

Then again, this is probably copy-and-paste from other files the
generator already spits out, so cleanups to one generated header should
probably done to all generated headers, and in a separate cleanup patch,
if at all.

> + *
> + * Authors:
> + *  Amos Kong 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +const char *const qmp_schema_table[] = {
> +"""
> +
> +if introspect_file:
> +for line in exprs_all[1]:
> +line = re.sub(r'\n', ' ', line.strip())
> +line = re.sub(r' +', ' ', line)
> +schema_table += '  "%s",\n' % (line)

Do we ever need to worry about someone using { "command" ...} instead of
the current style of { 'command' ...} in the qapi-schema.json file?  If
they do, then you would be generating invalid C code here by not
escaping the " properly.  Likewise, should you be asserting that there
are no other problematic characters like backslash?  Ideally, we will
never encounter such problems, but being robust might save some
head-scratching if someone introduces a typo.

I can live with what you have:
Reviewed-by: Eric Blake 

-- 
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 00/17] Add qemu-img subcommand to dump file metadata

[Paolo Bonzini]

Il 19/07/2013 12:48, Wenchao Xia ha scritto:
>   I think dump of allocated info in qemu-img in this series, can do
> dirty change tracking job for backing chain snapshot now, qemu's QMP
> interface is not very needed.
>   Only thing not perfect, is that it talks with string.

It uses JSON, actually.

> A library would be better, but I am not selling libqblock:),

Oh you should!  Are you going back to it?  It was very close to
mergeable state.

Paolo