Re: [Qemu-devel] [PATCH 1/2] virtfs-proxy-helper: Fix possible socket leak.

[Gonglei]

On 2014/11/27 20:49, Markus Armbruster wrote:

>  writes:
> 
>> From: Gonglei 
>>
>> Signed-off-by: Gonglei 
> 
> Reviewed-by: Markus Armbruster 

Hi, Michael

I guess Paolo forgot this patch :(
Maybe this one can be picked up by qemu-trivial. Thanks.

Regards,
-Gonglei

Re: [Qemu-devel] Trace calls for xenfb, ps2 and pcnet

[Michael Tokarev]

16.01.2015 22:21, Don Koch wrote:
> Add trace calls for debugging xenfb, ps2 and pcnet.

Applied series to -trivial, thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: Fix bogus error message for implied mon ID clashing

[Michael Tokarev]

13.01.2015 16:19, Markus Armbruster wrote:
> monitor_parse() desugars --monitor, --qmp and -qmp-pretty to --mon.
> The ID it picks can clash with a user-specified ID.  When it happens,
> the error message is misleading.

Thanks, applied to -trivial.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] virtfs-proxy-helper: Fix possible socket leak.

[Michael Tokarev]

21.01.2015 11:00, Gonglei wrote:
[]
> Hi, Michael
> 
> I guess Paolo forgot this patch :(
> Maybe this one can be picked up by qemu-trivial. Thanks.

I picked it up and applied to -trivial.
It is not a problem if the same patch will be applied to
several trees at once, so if Paolo will send a pull request
for it together with me it all will work ok.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 5/6] linux-user/main.c: Mark end_exclusive() as possibly unused

[Thomas Huth]

On Thu,  8 Jan 2015 12:19:47 +
Peter Maydell  wrote:

> The function end_exclusive() isn't used on all targets; mark it as
> such to avoid a clang warning.
> 
> Signed-off-by: Peter Maydell 
> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index bbd1cfd..0fda51c 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -169,7 +169,7 @@ static inline void start_exclusive(void)
>  }
> 
>  /* Finish an exclusive operation.  */
> -static inline void end_exclusive(void)
> +static inline void __attribute__((unused)) end_exclusive(void)
>  {
>  pending_cpus = 0;
>  pthread_cond_broadcast(&exclusive_resume);

IMHO it might be better to add a proper #ifdef guard around that
function. Consider that the calls to end_exclusive() might get removed
completely one day, then you won't get a compiler warning about the
unused function anymore if you used the attribute__((unused)) way.

 Thomas

Re: [Qemu-devel] [PATCH v0 2/2] pc-dimm: Make pc_existing_dimms_capacity global

[Bharata B Rao]

On Tue, Jan 20, 2015 at 11:18:37AM +0100, Igor Mammedov wrote:
> On Mon, 12 Jan 2015 09:32:34 +0530
> Bharata B Rao  wrote:
> 
> > Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
> > by PowerPC memory hotplug code too.
> > 
> > Signed-off-by: Bharata B Rao 
> > ---
> >  hw/i386/pc.c | 24 
> >  hw/mem/pc-dimm.c | 25 +
> >  include/hw/mem/pc-dimm.h |  1 +
> >  3 files changed, 26 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 125cf0a..2ec45a4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
> >  g_free(name);
> >  }
> >  
> > -static int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > -{
> > -Error *local_err = NULL;
> > -uint64_t *size = opaque;
> > -
> > -if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> > -DeviceState *dev = DEVICE(obj);
> > -
> > -if (dev->realized) {
> > -(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> > -&local_err);
> > -}
> > -
> > -if (local_err) {
> > -qerror_report_err(local_err);
> > -error_free(local_err);
> > -return 1;
> > -}
> > -}
> > -
> > -object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > -return 0;
> > -}
> > -
> >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >   DeviceState *dev, Error **errp)
> >  {
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index d431834..f02ce6e 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -22,6 +22,31 @@
> >  #include "qemu/config-file.h"
> >  #include "qapi/visitor.h"
> >  #include "qemu/range.h"
> > +#include "qapi/qmp/qerror.h"
> > +
> > +int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > +{
> since you are making it API, could you pass Error **errp argument
> and deal with error in caller?
> Along with it you can make function return void.

I don't think that can be done because pc_existing_dimms_capacity() calls
itself recursively via object_child_foreach() and hence its signature
if fixed.

> 
> > +Error *local_err = NULL;
> > +uint64_t *size = opaque;
> > +
> > +if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> > +DeviceState *dev = DEVICE(obj);
> > +
> > +if (dev->realized) {
> > +(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> > +&local_err);
> > +}
> > +
> > +if (local_err) {
> > +qerror_report_err(local_err);
> > +error_free(local_err);
> > +return 1;
> > +}
> > +}
> > +
> > +object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > +return 0;
> > +}

[Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups

[Igor Mammedov]

NOTE to maintainer: please update test data (ACPI blobs) in test cases

changes from v4:
 * rebased on top of PCI tree, dropping 2 patches
   that are already there

changes from v3:
 * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
 * copy GLP license block from acpi-build.c
 * assert on wrong Segcount earlier and extend condition to
   seg_count > 0 && seg_count <= 255
 * drop "pc: acpi: decribe bridge device as not hotpluggable"
 * keep original logic of creating bridge devices as it was done
   in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
 * if bus is non hotpluggable, add child slots to bus as non
   hotpluggable as it was done in original code.

changes from v2:
 * codding style fixups
 * check for SegCount earlier
 * use hotpluggable device object instead of not hotpluggable
for non present devices, and add it only when bus itself is
hotpluggable

changes from v1:
 * drop: [PATCH 7/9] acpi: replace opencoded notify codes with named values
 * use Michael's suggestion to improve build_append_nameseg()
 * drop long scope names and go back to recursion,
   but still significantly simplify building of PCI tree

this series is an attempt to shave off a bunch of
not directly related patches from already big dynamic
AML series (although it's dependency for it)

Tested: on XPsp3 to WS2012R2 and REHL6/7 guests.

Git tree for testing:
 https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification_v4

Igor Mammedov (5):
  pc: acpi-build: cleanup AcpiPmInfo initialization
  acpi: move generic aml building helpers into dedictated file
  acpi: add build_append_namestring() helper
  acpi: drop min-bytes in build_package()
  pc: acpi-build: simplify PCI bus tree generation

 hw/acpi/Makefile.objs  |   1 +
 hw/acpi/acpi-build-utils.c | 269 +
 hw/i386/acpi-build.c   | 469 -
 include/hw/acpi/acpi-build-utils.h |  23 ++
 4 files changed, 397 insertions(+), 365 deletions(-)
 create mode 100644 hw/acpi/acpi-build-utils.c
 create mode 100644 include/hw/acpi/acpi-build-utils.h

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package()

[Igor Mammedov]

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
 hw/acpi/acpi-build-utils.c | 14 --
 hw/i386/acpi-build.c   | 13 ++---
 include/hw/acpi/acpi-build-utils.h |  4 ++--
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index aed9066..602e68c 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -167,7 +167,7 @@ enum {
 PACKAGE_LENGTH_4BYTE_SHIFT = 20,
 };
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes)
+void build_prepend_package_length(GArray *package)
 {
 uint8_t byte;
 unsigned length = package->len;
@@ -183,11 +183,6 @@ void build_prepend_package_length(GArray *package, 
unsigned min_bytes)
 length_bytes = 4;
 }
 
-/* Force length to at least min_bytes.
- * This wastes memory but that's how bios did it.
- */
-length_bytes = MAX(length_bytes, min_bytes);
-
 /* PkgLength is the length of the inclusive length of the data. */
 length += length_bytes;
 
@@ -220,15 +215,15 @@ void build_prepend_package_length(GArray *package, 
unsigned min_bytes)
 build_prepend_byte(package, byte);
 }
 
-void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+void build_package(GArray *package, uint8_t op)
 {
-build_prepend_package_length(package, min_bytes);
+build_prepend_package_length(package);
 build_prepend_byte(package, op);
 }
 
 void build_extop_package(GArray *package, uint8_t op)
 {
-build_package(package, op, 1);
+build_package(package, op);
 build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
 }
 
@@ -272,4 +267,3 @@ void build_append_int(GArray *table, uint32_t value)
 build_append_value(table, value, 4);
 }
 }
-
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e3ac1a14..a010f3b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -293,7 +293,7 @@ static void build_append_and_cleanup_method(GArray *device, 
GArray *method)
 {
 uint8_t op = 0x14; /* MethodOp */
 
-build_package(method, op, 0);
+build_package(method, op);
 
 build_append_array(device, method);
 build_free_array(method);
@@ -314,7 +314,7 @@ static void build_append_notify_target_ifequal(GArray 
*method,
 build_append_byte(notify, 0x69); /* Arg1Op */
 
 /* Pack it up */
-build_package(notify, op, 1);
+build_package(notify, op);
 
 build_append_array(method, notify);
 
@@ -827,7 +827,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 build_append_byte(notify, 0x69); /* Arg1Op */
 
 /* Pack it up */
-build_package(notify, op, 0);
+build_package(notify, op);
 
 build_append_array(method, notify);
 
@@ -868,7 +868,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 if (bus->parent_dev) {
 build_extop_package(bus_table, op);
 } else {
-build_package(bus_table, op, 0);
+build_package(bus_table, op);
 }
 
 /* Append our bus description to parent table */
@@ -1008,7 +1008,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 build_append_byte(package, b);
 }
 
-build_package(package, op, 2);
+build_package(package, op);
 build_append_array(sb_scope, package);
 build_free_array(package);
 }
@@ -1056,8 +1056,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 build_append_array(sb_scope, hotplug_state.device_table);
 build_pci_bus_state_cleanup(&hotplug_state);
 }
-
-build_package(sb_scope, op, 3);
+build_package(sb_scope, op);
 build_append_array(table_data, sb_scope);
 build_free_array(sb_scope);
 }
diff --git a/include/hw/acpi/acpi-build-utils.h 
b/include/hw/acpi/acpi-build-utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-utils.h
@@ -14,8 +14,8 @@ void build_append_array(GArray *array, GArray *val);
 void GCC_FMT_ATTR(2, 3)
 build_append_namestring(GArray *array, const char *format, ...);
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes);
-void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_prepend_package_length(GArray *package);
+void build_package(GArray *package, uint8_t op);
 void build_append_value(GArray *table, uint32_t value, int size);
 void build_append_int(GArray *table, uint32_t value);
 void build_extop_package(GArray *package, uint8_t op);
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization

[Igor Mammedov]

zero initialize AcpiPmInfo struct to reduce code bloat
a little bit.

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
 hw/i386/acpi-build.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 77a124e..4c0536f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
 Object *obj = NULL;
 QObject *o;
 
+memset(pm, 0, sizeof(*pm));
+
 if (piix) {
 obj = piix;
 }
@@ -184,22 +186,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
 o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
 if (o) {
 pm->s3_disabled = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s3_disabled = false;
 }
 qobject_decref(o);
 o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
 if (o) {
 pm->s4_disabled = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s4_disabled = false;
 }
 qobject_decref(o);
 o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
 if (o) {
 pm->s4_val = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s4_val = false;
 }
 qobject_decref(o);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper

[Igor Mammedov]

Use build_append_namestring() instead of build_append_nameseg()
So user won't have to care whether name is NameSeg, NamePath or
NameString.

See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
v3:
 assert on wrong Segcount earlier and extend condition to
  seg_count > 0 && seg_count <= 255
 as suggested by Claudio Fontana 
---
 hw/acpi/acpi-build-utils.c | 90 +-
 hw/i386/acpi-build.c   | 35 +++
 include/hw/acpi/acpi-build-utils.h |  2 +-
 3 files changed, 106 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index 79aa610..aed9066 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray *val)
 
 #define ACPI_NAMESEG_LEN 4
 
-void GCC_FMT_ATTR(2, 3)
+static void GCC_FMT_ATTR(2, 3)
 build_append_nameseg(GArray *array, const char *format, ...)
 {
 /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
@@ -71,6 +71,94 @@ build_append_nameseg(GArray *array, const char *format, ...)
 g_array_append_vals(array, "", ACPI_NAMESEG_LEN - len);
 }
 
+static const uint8_t ACPI_DualNamePrefix  = 0x2E;
+static const uint8_t ACPI_MultiNamePrefix = 0x2F;
+
+static void
+build_append_namestringv(GArray *array, const char *format, va_list ap)
+{
+/* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+char *s;
+int len;
+va_list va_len;
+char **segs;
+char **segs_iter;
+int seg_count = 0;
+
+va_copy(va_len, ap);
+len = vsnprintf(NULL, 0, format, va_len);
+va_end(va_len);
+len += 1;
+s = g_new(typeof(*s), len);
+
+len = vsnprintf(s, len, format, ap);
+
+segs = g_strsplit(s, ".", 0);
+g_free(s);
+
+/* count segments */
+segs_iter = segs;
+while (*segs_iter) {
+++segs_iter;
+++seg_count;
+}
+/*
+ * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
+ * "SegCount can be from 1 to 255"
+ */
+assert(seg_count > 0 && seg_count <= 255);
+
+/* handle RootPath || PrefixPath */
+s = *segs;
+while (*s == '\\' || *s == '^') {
+g_array_append_val(array, *s);
+++s;
+}
+
+switch (seg_count) {
+case 1:
+if (!*s) { /* NullName */
+const uint8_t nullpath = 0;
+g_array_append_val(array, nullpath);
+} else {
+build_append_nameseg(array, "%s", s);
+}
+break;
+
+case 2:
+g_array_append_val(array, ACPI_DualNamePrefix);
+build_append_nameseg(array, "%s", s);
+build_append_nameseg(array, "%s", segs[1]);
+break;
+
+default:
+g_array_append_val(array, ACPI_MultiNamePrefix);
+g_array_append_val(array, seg_count);
+
+/* handle the 1st segment manually due to prefix/root path */
+build_append_nameseg(array, "%s", s);
+
+/* add the rest of segments */
+segs_iter = segs + 1;
+while (*segs_iter) {
+build_append_nameseg(array, "%s", *segs_iter);
+++segs_iter;
+}
+break;
+}
+g_strfreev(segs);
+}
+
+void GCC_FMT_ATTR(2, 3)
+build_append_namestring(GArray *array, const char *format, ...)
+{
+va_list ap;
+
+va_start(ap, format);
+build_append_namestringv(array, format, ap);
+va_end(ap);
+}
+
 /* 5.4 Definition Block Encoding */
 enum {
 PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8ee15ab..e3ac1a14 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,7 @@ static GArray *build_alloc_method(const char *name, uint8_t 
arg_count)
 {
 GArray *method = build_alloc_array();
 
-build_append_nameseg(method, "%s", name);
+build_append_namestring(method, "%s", name);
 build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
 
 return method;
@@ -575,7 +575,7 @@ build_append_notify_method(GArray *device, const char *name,
 
 for (i = 0; i < count; i++) {
 GArray *target = build_alloc_array();
-build_append_nameseg(target, format, i);
+build_append_namestring(target, format, i);
 assert(i < 256); /* Fits in 1 byte */
 build_append_notify_target_ifequal(method, target, i, 1);
 build_free_array(target);
@@ -703,24 +703,24 @@ static void build_pci_bus_end(PCIBus *bus, void 
*bus_state)
 
 if (bus->parent_dev) {
 op = 0x82; /* DeviceOp */
-build_append_nameseg(bus_table, "S%.02X",
+build_append_namestring(bus_table, "S%.02X",
  bus->parent_dev->devfn);
 build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_nameseg(bus_table, "_SUN");
+build_append_namestring(bus_table, "_SUN");
 build_append_value(bus_table, PCI_

[Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file

[Igor Mammedov]

the will be later used for composing AML primitives
and all that could be reused later for ARM machines
as well.

Signed-off-by: Igor Mammedov 
---
v3:
  * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
  * copy GLP license block from acpi-build.c
v2:
  * fix wrong ident in moved code
---
 hw/acpi/Makefile.objs  |   1 +
 hw/acpi/acpi-build-utils.c | 187 +
 hw/i386/acpi-build.c   | 162 +---
 include/hw/acpi/acpi-build-utils.h |  23 +
 4 files changed, 213 insertions(+), 160 deletions(-)
 create mode 100644 hw/acpi/acpi-build-utils.c
 create mode 100644 include/hw/acpi/acpi-build-utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index ee82073..cad0355 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o 
cpu_hotplug.o
 common-obj-$(CONFIG_ACPI) += memory_hotplug.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
 common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
+common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
new file mode 100644
index 000..79aa610
--- /dev/null
+++ b/hw/acpi/acpi-build-utils.c
@@ -0,0 +1,187 @@
+/* Support for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2014 Red Hat Inc
+ *
+ * Author: Michael S. Tsirkin 
+ * Author: Igor Mammedov 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include "hw/acpi/acpi-build-utils.h"
+
+GArray *build_alloc_array(void)
+{
+return g_array_new(false, true /* clear */, 1);
+}
+
+void build_free_array(GArray *array)
+{
+g_array_free(array, true);
+}
+
+void build_prepend_byte(GArray *array, uint8_t val)
+{
+g_array_prepend_val(array, val);
+}
+
+void build_append_byte(GArray *array, uint8_t val)
+{
+g_array_append_val(array, val);
+}
+
+void build_append_array(GArray *array, GArray *val)
+{
+g_array_append_vals(array, val->data, val->len);
+}
+
+#define ACPI_NAMESEG_LEN 4
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...)
+{
+/* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+char s[] = "";
+int len;
+va_list args;
+
+va_start(args, format);
+len = vsnprintf(s, sizeof s, format, args);
+va_end(args);
+
+assert(len <= ACPI_NAMESEG_LEN);
+
+g_array_append_vals(array, s, len);
+/* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+g_array_append_vals(array, "", ACPI_NAMESEG_LEN - len);
+}
+
+/* 5.4 Definition Block Encoding */
+enum {
+PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
+PACKAGE_LENGTH_2BYTE_SHIFT = 4,
+PACKAGE_LENGTH_3BYTE_SHIFT = 12,
+PACKAGE_LENGTH_4BYTE_SHIFT = 20,
+};
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes)
+{
+uint8_t byte;
+unsigned length = package->len;
+unsigned length_bytes;
+
+if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
+length_bytes = 1;
+} else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
+length_bytes = 2;
+} else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
+length_bytes = 3;
+} else {
+length_bytes = 4;
+}
+
+/* Force length to at least min_bytes.
+ * This wastes memory but that's how bios did it.
+ */
+length_bytes = MAX(length_bytes, min_bytes);
+
+/* PkgLength is the length of the inclusive length of the data. */
+length += length_bytes;
+
+switch (length_bytes) {
+case 1:
+byte = length;
+build_prepend_byte(package, byte);
+return;
+case 4:
+byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
+build_prepend_byte(package, byte);
+length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
+/* fall through */
+case 3:
+byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
+build_prepend_byte(package, byte);
+length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
+/* fall through */
+case 2:
+byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
+build_prepend_byte(package, byte);
+length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
+/* fall through */
+}
+/*
+

Re: [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:

zero initialize AcpiPmInfo struct to reduce code bloat
a little bit.

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
  hw/i386/acpi-build.c | 8 ++--
  1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 77a124e..4c0536f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
  Object *obj = NULL;
  QObject *o;

+memset(pm, 0, sizeof(*pm));
+
  if (piix) {
  obj = piix;
  }
@@ -184,22 +186,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
  o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
  if (o) {
  pm->s3_disabled = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s3_disabled = false;
  }
  qobject_decref(o);
  o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
  if (o) {
  pm->s4_disabled = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s4_disabled = false;
  }
  qobject_decref(o);
  o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
  if (o) {
  pm->s4_val = qint_get_int(qobject_to_qint(o));
-} else {
-pm->s4_val = false;
  }
  qobject_decref(o);




Reviewed-by: Marcel Apfelbaum 

Re: [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:

the will be later used for composing AML primitives
and all that could be reused later for ARM machines
as well.

Signed-off-by: Igor Mammedov 
---
v3:
   * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
   * copy GLP license block from acpi-build.c
v2:
   * fix wrong ident in moved code
---
  hw/acpi/Makefile.objs  |   1 +
  hw/acpi/acpi-build-utils.c | 187 +
  hw/i386/acpi-build.c   | 162 +---
  include/hw/acpi/acpi-build-utils.h |  23 +
  4 files changed, 213 insertions(+), 160 deletions(-)
  create mode 100644 hw/acpi/acpi-build-utils.c
  create mode 100644 include/hw/acpi/acpi-build-utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index ee82073..cad0355 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o 
cpu_hotplug.o
  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
  common-obj-$(CONFIG_ACPI) += acpi_interface.o
  common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
+common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
new file mode 100644
index 000..79aa610
--- /dev/null
+++ b/hw/acpi/acpi-build-utils.c
@@ -0,0 +1,187 @@
+/* Support for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2014 Red Hat Inc


2015?
Michael, can you change this on the fly?

Other than that:
Acked-by: Marcel Apfelbaum 


+ *
+ * Author: Michael S. Tsirkin 
+ * Author: Igor Mammedov 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include "hw/acpi/acpi-build-utils.h"
+
+GArray *build_alloc_array(void)
+{
+return g_array_new(false, true /* clear */, 1);
+}
+
+void build_free_array(GArray *array)
+{
+g_array_free(array, true);
+}
+
+void build_prepend_byte(GArray *array, uint8_t val)
+{
+g_array_prepend_val(array, val);
+}
+
+void build_append_byte(GArray *array, uint8_t val)
+{
+g_array_append_val(array, val);
+}
+
+void build_append_array(GArray *array, GArray *val)
+{
+g_array_append_vals(array, val->data, val->len);
+}
+
+#define ACPI_NAMESEG_LEN 4
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...)
+{
+/* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+char s[] = "";
+int len;
+va_list args;
+
+va_start(args, format);
+len = vsnprintf(s, sizeof s, format, args);
+va_end(args);
+
+assert(len <= ACPI_NAMESEG_LEN);
+
+g_array_append_vals(array, s, len);
+/* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+g_array_append_vals(array, "", ACPI_NAMESEG_LEN - len);
+}
+
+/* 5.4 Definition Block Encoding */
+enum {
+PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
+PACKAGE_LENGTH_2BYTE_SHIFT = 4,
+PACKAGE_LENGTH_3BYTE_SHIFT = 12,
+PACKAGE_LENGTH_4BYTE_SHIFT = 20,
+};
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes)
+{
+uint8_t byte;
+unsigned length = package->len;
+unsigned length_bytes;
+
+if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
+length_bytes = 1;
+} else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
+length_bytes = 2;
+} else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
+length_bytes = 3;
+} else {
+length_bytes = 4;
+}
+
+/* Force length to at least min_bytes.
+ * This wastes memory but that's how bios did it.
+ */
+length_bytes = MAX(length_bytes, min_bytes);
+
+/* PkgLength is the length of the inclusive length of the data. */
+length += length_bytes;
+
+switch (length_bytes) {
+case 1:
+byte = length;
+build_prepend_byte(package, byte);
+return;
+case 4:
+byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
+build_prepend_byte(package, byte);
+length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
+/* fall through */
+case 3:
+byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
+build_prepend_byte(package, byte);
+length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
+/* fall through */
+case 2:
+byte = length 

Re: [Qemu-devel] [PATCH 2/3] Migration: Add lots of trace events

[Dr. David Alan Gilbert]

* Amit Shah (amit.s...@redhat.com) wrote:
> On (Tue) 20 Jan 2015 [14:48:02], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > Mostly on the load side, so that when we get a complaint about
> > a migration failure we can figure out what it didn't like.
> 
> Nice!
> 
> Just one note below.
> 
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> >  migration/vmstate.c | 22 +++---
> >  savevm.c| 10 +++---
> >  trace-events| 11 ++-
> >  3 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 2c0b135..7b8dc3f 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -75,14 +75,19 @@ int vmstate_load_state(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >  VMStateField *field = vmsd->fields;
> >  int ret;
> >  
> > +trace_vmstate_load_state(vmsd->name, version_id);
> >  if (version_id > vmsd->version_id) {
> > +trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> >  return -EINVAL;
> >  }
> >  if  (version_id < vmsd->minimum_version_id) {
> >  if (vmsd->load_state_old &&
> >  version_id >= vmsd->minimum_version_id_old) {
> > -return vmsd->load_state_old(f, opaque, version_id);
> > +ret = vmsd->load_state_old(f, opaque, version_id);
> > +trace_vmstate_load_state_end(vmsd->name, "old path", ret);
> > +return ret;
> >  }
> > +trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> >  return -EINVAL;
> >  }
> >  if (vmsd->pre_load) {
> > @@ -92,6 +97,7 @@ int vmstate_load_state(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >  }
> >  }
> >  while (field->name) {
> > +trace_vmstate_load_state_field(vmsd->name, field->name);
> >  if ((field->field_exists &&
> >   field->field_exists(opaque, version_id)) ||
> >  (!field->field_exists &&
> > @@ -134,9 +140,10 @@ int vmstate_load_state(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >  return ret;
> >  }
> >  if (vmsd->post_load) {
> > -return vmsd->post_load(opaque, version_id);
> > +ret = vmsd->post_load(opaque, version_id);
> >  }
> > -return 0;
> > +trace_vmstate_load_state_end(vmsd->name, "end", ret);
> > +return ret;
> >  }
> 
> "return 0" becomes "return ret", and ret isn't assigned anywhere.

Oops, yes, I'll reroll it.

Dave

> 
> >  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> > @@ -193,6 +200,8 @@ static const VMStateDescription *
> >  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription 
> > *vmsd,
> > void *opaque)
> >  {
> > +trace_vmstate_subsection_load(vmsd->name);
> > +
> >  while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
> >  char idstr[256];
> >  int ret;
> > @@ -202,20 +211,24 @@ static int vmstate_subsection_load(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >  len = qemu_peek_byte(f, 1);
> >  if (len < strlen(vmsd->name) + 1) {
> >  /* subsection name has be be "section_name/a" */
> > +trace_vmstate_subsection_load_bad(vmsd->name, "(short)");
> >  return 0;
> 
> Nice how you use the () to differentiate from the idstr below..
> 
> >  }
> >  size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
> >  if (size != len) {
> > +trace_vmstate_subsection_load_bad(vmsd->name, "(peek fail)");
> >  return 0;
> >  }
> >  idstr[size] = 0;
> >  
> >  if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
> > +trace_vmstate_subsection_load_bad(vmsd->name, idstr);
> >  /* it don't have a valid subsection name */
> >  return 0;
> 
> 
>   Amit
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:

Use build_append_namestring() instead of build_append_nameseg()
So user won't have to care whether name is NameSeg, NamePath or
NameString.

See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
v3:
  assert on wrong Segcount earlier and extend condition to
   seg_count > 0 && seg_count <= 255
  as suggested by Claudio Fontana 
---
  hw/acpi/acpi-build-utils.c | 90 +-
  hw/i386/acpi-build.c   | 35 +++
  include/hw/acpi/acpi-build-utils.h |  2 +-
  3 files changed, 106 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index 79aa610..aed9066 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray *val)

  #define ACPI_NAMESEG_LEN 4

-void GCC_FMT_ATTR(2, 3)
+static void GCC_FMT_ATTR(2, 3)
  build_append_nameseg(GArray *array, const char *format, ...)
  {
  /* It would be nicer to use g_string_vprintf but it's only there in 2.22 
*/
@@ -71,6 +71,94 @@ build_append_nameseg(GArray *array, const char *format, ...)
  g_array_append_vals(array, "", ACPI_NAMESEG_LEN - len);
  }

+static const uint8_t ACPI_DualNamePrefix  = 0x2E;
+static const uint8_t ACPI_MultiNamePrefix = 0x2F;
+
+static void
+build_append_namestringv(GArray *array, const char *format, va_list ap)
+{
+/* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+char *s;
+int len;
+va_list va_len;
+char **segs;
+char **segs_iter;
+int seg_count = 0;
+
+va_copy(va_len, ap);
+len = vsnprintf(NULL, 0, format, va_len);
+va_end(va_len);
+len += 1;
+s = g_new(typeof(*s), len);
+
+len = vsnprintf(s, len, format, ap);
+
+segs = g_strsplit(s, ".", 0);
+g_free(s);
+
+/* count segments */
+segs_iter = segs;
+while (*segs_iter) {
+++segs_iter;
+++seg_count;
+}
+/*
+ * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
+ * "SegCount can be from 1 to 255"
+ */
+assert(seg_count > 0 && seg_count <= 255);
+
+/* handle RootPath || PrefixPath */
+s = *segs;
+while (*s == '\\' || *s == '^') {
+g_array_append_val(array, *s);
+++s;
+}
+
+switch (seg_count) {
+case 1:
+if (!*s) { /* NullName */
+const uint8_t nullpath = 0;
+g_array_append_val(array, nullpath);
+} else {
+build_append_nameseg(array, "%s", s);
+}
+break;
+
+case 2:
+g_array_append_val(array, ACPI_DualNamePrefix);
+build_append_nameseg(array, "%s", s);
+build_append_nameseg(array, "%s", segs[1]);
+break;
+
+default:
+g_array_append_val(array, ACPI_MultiNamePrefix);
+g_array_append_val(array, seg_count);
+
+/* handle the 1st segment manually due to prefix/root path */
+build_append_nameseg(array, "%s", s);
+
+/* add the rest of segments */
+segs_iter = segs + 1;
+while (*segs_iter) {
+build_append_nameseg(array, "%s", *segs_iter);
+++segs_iter;
+}
+break;
+}
+g_strfreev(segs);
+}
+
+void GCC_FMT_ATTR(2, 3)
+build_append_namestring(GArray *array, const char *format, ...)
+{
+va_list ap;
+
+va_start(ap, format);
+build_append_namestringv(array, format, ap);
+va_end(ap);
+}
+
  /* 5.4 Definition Block Encoding */
  enum {
  PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8ee15ab..e3ac1a14 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,7 @@ static GArray *build_alloc_method(const char *name, uint8_t 
arg_count)
  {
  GArray *method = build_alloc_array();

-build_append_nameseg(method, "%s", name);
+build_append_namestring(method, "%s", name);
  build_append_byte(method, arg_count); /* MethodFlags: ArgCount */

  return method;
@@ -575,7 +575,7 @@ build_append_notify_method(GArray *device, const char *name,

  for (i = 0; i < count; i++) {
  GArray *target = build_alloc_array();
-build_append_nameseg(target, format, i);
+build_append_namestring(target, format, i);
  assert(i < 256); /* Fits in 1 byte */
  build_append_notify_target_ifequal(method, target, i, 1);
  build_free_array(target);
@@ -703,24 +703,24 @@ static void build_pci_bus_end(PCIBus *bus, void 
*bus_state)

  if (bus->parent_dev) {
  op = 0x82; /* DeviceOp */
-build_append_nameseg(bus_table, "S%.02X",
+build_append_namestring(bus_table, "S%.02X",
   bus->parent_dev->devfn);
  build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_nameseg(bus_table, "_SUN");
+build_append_name

Re: [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package()

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:

Signed-off-by: Igor Mammedov 
Reviewed-by: Claudio Fontana 
---
  hw/acpi/acpi-build-utils.c | 14 --
  hw/i386/acpi-build.c   | 13 ++---
  include/hw/acpi/acpi-build-utils.h |  4 ++--
  3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index aed9066..602e68c 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -167,7 +167,7 @@ enum {
  PACKAGE_LENGTH_4BYTE_SHIFT = 20,
  };

-void build_prepend_package_length(GArray *package, unsigned min_bytes)
+void build_prepend_package_length(GArray *package)
  {
  uint8_t byte;
  unsigned length = package->len;
@@ -183,11 +183,6 @@ void build_prepend_package_length(GArray *package, 
unsigned min_bytes)
  length_bytes = 4;
  }

-/* Force length to at least min_bytes.
- * This wastes memory but that's how bios did it.
- */
-length_bytes = MAX(length_bytes, min_bytes);
-
  /* PkgLength is the length of the inclusive length of the data. */
  length += length_bytes;

@@ -220,15 +215,15 @@ void build_prepend_package_length(GArray *package, 
unsigned min_bytes)
  build_prepend_byte(package, byte);
  }

-void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+void build_package(GArray *package, uint8_t op)
  {
-build_prepend_package_length(package, min_bytes);
+build_prepend_package_length(package);
  build_prepend_byte(package, op);
  }

  void build_extop_package(GArray *package, uint8_t op)
  {
-build_package(package, op, 1);
+build_package(package, op);
  build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
  }

@@ -272,4 +267,3 @@ void build_append_int(GArray *table, uint32_t value)
  build_append_value(table, value, 4);
  }
  }
-
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e3ac1a14..a010f3b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -293,7 +293,7 @@ static void build_append_and_cleanup_method(GArray *device, 
GArray *method)
  {
  uint8_t op = 0x14; /* MethodOp */

-build_package(method, op, 0);
+build_package(method, op);

  build_append_array(device, method);
  build_free_array(method);
@@ -314,7 +314,7 @@ static void build_append_notify_target_ifequal(GArray 
*method,
  build_append_byte(notify, 0x69); /* Arg1Op */

  /* Pack it up */
-build_package(notify, op, 1);
+build_package(notify, op);

  build_append_array(method, notify);

@@ -827,7 +827,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
  build_append_byte(notify, 0x69); /* Arg1Op */

  /* Pack it up */
-build_package(notify, op, 0);
+build_package(notify, op);

  build_append_array(method, notify);

@@ -868,7 +868,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
  if (bus->parent_dev) {
  build_extop_package(bus_table, op);
  } else {
-build_package(bus_table, op, 0);
+build_package(bus_table, op);
  }

  /* Append our bus description to parent table */
@@ -1008,7 +1008,7 @@ build_ssdt(GArray *table_data, GArray *linker,
  build_append_byte(package, b);
  }

-build_package(package, op, 2);
+build_package(package, op);
  build_append_array(sb_scope, package);
  build_free_array(package);
  }
@@ -1056,8 +1056,7 @@ build_ssdt(GArray *table_data, GArray *linker,
  build_append_array(sb_scope, hotplug_state.device_table);
  build_pci_bus_state_cleanup(&hotplug_state);
  }
-
-build_package(sb_scope, op, 3);
+build_package(sb_scope, op);
  build_append_array(table_data, sb_scope);
  build_free_array(sb_scope);
  }
diff --git a/include/hw/acpi/acpi-build-utils.h 
b/include/hw/acpi/acpi-build-utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-utils.h
@@ -14,8 +14,8 @@ void build_append_array(GArray *array, GArray *val);
  void GCC_FMT_ATTR(2, 3)
  build_append_namestring(GArray *array, const char *format, ...);

-void build_prepend_package_length(GArray *package, unsigned min_bytes);
-void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_prepend_package_length(GArray *package);
+void build_package(GArray *package, uint8_t op);
  void build_append_value(GArray *table, uint32_t value, int size);
  void build_append_int(GArray *table, uint32_t value);
  void build_extop_package(GArray *package, uint8_t op);


Reviewed-by: Marcel Apfelbaum 

Re: [Qemu-devel] [Qemu-stable] [PATCH] sb16: fix interrupt acknowledgement

[Michael Tokarev]

20.01.2015 19:23, Paolo Bonzini wrote:
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -999,7 +999,7 @@ static IO_READ_PROTO (dsp_read)
>  retval = (!s->out_data_len || s->highspeed) ? 0 : 0x80;
>  if (s->mixer_regs[0x82] & 1) {
>  ack = 1;
> -s->mixer_regs[0x82] &= 1;
> +s->mixer_regs[0x82] &= ~1;

Shouldn't it be ~1u instead?

/mjt

Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Markus Armbruster]

John Snow  writes:

> On 01/20/2015 03:26 AM, Markus Armbruster wrote:
>> John Snow  writes:
>>
>>> On 01/19/2015 05:08 AM, Markus Armbruster wrote:
 John Snow  writes:

> On 01/16/2015 10:36 AM, Max Reitz wrote:
>> On 2015-01-12 at 11:30, John Snow wrote:
>>> From: Fam Zheng 
>>>
>>> The new command pair is added to manage user created dirty bitmap. The
>>> dirty bitmap's name is mandatory and must be unique for the same device,
>>> but different devices can have bitmaps with the same names.
>>>
>>> The granularity is an optional field. If it is not specified, we will
>>> choose a default granularity based on the cluster size if available,
>>> clamped to between 4K and 64K to mirror how the 'mirror' code was
>>> already choosing granularity. If we do not have cluster size info
>>> available, we choose 64K. This code has been factored out into a helper
>>> shared with block/mirror.
>>>
>>> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
>>> which takes a device name and a dirty bitmap name and validates the
>>> lookup, returning NULL and setting errp if there is a problem with
>>> either field. This helper will be re-used in future patches in this
>>> series.
>>>
>>> The types added to block-core.json will be re-used in future patches
>>> in this series, see:
>>> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable,
>>> disable}'
>>>
>>> Signed-off-by: Fam Zheng 
>>> Signed-off-by: John Snow 
>>> ---
>>> block.c   |  20 ++
>>> block/mirror.c|  10 +
>>> blockdev.c| 100
>>> ++
>>> include/block/block.h |   1 +
>>> qapi/block-core.json  |  55 +++
>>> qmp-commands.hx   |  51 +
>>> 6 files changed, 228 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index bfeae6b..3eb77ee 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap, int64_t sector
>>> }
>>> }
>>> +/**
>>> + * Chooses a default granularity based on the existing cluster size,
>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that 
>>> there
>>> + * is no cluster size information available.
>>> + */
>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
>>> +{
>>> +BlockDriverInfo bdi;
>>> +uint64_t granularity;
>>> +
>>> +if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
>>> +granularity = MAX(4096, bdi.cluster_size);
>>> +granularity = MIN(65536, granularity);
>>> +} else {
>>> +granularity = 65536;
>>> +}
>>> +
>>> +return granularity;
>>> +}
>>> +
>>> void bdrv_dirty_iter_init(BlockDriverState *bs,
>>>   BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>>> {
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index d819952..fc545f1 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -667,15 +667,7 @@ static void mirror_start_job(BlockDriverState
>>> *bs, BlockDriverState *target,
>>> MirrorBlockJob *s;
>>> if (granularity == 0) {
>>> -/* Choose the default granularity based on the target file's
>>> cluster
>>> - * size, clamped between 4k and 64k.  */
>>> -BlockDriverInfo bdi;
>>> -if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) 
>>> {
>>> -granularity = MAX(4096, bdi.cluster_size);
>>> -granularity = MIN(65536, granularity);
>>> -} else {
>>> -granularity = 65536;
>>> -}
>>> +granularity = bdrv_get_default_bitmap_granularity(target);
>>> }
>>> assert ((granularity & (granularity - 1)) == 0);
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 5651a8e..95251c7 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1173,6 +1173,48 @@ out_aio_context:
>>> return NULL;
>>> }
>>> +/**
>>> + * Return a dirty bitmap (if present), after validating
>>> + * the node reference and bitmap names. Returns NULL on error,
>>> + * including when the BDS and/or bitmap is not found.
>>> + */
>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node_ref,
>>> +  const char *name,
>>> +  BlockDriverState
>>> **pbs,
>>> +  Error **errp)
>>> +{
>>> +BlockDriverState *b

[Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation

[Igor Mammedov]

it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
  which is easier to follow.
* drops unnecessary loops and bitmaps,
  creating devices and notification method in the same loop.
* saves us ~100LOC

Signed-off-by: Igor Mammedov 
---
v4:
  * keep original logic of creating bridge devices as it was done
in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
  * if bus is non hotpluggable, add child slots to bus as non
hotpluggable as it was done in original code.
v3:
  * use hotpluggable device object instead of not hotpluggable
for non present devices, and add it only when bus itself is
hotpluggable
---
 hw/i386/acpi-build.c | 275 +--
 1 file changed, 90 insertions(+), 185 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a010f3b..5255428 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -106,7 +106,6 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
 bool has_hpet;
 bool has_tpm;
-DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
 const unsigned char *dsdt_code;
 unsigned dsdt_size;
 uint16_t pvpanic_port;
@@ -651,69 +650,37 @@ static void acpi_set_pci_info(void)
 }
 }
 
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
- AcpiBuildPciBusHotplugState *parent,
- bool pcihp_bridge_en)
+static void build_append_pcihp_notify_entry(GArray *method, int slot)
 {
-state->parent = parent;
-state->device_table = build_alloc_array();
-state->notify_table = build_alloc_array();
-state->pcihp_bridge_en = pcihp_bridge_en;
-}
-
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-build_free_array(state->device_table);
-build_free_array(state->notify_table);
-}
+GArray *ifctx;
 
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
-{
-AcpiBuildPciBusHotplugState *parent = parent_state;
-AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
-build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+ifctx = build_alloc_array();
+build_append_byte(ifctx, 0x7B); /* AndOp */
+build_append_byte(ifctx, 0x68); /* Arg0Op */
+build_append_int(ifctx, 0x1U << slot);
+build_append_byte(ifctx, 0x00); /* NullName */
+build_append_byte(ifctx, 0x86); /* NotifyOp */
+build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
+build_append_byte(ifctx, 0x69); /* Arg1Op */
 
-return child;
+/* Pack it up */
+build_package(ifctx, 0xA0 /* IfOp */);
+build_append_array(method, ifctx);
+build_free_array(ifctx);
 }
 
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+ bool pcihp_bridge_en)
 {
-AcpiBuildPciBusHotplugState *child = bus_state;
-AcpiBuildPciBusHotplugState *parent = child->parent;
 GArray *bus_table = build_alloc_array();
-DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-uint8_t op;
-int i;
+GArray *method = NULL;
 QObject *bsel;
-GArray *method;
-bool bus_hotplug_support = false;
-
-/*
- * Skip bridge subtree creation if bridge hotplug is disabled
- * to make acpi tables compatible with legacy machine types.
- */
-if (!child->pcihp_bridge_en && bus->parent_dev) {
-return;
-}
+PCIBus *sec;
+int i;
 
 if (bus->parent_dev) {
-op = 0x82; /* DeviceOp */
-build_append_namestring(bus_table, "S%.02X",
- bus->parent_dev->devfn);
-build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_namestring(bus_table, "_SUN");
-build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_namestring(bus_table, "_ADR");
-build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) 
|
-   PCI_FUNC(bus->parent_dev->devfn), 4);
+build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
 } else {
-op = 0x10; /* ScopeOp */;
 build_append_namestring(bus_table, "PCI0");
 }
 
@@ -722,17 +689,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 build_append_byte(bus_table, 0x08); /* NameOp */
 build_append_namestring(bus_table, "BSEL");
 build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
-} else {
-/* No bsel - no slots a

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Cornelia Huck]

On Tue, 20 Jan 2015 13:33:27 +0100
Markus Armbruster  wrote:

> Cornelia Huck  writes:
> 
> > On Tue, 20 Jan 2015 10:45:41 +0100
> > Markus Armbruster  wrote:
> >
> >> This patch makes Coverity unhappy:
> >> 
> >> *** CID 1264326:  Unintended sign extension  (SIGN_EXTENSION)
> >> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call()
> >> 781 stq_p(&fib.pal, pbdev->pal);
> >> 782 stq_p(&fib.iota, pbdev->g_iota);
> >> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr);
> >> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
> >> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
> >> 786 
> >> >>> CID 1264326:  Unintended sign extension  (SIGN_EXTENSION)
> >> >>> Suspicious implicit sign extension: "pbdev->isc" with type
> >> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc <<
> >> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then
> >> >>> sign-extended to type "unsigned long" (64 bits, unsigned).  If
> >> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than
> >> >>> 0x7FFF, the upper bits of the result will all be 1.
> >> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> >> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> >> 789pbdev->routes.adapter.summary_offset;
> >> 790 stw_p(&fib.data, data);
> >> 791 
> >> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
> >
> > There's a fix for this (and the memory leak):
> >
> > http://marc.info/?l=qemu-devel&m=142124886620078&w=2
> >
> > The patch is sitting in my queue, will send with the next pile of s390x
> > updates.
> 
> I can't see how
> 
> @@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, 
> uint64_t fiba)
>  data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> pbdev->routes.adapter.summary_offset;
> -stw_p(&fib.data, data);
> +stl_p(&fib.data, data);
> 
>  if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>  fib.fc |= 0x80;
> 
> fixes the implicit sign extension within the assignment preceding it.

What, I am expected to actually read the explanations? :)

> Regarding the leak, I prefer my patch, because it avoids the free on
> error.  But you're the maintainer.

Indeed, that's a good point.

I'll drop Frank's original patch and instead take your memory leak fix.
Will take a patch from Frank for the sign extension stuff (and the
stw/stl fix) as well once it has been posted.

Re: [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:

it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
   which is easier to follow.
* drops unnecessary loops and bitmaps,
   creating devices and notification method in the same loop.
* saves us ~100LOC

Signed-off-by: Igor Mammedov 
---
v4:
   * keep original logic of creating bridge devices as it was done
 in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
   * if bus is non hotpluggable, add child slots to bus as non
 hotpluggable as it was done in original code.
v3:
   * use hotpluggable device object instead of not hotpluggable
 for non present devices, and add it only when bus itself is
 hotpluggable
---
  hw/i386/acpi-build.c | 275 +--
  1 file changed, 90 insertions(+), 185 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a010f3b..5255428 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -106,7 +106,6 @@ typedef struct AcpiPmInfo {
  typedef struct AcpiMiscInfo {
  bool has_hpet;
  bool has_tpm;
-DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
  const unsigned char *dsdt_code;
  unsigned dsdt_size;
  uint16_t pvpanic_port;
@@ -651,69 +650,37 @@ static void acpi_set_pci_info(void)
  }
  }

-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
- AcpiBuildPciBusHotplugState *parent,
- bool pcihp_bridge_en)
+static void build_append_pcihp_notify_entry(GArray *method, int slot)
  {
-state->parent = parent;
-state->device_table = build_alloc_array();
-state->notify_table = build_alloc_array();
-state->pcihp_bridge_en = pcihp_bridge_en;
-}
-
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-build_free_array(state->device_table);
-build_free_array(state->notify_table);
-}
+GArray *ifctx;

-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
-{
-AcpiBuildPciBusHotplugState *parent = parent_state;
-AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
-build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+ifctx = build_alloc_array();
+build_append_byte(ifctx, 0x7B); /* AndOp */
+build_append_byte(ifctx, 0x68); /* Arg0Op */
+build_append_int(ifctx, 0x1U << slot);
+build_append_byte(ifctx, 0x00); /* NullName */
+build_append_byte(ifctx, 0x86); /* NotifyOp */
+build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
+build_append_byte(ifctx, 0x69); /* Arg1Op */

-return child;
+/* Pack it up */
+build_package(ifctx, 0xA0 /* IfOp */);
+build_append_array(method, ifctx);
+build_free_array(ifctx);
  }

-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+ bool pcihp_bridge_en)
  {
-AcpiBuildPciBusHotplugState *child = bus_state;
-AcpiBuildPciBusHotplugState *parent = child->parent;
  GArray *bus_table = build_alloc_array();
-DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-uint8_t op;
-int i;
+GArray *method = NULL;
  QObject *bsel;
-GArray *method;
-bool bus_hotplug_support = false;
-
-/*
- * Skip bridge subtree creation if bridge hotplug is disabled
- * to make acpi tables compatible with legacy machine types.
- */
-if (!child->pcihp_bridge_en && bus->parent_dev) {
-return;
-}
+PCIBus *sec;
+int i;

  if (bus->parent_dev) {
-op = 0x82; /* DeviceOp */
-build_append_namestring(bus_table, "S%.02X",
- bus->parent_dev->devfn);
-build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_namestring(bus_table, "_SUN");
-build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-build_append_byte(bus_table, 0x08); /* NameOp */
-build_append_namestring(bus_table, "_ADR");
-build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) 
|
-   PCI_FUNC(bus->parent_dev->devfn), 4);
+build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
  } else {
-op = 0x10; /* ScopeOp */;
  build_append_namestring(bus_table, "PCI0");
  }

@@ -722,17 +689,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
  build_append_byte(bus_table, 0x08); /* NameOp */
  build_append_namestring(bus_table, "BSEL");
  build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-memset(slot_hotplug_enable, 0xff, siz

Re: [Qemu-devel] [PATCH] s390: Plug memory leak on s390_pci_generate_event() error path

[Cornelia Huck]

On Tue, 20 Jan 2015 10:56:37 +0100
Markus Armbruster  wrote:

> Signed-off-by: Markus Armbruster 
> ---
>  hw/s390x/s390-pci-bus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1201b8d..d25ac74 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -187,7 +187,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_fh(uint32_t fh)
>  static void s390_pci_generate_event(uint8_t cc, uint16_t pec, uint32_t fh,
>  uint32_t fid, uint64_t faddr, uint32_t e)
>  {
> -SeiContainer *sei_cont = g_malloc0(sizeof(SeiContainer));
> +SeiContainer *sei_cont;
>  S390pciState *s = S390_PCI_HOST_BRIDGE(
>  object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> 
> @@ -195,6 +195,7 @@ static void s390_pci_generate_event(uint8_t cc, uint16_t 
> pec, uint32_t fh,
>  return;
>  }
> 
> +sei_cont = g_malloc0(sizeof(SeiContainer));
>  sei_cont->fh = fh;
>  sei_cont->fid = fid;
>  sei_cont->cc = cc;

Applied to my s390-next branch.

Re: [Qemu-devel] [Qemu-stable] [PATCH] sb16: fix interrupt acknowledgement

[Paolo Bonzini]

On 21/01/2015 10:32, Michael Tokarev wrote:
>> > --- a/hw/audio/sb16.c
>> > +++ b/hw/audio/sb16.c
>> > @@ -999,7 +999,7 @@ static IO_READ_PROTO (dsp_read)
>> >  retval = (!s->out_data_len || s->highspeed) ? 0 : 0x80;
>> >  if (s->mixer_regs[0x82] & 1) {
>> >  ack = 1;
>> > -s->mixer_regs[0x82] &= 1;
>> > +s->mixer_regs[0x82] &= ~1;
> Shouldn't it be ~1u instead?

It's the same since mixer_regs is an array of uint8_t.

I'm not a fan of unsigned suffixes, especially after ~ where most of the
time they're wrong.  For example if x is uint64_t:

   x &= ~1is right
   x &= ~1ull is right
   x &= ~1u   also clears bits 32...64
   x &= ~1ul  also clears bits 32...64 on 32-bit machines

Paolo

Re: [Qemu-devel] [RFC PATCH 2/4] pcie-aer: Fix command pcie_aer_inject_error is invalid

[Chen Fan]

On 01/16/2015 03:56 PM, Chen Fan wrote:


On 01/12/2015 09:56 PM, Marcel Apfelbaum wrote:

On 01/12/2015 05:04 AM, Chen Fan wrote:

in spec "PCI Express 3.0" section 6.2.6 Figure 6-3 virtual bridge part,
the flowchart showing tell us SERR# enable at Bridge Control register
associate with system error at Secondary Status register can send error
message. but bridge_control from dev->config is NULL, and SERR# was set
in dev->wmask in pcie_aer_init()

wmask denotes the register bits that can be written by the guest.

If you are referring to:
   pci_word_test_and_set_mask(dev->wmask + PCI_BRIDGE_CONTROL,
  PCI_BRIDGE_CTL_SERR);
that means that the OS *is able* to turn on/off SERR forwarding.

Hi marcel,

I saw the OS that turn on SERR# is to via evaluate _HPP (Hot Plug 
Parameters) method in ACPI.

is it the only way to turn on SERR#?


I saw there was one option called*//*PCI SERR# Generation **searched 
from web pages in firmware on hardware.

Do it turn on the SERR#? if so, we can enable it in seabios.

Thanks,
Chen



Thanks,
Chen




 which was implemented by root port and
swith devices, so we should add wmask (for w/r) bit set for bridge 
control.

we can user command like:
qemu_system_x86_64:
-device ioh3420,bus=pcie.0,addr=1c.0,multifunction=on,port=1,id=bridge1
-device x3130-upstream,bus=bridge1,id=up.1,addr=00.0
-device 
xio3130-downstream,bus=up.1,id=down.1,port=1,addr=00.0,chassis=5


(qemu) pcie_aer_inject_error net0 POISON_TLP

after that,
guest can output the error message.

Signed-off-by: Chen Fan 
---
  hw/pci/pcie_aer.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 7ca077a..571dc92 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -231,7 +231,8 @@ pcie_aer_msg_alldev(PCIDevice *dev, const 
PCIEAERMsg *msg)

   */
  static bool pcie_aer_msg_vbridge(PCIDevice *dev, const PCIEAERMsg 
*msg)

  {
-uint16_t bridge_control = pci_get_word(dev->config + 
PCI_BRIDGE_CONTROL);
Here we check if the Guest OS/firmware actually turned the #SERR 
forwarding on.


+uint16_t bridge_control = pci_get_word(dev->config + 
PCI_BRIDGE_CONTROL) |
+  pci_get_word(dev->wmask + 
PCI_BRIDGE_CONTROL);

I don't think that this check is correct given the above comments.
Please correct me if I mislead you,
Thanks,
Marcel




  if (pcie_aer_msg_is_uncor(msg)) {
  /* Received System Error */



.

[Qemu-devel] [PATCH v2 0/3] Migration tracing and errors

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Tidy up some old printfs
Add some more error_reports that previously just failed with invalid.
Add lots and lots of tracing.

Dr. David Alan Gilbert (3):
  savevm: Convert fprintf to error_report
  Migration: Add lots of trace events
  Print errors in some of the early migration failure cases.

 migration/vmstate.c | 31 ---
 savevm.c| 38 --
 trace-events| 11 ++-
 3 files changed, 58 insertions(+), 22 deletions(-)

-- 

v2
   Fix missing ret initialisation
2.1.0

[Qemu-devel] [PATCH v2 1/3] savevm: Convert fprintf to error_report

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Convert a bunch of fprintfs to error_reports

Signed-off-by: Dr. David Alan Gilbert 
---
 migration/vmstate.c |  7 ---
 savevm.c| 21 +++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 3dde574..2c0b135 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -3,6 +3,7 @@
 #include "migration/qemu-file.h"
 #include "migration/vmstate.h"
 #include "qemu/bitops.h"
+#include "qemu/error-report.h"
 #include "trace.h"
 
 static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
*vmsd,
@@ -122,8 +123,8 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 }
 }
 } else if (field->flags & VMS_MUST_EXIST) {
-fprintf(stderr, "Input validation failed: %s/%s\n",
-vmsd->name, field->name);
+error_report("Input validation failed: %s/%s",
+ vmsd->name, field->name);
 return -1;
 }
 field++;
@@ -167,7 +168,7 @@ void vmstate_save_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 }
 } else {
 if (field->flags & VMS_MUST_EXIST) {
-fprintf(stderr, "Output state validation failed: %s/%s\n",
+error_report("Output state validation failed: %s/%s",
 vmsd->name, field->name);
 assert(!(field->flags & VMS_MUST_EXIST));
 }
diff --git a/savevm.c b/savevm.c
index 08ec678..48c2396 100644
--- a/savevm.c
+++ b/savevm.c
@@ -898,7 +898,7 @@ int qemu_loadvm_state(QEMUFile *f)
 
 v = qemu_get_be32(f);
 if (v == QEMU_VM_FILE_VERSION_COMPAT) {
-fprintf(stderr, "SaveVM v2 format is obsolete and don't work 
anymore\n");
+error_report("SaveVM v2 format is obsolete and don't work anymore");
 return -ENOTSUP;
 }
 if (v != QEMU_VM_FILE_VERSION) {
@@ -925,15 +925,16 @@ int qemu_loadvm_state(QEMUFile *f)
 /* Find savevm section */
 se = find_se(idstr, instance_id);
 if (se == NULL) {
-fprintf(stderr, "Unknown savevm section or instance '%s' 
%d\n", idstr, instance_id);
+error_report("Unknown savevm section or instance '%s' %d",
+ idstr, instance_id);
 ret = -EINVAL;
 goto out;
 }
 
 /* Validate version */
 if (version_id > se->version_id) {
-fprintf(stderr, "savevm: unsupported version %d for '%s' 
v%d\n",
-version_id, idstr, se->version_id);
+error_report("savevm: unsupported version %d for '%s' v%d",
+ version_id, idstr, se->version_id);
 ret = -EINVAL;
 goto out;
 }
@@ -948,8 +949,8 @@ int qemu_loadvm_state(QEMUFile *f)
 
 ret = vmstate_load(f, le->se, le->version_id);
 if (ret < 0) {
-fprintf(stderr, "qemu: warning: error while loading state for 
instance 0x%x of device '%s'\n",
-instance_id, idstr);
+error_report("error while loading state for instance 0x%x of"
+ " device '%s'", instance_id, idstr);
 goto out;
 }
 break;
@@ -963,20 +964,20 @@ int qemu_loadvm_state(QEMUFile *f)
 }
 }
 if (le == NULL) {
-fprintf(stderr, "Unknown savevm section %d\n", section_id);
+error_report("Unknown savevm section %d", section_id);
 ret = -EINVAL;
 goto out;
 }
 
 ret = vmstate_load(f, le->se, le->version_id);
 if (ret < 0) {
-fprintf(stderr, "qemu: warning: error while loading state 
section id %d\n",
-section_id);
+error_report("error while loading state section id %d(%s)",
+ section_id, le->se->idstr);
 goto out;
 }
 break;
 default:
-fprintf(stderr, "Unknown savevm section type %d\n", section_type);
+error_report("Unknown savevm section type %d", section_type);
 ret = -EINVAL;
 goto out;
 }
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/3] Migration: Add lots of trace events

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Mostly on the load side, so that when we get a complaint about
a migration failure we can figure out what it didn't like.

Signed-off-by: Dr. David Alan Gilbert 
---
 migration/vmstate.c | 24 
 savevm.c| 10 +++---
 trace-events| 11 ++-
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 2c0b135..dae5dd6 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -73,16 +73,21 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
void *opaque, int version_id)
 {
 VMStateField *field = vmsd->fields;
-int ret;
+int ret = 0;
 
+trace_vmstate_load_state(vmsd->name, version_id);
 if (version_id > vmsd->version_id) {
+trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
 return -EINVAL;
 }
 if  (version_id < vmsd->minimum_version_id) {
 if (vmsd->load_state_old &&
 version_id >= vmsd->minimum_version_id_old) {
-return vmsd->load_state_old(f, opaque, version_id);
+ret = vmsd->load_state_old(f, opaque, version_id);
+trace_vmstate_load_state_end(vmsd->name, "old path", ret);
+return ret;
 }
+trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
 return -EINVAL;
 }
 if (vmsd->pre_load) {
@@ -92,6 +97,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription 
*vmsd,
 }
 }
 while (field->name) {
+trace_vmstate_load_state_field(vmsd->name, field->name);
 if ((field->field_exists &&
  field->field_exists(opaque, version_id)) ||
 (!field->field_exists &&
@@ -134,9 +140,10 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 return ret;
 }
 if (vmsd->post_load) {
-return vmsd->post_load(opaque, version_id);
+ret = vmsd->post_load(opaque, version_id);
 }
-return 0;
+trace_vmstate_load_state_end(vmsd->name, "end", ret);
+return ret;
 }
 
 void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
@@ -193,6 +200,8 @@ static const VMStateDescription *
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque)
 {
+trace_vmstate_subsection_load(vmsd->name);
+
 while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
 char idstr[256];
 int ret;
@@ -202,20 +211,24 @@ static int vmstate_subsection_load(QEMUFile *f, const 
VMStateDescription *vmsd,
 len = qemu_peek_byte(f, 1);
 if (len < strlen(vmsd->name) + 1) {
 /* subsection name has be be "section_name/a" */
+trace_vmstate_subsection_load_bad(vmsd->name, "(short)");
 return 0;
 }
 size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
 if (size != len) {
+trace_vmstate_subsection_load_bad(vmsd->name, "(peek fail)");
 return 0;
 }
 idstr[size] = 0;
 
 if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
+trace_vmstate_subsection_load_bad(vmsd->name, idstr);
 /* it don't have a valid subsection name */
 return 0;
 }
 sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
 if (sub_vmsd == NULL) {
+trace_vmstate_subsection_load_bad(vmsd->name, "(lookup)");
 return -ENOENT;
 }
 qemu_file_skip(f, 1); /* subsection */
@@ -225,9 +238,12 @@ static int vmstate_subsection_load(QEMUFile *f, const 
VMStateDescription *vmsd,
 
 ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
 if (ret) {
+trace_vmstate_subsection_load_bad(vmsd->name, "(child)");
 return ret;
 }
 }
+
+trace_vmstate_subsection_load_good(vmsd->name);
 return 0;
 }
 
diff --git a/savevm.c b/savevm.c
index 48c2396..1cc0f02 100644
--- a/savevm.c
+++ b/savevm.c
@@ -674,7 +674,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
 qemu_put_be32(f, se->section_id);
 
 ret = se->ops->save_live_iterate(f, se->opaque);
-trace_savevm_section_end(se->idstr, se->section_id);
+trace_savevm_section_end(se->idstr, se->section_id, ret);
 
 if (ret < 0) {
 qemu_file_set_error(f, ret);
@@ -714,7 +714,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
 qemu_put_be32(f, se->section_id);
 
 ret = se->ops->save_live_complete(f, se->opaque);
-trace_savevm_section_end(se->idstr, se->section_id);
+trace_savevm_section_end(se->idstr, se->section_id, ret);
 if (ret < 0) {
 qemu_file_set_error(f, ret);
 return;
@@ -741,7 +741,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
 qemu_put_be32(f, se->version_id);
 
 vmstate_save(f, se);
-trace_savevm_se

[Qemu-devel] [PATCH v2 3/3] Print errors in some of the early migration failure cases.

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Signed-off-by: Dr. David Alan Gilbert 
---
 savevm.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/savevm.c b/savevm.c
index 1cc0f02..98895fe 100644
--- a/savevm.c
+++ b/savevm.c
@@ -883,16 +883,20 @@ int qemu_loadvm_state(QEMUFile *f)
 QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
 QLIST_HEAD_INITIALIZER(loadvm_handlers);
 LoadStateEntry *le, *new_le;
+Error *local_err = NULL;
 uint8_t section_type;
 unsigned int v;
 int ret;
 
-if (qemu_savevm_state_blocked(NULL)) {
+if (qemu_savevm_state_blocked(&local_err)) {
+error_report("%s", error_get_pretty(local_err));
+error_free(local_err);
 return -EINVAL;
 }
 
 v = qemu_get_be32(f);
 if (v != QEMU_VM_FILE_MAGIC) {
+error_report("Not a migration stream");
 return -EINVAL;
 }
 
@@ -902,6 +906,7 @@ int qemu_loadvm_state(QEMUFile *f)
 return -ENOTSUP;
 }
 if (v != QEMU_VM_FILE_VERSION) {
+error_report("Unsupported migration stream version");
 return -ENOTSUP;
 }
 
-- 
2.1.0

Re: [Qemu-devel] kvmclock, Migration, and NTP clock jitter

[Mohammed Gamal]

On Fri, Jan 16, 2015 at 11:21 AM, Mohammed Gamal <
mohammed.ga...@profitbricks.com> wrote:

> On Thu, Jan 15, 2015 at 06:27:54PM +0100, Paolo Bonzini wrote:
> >
> >
> > On 15/01/2015 17:39, Mohammed Gamal wrote:
> > > The increase in the jitter and offset values is well within the 500 ppm
> > > frequency tolerance limit, and therefore are easily corrected by
> > > subsequent NTP clock sync events, but some live migrations do cause
> much
> > > higher jitter and offset jumps, which can not be corrected by NTP and
> > > cause the time to go way off. Any idea why this is the case?
> >
> > It might be fixed in QEMU 2.2.
> >
> > See https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01239.html
> >
> > Paolo
>
> Hi Paolo,
>
> I did try to backport these patches to qemu 1.2. However, migrations
> resulted in *higher* jitter and offset values (i.e. in the order of 100+
> ppm).
> I am not sure if I've done the backporting correctly though. Here are my
> patches on top of the qemu 1.2 stable tree.
>

Anyone?

Re: [Qemu-devel] [PATCH 0/6] linux-user: Fix various clang warnings

[Riku Voipio]

Hi,

On Tue, Jan 20, 2015 at 02:54:20PM +, Peter Maydell wrote:
> Ping!

I've just updated my linux-user-for-upstream tree. I'll get it
reviewed/tested tonight.

> thanks
> -- PMM
> 
> On 8 January 2015 at 12:19, Peter Maydell  wrote:
> > This patchset fixes warnings produced by clang in the linux-user code.
> > Mostly this is deleting or marking unused functions/data, but it
> > does include a genuine bugfix for Alpha.
> >
> > I think that this means I have patches out on the list now for
> > all the clang warnings we currently generate; maybe some day soon
> > we can enable warnings-as-errors...
> >
> > Peter Maydell (6):
> >   linux-user/signal.c: Remove current_exec_domain_sig()
> >   linux-user/alpha: Add define for NR_shmat to enable shmat syscall
> >   linux-user/arm/nwfpe: Delete unused aCC array
> >   linux-user/main.c: Call cpu_exec_start/end on all target archs
> >   linux-user/main.c: Mark end_exclusive() as possibly unused
> >   linux-user/signal.c: Remove unnecessary wrapper copy_siginfo_to_user
> >
> >  linux-user/alpha/syscall_nr.h   |  4 +++
> >  linux-user/arm/nwfpe/fpopcode.c | 22 ---
> >  linux-user/main.c   | 20 +-
> >  linux-user/signal.c | 59 
> > -
> >  4 files changed, 40 insertions(+), 65 deletions(-)

[Qemu-devel] [question] incremental backup a running vm

[Zhang Haoyu]

Hi,

Does drive_mirror support incremental backup a running vm?
Or other mechanism does?

incremental backup a running vm requirements:
First time backup, all of the allocated data will be mirrored to destination,
then a copied bitmap will be saved to a file, then the bitmap file will log 
dirty for
the changed data.
Next time backup, only the dirty data will be mirrored to destination.
Even the VM shutdown and start after several days,
the bitmap will be loaded while starting vm.
Any ideas?

Thanks,
Zhang Haoyu

Re: [Qemu-devel] [PATCH v0 2/2] pc-dimm: Make pc_existing_dimms_capacity global

[Igor Mammedov]

On Wed, 21 Jan 2015 14:28:18 +0530
Bharata B Rao  wrote:

> On Tue, Jan 20, 2015 at 11:18:37AM +0100, Igor Mammedov wrote:
> > On Mon, 12 Jan 2015 09:32:34 +0530
> > Bharata B Rao  wrote:
> > 
> > > Move pc_existing_dimms_capacity() to pc-dimm.c since it would be needed
> > > by PowerPC memory hotplug code too.
> > > 
> > > Signed-off-by: Bharata B Rao 
> > > ---
> > >  hw/i386/pc.c | 24 
> > >  hw/mem/pc-dimm.c | 25 +
> > >  include/hw/mem/pc-dimm.h |  1 +
> > >  3 files changed, 26 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 125cf0a..2ec45a4 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1552,30 +1552,6 @@ void qemu_register_pc_machine(QEMUMachine *m)
> > >  g_free(name);
> > >  }
> > >  
> > > -static int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > > -{
> > > -Error *local_err = NULL;
> > > -uint64_t *size = opaque;
> > > -
> > > -if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> > > -DeviceState *dev = DEVICE(obj);
> > > -
> > > -if (dev->realized) {
> > > -(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> > > -&local_err);
> > > -}
> > > -
> > > -if (local_err) {
> > > -qerror_report_err(local_err);
> > > -error_free(local_err);
> > > -return 1;
> > > -}
> > > -}
> > > -
> > > -object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > > -return 0;
> > > -}
> > > -
> > >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> > >   DeviceState *dev, Error **errp)
> > >  {
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index d431834..f02ce6e 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -22,6 +22,31 @@
> > >  #include "qemu/config-file.h"
> > >  #include "qapi/visitor.h"
> > >  #include "qemu/range.h"
> > > +#include "qapi/qmp/qerror.h"
> > > +
> > > +int pc_existing_dimms_capacity(Object *obj, void *opaque)
> > > +{
> > since you are making it API, could you pass Error **errp argument
> > and deal with error in caller?
> > Along with it you can make function return void.
> 
> I don't think that can be done because pc_existing_dimms_capacity() calls
> itself recursively via object_child_foreach() and hence its signature
> if fixed.
how about:

typedef struct somename {
uint64_t size;
Error **errp;
} somename;


somename *foo = opaque;

...

and if you'd be able to hide this structure from API caller,
then function could return it's existing capacity via its return value.
For example:

static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
{
...
}

/* API */
uint64_t pc_existing_dimms_capacity(Error **errp)
{
  ...
  object_child_foreach(obj, pc_existing_dimms_capacity_internal, opaque);
  ...
}
> > > +
> > > +if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> > > +DeviceState *dev = DEVICE(obj);
> > > +
> > > +if (dev->realized) {
> > > +(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> > > +&local_err);
> > > +}
> > > +
> > > +if (local_err) {
> > > +qerror_report_err(local_err);
> > > +error_free(local_err);
> > > +return 1;
> > > +}
> > > +}
> > > +
> > > +object_child_foreach(obj, pc_existing_dimms_capacity, opaque);
> > > +return 0;
> > > +}
> 

Re: [Qemu-devel] [question] incremental backup a running vm

[Paolo Bonzini]

On 21/01/2015 11:32, Zhang Haoyu wrote:
> Hi,
> 
> Does drive_mirror support incremental backup a running vm?
> Or other mechanism does?
> 
> incremental backup a running vm requirements:
> First time backup, all of the allocated data will be mirrored to destination,
> then a copied bitmap will be saved to a file, then the bitmap file will log 
> dirty for
> the changed data.
> Next time backup, only the dirty data will be mirrored to destination.
> Even the VM shutdown and start after several days,
> the bitmap will be loaded while starting vm.
> Any ideas?

Drive-mirror is for storage migration.  For backup there is another job,
drive-backup.  drive-backup copies a point-in-time snapshot of one or
more disks corresponding to when the backup was started.

Incremental backup is being worked on.  You can see patches on the list.

Paolo

Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Paolo Bonzini]

On 21/01/2015 03:14, Gonglei wrote:
> On 2015/1/21 0:10, Paolo Bonzini wrote:
> 
>>
>>
>> On 19/01/2015 14:23, arei.gong...@huawei.com wrote:
>>> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState 
>>> *dev, char *p, int size)
>>>  d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>  }
>>>  if (d) {
>>> +l += snprintf(p + l, size - l, "%s/", d);
>>> +g_free(d);
>>> +}
>>> +
>>> +d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
>>
>> This changes preexisting behavior.  If d was true, you wouldn't go down
>> the following else.  Now it does.
>>
> 
> On the face of things, it is. But actually they are equal. Please notice I 
> added a "/" at the
> end p, and then the function can return if d was NULL.
>   l += snprintf(p + l, size - l, "%s/", d);

But in this case I think the "return l" should become unconditional.  
It should move out of the "else".

>> I was thinking it should be handled though the "suffix" argument to
>> add_boot_device_path, but that's harder now that the suffix has to be
>> passed to device_add_bootindex_property.
>>
> 
> Yes.
> 
>> Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
>> get_boot_devices_list, and convert non-NULL suffixes to implementations
>> of FWPathProvider?
> 
> Maybe your meaning is "convert NULL suffixes to implementations
>  of FWPathProvider"?  Something like below:

No, I meant non-NULL.

In the beginning it can be something like this:

diff --git a/bootdevice.c b/bootdevice.c
index 5914417..916bfb7 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -210,7 +210,8 @@ char *get_boot_devices_list(size_t *size, bool 
ignore_suffixes)
 char *list = NULL;
 
 QTAILQ_FOREACH(i, &fw_boot_order, link) {
-char *devpath = NULL, *bootpath;
+char *devpath = NULL,  *suffix = NULL;
+char *bootpath;
 size_t len;
 
 if (i->dev) {
@@ -218,21 +219,22 @@ char *get_boot_devices_list(size_t *size, bool 
ignore_suffixes)
 assert(devpath);
 }
 
-if (i->suffix && !ignore_suffixes && devpath) {
-size_t bootpathlen = strlen(devpath) + strlen(i->suffix) + 1;
-
-bootpath = g_malloc(bootpathlen);
-snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
-g_free(devpath);
-} else if (devpath) {
-bootpath = devpath;
-} else if (!ignore_suffixes) {
-assert(i->suffix);
-bootpath = g_strdup(i->suffix);
-} else {
-bootpath = g_strdup("");
+if (!ignore_suffixes) {
+d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, 
i->dev);
+if (d) {
+assert(!i->suffix);
+suffix = d;
+} else {
+suffix = g_strdup(i->suffix);
+}
 }
 
+bootpath = g_strdup_printf("%s%s",
+   devpath ? devpath : "",
+   suffix ? suffix : "");
+g_free(devpath);
+g_free(suffix);
+
 if (total) {
 list[total-1] = '\n';
 }


Then as time permits the suffix can be phased out and replaced by
FWPathProvider on the device.

Paolo

> But I feel this more complicated. Isn't ?
> 
> Regards,
> -Gonglei
> 
>> Paolo
>>
>>> +if (d) {
>>>  l += snprintf(p + l, size - l, "%s", d);
>>>  g_free(d);
>>>  } else {
> 
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Alex Bennée]

Greg Bellows  writes:

> On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée  wrote:
>>
>> Greg Bellows  writes:
>>
>>> Add 32-bit to/from 64-bit register synchronization on register gets and 
>>> puts.
>>> Set EL1_32BIT feature flag passed to KVM
>>>
>>> Signed-off-by: Greg Bellows 

>>>  }
>>>
>>>  /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
>>> -val = pstate_read(env);
>>> +if (is_a64(env)) {
>>> +val = pstate_read(env);
>>> +} else {
>>> +val = cpsr_read(env);
>>> +}
>>
>> I know why we do this (especially given where my attempt ended up) but
>> perhaps we could at list have a single state aware accessor so we don't
>> end up duplicating this test all over the place?
>
> I'd happily add an accessor function, but I only found 1 other
> location that does this conditional so I'm not sure it is warranted.

The migration/serialisation code? Today one other, tomorrow just one more?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 5/6] linux-user/main.c: Mark end_exclusive() as possibly unused

[Peter Maydell]

On 21 January 2015 at 08:40, Thomas Huth  wrote:
> IMHO it might be better to add a proper #ifdef guard around that
> function. Consider that the calls to end_exclusive() might get removed
> completely one day, then you won't get a compiler warning about the
> unused function anymore if you used the attribute__((unused)) way.

Too painful, and these functions are likely going to go away/change
with the multithreaded-system-emulation support (I hope).

-- PMM

Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Peter Maydell]

On 21 January 2015 at 10:54, Alex Bennée  wrote:
>
> Greg Bellows  writes:
>
>> On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée  wrote:
>>> I know why we do this (especially given where my attempt ended up) but
>>> perhaps we could at list have a single state aware accessor so we don't
>>> end up duplicating this test all over the place?
>>
>> I'd happily add an accessor function, but I only found 1 other
>> location that does this conditional so I'm not sure it is warranted.
>
> The migration/serialisation code? Today one other, tomorrow just one more?

Meh. I suggested to Greg that just inlining this at point of use
was the simplest approach. Maybe one day we'll clean this stuff
up but yet another accessor function doesn't seem too great either.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Alex Bennée]

Greg Bellows  writes:

> Thanks Alex comments inline
>

>>
>> Aren't we leaking here? strtok returns the next token (or NULL) so don't
>> we loose the original ptr?
>>
>>
> ​As I understand it, strtok uses static pointers to track the location
> within an existing string rather than allocating storage that the user must
> free.  This is apparently what makes the version I used non-reentrant.​  In
> which case, there should not be an leak due to its use.

Yeah - I realised this after re-reading the man page. Non-re-entrant
isn't a particular problem these days but it still feels dirty

>> Also while using glib you might want to consider using glib's own
>> tokenising functions (e.g. g_strsplit). This has the advantage of having
>> helper functions like g_strfreev() which can clean-up everything in one go.
>>
>
> ​I certainly can use the glib version, but in this case I did not see the
> advantage. In fact, it actually may be less performant​ to use the glib
> version given it needs to allocate/free memory.  I am fine either way if
> anyone feels strongly.

I suspect this discussion is trumped by moving to the feat_foo=on/off
parsing style referenced elsewhere so we can use common code.

-- 
Alex Bennée

[Qemu-devel] [PATCH] qemu-ga-win: Fail loudly on bare 'set-time'

[Michal Privoznik]

The command is not implemented correctly yet. The documentation allows
to not pass any value to set, in which case the time is re-read from
RTC. However, reading CMOS on Windows is not trivial to implement. So
instead of pretending we've set the correct time, fail explicitly.

Signed-off-by: Michal Privoznik 
---
 qga/commands-win32.c | 44 ++--
 qga/qapi-schema.json |  5 -
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3bcbeae..4fe45f0 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -395,31 +395,31 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, 
Error **errp)
 FILETIME tf;
 LONGLONG time;
 
-if (has_time) {
-/* Okay, user passed a time to set. Validate it. */
-if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
-error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
-return;
-}
+if (!has_time) {
+/* Unfortunately, Windows libraries don't provide an easy way to access
+ * RTC yet:
+ *
+ * https://msdn.microsoft.com/en-us/library/aa908981.aspx
+ */
+error_setg(errp, "Time argument is required on this platform");
+return;
+}
 
-time = time_ns / 100 + W32_FT_OFFSET;
+/* Validate time passed by user. */
+if (time_ns < 0 || time_ns / 100 > INT64_MAX - W32_FT_OFFSET) {
+error_setg(errp, "Time %" PRId64 "is invalid", time_ns);
+return;
+}
 
-tf.dwLowDateTime = (DWORD) time;
-tf.dwHighDateTime = (DWORD) (time >> 32);
+time = time_ns / 100 + W32_FT_OFFSET;
 
-if (!FileTimeToSystemTime(&tf, &ts)) {
-error_setg(errp, "Failed to convert system time %d",
-   (int)GetLastError());
-return;
-}
-} else {
-/* Otherwise read the time from RTC which contains the correct value.
- * Hopefully. */
-GetSystemTime(&ts);
-if (ts.wYear < 1601 || ts.wYear > 30827) {
-error_setg(errp, "Failed to get time");
-return;
-}
+tf.dwLowDateTime = (DWORD) time;
+tf.dwHighDateTime = (DWORD) (time >> 32);
+
+if (!FileTimeToSystemTime(&tf, &ts)) {
+error_setg(errp, "Failed to convert system time %d",
+   (int)GetLastError());
+return;
 }
 
 acquire_privilege(SE_SYSTEMTIME_NAME, &local_err);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 376e79f..ce73dd3 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -121,7 +121,10 @@
 # given value, then sets the Hardware Clock (RTC) to the
 # current System Time. This will make it easier for a guest
 # to resynchronize without waiting for NTP. If no @time is
-# specified, then the time to set is read from RTC.
+# specified, then the time to set is read from RTC. However,
+# this may not be supported on all platforms (i.e. Windows).
+# If that's the case users are advised to always pass a
+# value.
 #
 # @time: #optional time of nanoseconds, relative to the Epoch
 #of 1970-01-01 in UTC.
-- 
2.0.5

Re: [Qemu-devel] [PATCH RFC v6 10/20] s390x/virtio-ccw: add virtio set-revision call

[Cornelia Huck]

On Tue, 20 Jan 2015 11:08:24 +
Stefan Hajnoczi  wrote:

> On Thu, Dec 11, 2014 at 02:25:12PM +0100, Cornelia Huck wrote:
> > @@ -608,6 +631,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >  }
> >  }
> >  break;
> > +case CCW_CMD_SET_VIRTIO_REV:
> > +len = sizeof(revinfo);
> > +if (ccw.count < len || (check_len && ccw.count > len)) {
> > +ret = -EINVAL;
> > +break;
> > +}
> > +if (!ccw.cda) {
> > +ret = -EFAULT;
> > +break;
> > +}
> > +cpu_physical_memory_read(ccw.cda, &revinfo, len);
> > +if (dev->revision >= 0 ||
> > +revinfo.revision > virtio_ccw_rev_max(dev)) {
> 
> In the next patch virtio_ccw_handle_set_vq() uses big-endian memory
> access functions to load a struct from guest memory.
> 
> Here you just copy the struct in without byteswaps.
> 
> Are the byteswaps missing here?  (I guess this normally runs big-endian
> guests on big-endian hosts so it's not noticable.)

Indeed, these are supposed to be big-endian. I'll double check the
other payloads.

Thanks for spotting this!

Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

[Igor Mammedov]

On Tue, 20 Jan 2015 16:45:19 -0600
Greg Bellows  wrote:

> On Tue, Jan 20, 2015 at 10:25 AM, Igor Mammedov  wrote:
> > On Tue, 20 Jan 2015 16:08:09 +
> > Peter Maydell  wrote:
> >
> >> On 20 January 2015 at 15:59, Igor Mammedov  wrote:
> >> > On Tue, 20 Jan 2015 15:34:23 +
> >> > Peter Maydell  wrote:
> >> >
> >> >> On 20 January 2015 at 15:22, Igor Mammedov  wrote:
> >> >> > Please do not use legacy +-feature format and support only foo=val 
> >> >> > format.
> >> >> > Other targets have it only for to being able support legacy setups
> >> >> > which use +- format.
> >> >>
> >> >> I thought this was the standard format for CPU features. Do you
> >> >> have an example of a CPU feature being set using foo=val format?
> >> > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and
> >> > in addition to it we ca use canonized format for generic properties
> >> > like, foo1=on,foo2=off,foo3=on
> >> >
> >> > We try to move out of legacy format, so that it would be possible
> >> > to reuse generic property parsing infrastructure like with any
> >> > device object. That would allow to use -device/device_add for CPUs.
> >>
> >> -device/-device_add for CPUs is pretty fraught in the general
> >> case because there's no obvious place to plug them and have
> >> them be wired up properly.
> > That depends on CPU of-cause, but we are close to having device_add
> > working with x86 CPUs.
> >
> >> You'd need to use -global for CPU
> >> properties, which is a nightmare...
> > mine thoughts on it were that '-cpu type,feat...' would  eventually
> > do conversion of features to global properties transparently for
> > user using target specific cc->parse_features() callback. Which
> > Greg could actually do here. We would happy to reuse it with x86 CPUs.
> >
> >>
> >> Anyway, I'm not particularly attached to the exact command
> >> line syntax we've used here -- I was just looking for "we have
> >> a CPU property, and use the same syntax for specifying CPU
> >> feature enable/disable that other CPUs do"...
> > Then '-cpu arm_foo,featX=on/off' should do the job.
> >
> >
> >>
> >> -- PMM
> >
> 
> For now I went with the "-cpu arm_foo,aarch64=off" approach so as to
> not complicate it right now with adding full-blown CPU properties.
I see that there is quite a mess with feature bits and properties
on ARM target already, so I won't ask to rewrite all of it to.
But since you need control only one feature than make it property
and set related feature bit from property setter or like it's done
for arm_cpu_has_el3_property.
Then in arm_cpu_parse_features() you can just do generic setting:

 object_property_parse(OBJECT(cpu), val, featurename, &local_err);

for foo=on/off format and forget about touching it again,
further down the road one would just need to add a property to
mange needed feature bits.

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Ian Jackson]

Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property to 
support IGD GFX passthrough"):
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,gfx_passthru=on". This need
> to bring several changes on tool side.

Has the corresponding patch to qemu-upstream been accepted yet ?

I'd like to see a confirmation from the qemu side that this is going
into their tree and that the command-line option syntax has been
agreed.

Thanks,
Ian.

Re: [Qemu-devel] [PATCH v3 1/6] pci: reorganize QEMU_PCI_CAP_*

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:23AM +0800, Hu Tao wrote:
> This makes code more readable.
> 
> Signed-off-by: Hu Tao 
> Reviewed-by: Marcel Apfelbaum 
> ---
>  include/hw/pci/pci.h | 39 ---
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index c352c7b..b18759a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -142,25 +142,26 @@ enum {
>  
>  /* Bits in cap_present field. */
>  enum {
> -QEMU_PCI_CAP_MSI = 0x1,
> -QEMU_PCI_CAP_MSIX = 0x2,
> -QEMU_PCI_CAP_EXPRESS = 0x4,
> -
> -/* multifunction capable device */
> -#define QEMU_PCI_CAP_MULTIFUNCTION_BITNR3
> -QEMU_PCI_CAP_MULTIFUNCTION = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
> -
> -/* command register SERR bit enabled */
> -#define QEMU_PCI_CAP_SERR_BITNR 4
> -QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> -/* Standard hot plug controller. */
> -#define QEMU_PCI_SHPC_BITNR 5
> -QEMU_PCI_CAP_SHPC = (1 << QEMU_PCI_SHPC_BITNR),
> -#define QEMU_PCI_SLOTID_BITNR 6
> -QEMU_PCI_CAP_SLOTID = (1 << QEMU_PCI_SLOTID_BITNR),
> -/* PCI Express capability - Power Controller Present */
> -#define QEMU_PCIE_SLTCAP_PCP_BITNR 7
> -QEMU_PCIE_SLTCAP_PCP = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
> +QEMU_PCI_CAP_MSI_BITNR = 0,
> +QEMU_PCI_CAP_MSIX_BITNR,
> +QEMU_PCI_CAP_EXPRESS_BITNR,
> +QEMU_PCI_CAP_MULTIFUNCTION_BITNR, /* multifunction capable device */
> +QEMU_PCI_CAP_SERR_BITNR,  /* command register SERR bit enabled */
> +QEMU_PCI_SHPC_BITNR,  /* Standard hot plug controller */
> +QEMU_PCI_SLOTID_BITNR,
> +QEMU_PCIE_SLTCAP_PCP_BITNR, /* PCIE Slot - Power Controller
>+Present */

Pls shorten so it fits on one line: /* PCIE Slot - Power Controller Present */

> +};
> +
> +enum {
> +QEMU_PCI_CAP_MSI= (1 << QEMU_PCI_CAP_MSI_BITNR),
> +QEMU_PCI_CAP_MSIX   = (1 << QEMU_PCI_CAP_MSIX_BITNR),
> +QEMU_PCI_CAP_EXPRESS= (1 << QEMU_PCI_CAP_EXPRESS_BITNR),
> +QEMU_PCI_CAP_MULTIFUNCTION  = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
> +QEMU_PCI_CAP_SERR   = (1 << QEMU_PCI_CAP_SERR_BITNR),
> +QEMU_PCI_CAP_SHPC   = (1 << QEMU_PCI_SHPC_BITNR),
> +QEMU_PCI_CAP_SLOTID = (1 << QEMU_PCI_SLOTID_BITNR),
> +QEMU_PCIE_SLTCAP_PCP= (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
>  };
>  
>  #define TYPE_PCI_DEVICE "pci-device"
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 2/6] pci: introduce pci_host_config_enabled()

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:24AM +0800, Hu Tao wrote:
> This makes code more readable.
> 
> Signed-off-by: Hu Tao 
> Reviewed-by: Marcel Apfelbaum 
> ---
>  hw/mips/gt64xxx_pci.c | 4 ++--
>  hw/pci/pci_host.c | 5 +++--
>  include/hw/pci/pci_host.h | 5 +
>  3 files changed, 10 insertions(+), 4 deletions(-)

We have a ton of other places hard-coding 1<<31,
why special-case these?

> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 1f2fe5f..f118c9c 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
>  if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>  val = bswap32(val);
>  }
> -if (phb->config_reg & (1u << 31)) {
> +if (pci_host_config_enabled(phb)) {
>  pci_data_write(phb->bus, phb->config_reg, val, 4);
>  }
>  break;
> @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
>  val = phb->config_reg;
>  break;
>  case GT_PCI0_CFGDATA:
> -if (!(phb->config_reg & (1 << 31))) {
> +if (!pci_host_config_enabled(phb)) {
>  val = 0x;
>  } else {
>  val = pci_data_read(phb->bus, phb->config_reg, 4);
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..9bc47d8 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
>  PCIHostState *s = opaque;
>  PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n",
>  addr, len, (unsigned)val);
> -if (s->config_reg & (1u << 31))
> +if (pci_host_config_enabled(s)) {
>  pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
> +}
>  }
>  
>  static uint64_t pci_host_data_read(void *opaque,
> @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
>  {
>  PCIHostState *s = opaque;
>  uint32_t val;
> -if (!(s->config_reg & (1U << 31))) {
> +if (!pci_host_config_enabled(s)) {
>  return 0x;
>  }
>  val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index ba31595..b48791d 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -65,6 +65,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, 
> uint32_t addr,
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  
> +static inline bool pci_host_config_enabled(struct PCIHostState *pci_host)
> +{
> +return pci_host->config_reg & (1U << 31);
> +}
> +

Better:

#define PCI_HOST_CONFIG_ENABLE (1U << 31)

then everyone can just do s->config_reg & PCI_HOST_CONFIG_ENABLE

better as it'll work for code like this:
phb->config_reg = (pciaddr) | (1u << 31);


>  extern const MemoryRegionOps pci_host_conf_le_ops;
>  extern const MemoryRegionOps pci_host_conf_be_ops;
>  extern const MemoryRegionOps pci_host_data_le_ops;
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH RFC v6 10/20] s390x/virtio-ccw: add virtio set-revision call

[Thomas Huth]

On Wed, 21 Jan 2015 12:23:18 +0100
Cornelia Huck  wrote:

> On Tue, 20 Jan 2015 11:08:24 +
> Stefan Hajnoczi  wrote:
> 
> > On Thu, Dec 11, 2014 at 02:25:12PM +0100, Cornelia Huck wrote:
> > > @@ -608,6 +631,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> > >  }
> > >  }
> > >  break;
> > > +case CCW_CMD_SET_VIRTIO_REV:
> > > +len = sizeof(revinfo);
> > > +if (ccw.count < len || (check_len && ccw.count > len)) {
> > > +ret = -EINVAL;
> > > +break;
> > > +}
> > > +if (!ccw.cda) {
> > > +ret = -EFAULT;
> > > +break;
> > > +}
> > > +cpu_physical_memory_read(ccw.cda, &revinfo, len);
> > > +if (dev->revision >= 0 ||
> > > +revinfo.revision > virtio_ccw_rev_max(dev)) {
> > 
> > In the next patch virtio_ccw_handle_set_vq() uses big-endian memory
> > access functions to load a struct from guest memory.
> > 
> > Here you just copy the struct in without byteswaps.
> > 
> > Are the byteswaps missing here?  (I guess this normally runs big-endian
> > guests on big-endian hosts so it's not noticable.)
> 
> Indeed, these are supposed to be big-endian. I'll double check the
> other payloads.

Right. Cornelia, can you take care of this or shall I rework the patch?

NB: Actually, there are a couple of "XXX config space endianness"
comments in that virtio_ccw_cb() function, so there are likely a bunch
of problems when this code should be run on little endian hosts one day.
So far, this code only runs with big-endian guests on big-endian hosts
since the virtio-ccw machine is currently KVM-only as far as I know,
that's likely why nobody complained about this yet.

 Thomas

Re: [Qemu-devel] [PATCH v3 3/6] pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:25AM +0800, Hu Tao wrote:
> PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA are
> defined in PCI specification, so move them to common place.

they are listed in the spec, but they are still PC specific.
Spec says:

Two DWORD I/O locations are used to generate configuration transactions
>   for PC-AT compatible systems.
The first DWORD location (CF8h) references a read/write register
that is named CONFIG_ADDRESS. The second DWORD address (CFCh) 
references a
read/write register named CONFIG_DATA.


So this should at least have PC_ included in name, and probably go into
some pc - specific header.


> Signed-off-by: Hu Tao 
> Reviewed-by: Marcel Apfelbaum 
> ---
>  hw/pci-host/piix.c|  8 
>  hw/pci-host/prep.c|  6 --
>  hw/pci-host/q35.c |  8 
>  include/hw/pci-host/q35.h |  3 ---
>  include/hw/pci/pci_host.h |  5 +
>  tests/libqos/pci-pc.c | 25 +
>  6 files changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 1530038..76f3757 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev, 
> Error **errp)
>  PCIHostState *s = PCI_HOST_BRIDGE(dev);
>  SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
> -sysbus_init_ioports(sbd, 0xcf8, 4);
> +sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, &s->conf_mem);
> +sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
>  
> -sysbus_add_io(sbd, 0xcfc, &s->data_mem);
> -sysbus_init_ioports(sbd, 0xcfc, 4);
> +sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, &s->data_mem);
> +sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
>  }
>  
>  static int i440fx_initfn(PCIDevice *dev)
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 1de3681..2ae21ad 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -228,11 +228,13 @@ static void raven_pcihost_realizefn(DeviceState *d, 
> Error **errp)
>  
>  memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
>"pci-conf-idx", 4);
> -memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
> +memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_ADDR,
> +&h->conf_mem);
>  
>  memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
>"pci-conf-data", 4);
> -memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
> +memory_region_add_subregion(&s->pci_io, PCI_HOST_BRIDGE_CONFIG_DATA,
> +&h->data_mem);
>  
>  memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_pci_io_ops, s,
>"pciio", 0x0040);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..666afea 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error 
> **errp)
>  Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>  SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> -sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
> +sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
>  
> -sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> -sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
> +sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
> +sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
>  
>  pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
> s->mch.pci_address_space, s->mch.address_space_io,
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 025d6e6..3a026b0 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
>  /* PCI configuration */
>  #define MCH_HOST_BRIDGE"MCH"
>  
> -#define MCH_HOST_BRIDGE_CONFIG_ADDR0xcf8
> -#define MCH_HOST_BRIDGE_CONFIG_DATA0xcfc
> -
>  /* D0:F0 configuration space */
>  #define MCH_HOST_BRIDGE_REVISION_DEFAULT   0x0
>  
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index b48791d..2bae45a 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -30,6 +30,11 @@
>  
>  #include "hw/sysbus.h"
>  
> +/* PCI configuration */
> +
> +#define PCI_HOST_BRIDGE_CONFIG_ADDR  0xcf8
> +#define PCI_HOST_BRIDGE_CONFIG_DATA  0xcfc
> +
>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
>  #define PCI_HOST_BRIDGE(obj) \
>  OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
> diff --git a/tests/libqos/pci-pc.c b/te

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Markus Armbruster]

Markus Armbruster  writes:

> Frank Blaschka  writes:
>
>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
>>> Markus Armbruster  writes:
>>> 
>>> > Cornelia Huck  writes:
>>> >
>>> >> On Tue, 20 Jan 2015 10:45:41 +0100
>>> >> Markus Armbruster  wrote:
>>> >>
>>> >>> This patch makes Coverity unhappy:
>>> >>> 
>>> >>> *** CID 1264326:  Unintended sign extension  (SIGN_EXTENSION)
>>> >>> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call()
>>> >>> 781 stq_p(&fib.pal, pbdev->pal);
>>> >>> 782 stq_p(&fib.iota, pbdev->g_iota);
>>> >>> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr);
>>> >>> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
>>> >>> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
>>> >>> 786 
>>> >>> >>> CID 1264326:  Unintended sign extension  (SIGN_EXTENSION)
>>> >>> >>> Suspicious implicit sign extension: "pbdev->isc" with type
>>> >>> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc <<
>>> >>> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then
>>> >>> >>> sign-extended to type "unsigned long" (64 bits, unsigned).  If
>>> >>> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than
>>> >>> >>> 0x7FFF, the upper bits of the result will all be 1.
>>> >>> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
>>> >>> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
>>> >>> 789pbdev->routes.adapter.summary_offset;
>>> >>> 790 stw_p(&fib.data, data);
>>> >>> 791 
>>> >>> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>>> >>
>>> >> There's a fix for this (and the memory leak):
>>> >>
>>> >> http://marc.info/?l=qemu-devel&m=142124886620078&w=2
>>> >>
>>> >> The patch is sitting in my queue, will send with the next pile of s390x
>>> >> updates.
>>> >
>>> > I can't see how
>>> >
>>> > @@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, 
>>> > uint64_t fiba)
>>> >  data = (pbdev->isc << 28) | (pbdev->noi << 16) |
>>> > (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
>>> > pbdev->routes.adapter.summary_offset;
>>> > -stw_p(&fib.data, data);
>>> > +stl_p(&fib.data, data);
>>> >  
>>> >  if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>>> >  fib.fc |= 0x80;
>>> >
>>> > fixes the implicit sign extension within the assignment preceding it.
>>> > Let me explain it again real slow:
>>> >
>>> > 1. pbdev->isc gets promoted from uint8_t to int as operand of binary <<
>>> >(usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8)
>>> >
>>> > 2. The int result is shifted left 28 bits.  This can set the MSB.
>>> >
>>> > 3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted
>>> >left 16 bits.
>> uint16_t to int
>
> Yes, that's what I meant :)
>
>>> >
>>> > 4. The two shift results stay int and get ored.
>>> >
>>> > 5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left
>>> >8 bits.
>>> >
>>> > 6. The next or's left operand is the int result of 4 and the right
>>> >operant is the uint64_t result of 5.  Therefore, the left operand is
>>> >*sign-extended* from int to uint64_t.  This copies bit#7 of
>>> >pbdev->isc to bits#31..63.  Whoops.
>>> 
>>> I neglected to say: we don't currently use the upper 32 bits, and as
>>> long as we do that, the sign extension is harmless.  I'd recommend to
>>> avoid it all the same, for robustness, and to hush up Coverity.
>>>
>>
>> Hi Markus,
>>
>> thx for your explanation. I did not see a problem since ISC is not bigger
>> than 0x7 so MSB is never set. But the time I wrote the code I was not aware 
>> of
>> ind_offset is uint64_t since zpci defines only a 6 bit field for this value.
>
> Okay.
>
>> How can I avoid the sign extension and make Coverity happy?
>
> Casting pbdev->routes.adapter.ind_offset to uint32_t should do.  Then,
> all operands of | are either int (promoted from narrower unsigned type)
> or uint32_t (type cast).  Conversion from int to uint32_t won't
> sign-extend as long as int is at least 32 bits.  Surely the case for
> anything that can run QEMU.

Yup, it hushes up Coverity (I checked).

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 5ea13e5..2bed3f5 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -785,8 +785,8 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t 
fiba)
 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
 
 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
-   (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
-   pbdev->routes.adapter.summary_offset;
+   ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
+   (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
 stw_p(&fib.data, data);
 
 if (pbdev->fh >> ENABLE_BIT_OFFSET) {

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Ian Campbell]

On Wed, 2015-01-21 at 11:37 +, Ian Jackson wrote:
> Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property to 
> support IGD GFX passthrough"):
> > When we're working to support IGD GFX passthrough with qemu
> > upstream, instead of "-gfx_passthru" we'd like to make that
> > a machine option, "-machine xxx,gfx_passthru=on". This need
> > to bring several changes on tool side.
> 
> Has the corresponding patch to qemu-upstream been accepted yet ?
> 
> I'd like to see a confirmation from the qemu side that this is going
> into their tree and that the command-line option syntax has been
> agreed.

Do we need to detect old vs. new qemu to know when this option is valid?

Ian.

Re: [Qemu-devel] [PATCH v3 4/6] pci: remove the limit parameter of pci_host_config_read_common

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:26AM +0800, Hu Tao wrote:
> Since the limit parameter is always set to the size of pci device's
> configuration space, and we can determine the size from the type of pci
> device.
> 
> Signed-off-by: Hu Tao 

Not true e.g. for pci_data_read, is it?
Need a bit more comments to explain why it's
correct there.

> ---
>  hw/pci/pci_host.c | 15 +++
>  hw/pci/pcie_host.c|  9 +
>  hw/ppc/spapr_pci.c|  3 +--
>  include/hw/pci/pci_host.h |  2 +-
>  4 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 9bc47d8..2b11551 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -58,12 +58,20 @@ void pci_host_config_write_common(PCIDevice *pci_dev, 
> uint32_t addr,
>  }
>  
>  uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> - uint32_t limit, uint32_t len)
> + uint32_t len)
>  {
> +uint32_t limit = pci_config_size(pci_dev);
>  uint32_t ret;
>  
>  assert(len <= 4);
> -ret = pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
> +
> +if (limit <= addr) {
> +/* conventional pci device can be behind pcie-to-pci bridge.
> +   256 <= addr < 4K has no effects. */
> +ret = ~0x0;
> +} else {
> +ret = pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
> +}
>  trace_pci_cfg_read(pci_dev->name, PCI_SLOT(pci_dev->devfn),
> PCI_FUNC(pci_dev->devfn), addr, ret);
>  
> @@ -95,8 +103,7 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  return ~0x0;
>  }
>  
> -val = pci_host_config_read_common(pci_dev, config_addr,
> -  PCI_CONFIG_SPACE_SIZE, len);
> +val = pci_host_config_read_common(pci_dev, config_addr, len);
>  PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
>  __func__, pci_dev->name, config_addr, val, len);
>  
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index 3db038f..cf8587b 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -62,19 +62,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
>  PCIBus *s = e->pci.bus;
>  PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>  uint32_t addr;
> -uint32_t limit;
>  
>  if (!pci_dev) {
>  return ~0x0;
>  }
>  addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
> -limit = pci_config_size(pci_dev);
> -if (limit <= addr) {
> -/* conventional pci device can be behind pcie-to-pci bridge.
> -   256 <= addr < 4K has no effects. */
> -return ~0x0;
> -}
> -return pci_host_config_read_common(pci_dev, addr, limit, len);
> +return pci_host_config_read_common(pci_dev, addr, len);
>  }
>  
>  static const MemoryRegionOps pcie_mmcfg_ops = {
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 21b95b3..59c6608 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -105,8 +105,7 @@ static void finish_read_pci_config(sPAPREnvironment 
> *spapr, uint64_t buid,
>  return;
>  }
>  
> -val = pci_host_config_read_common(pci_dev, addr,
> -  pci_config_size(pci_dev), size);
> +val = pci_host_config_read_common(pci_dev, addr, size);
>  
>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  rtas_st(rets, 1, val);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 2bae45a..72a1b8b 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -65,7 +65,7 @@ typedef struct PCIHostBridgeClass {
>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>uint32_t limit, uint32_t val, uint32_t 
> len);
>  uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> - uint32_t limit, uint32_t len);
> + uint32_t len);
>  
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 5/6] pci: remove the limit parameter of pci_host_config_write_common

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:27AM +0800, Hu Tao wrote:
> Since the limit parameter is always set to the size of pci device's
> configuration space, and we can determine the size from the type of pci
> device.

Same comment.

> Signed-off-by: Hu Tao 
> ---
>  hw/pci/pci_host.c | 13 ++---
>  hw/pci/pcie_host.c|  9 +
>  hw/ppc/spapr_pci.c|  3 +--
>  include/hw/pci/pci_host.h |  2 +-
>  4 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 2b11551..4a59b0e 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -49,8 +49,16 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, 
> uint32_t addr)
>  }
>  
>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
> -  uint32_t limit, uint32_t val, uint32_t len)
> +  uint32_t val, uint32_t len)
>  {
> +uint32_t limit = pci_config_size(pci_dev);
> +
> +if (limit <= addr) {
> +/* conventional pci device can be behind pcie-to-pci bridge.
> +   256 <= addr < 4K has no effects. */
> +return;
> +}
> +
>  assert(len <= 4);
>  trace_pci_cfg_write(pci_dev->name, PCI_SLOT(pci_dev->devfn),
>  PCI_FUNC(pci_dev->devfn), addr, val);
> @@ -89,8 +97,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, 
> int len)
>  
>  PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
>  __func__, pci_dev->name, config_addr, val, len);
> -pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
> - val, len);
> +pci_host_config_write_common(pci_dev, config_addr, val, len);
>  }
>  
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index cf8587b..e3a2a80 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -39,19 +39,12 @@ static void pcie_mmcfg_data_write(void *opaque, hwaddr 
> mmcfg_addr,
>  PCIBus *s = e->pci.bus;
>  PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>  uint32_t addr;
> -uint32_t limit;
>  
>  if (!pci_dev) {
>  return;
>  }
>  addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
> -limit = pci_config_size(pci_dev);
> -if (limit <= addr) {
> -/* conventional pci device can be behind pcie-to-pci bridge.
> -   256 <= addr < 4K has no effects. */
> -return;
> -}
> -pci_host_config_write_common(pci_dev, addr, limit, val, len);
> +pci_host_config_write_common(pci_dev, addr, val, len);
>  }
>  
>  static uint64_t pcie_mmcfg_data_read(void *opaque,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 59c6608..8c566dd 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -171,8 +171,7 @@ static void finish_write_pci_config(sPAPREnvironment 
> *spapr, uint64_t buid,
>  return;
>  }
>  
> -pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
> - val, size);
> +pci_host_config_write_common(pci_dev, addr, val, size);
>  
>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 72a1b8b..67e007f 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -63,7 +63,7 @@ typedef struct PCIHostBridgeClass {
>  
>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
> -  uint32_t limit, uint32_t val, uint32_t 
> len);
> +  uint32_t val, uint32_t len);
>  uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
>   uint32_t len);
>  
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 10:20:28AM +0800, Hu Tao wrote:
> Introduce PCI_DEVFN_AUTO rather than using -1 in code.
> 
> Signed-off-by: Hu Tao 
> ---
>  hw/core/qdev-properties.c | 1 +
>  hw/pci/pci.c  | 5 ++---
>  include/hw/pci/pci.h  | 2 ++
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..df4ad14 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -6,6 +6,7 @@
>  #include "net/hub.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/char.h"
> +#include "hw/pci/pci.h"
>  
>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>Error **errp)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 371699c..73c7dec 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static void pcibus_reset(BusState *qbus);
>  
>  static Property pci_props[] = {
> -DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> +DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
>  DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>  DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>  DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> @@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>  address_space_destroy(&pci_dev->bus_master_as);
>  }
>  
> -/* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>   const char *name, int devfn)
>  {
> @@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev, PCIBus *bus,
>  PCIConfigWriteFunc *config_write = pc->config_write;
>  AddressSpace *dma_as;
>  
> -if (devfn < 0) {
> +if (devfn == PCI_DEVFN_AUTO) {

How do we know we found all places that do this?
How do we know parameter was already validate for
other values < 0?
Can you pls mention in the commit log why you think
it's safe?


>  for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>  devfn += PCI_FUNC_MAX) {
>  if (!bus->devices[devfn])
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b18759a..9206b12 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -164,6 +164,8 @@ enum {
>  QEMU_PCIE_SLTCAP_PCP= (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
>  };
>  
> +#define PCI_DEVFN_AUTO -1
> +
>  #define TYPE_PCI_DEVICE "pci-device"
>  #define PCI_DEVICE(obj) \
>   OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH 1/1] virtio: fix feature bit checks

[Thomas Huth]

On Fri, 12 Dec 2014 10:01:46 +0100
Cornelia Huck  wrote:

> Several places check against the feature bit number instead of against
> the feature bit. Fix them.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/scsi/virtio-scsi.c   | 2 +-
>  hw/virtio/dataplane/vring.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index ef48550..a44c410 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -144,7 +144,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
>   *
>   * TODO: always disable this workaround for virtio 1.0 devices.
>   */
> -if ((vdev->guest_features & VIRTIO_F_ANY_LAYOUT) == 0) {
> +if ((vdev->guest_features & (1 << VIRTIO_F_ANY_LAYOUT)) == 0) {
>  req_size = req->elem.out_sg[0].iov_len;
>  resp_size = req->elem.in_sg[0].iov_len;
>  }
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 61f6d83..78c6f45 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -133,12 +133,12 @@ bool vring_should_notify(VirtIODevice *vdev, Vring 
> *vring)
>   * interrupts. */
>  smp_mb();
> 
> -if ((vdev->guest_features & VIRTIO_F_NOTIFY_ON_EMPTY) &&
> +if ((vdev->guest_features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) &&
>  unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
>  return true;
>  }
> 
> -if (!(vdev->guest_features & VIRTIO_RING_F_EVENT_IDX)) {
> +if (!(vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX))) {
>  return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
>  }
>  old = vring->signalled_used;

Ping ... somebody taking care of this? It's a bug fix for current
code, so IMHO this patch should not wait for the inclusion of the
other virtio-1 stuff...

 Thomas

Re: [Qemu-devel] [PATCH v3 0/6] Some PCI related cleanup patches

[Michael S. Tsirkin]

On Wed, Jan 21, 2015 at 02:41:33PM +0800, Hu Tao wrote:
> ping...

At some point you said "will resend".

> On Thu, Dec 11, 2014 at 10:20:22AM +0800, Hu Tao wrote:
> > Hi,
> > 
> > This is v3 of PCI clenaup series. See each patch for the detail.
> > 
> > Regards,
> > Hu
> > 
> > changes:
> > 
> > v3:
> >   - rebase on top of 7fb8da2b886, all 5 patches applied cleanly.
> >   - new patch: pci: introduce PCI_DEVFN_AUTO
> > 
> > v2:
> >   - remove patch 3 from v1 which is incorrect.
> >   - rename defined macros as per Marcel's suggestion
> >   - place macros in pci_host.h as per Marcel's suggestion
> >   - new patch 'pci: reorganize QEMU_PCI_CAP_*'
> > 
> > Hu Tao (6):
> >   pci: reorganize QEMU_PCI_CAP_*
> >   pci: introduce pci_host_config_enabled()
> >   pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and
> > PCI_HOST_BRIDGE_CONFIG_DATA.
> >   pci: remove the limit parameter of pci_host_config_read_common
> >   pci: remove the limit parameter of pci_host_config_write_common
> >   pci: introduce PCI_DEVFN_AUTO
> > 
> >  hw/core/qdev-properties.c |  1 +
> >  hw/mips/gt64xxx_pci.c |  4 ++--
> >  hw/pci-host/piix.c|  8 
> >  hw/pci-host/prep.c|  6 --
> >  hw/pci-host/q35.c |  8 
> >  hw/pci/pci.c  |  5 ++---
> >  hw/pci/pci_host.c | 33 -
> >  hw/pci/pcie_host.c| 18 ++
> >  hw/ppc/spapr_pci.c|  6 ++
> >  include/hw/pci-host/q35.h |  3 ---
> >  include/hw/pci/pci.h  | 41 ++---
> >  include/hw/pci/pci_host.h | 14 --
> >  tests/libqos/pci-pc.c | 25 +
> >  13 files changed, 92 insertions(+), 80 deletions(-)
> > 
> > -- 
> > 1.9.3
> > 
> > 

Re: [Qemu-devel] [PATCH RFC v6 10/20] s390x/virtio-ccw: add virtio set-revision call

[Cornelia Huck]

On Wed, 21 Jan 2015 12:51:41 +0100
Thomas Huth  wrote:

> On Wed, 21 Jan 2015 12:23:18 +0100
> Cornelia Huck  wrote:
> 
> > On Tue, 20 Jan 2015 11:08:24 +
> > Stefan Hajnoczi  wrote:
> > 
> > > On Thu, Dec 11, 2014 at 02:25:12PM +0100, Cornelia Huck wrote:
> > > > @@ -608,6 +631,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> > > >  }
> > > >  }
> > > >  break;
> > > > +case CCW_CMD_SET_VIRTIO_REV:
> > > > +len = sizeof(revinfo);
> > > > +if (ccw.count < len || (check_len && ccw.count > len)) {
> > > > +ret = -EINVAL;
> > > > +break;
> > > > +}
> > > > +if (!ccw.cda) {
> > > > +ret = -EFAULT;
> > > > +break;
> > > > +}
> > > > +cpu_physical_memory_read(ccw.cda, &revinfo, len);
> > > > +if (dev->revision >= 0 ||
> > > > +revinfo.revision > virtio_ccw_rev_max(dev)) {
> > > 
> > > In the next patch virtio_ccw_handle_set_vq() uses big-endian memory
> > > access functions to load a struct from guest memory.
> > > 
> > > Here you just copy the struct in without byteswaps.
> > > 
> > > Are the byteswaps missing here?  (I guess this normally runs big-endian
> > > guests on big-endian hosts so it's not noticable.)
> > 
> > Indeed, these are supposed to be big-endian. I'll double check the
> > other payloads.
> 
> Right. Cornelia, can you take care of this or shall I rework the patch?

Currently already working on a patch :)

> NB: Actually, there are a couple of "XXX config space endianness"
> comments in that virtio_ccw_cb() function, so there are likely a bunch
> of problems when this code should be run on little endian hosts one day.
> So far, this code only runs with big-endian guests on big-endian hosts
> since the virtio-ccw machine is currently KVM-only as far as I know,
> that's likely why nobody complained about this yet.

The transport can't take care of the config space endianness, this
needs to be done by the individual devices. Probably best to simply
ditch the comments.

[Qemu-devel] [PATCH] s390x/pci: avoid sign extension in stpcifc

[Frank Blaschka]

this patch avoids sign extension and fixes a data conversion
bug in stpcifc. Both issues where found by Coverity.

Signed-off-by: Frank Blaschka 
---
 hw/s390x/s390-pci-inst.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 5ea13e5..4d4015c 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -785,9 +785,9 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t 
fiba)
 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
 
 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
-   (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
-   pbdev->routes.adapter.summary_offset;
-stw_p(&fib.data, data);
+   ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
+   (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
+stl_p(&fib.data, data);
 
 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
 fib.fc |= 0x80;
-- 
2.1.4

[Qemu-devel] Can we make better use of Coverity?

[Markus Armbruster]

We're using the Coverity Scan service[*].  We've put in some effort, and
we've gotten some mileage out of it, but I feel we could get more.

Judging from the report e-mail I have lying about, we're scanning about
once a month on average.  These reports cuts off after 20 new defects.
When there are more, which is common, people have to go to the web
dashboard to see them.  When I get one with ten, I may have a look, when
I get one "Showing 20 of 100 defect(s)", I despair of the task, and put
it off.

I also use Coverity locally (requires a license) with a derived model
for GLib to increase scanning power.  Since last July, the number of
defects I get that way has increased from ~400 to ~700.  Not quite as
bad as it sounds, because ~100 of the new ones are DEADCODE.  Still, it
suggests we haven't made much progress in reducing the number of defects
to a manageable level.

Some of the new defects are avoidable.  For instance, we've added 16
MISSING_BREAK.  Probably just missing /* fall through */, but we can't
be sure without examining each case.  Patch review fail.

At the other end of the spectrum, I see 36 new UNINIT defects.

I think we should scan much more regularly.  Once a week, full auto?

I further think we should send the e-mail report to the list, to have
more eyes on it.

Opinions?


[*] https://scan.coverity.com/projects/378

[Qemu-devel] [PATCH 0/2] clean up SYS_signalfd

[Laszlo Ersek]

SYS_signalfd is deprecated and absent on some Linux systems that do
support signalfd().

Also, in 2015 the signalfd() libc function should be available wherever
the underlying syscall(s) are available.

Drop SYS_signalfd and check for & use signalfd() directly.

Laszlo Ersek (2):
  qemu_signalfd_available(): remove function due to lack of callers
  signalfd(): modernize detection and use

 configure   | 11 ---
 include/qemu/compatfd.h |  1 -
 util/compatfd.c | 24 ++--
 3 files changed, 10 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] qemu_signalfd_available(): remove function due to lack of callers

[Laszlo Ersek]

Noone calls qemu_signalfd_available(), let's remove it.

Signed-off-by: Laszlo Ersek 
---
 include/qemu/compatfd.h |  1 -
 util/compatfd.c | 19 ---
 2 files changed, 20 deletions(-)

diff --git a/include/qemu/compatfd.h b/include/qemu/compatfd.h
index 6b04877..fc37915 100644
--- a/include/qemu/compatfd.h
+++ b/include/qemu/compatfd.h
@@ -39,6 +39,5 @@ struct qemu_signalfd_siginfo {
 };
 
 int qemu_signalfd(const sigset_t *mask);
-bool qemu_signalfd_available(void);
 
 #endif
diff --git a/util/compatfd.c b/util/compatfd.c
index 341ada6..e857150 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -108,22 +108,3 @@ int qemu_signalfd(const sigset_t *mask)
 
 return qemu_signalfd_compat(mask);
 }
-
-bool qemu_signalfd_available(void)
-{
-#ifdef CONFIG_SIGNALFD
-sigset_t mask;
-int fd;
-bool ok;
-sigemptyset(&mask);
-errno = 0;
-fd = syscall(SYS_signalfd, -1, &mask, _NSIG / 8);
-ok = (errno != ENOSYS);
-if (fd >= 0) {
-close(fd);
-}
-return ok;
-#else
-return false;
-#endif
-}
-- 
1.8.3.1

Re: [Qemu-devel] Can we make better use of Coverity?

[Peter Maydell]

On 21 January 2015 at 12:47, Markus Armbruster  wrote:
> We're using the Coverity Scan service[*].  We've put in some effort, and
> we've gotten some mileage out of it, but I feel we could get more.
>
> Judging from the report e-mail I have lying about, we're scanning about
> once a month on average.  These reports cuts off after 20 new defects.
> When there are more, which is common, people have to go to the web
> dashboard to see them.  When I get one with ten, I may have a look, when
> I get one "Showing 20 of 100 defect(s)", I despair of the task, and put
> it off.

Right, but coverity reports lots of stuff, much of which is either
wrong or just not very important. The interesting stats here are:
(1) the "high impact outstanding" buglist: we have just 33 of these
(2) the per-component lists: where somebody's been working on the
bug list for that component there are often not many bugs (there
are just 2 outstanding for "arm", for instance)

> I think we should scan much more regularly.  Once a week, full auto?

I think a regular automated scan would be useful, yes.

> I further think we should send the e-mail report to the list, to have
> more eyes on it.

I agree that we'd benefit much more from more people seeing the
list of coverity reports.

-- PMM

[Qemu-devel] [PATCH 2/2] signalfd(): modernize detection and use

[Laszlo Ersek]

qemu_signalfd() provides the (effects of the) Linux signalfd() syscall on
platforms that lack it. However, the check for the availability of
signalfd() in configure, and its use in qemu_signalfd() when it *is*
available, are seriously outdated (they date back to 2010-2011).

To wit, the SYS_signalfd-based check in configure can actually fail on at
least modern aarch64 Linux systems that *do* have signalfd().
(SYS_signalfd is deprecated and has been replaced by SYS_signalfd4 (note
the "4"). The signalfd() libc function selects the appropriate system
call.)

We should consider signalfd() accessible as a first class libc function on
platforms that support it. Modernize the related parts in configure and
"util/compatfd.c" (so that we end up with something similar to eventfd()'s
detection).

Signed-off-by: Laszlo Ersek 
---
 configure   | 11 ---
 util/compatfd.c |  5 ++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index 5ea1014..de0d21c 100755
--- a/configure
+++ b/configure
@@ -3280,10 +3280,15 @@ fi
 # signalfd probe
 signalfd="no"
 cat > $TMPC << EOF
-#include 
-#include 
+#include 
 #include 
-int main(void) { return syscall(SYS_signalfd, -1, NULL, _NSIG / 8); }
+int main(void)
+{
+sigset_t s;
+
+sigfillset(&s);
+return signalfd(-1, &s, SFD_CLOEXEC);
+}
 EOF
 
 if compile_prog "" "" ; then
diff --git a/util/compatfd.c b/util/compatfd.c
index e857150..7983391 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -17,7 +17,7 @@
 #include "qemu/compatfd.h"
 #include "qemu/thread.h"
 
-#include 
+#include 
 
 struct sigfd_compat_info
 {
@@ -99,9 +99,8 @@ int qemu_signalfd(const sigset_t *mask)
 #if defined(CONFIG_SIGNALFD)
 int ret;
 
-ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
+ret = signalfd(-1, mask, SFD_CLOEXEC);
 if (ret != -1) {
-qemu_set_cloexec(ret);
 return ret;
 }
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/4] ARM: Add support for a generic PCI Express host bridge

[Claudio Fontana]

Hi Alex,

are you planning a respin of this one?

Between your series and Alvise's I would just need one of the two to get 
merged, they are both fine for me, pending some small things that have been 
raised in the comments..

Ciao & thanks,

Claudio

On 06.01.2015 17:03, Alexander Graf wrote:
> Linux implements a nice binding to describe a "generic" PCI Express host 
> bridge
> using only device tree.
> 
> This patch set adds enough emulation logic to expose the parts that are
> "generic" as a simple sysbus device and maps it into ARM's virt machine.
> 
> With this patch set, we can finally spawn PCI devices on ARM VMs. I was able
> to have a fully DRM enabled virtual machine with VGA, e1000 and XHCI (for
> keyboard and mouse) up and working.
> 
> It's only a small step for QEMU, but a big step for ARM VM's usability.
> 
> 
> Happy new year!
> 
> Alexander Graf (4):
>   pci: Split pcie_host_mmcfg_map()
>   pci: Add generic PCIe host bridge
>   arm: Add PCIe host bridge in virt machine
>   arm: enable Bochs PCI VGA
> 
>  default-configs/arm-softmmu.mak |   3 +
>  hw/arm/virt.c   |  83 +++--
>  hw/pci-host/Makefile.objs   |   1 +
>  hw/pci-host/gpex.c  | 156 
> 
>  hw/pci/pcie_host.c  |   9 ++-
>  include/hw/pci-host/gpex.h  |  56 +++
>  include/hw/pci/pcie_host.h  |   1 +
>  7 files changed, 302 insertions(+), 7 deletions(-)
>  create mode 100644 hw/pci-host/gpex.c
>  create mode 100644 include/hw/pci-host/gpex.h
> 


-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München

office: +49 89 158834 4135
mobile: +49 15253060158

Re: [Qemu-devel] [PATCH 0/4] ARM: Add support for a generic PCI Express host bridge

[Alexander Graf]

On 21.01.15 13:59, Claudio Fontana wrote:
> Hi Alex,
> 
> are you planning a respin of this one?

Yup, will send a respin with 4 IRQs this week.


Alex

Re: [Qemu-devel] [PATCH 0/4] ARM: Add support for a generic PCI Express host bridge

[Peter Maydell]

On 21 January 2015 at 13:01, Alexander Graf  wrote:
>
>
> On 21.01.15 13:59, Claudio Fontana wrote:
>> Hi Alex,
>>
>> are you planning a respin of this one?
>
> Yup, will send a respin with 4 IRQs this week.

I've finished reading my thousand-page book on PCIe,
so hopefully will be able to review the respin :-)

-- PMM

[Qemu-devel] target-tricore: MISSING_BREAK in gen_compute_branch(), false positive?

[Markus Armbruster]

Coverity reports

Error: MISSING_BREAK:
target-tricore/translate.c:1648: unterminated_case: This case (value 
"OPC1_32_B_JLA") is not terminated by a 'break' statement.
target-tricore/translate.c:1650: fallthrough: The above case falls through 
to this one.

Here's the code:

case OPC1_32_B_JLA:
tcg_gen_movi_tl(cpu_gpr_a[11], ctx->next_pc);
case OPC1_32_B_JA:
gen_goto_tb(ctx, 0, EA_B_ABSOLUT(offset));
break;

If the fall through is intentional, please add a comment, like this:

case OPC1_32_B_JLA:
tcg_gen_movi_tl(cpu_gpr_a[11], ctx->next_pc);
/* fall through */
case OPC1_32_B_JA:
gen_goto_tb(ctx, 0, EA_B_ABSOLUT(offset));
break;

[Qemu-devel] [PATCH 1/1] vmstate-static-checker: update whitelist

[Amit Shah]

Commit 22382bb96c8bd88370c1ff0cb28c3ee6bee79ed3 renamed the
'hw_cursor_x' and 'hw_cursor_y' fields in cirrus_vga.  Update the static
checker's whitelist to allow matching against the old and new names.

Signed-off-by: Amit Shah 
---
 scripts/vmstate-static-checker.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/vmstate-static-checker.py 
b/scripts/vmstate-static-checker.py
index f7ce3fc..b6c0bbe 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -53,6 +53,8 @@ def check_fields_match(name, s_field, d_field):
'parent_obj.parent_obj.parent_obj',
'port.br.dev.exp.aer_log',
 
'parent_obj.parent_obj.parent_obj.exp.aer_log'],
+'cirrus_vga': ['hw_cursor_x', 'vga.hw_cursor_x',
+   'hw_cursor_y', 'vga.hw_cursor_y'],
 'lsiscsi': ['dev', 'parent_obj'],
 'mch': ['d', 'parent_obj'],
 'pci_bridge': ['bridge.dev', 'parent_obj', 'bridge.dev.shpc', 'shpc'],
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Peter Maydell]

On 21 January 2015 at 11:54, Markus Armbruster  wrote:
> Markus Armbruster  writes:
>
>> Frank Blaschka  writes:
>>
>>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
 Markus Armbruster  writes:
 > 1. pbdev->isc gets promoted from uint8_t to int as operand of binary <<
 >(usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8)
 >
 > 2. The int result is shifted left 28 bits.  This can set the MSB.
 >
 > 3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted
 >left 16 bits.
>>> uint16_t to int
>>
>> Yes, that's what I meant :)
>>
 >
 > 4. The two shift results stay int and get ored.
 >
 > 5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left
 >8 bits.
 >
 > 6. The next or's left operand is the int result of 4 and the right
 >operant is the uint64_t result of 5.  Therefore, the left operand is
 >*sign-extended* from int to uint64_t.  This copies bit#7 of
 >pbdev->isc to bits#31..63.  Whoops.

 I neglected to say: we don't currently use the upper 32 bits, and as
 long as we do that, the sign extension is harmless.  I'd recommend to
 avoid it all the same, for robustness, and to hush up Coverity.


> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 5ea13e5..2bed3f5 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -785,8 +785,8 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, 
> uint64_t fiba)
>  stq_p(&fib.fmb_addr, pbdev->fmb_addr);
>
>  data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> -   (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> -   pbdev->routes.adapter.summary_offset;
> +   ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
> +   (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
>  stw_p(&fib.data, data);
>
>  if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>

This doesn't make sense to me as a fix for the problem you describe
above. Either
 (1) pbdev->isc may have bit 3 set: in this case shifting it left
 by 28 is undefined behaviour in C, and we must not do it
 (and adding a cast to ind_offset doesn't help us at all)
 (2) pbdev->isc is guaranteed never to have bit 3 set: in this
 case the sign extension to uint64_t in step 6 above will
 have no effect, because the sign bit in the int result will
 be clear

So you can either:
 (1) cast pbdev->isc to uint32_t before shifting, thus ensuring that
 we do all our | operations on unsigned types and that we won't
 shift into the sign bit regardless of pbdev->isc's value
 (2) state that we know pbdev->isc is always less than 8 and so this
 is a coverity false positive to be suppressed via the web UI

But the patch you have doesn't seem like the right thing to me.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] clean up SYS_signalfd

[Paolo Bonzini]

On 21/01/2015 13:57, Laszlo Ersek wrote:
> SYS_signalfd is deprecated and absent on some Linux systems that do
> support signalfd().
> 
> Also, in 2015 the signalfd() libc function should be available wherever
> the underlying syscall(s) are available.
> 
> Drop SYS_signalfd and check for & use signalfd() directly.
> 
> Laszlo Ersek (2):
>   qemu_signalfd_available(): remove function due to lack of callers
>   signalfd(): modernize detection and use
> 
>  configure   | 11 ---
>  include/qemu/compatfd.h |  1 -
>  util/compatfd.c | 24 ++--
>  3 files changed, 10 insertions(+), 26 deletions(-)

I suspect this will stop using signalfd on RHEL5.  I wouldn't weep, but
it's worth pointing it out.

Paolo

[Qemu-devel] [PATCH] xilinx_ethlite: Clean up after commit 2f991ad

[Markus Armbruster]

The "fall through" added by the commit is clearly intentional.  Mark
it so.  Hushes up Coverity.

Signed-off-by: Markus Armbruster 
---
 hw/net/xilinx_ethlite.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c
index 9536f64..ad6b553 100644
--- a/hw/net/xilinx_ethlite.c
+++ b/hw/net/xilinx_ethlite.c
@@ -146,6 +146,7 @@ eth_write(void *opaque, hwaddr addr,
 if (!(value & CTRL_S)) {
 qemu_flush_queued_packets(qemu_get_queue(s->nic));
 }
+/* fall through */
 case R_TX_LEN0:
 case R_TX_LEN1:
 case R_TX_GIE0:
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] dataplane: move vring_more_avail() into vring.c

[Greg Kurz]

On Mon, 19 Jan 2015 17:04:36 +
Stefan Hajnoczi  wrote:

> vring_more_avail() was an inline function in vring.h.  No external
> callers use it so it's not necessary to export it.
> 
> Furthermore, we'll need virtio-access.h for endian-aware memory accesses
> but that only works in per-target object files (obj-y) and not
> build-once object files (common-obj-y) like the virtio-blk and
> virtio-scsi devices.
> 
> Move vring_more_avail() into vring.c so that virtio devices like
> virtio-blk and virtio-scsi can continue to use vring.h without being
> built once per target.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---

Yes, this function should definitely be declared in more appropriate scope.
And BTW I remember Cornelia had the same concern in the virtio-1 serie. :)

Reviewed-by: Greg Kurz 

>  hw/virtio/dataplane/vring.c | 6 ++
>  include/hw/virtio/dataplane/vring.h | 6 --
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 61f6d83..cb31d7f 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -21,6 +21,12 @@
>  #include "hw/virtio/dataplane/vring.h"
>  #include "qemu/error-report.h"
> 
> +/* Are there more descriptors available? */
> +static inline bool vring_more_avail(Vring *vring)
> +{
> +return vring->vr.avail->idx != vring->last_avail_idx;
> +}
> +
>  /* vring_map can be coupled with vring_unmap or (if you still have the
>   * value returned in *mr) memory_region_unref.
>   */
> diff --git a/include/hw/virtio/dataplane/vring.h 
> b/include/hw/virtio/dataplane/vring.h
> index d3e086a..1e871e6 100644
> --- a/include/hw/virtio/dataplane/vring.h
> +++ b/include/hw/virtio/dataplane/vring.h
> @@ -36,12 +36,6 @@ static inline unsigned int vring_get_num(Vring *vring)
>  return vring->vr.num;
>  }
> 
> -/* Are there more descriptors available? */
> -static inline bool vring_more_avail(Vring *vring)
> -{
> -return vring->vr.avail->idx != vring->last_avail_idx;
> -}
> -
>  /* Fail future vring_pop() and vring_push() calls until reset */
>  static inline void vring_set_broken(Vring *vring)
>  {

Re: [Qemu-devel] [PATCH] dataplane: endianness-aware accesses

[Stefan Hajnoczi]

On Tue, Jan 20, 2015 at 4:27 PM, Cornelia Huck  wrote:
> @@ -154,7 +157,7 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
>  }
>
>
> -static int get_desc(Vring *vring, VirtQueueElement *elem,
> +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
>  struct vring_desc *desc)

vdev is an unused argument?  I think this hunk can be dropped.

The rest looks good.

Re: [Qemu-devel] [PATCH 0/2] dataplane: use endian-aware memory accessors

[Stefan Hajnoczi]

On Mon, Jan 19, 2015 at 5:04 PM, Stefan Hajnoczi  wrote:
> In commit 0f5d1d2a49778863db54b4b1ac2dc008a8f21f11 ("virtio: memory accessors
> for endian-ambivalent targets") endian-aware memory accessors were added to
> support bi-endian targets like recent ppc64 systems.
>
> The dataplane vring.c code does not use these accessors and is therefore 
> unable
> to emulate virtio devices when the endianness differs between the device and
> host.
>
> These patches modify vring.c to use the endian-aware accessors.
>
> I have tested that x86_64 guests still function correctly.
>
> I have only compile-tested on ppc64 and require help testing that dataplane
> works.  If you have access to a bi-endian system, please try:
>
>   qemu ... -object iothread,id=iothread0 \
>-drive if=none,id=drive0,file=... \
>-device virtio-blk-pci,iothread=iothread0,drive=drive0
>
> Stefan Hajnoczi (2):
>   dataplane: move vring_more_avail() into vring.c
>   dataplane: use virtio_ld/st_p() for endian-aware memory access
>
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  hw/scsi/virtio-scsi-dataplane.c |  2 +-
>  hw/virtio/Makefile.objs |  2 +-
>  hw/virtio/dataplane/Makefile.objs   |  2 +-
>  hw/virtio/dataplane/vring.c | 69 
> ++---
>  include/hw/virtio/dataplane/vring.h |  9 ++---
>  6 files changed, 55 insertions(+), 31 deletions(-)

NACK

Cornelia Huck already has an equivalent patch.

Re: [Qemu-devel] Can we make better use of Coverity?

[Daniel P. Berrange]

On Wed, Jan 21, 2015 at 01:47:22PM +0100, Markus Armbruster wrote:
> We're using the Coverity Scan service[*].  We've put in some effort, and
> we've gotten some mileage out of it, but I feel we could get more.
> 
> Judging from the report e-mail I have lying about, we're scanning about
> once a month on average.  These reports cuts off after 20 new defects.
> When there are more, which is common, people have to go to the web
> dashboard to see them.  When I get one with ten, I may have a look, when
> I get one "Showing 20 of 100 defect(s)", I despair of the task, and put
> it off.
> 
> I also use Coverity locally (requires a license) with a derived model
> for GLib to increase scanning power.  Since last July, the number of
> defects I get that way has increased from ~400 to ~700.  Not quite as
> bad as it sounds, because ~100 of the new ones are DEADCODE.  Still, it
> suggests we haven't made much progress in reducing the number of defects
> to a manageable level.
> 
> Some of the new defects are avoidable.  For instance, we've added 16
> MISSING_BREAK.  Probably just missing /* fall through */, but we can't
> be sure without examining each case.  Patch review fail.
> 
> At the other end of the spectrum, I see 36 new UNINIT defects.
> 
> I think we should scan much more regularly.  Once a week, full auto?

I agree that you need to scan much more regularly. Given the number of
patches QEMU merges, with only monthly scans you're creating a big job
for whoever has to deal with the monthly report because chances are
there will be alot of new stuff reported each month to wade through.

In libvirt we now have a coverity scan being run once a day, so when
we get new problems reported, the code in question is still fresh in
the mind of the reviewers & patch author. Daily scans also spread out
the workload much better. Only get a small number of new problems to
analyse a couple of times a week - never any real huge burden for the
person managing the coverity scan & more likely to get others to help
too if there's only a couple of things for them to look at instead of
a list of 700+ to wade through. I think these contribute to make it
practical for us to keep libvirt at zero coverity problems all the
time.

If you set the current 700 issues you have reported as your baseline,
then it is still practical to run coverity daily on QEMU. Just have
it report only new issues, ignoring the backlog, and ensure those all
get fixes posted the same day so the backlog doesn't grow. Deal with
the historical backlog of issues separately as time allows.

Also I'd suggest making "no new coverity issues" be a release blocker
item so people see you are taking it seriously and so be encouraged
to help out to ensure the release doesn't slip.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PATCH v4] PPC: E500: Add FSL I2C controller and integrate RTC with it

[Amit Tomar]

This patch adds an emulation model for i2c controller found on most of the FSL 
SoCs.
It also integrates the RTC(ds1338) that sits on the i2c Bus with e500 machine 
model.


Signed-off-by: Amit Singh Tomar 
---
Changes in v4:
* Addressed Alex's comments given for v2.

Changes in v3: 
  * Reordered the subject line to appropriate one

Changes in v2: 
* Moved it to GPL v2+ 
* Replaced the printf with DPRINTF
* Fixed the coding style issues
* Changed the subject line
---
 default-configs/ppc-softmmu.mak   |2 +
 default-configs/ppc64-softmmu.mak |2 +
 hw/i2c/Makefile.objs  |1 +
 hw/i2c/mpc_i2c.c  |  359 +
 hw/ppc/e500.c |   51 ++
 5 files changed, 415 insertions(+)
 create mode 100644 hw/i2c/mpc_i2c.c

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index d725b23..6fdd39a 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -9,6 +9,8 @@ CONFIG_M48T59=y
 CONFIG_VGA=y
 CONFIG_VGA_PCI=y
 CONFIG_SERIAL=y
+CONFIG_MPC_I2C=y
+CONFIG_DS1338=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCKBD=y
diff --git a/default-configs/ppc64-softmmu.mak 
b/default-configs/ppc64-softmmu.mak
index bd30d69..3ad3f58 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -9,6 +9,8 @@ CONFIG_M48T59=y
 CONFIG_VGA=y
 CONFIG_VGA_PCI=y
 CONFIG_SERIAL=y
+CONFIG_MPC_I2C=y
+CONFIG_DS1338=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCKBD=y
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 648278e..6e6d00d 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
 common-obj-$(CONFIG_APM) += pm_smbus.o
 common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
+common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
 obj-$(CONFIG_OMAP) += omap_i2c.o
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
new file mode 100644
index 000..91528a3
--- /dev/null
+++ b/hw/i2c/mpc_i2c.c
@@ -0,0 +1,359 @@
+/*
+ * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: Amit Tomar, 
+ *
+ * Description:
+ * This file is derived from IMX I2C controller,
+ * by Jean-Christophe DUBOIS .
+ *
+ * Thanks to Scott Wood and Alexander Graf for their kind help on this.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2 or later,
+ * as published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+*/
+
+#include "hw/sysbus.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/bitops.h"
+#include "hw/ptimer.h"
+
+/* #define DEBUG_I2C */
+
+#ifdef DEBUG_I2C
+#define DPRINTF(fmt, ...)  \
+do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
+} while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define TYPE_MPC_I2C "mpc-i2c"
+#define MPC_I2C(obj) \
+OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
+
+#define MPC_I2C_ADR   0x00
+#define MPC_I2C_FDR   0x04
+#define MPC_I2C_CR0x08
+#define MPC_I2C_SR0x0c
+#define MPC_I2C_DR0x10
+#define MPC_I2C_DFSRR 0x14
+
+#define CCR_MEN  (1<<7)
+#define CCR_MIEN (1<<6)
+#define CCR_MSTA (1<<5)
+#define CCR_MTX  (1<<4)
+#define CCR_TXAK (1<<3)
+#define CCR_RSTA (1<<2)
+#define CCR_BCST (1<<0)
+
+#define CSR_MCF  (1<<7)
+#define CSR_MAAS (1<<6)
+#define CSR_MBB  (1<<5)
+#define CSR_MAL  (1<<4)
+#define CSR_SRW  (1<<2)
+#define CSR_MIF  (1<<1)
+#define CSR_RXAK (1<<0)
+
+#define CADR_MASK 0xFE
+#define CFDR_MASK 0x3F
+#define CCR_MASK  0xFC
+#define CSR_MASK  0xED
+#define CDR_MASK  0xFF
+
+#define CYCLE_RESET 0xFF
+
+typedef struct MPCI2CState {
+SysBusDevice parent_obj;
+
+I2CBus *bus;
+qemu_irq irq;
+MemoryRegion iomem;
+
+uint8_t address;
+uint8_t adr;
+uint8_t fdr;
+uint8_t cr;
+uint8_t sr;
+uint8_t dr;
+uint8_t dfssr;
+} MPCI2CState;
+
+static bool mpc_i2c_is_enabled(MPCI2CState *s)
+{
+return s->cr & CCR_MEN;
+}
+
+static bool mpc_i2c_is_master(MPCI2CState *s)
+{
+return s->cr & CCR_MSTA;
+}
+
+static bool mpc_i2c_direction_is_tx(MPCI2CState *s)
+{
+return s->cr & CCR_MTX;
+}
+
+static bool mpc_i2c_irq_pending(MPCI2CState *s)
+{
+return s->sr & CSR_MIF;
+}
+
+static bool mpc_i2c_irq_is_enabled(MPCI2CState *s)
+{
+return s->cr & CCR_MIEN;
+}
+
+static void mpc_i2c_reset(DeviceState *dev)
+{
+MPCI2CState *i2c = MPC_I2C(dev);
+
+i2c->address = 0xFF;
+i2c->adr = 0x00;
+i2c->fdr = 0x00;
+i2c->cr =  0x00;
+i2c->sr =  0x81;
+i2c->dr =  0x00;
+}
+
+static void mpc_i2c_irq(MPCI2CState *s)
+{
+bool irq_active = false;
+
+if (mpc_i2c_is_enabled(s) && mpc_i2c_irq_is_enabled(s)
+ 

Re: [Qemu-devel] [PATCH 1/1] vmstate-static-checker: update whitelist

[Gerd Hoffmann]

On Mi, 2015-01-21 at 18:36 +0530, Amit Shah wrote:
> Commit 22382bb96c8bd88370c1ff0cb28c3ee6bee79ed3 renamed the
> 'hw_cursor_x' and 'hw_cursor_y' fields in cirrus_vga.  Update the
> static
> checker's whitelist to allow matching against the old and new names.
> 
> Signed-off-by: Amit Shah 

Reviewed-by: Gerd Hoffmann 

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Markus Armbruster]

Peter Maydell  writes:

> On 21 January 2015 at 11:54, Markus Armbruster  wrote:
>> Markus Armbruster  writes:
>>
>>> Frank Blaschka  writes:
>>>
 On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
> Markus Armbruster  writes:
> > 1. pbdev->isc gets promoted from uint8_t to int as operand of binary <<
> >(usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8)
> >
> > 2. The int result is shifted left 28 bits.  This can set the MSB.
> >
> > 3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted
> >left 16 bits.
 uint16_t to int
>>>
>>> Yes, that's what I meant :)
>>>
> >
> > 4. The two shift results stay int and get ored.
> >
> > 5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left
> >8 bits.
> >
> > 6. The next or's left operand is the int result of 4 and the right
> >operant is the uint64_t result of 5.  Therefore, the left operand is
> >*sign-extended* from int to uint64_t.  This copies bit#7 of
> >pbdev->isc to bits#31..63.  Whoops.
>
> I neglected to say: we don't currently use the upper 32 bits, and as
> long as we do that, the sign extension is harmless.  I'd recommend to
> avoid it all the same, for robustness, and to hush up Coverity.
>
>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 5ea13e5..2bed3f5 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -785,8 +785,8 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t
>> r1, uint64_t fiba)
>>  stq_p(&fib.fmb_addr, pbdev->fmb_addr);
>>
>>  data = (pbdev->isc << 28) | (pbdev->noi << 16) |
>> -   (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
>> -   pbdev->routes.adapter.summary_offset;
>> +   ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
>> +   (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
>>  stw_p(&fib.data, data);
>>
>>  if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>>
>
> This doesn't make sense to me as a fix for the problem you describe
> above. Either
>  (1) pbdev->isc may have bit 3 set: in this case shifting it left
>  by 28 is undefined behaviour in C,

Correct.

> and we must not do it

I suspect we shift signed values all over the place, without regard for
signed overflow.  Machines are fine with that, but some day some
compiler wiseguy may find a way to save a femtosecond or two for some
program that never does that, breaking programs that do it, and then
we'll be in trouble.

We should follow the kernel's lead and compile with
-fno-strict-overflow.

>  (and adding a cast to ind_offset doesn't help us at all)

Correct, it doesn't help with the signed left shift of pbdev->isc.

>  (2) pbdev->isc is guaranteed never to have bit 3 set: in this
>  case the sign extension to uint64_t in step 6 above will
>  have no effect, because the sign bit in the int result will
>  be clear
>
> So you can either:
>  (1) cast pbdev->isc to uint32_t before shifting, thus ensuring that
>  we do all our | operations on unsigned types and that we won't
>  shift into the sign bit regardless of pbdev->isc's value
>  (2) state that we know pbdev->isc is always less than 8 and so this
>  is a coverity false positive to be suppressed via the web UI
>
> But the patch you have doesn't seem like the right thing to me.

Frank's code, Frank's choice :)

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Gerd Hoffmann]

On Mi, 2015-01-21 at 11:37 +, Ian Jackson wrote:
> Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property to 
> support IGD GFX passthrough"):
> > When we're working to support IGD GFX passthrough with qemu
> > upstream, instead of "-gfx_passthru" we'd like to make that
> > a machine option, "-machine xxx,gfx_passthru=on". This need
> > to bring several changes on tool side.
> 
> Has the corresponding patch to qemu-upstream been accepted yet ?
> 
> I'd like to see a confirmation from the qemu side that this is going
> into their tree and that the command-line option syntax has been
> agreed.

Suggested at patch review, not merged (guess thats why it is tagged
'rfc', to get both qemu+xen on the same page).

While being at it:  Should we name this 'igd-passthru' instead of
'gfx-passthru'?  The hostbridge / isabridge quirks needed are actually
specific to igd and are not needed for -- say -- nvidia gfx cards.

cheers,
  Gerd

Re: [Qemu-devel] Can we make better use of Coverity?

[Markus Armbruster]

Peter Maydell  writes:

> On 21 January 2015 at 12:47, Markus Armbruster  wrote:
>> We're using the Coverity Scan service[*].  We've put in some effort, and
>> we've gotten some mileage out of it, but I feel we could get more.
>>
>> Judging from the report e-mail I have lying about, we're scanning about
>> once a month on average.  These reports cuts off after 20 new defects.
>> When there are more, which is common, people have to go to the web
>> dashboard to see them.  When I get one with ten, I may have a look, when
>> I get one "Showing 20 of 100 defect(s)", I despair of the task, and put
>> it off.
>
> Right, but coverity reports lots of stuff, much of which is either
> wrong or just not very important. The interesting stats here are:
> (1) the "high impact outstanding" buglist: we have just 33 of these
> (2) the per-component lists: where somebody's been working on the
> bug list for that component there are often not many bugs (there
> are just 2 outstanding for "arm", for instance)

I agree the sky is most definitely not falling.

The defect density is quite uneven (see appended table).  "arm" is in
good shape indeed, and the largest low-density component.  Top-scorers
are bt, slirp and 9pfs.  Figures; they feel barely maintained these
days.

>> I think we should scan much more regularly.  Once a week, full auto?
>
> I think a regular automated scan would be useful, yes.

Need a volunteer to script that.  Any takers?

>> I further think we should send the e-mail report to the list, to have
>> more eyes on it.
>
> I agree that we'd benefit much more from more people seeing the
> list of coverity reports.

I figure that's just a matter of creating a dummy member with the list
address.  Any objections?


Defect density by component, from
https://scan.coverity.com/projects/378?tab=overview

Component Name  Line of CodeDefect density
bt4,610 1.74
slirp 6,968 1.44
9pfs  9,493 1.37
user 32,263 0.68
mips 34,321 0.52
Other   390,967 0.51
net  29,412 0.44
lm32  2,836 0.35
ui   43,771 0.32
block55,171 0.31
ppc  50,323 0.28
disas38,362 0.26
i386 36,786 0.22
migration 5,249 0.19
usb  26,524 0.19
m68k  5,533 0.18
s390 17,171 0.17
sparc14,677 0.14
tricore   7,801 0.13
pci  11,292 0.09
scsi 14,521 0.07
arm  69,085 0.01
cris  6,341 0.00
libcacard 3,779 0.00
microblaze3,482 0.00
monitor  30,044 0.00
nbd   1,714 0.00
openrisc  3,102 0.00
tcg  10,659 0.00
trace 9,090 0.00
unicore32 3,191 0.00
xtensa7,393 0.00

The size of "Other" shows that our component definitions could use a
little love, too :)

Re: [Qemu-devel] [PATCH 1/1] virtio: fix feature bit checks

[Michael S. Tsirkin]

On Wed, Jan 21, 2015 at 01:07:59PM +0100, Thomas Huth wrote:
> On Fri, 12 Dec 2014 10:01:46 +0100
> Cornelia Huck  wrote:
> 
> > Several places check against the feature bit number instead of against
> > the feature bit. Fix them.
> > 
> > Reported-by: Thomas Huth 
> > Signed-off-by: Cornelia Huck 
> > ---
> >  hw/scsi/virtio-scsi.c   | 2 +-
> >  hw/virtio/dataplane/vring.c | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index ef48550..a44c410 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -144,7 +144,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
> >   *
> >   * TODO: always disable this workaround for virtio 1.0 devices.
> >   */
> > -if ((vdev->guest_features & VIRTIO_F_ANY_LAYOUT) == 0) {
> > +if ((vdev->guest_features & (1 << VIRTIO_F_ANY_LAYOUT)) == 0) {
> >  req_size = req->elem.out_sg[0].iov_len;
> >  resp_size = req->elem.in_sg[0].iov_len;
> >  }
> > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> > index 61f6d83..78c6f45 100644
> > --- a/hw/virtio/dataplane/vring.c
> > +++ b/hw/virtio/dataplane/vring.c
> > @@ -133,12 +133,12 @@ bool vring_should_notify(VirtIODevice *vdev, Vring 
> > *vring)
> >   * interrupts. */
> >  smp_mb();
> > 
> > -if ((vdev->guest_features & VIRTIO_F_NOTIFY_ON_EMPTY) &&
> > +if ((vdev->guest_features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) &&
> >  unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
> >  return true;
> >  }
> > 
> > -if (!(vdev->guest_features & VIRTIO_RING_F_EVENT_IDX)) {
> > +if (!(vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX))) {
> >  return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
> >  }
> >  old = vring->signalled_used;
> 
> Ping ... somebody taking care of this? It's a bug fix for current
> code, so IMHO this patch should not wait for the inclusion of the
> other virtio-1 stuff...
> 
>  Thomas

It's in my queue.

[Qemu-devel] Running read-only internal snapshot (loadvm)

[Jarkko Turkulainen]

Hello,

Is it possible to do -loadvm with a read-only image? What I'm trying to do
is running already booted OS but discarding all the changes. So basically,
I'd like to combine -loadvm and -snapshot. The image needs to be read-only
because there are multiple simultaneous instances running. As a workaround,
I've been doing the -loadvm on a temporary copy of the image, but that is
not really effective, it takes time (or memory) to copy the image around.

Re: [Qemu-devel] [PATCH] seccomp: add mlockall to whitelist

[Eduardo Otubo]

On Tue, Jan 20, 2015 at 02:32:33PM +0100, Paolo Bonzini wrote:
> This is used by "-realtime mlock=on".
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  qemu-seccomp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index b0c6269..f9de0d3 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -229,6 +229,7 @@ static const struct QemuSeccompSyscall 
> seccomp_whitelist[] = {
>  { SCMP_SYS(shmdt), 240 },
>  { SCMP_SYS(timerfd_create), 240 },
>  { SCMP_SYS(shmctl), 240 },
> +{ SCMP_SYS(mlockall), 240 },
>  { SCMP_SYS(mlock), 240 },
>  { SCMP_SYS(munlock), 240 },
>  { SCMP_SYS(semctl), 240 },
> -- 
> 2.1.0
> 

Signed-off-by: Eduardo Otubo 
Acked-by: Eduardo Otubo 

I'll make a pull request by Friday at the end of the day. Thanks for the
patch.

-- 
Eduardo Otubo
ProfitBricks GmbH

Re: [Qemu-devel] Can we make better use of Coverity?

[Paolo Bonzini]

On 21/01/2015 13:47, Markus Armbruster wrote:
> I also use Coverity locally (requires a license) with a derived model
> for GLib to increase scanning power.  Since last July, the number of
> defects I get that way has increased from ~400 to ~700.  Not quite as
> bad as it sounds, because ~100 of the new ones are DEADCODE.  Still, it
> suggests we haven't made much progress in reducing the number of defects
> to a manageable level.

While I agree that the current frequency of scans is too low, things are
not too bad.  When I do run a scan, I get 10-20 issues.  This is a
volume that I can triage, and I can also (depending on the component)
send the most egregious out to the maintainer or the author of the
offending patch.  It takes me between an hour and two.

There are "only" 221 outstanding defects on Coverity scan, most of which
actually have never been triaged.  This means that maintainers are good
at fixing bugs.  In fact, about 120 of these 221 defects are split
between the "9p", "bt", "disas", "other", "slirp" and "user" components
(i.e. the worst components + the catchall component).  None of the bad
components are in active development; unfortunately, this means that
70-80 defects probably will never be fixed.

Every now and then I refine the components, mostly by looking at defects
in the "other" category.  This had the nice side effect of making
"other" no longer one of the bad components; it's way below the average
of the project.  As a rule of thumb, either we know something is bad, or
we maintain it well.  Again, this is not bad news.

QEMU is also using a GLib model on Coverity Scan, as well as a
QEMU-specific model, which suggests one of the following:

1) your model is not tailored well to QEMU;

2) you are not weeding out false positives.

Between the model, the triaging, and the fixing efforts, our defect rate
has gone down from 0.88 to 0.24 in a year, which I think is pretty good.
 (We could probably it down to 0.15, it's hard to go below that).

> Some of the new defects are avoidable.  For instance, we've added 16
> MISSING_BREAK.  Probably just missing /* fall through */, but we can't
> be sure without examining each case.  Patch review fail.

Or just that we do not care.  Missing /* fall through */ should either
be flagged by the compiler, or treated as a bonus.  Detecting missing
fall through comments is a waste of reviewer brain bandwidth.

> At the other end of the spectrum, I see 36 new UNINIT defects.
> 
> I think we should scan much more regularly.  Once a week, full auto?
> 
> I further think we should send the e-mail report to the list, to have
> more eyes on it.
> 
> Opinions?
> 
> 
> [*] https://scan.coverity.com/projects/378
> 

Re: [Qemu-devel] [PATCH] vhost-user: multiqueue support

[Michael S. Tsirkin]

On Sat, Dec 06, 2014 at 06:52:56PM +0200, Nikolay Nikolaev wrote:
> Vhost-user will implement the multiqueueu support in a similar way to what
> vhost already has - a separate thread for each queue.
> 
> To enable multiquue funcionality - a new command line parameter
> "queues" is introduced for the vhost-user netdev.
> 
> Signed-off-by: Nikolay Nikolaev 

Nikolay - plan to repost addressing comments?

> ---
>  docs/specs/vhost-user.txt |7 +++
>  hw/virtio/vhost-user.c|6 +-
>  net/vhost-user.c  |   35 +++
>  qapi-schema.json  |5 -
>  qemu-options.hx   |5 +++--
>  5 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 650bb18..d3857f5 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -127,6 +127,13 @@ in the ancillary data:
>  If Master is unable to send the full message or receives a wrong reply it 
> will
>  close the connection. An optional reconnection mechanism can be implemented.
>  
> +Multi queue suport
> +-
> +The protocol supports multiple queues by setting all index fields in the sent
> +messages to a value calculated by the following formula:
> + + 
> +The  is increased by 2.
> +
>  Message types
>  -
>  

How is the support negotiated though?
What if I set queues=N with a legacy backend that
does not support multiqueue?

> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index aefe0bb..83ebcaa 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -253,17 +253,20 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>  case VHOST_SET_VRING_NUM:
>  case VHOST_SET_VRING_BASE:
>  memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> +msg.state.index += dev->vq_index;
>  msg.size = sizeof(m.state);
>  break;
>  
>  case VHOST_GET_VRING_BASE:
>  memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> +msg.state.index += dev->vq_index;
>  msg.size = sizeof(m.state);
>  need_reply = 1;
>  break;
>  
>  case VHOST_SET_VRING_ADDR:
>  memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> +msg.addr.index += dev->vq_index;
>  msg.size = sizeof(m.addr);
>  break;
>  
> @@ -271,7 +274,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>  case VHOST_SET_VRING_CALL:
>  case VHOST_SET_VRING_ERR:
>  file = arg;
> -msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> +msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
>  msg.size = sizeof(m.u64);
>  if (ioeventfd_enabled() && file->fd > 0) {
>  fds[fd_num++] = file->fd;
> @@ -313,6 +316,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>  error_report("Received bad msg size.\n");
>  return -1;
>  }
> +msg.state.index -= dev->vq_index;
>  memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
>  break;
>  default:
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 24e050c..1ea2f98 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -134,25 +134,27 @@ static void net_vhost_user_event(void *opaque, int 
> event)
>  
>  static int net_vhost_user_init(NetClientState *peer, const char *device,
> const char *name, CharDriverState *chr,
> -   bool vhostforce)
> +   bool vhostforce, uint32_t queues)
>  {
>  NetClientState *nc;
>  VhostUserState *s;
> +int i;
>  
> -nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> +for (i = 0; i < queues; i++) {
> +nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
>  
> -snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> - chr->label);
> +snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> + i, chr->label);
>  
> -s = DO_UPCAST(VhostUserState, nc, nc);
> +s = DO_UPCAST(VhostUserState, nc, nc);
>  
> -/* We don't provide a receive callback */
> -s->nc.receive_disabled = 1;
> -s->chr = chr;
> -s->vhostforce = vhostforce;
> -
> -qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> +/* We don't provide a receive callback */
> +s->nc.receive_disabled = 1;
> +s->chr = chr;
> +s->vhostforce = vhostforce;
>  
> +qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> +}
>  return 0;
>  }
>  
> @@ -228,6 +230,7 @@ static int net_vhost_check_net(QemuOpts *opts, void 
> *opaque)
>  int net_init_vhost_user(const NetClientOptions *opts, const char *na

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

On Jan 21, 2015, at 2:54 AM, Markus Armbruster wrote:

> Programmingkid  writes:
> 
>> On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote:
>> 
>>> Programmingkid  writes:
>>> 
 On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:
 
> On 01/20/2015 07:29 AM, Programmingkid wrote:
>> 
>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>> 
>>> Programmingkid  writes:
>>> 
 Subject was: 
 Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
 on Mac OS X so that it reports the correct length of a real CD
>>> 
>>> Patch history information goes...
> 
>>> 
>>> ... below the --- divider.
>> 
>> I thought I did this. The information above is the description of
>> the patch.
>> Not its history.
> 
> Anything that mentions 'v7' is history.  When you read 'git log', you
> will not see mentions of 'v7', because no one cares how many tries it
> took to get a patch into git.  Knowing about v7 only matters to the
> reviewers of v8, hence it is patch history that belongs after the divider.
 
 Ok. 
 
> 
> 
 +
 +if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0
 +   && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) {
> 
> Indentation looks off here.
 
 It does look a little odd, but it also communicates that this is one
 statement (IMHO).
>>> 
>>> It's not how the rest of QEMU is indented.  Please try to blend in :)
>>> 
>>> I feel bad about notpicking v8 of an obviously useful and patch that is
>>> basically just fine except for these little things.  Thanks for
>>> persevering!
>> 
>> Thanks for your encouragement. I personally would have had that whole if
>> condition on one line, but others want an 80 line maximum. 
> 
> Humans tend to have trouble following long lines with their eyes (I sure
> do).  Typographic manuals suggest to limit columns to roughly 60
> characters for exactly that reason[*].  Code can be a bit wider since
> it's commonly indented[**].
> 
> For commit messages, 70 characters is already a compromise between
> legibility and "writability".
> 
> 
> [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
> 
> [**] Deep nesting is no excuse for exceeding the width limit, because
> deep nesting is no excuse for *anything* :)

 I will indent my code at around the 60 to 70
character size.

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Peter Maydell]

On 21 January 2015 at 13:41, Markus Armbruster  wrote:
> I suspect we shift signed values all over the place, without regard for
> signed overflow.  Machines are fine with that, but some day some
> compiler wiseguy may find a way to save a femtosecond or two for some
> program that never does that, breaking programs that do it, and then
> we'll be in trouble.

clang with its undefined behaviour sanitizers will warn at runtime
when we do this. I've sent out some patches to fix instances of
this in the past. Coverity will also warn in some cases I think.

> We should follow the kernel's lead and compile with
> -fno-strict-overflow.

I don't believe that option affects signed shifts, only signed
addition, subtraction and multiplication.

-- PMM

Re: [Qemu-devel] Can we make better use of Coverity?

[Markus Armbruster]

Paolo Bonzini  writes:

> On 21/01/2015 13:47, Markus Armbruster wrote:
>> I also use Coverity locally (requires a license) with a derived model
>> for GLib to increase scanning power.  Since last July, the number of
>> defects I get that way has increased from ~400 to ~700.  Not quite as
>> bad as it sounds, because ~100 of the new ones are DEADCODE.  Still, it
>> suggests we haven't made much progress in reducing the number of defects
>> to a manageable level.
>
> While I agree that the current frequency of scans is too low, things are
> not too bad.  When I do run a scan, I get 10-20 issues.  This is a
> volume that I can triage, and I can also (depending on the component)
> send the most egregious out to the maintainer or the author of the
> offending patch.  It takes me between an hour and two.
>
> There are "only" 221 outstanding defects on Coverity scan, most of which
> actually have never been triaged.  This means that maintainers are good
> at fixing bugs.  In fact, about 120 of these 221 defects are split
> between the "9p", "bt", "disas", "other", "slirp" and "user" components
> (i.e. the worst components + the catchall component).  None of the bad
> components are in active development; unfortunately, this means that
> 70-80 defects probably will never be fixed.
>
> Every now and then I refine the components, mostly by looking at defects
> in the "other" category.  This had the nice side effect of making
> "other" no longer one of the bad components; it's way below the average
> of the project.  As a rule of thumb, either we know something is bad, or
> we maintain it well.  Again, this is not bad news.
>
> QEMU is also using a GLib model on Coverity Scan, as well as a
> QEMU-specific model, which suggests one of the following:

What do you mean by "a GLib model"?  scripts/coverity-model.c?

> 1) your model is not tailored well to QEMU;

I use cov-analyze with

-co BAD_FREE:allow_first_field:true
--security
--concurrency
--derived-model-file $wherever/glib-2.38.2.xmldb
--user-model-file scripts/coverity-model.xmldb

where coverity-model.xmldb is made with

$ cov-make-library -of scripts/coverity-model.xmldb scripts/coverity-model.c

and glib-2.38.2.xmldb is made with

$ cov-collect-models --dir cov -of glib-2.38.2.xmldb

after a cov-analyze run of a fresh compile of Fedora-20's GLib.

> 2) you are not weeding out false positives.

Guilty as charged.  The proper place to do that is the Scan service,
where all of us can profit.

> Between the model, the triaging, and the fixing efforts, our defect rate
> has gone down from 0.88 to 0.24 in a year, which I think is pretty good.
>  (We could probably it down to 0.15, it's hard to go below that).

As I said: "We've put in some effort, and we've gotten some mileage out
of it, but I feel we could get more."

>> Some of the new defects are avoidable.  For instance, we've added 16
>> MISSING_BREAK.  Probably just missing /* fall through */, but we can't
>> be sure without examining each case.  Patch review fail.
>
> Or just that we do not care.  Missing /* fall through */ should either
> be flagged by the compiler,

Unfortunately, gcc doesn't.  Relying on tools for this is fine, but
requires actual use of said tools.  Which this thread is about :)

> or treated as a bonus.  Detecting missing
> fall through comments is a waste of reviewer brain bandwidth.
>
>> At the other end of the spectrum, I see 36 new UNINIT defects.
>> 
>> I think we should scan much more regularly.  Once a week, full auto?
>> 
>> I further think we should send the e-mail report to the list, to have
>> more eyes on it.
>> 
>> Opinions?
>> 
>> 
>> [*] https://scan.coverity.com/projects/378

Re: [Qemu-devel] [PATCH] seccomp: add mlockall to whitelist

[Eduardo Habkost]

On Tue, Jan 20, 2015 at 02:32:33PM +0100, Paolo Bonzini wrote:
> This is used by "-realtime mlock=on".
> 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Eduardo Habkost 
Tested-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] Can we make better use of Coverity?

[Paolo Bonzini]

On 21/01/2015 15:57, Markus Armbruster wrote:
>> QEMU is also using a GLib model on Coverity Scan, as well as a
>> QEMU-specific model, which suggests one of the following:
> 
> What do you mean by "a GLib model"?  scripts/coverity-model.c?

Yes.  It models g_malloc0 in a way that avoids a lot of false positives,
but still is able to flag leaks.

>> 2) you are not weeding out false positives.
> 
> Guilty as charged.  The proper place to do that is the Scan service,
> where all of us can profit.

Yup.  So the numbers are off by a couple hundred or so, assuming 20%
false positive rate.

>> Between the model, the triaging, and the fixing efforts, our defect rate
>> has gone down from 0.88 to 0.24 in a year, which I think is pretty good.
>>  (We could probably it down to 0.15, it's hard to go below that).
> 
> As I said: "We've put in some effort, and we've gotten some mileage out
> of it, but I feel we could get more."

Definitely.  But we've gotten much more than "some mileage" IMO.

>>> Some of the new defects are avoidable.  For instance, we've added 16
>>> MISSING_BREAK.  Probably just missing /* fall through */, but we can't
>>> be sure without examining each case.  Patch review fail.
>>
>> Or just that we do not care.  Missing /* fall through */ should either
>> be flagged by the compiler,
> 
> Unfortunately, gcc doesn't.  Relying on tools for this is fine, but
> requires actual use of said tools.  Which this thread is about :)

Sure.  But even then, MISSING_BREAK is not the #1 reason to have
Coverity around. :)

Paolo

[Qemu-devel] [PATCH] exec: fix madvise of NULL pointer

[Paolo Bonzini]

Coverity flags this as "dereference after null check".  Not quite a
dereference, since it will just EFAULT, but still nice to fix.

Signed-off-by: Paolo Bonzini 
---
 exec.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index 081818e..bfbfd23 100644
--- a/exec.c
+++ b/exec.c
@@ -1402,12 +1402,13 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, 
Error **errp)
 cpu_physical_memory_set_dirty_range(new_block->offset,
 new_block->used_length);
 
-qemu_ram_setup_dump(new_block->host, new_block->max_length);
-qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE);
-qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK);
-
-if (kvm_enabled()) {
-kvm_setup_guest_memory(new_block->host, new_block->max_length);
+if (new_block->host) {
+qemu_ram_setup_dump(new_block->host, new_block->max_length);
+qemu_madvise(new_block->host, new_block->max_length, 
QEMU_MADV_HUGEPAGE);
+qemu_madvise(new_block->host, new_block->max_length, 
QEMU_MADV_DONTFORK);
+if (kvm_enabled()) {
+kvm_setup_guest_memory(new_block->host, new_block->max_length);
+}
 }
 
 return new_block->offset;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 11/12] qcow2/overlaps: Protect inactive L2 tables

[Stefan Hajnoczi]

On Mon, Nov 24, 2014 at 04:56:59PM +0100, Max Reitz wrote:
> @@ -136,6 +138,34 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>size_to_clusters(s, sn->l1_size *
>sizeof(uint64_t)),
>QCOW2_OL_INACTIVE_L1);
> +
> +if (!(s->overlap_check & QCOW2_OL_INACTIVE_L2)) {
> +continue;
> +}
> +
> +l1_table = qemu_try_blockalign(bs->file,
> +   sn->l1_size * sizeof(uint64_t));

At this point we haven't validated sn->l1_size <= QCOW_MAX_L1_SIZE.

A bogus l1_size means we do a huge read and add junk into the metadata
list.  I think it would be best to check the value here.


pgpg7Y0ZqtxkD.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Paolo Bonzini]

On 21/01/2015 14:41, Markus Armbruster wrote:
> I suspect we shift signed values all over the place, without regard for
> signed overflow.  Machines are fine with that, but some day some
> compiler wiseguy may find a way to save a femtosecond or two for some
> program that never does that, breaking programs that do it, and then
> we'll be in trouble.

As I said before, if there was a way to save those femtoseconds, they
would have already tried.

More likely, the compiler people know they would become the main
attractions at pitch and feather spectacles, so they're not going to
treat that as undefined behavior.

Paolo

Re: [Qemu-devel] [PATCH v2 12/12] qcow2: Use new metadata overlap check function

[Stefan Hajnoczi]

On Mon, Nov 24, 2014 at 04:57:00PM +0100, Max Reitz wrote:
> @@ -2166,126 +2166,6 @@ fail:
>  return ret;
>  }
>  
> -#define overlaps_with(ofs, sz) \
> -ranges_overlap(offset, size, ofs, sz)

Dropping this macro makes me happy.


pgpxA8Q5wIqTT.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 5/7] ppc: force update of all msr bits in cpu_post_load

[Mark Cave-Ayland]

On 20/01/15 15:01, Paolo Bonzini wrote:

> On 23/12/2014 01:36, Mark Cave-Ayland wrote:
>> Since env->msr has already been restored by the time cpu_post_load is called,
>> make sure that ppc_store_msr() is explicitly called with all msr bits marked
>> as invalid.
>>
>> This solves the issue where MSR flags aren't set correctly when restoring a 
>> VM
>> snapshot, in particular the internal env->excp_prefix value when MSR_EP has
>> been altered by a guest.
>>
>> Signed-off-by: Mark Cave-Ayland 
>> ---
>>  target-ppc/machine.c |8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index c801b82..9669063 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -159,6 +159,7 @@ static int cpu_post_load(void *opaque, int version_id)
>>  PowerPCCPU *cpu = opaque;
>>  CPUPPCState *env = &cpu->env;
>>  int i;
>> +target_ulong msr;
>>  
>>  /*
>>   * We always ignore the source PVR. The user or management
>> @@ -190,7 +191,12 @@ static int cpu_post_load(void *opaque, int version_id)
>>  /* Restore htab_base and htab_mask variables */
>>  ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>>  }
>> -hreg_compute_hflags(env);
>> +
>> +/* Mark all msr bits invalid before restoring */
>> +msr = env->msr;
>> +env->msr = ~env->msr;
>> +ppc_store_msr(env, msr);
>> +
>>  hreg_compute_mem_idx(env);
>>  
>>  return 0;
>>
> 
> This is wrong for this line of hreg_store_msr:
> 
> if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
>  ((value ^ env->msr) & (1 << MSR_TGPR {
> /* Swap temporary saved registers with GPRs */
> hreg_swap_gpr_tgpr(env);
> }
> 
> I think you need to change the ~ with "^ ~(1 << MSR_TGPR)".

It took me a little time to decipher how that worked, but yes it seems
fine in my tests here. I'll resend the series with with your Reviewed-by
added to the previous patch.


Many thanks,

Mark.

Re: [Qemu-devel] [PATCH] dataplane: endianness-aware accesses

[Cornelia Huck]

On Wed, 21 Jan 2015 13:30:02 +
Stefan Hajnoczi  wrote:

> On Tue, Jan 20, 2015 at 4:27 PM, Cornelia Huck  
> wrote:
> > @@ -154,7 +157,7 @@ bool vring_should_notify(VirtIODevice *vdev, Vring 
> > *vring)
> >  }
> >
> >
> > -static int get_desc(Vring *vring, VirtQueueElement *elem,
> > +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement 
> > *elem,
> >  struct vring_desc *desc)
> 
> vdev is an unused argument?  I think this hunk can be dropped.

You're right, the vdev argument is a leftover from my old patch.

> The rest looks good.

Thanks! Will resend soon.

Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Eric Blake]

On 01/21/2015 02:34 AM, Markus Armbruster wrote:
> Opinions?
> 

I'm still thinking about my reply (it's a big enough question that I
want to make sure I consider ramifications), so this is just a
meta-reply to let you know I'm tracking this conversation.

 CC'ing Eric Blake, as well, for comments on a "unified parameter"
 interface in general.
>>>
>>> Good move.
>>>
>>>
>>> [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html
>>>
>>
>> Adding Eric back in, where'd he go?
> 
> Looks like you announced the cc:, but didn't actually do it, twice %-)

I know that on some lists I'm subscribed to, mailman has an annoying
configuration where if a list member is subscribed to the list and has
requested that mailman not send dupe emails, then mailman rewrites CC:
lines to drop that list member from the copy of the mail that other list
readers receive.  I hate the behavior, but it may be why you appear to
not see me in cc, even though I've definitely been receiving the mails.
 At any rate, I have my mail filters set to flag mails that call out my
name, even if I get left out from cc, so I can usually spot threads like
this where you are trying to get my attention, regardless of whether you
remembered to include me directly.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Can we make better use of Coverity?

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> On Wed, Jan 21, 2015 at 01:47:22PM +0100, Markus Armbruster wrote:
>> We're using the Coverity Scan service[*].  We've put in some effort, and
>> we've gotten some mileage out of it, but I feel we could get more.
>> 
>> Judging from the report e-mail I have lying about, we're scanning about
>> once a month on average.  These reports cuts off after 20 new defects.
>> When there are more, which is common, people have to go to the web
>> dashboard to see them.  When I get one with ten, I may have a look, when
>> I get one "Showing 20 of 100 defect(s)", I despair of the task, and put
>> it off.
>> 
>> I also use Coverity locally (requires a license) with a derived model
>> for GLib to increase scanning power.  Since last July, the number of
>> defects I get that way has increased from ~400 to ~700.  Not quite as
>> bad as it sounds, because ~100 of the new ones are DEADCODE.  Still, it
>> suggests we haven't made much progress in reducing the number of defects
>> to a manageable level.
>> 
>> Some of the new defects are avoidable.  For instance, we've added 16
>> MISSING_BREAK.  Probably just missing /* fall through */, but we can't
>> be sure without examining each case.  Patch review fail.
>> 
>> At the other end of the spectrum, I see 36 new UNINIT defects.
>> 
>> I think we should scan much more regularly.  Once a week, full auto?
>
> I agree that you need to scan much more regularly. Given the number of
> patches QEMU merges, with only monthly scans you're creating a big job
> for whoever has to deal with the monthly report because chances are
> there will be alot of new stuff reported each month to wade through.
>
> In libvirt we now have a coverity scan being run once a day, so when
> we get new problems reported, the code in question is still fresh in
> the mind of the reviewers & patch author. Daily scans also spread out
> the workload much better. Only get a small number of new problems to
> analyse a couple of times a week - never any real huge burden for the
> person managing the coverity scan & more likely to get others to help
> too if there's only a couple of things for them to look at instead of
> a list of 700+ to wade through. I think these contribute to make it
> practical for us to keep libvirt at zero coverity problems all the
> time.

That's a pretty comfy place to be with Coverity.

> If you set the current 700 issues you have reported as your baseline,
> then it is still practical to run coverity daily on QEMU. Just have
> it report only new issues, ignoring the backlog, and ensure those all
> get fixes posted the same day so the backlog doesn't grow. Deal with
> the historical backlog of issues separately as time allows.
>
> Also I'd suggest making "no new coverity issues" be a release blocker
> item so people see you are taking it seriously and so be encouraged
> to help out to ensure the release doesn't slip.

I wasn't bold enough to suggest "daily", let alone "release blocker".
I'd be fine with either, but I'd also be fine with baby steps in the
direction.

As Paolo pointed out, a good part of the backlog is in code nobody cares
enough about to actually maintain, yet somebody cares just enough to
protest violently when anyone suggests to ditch it.

[Qemu-devel] [PATCH V2] s390x/pci: avoid sign extension in stpcifc

[Frank Blaschka]

this patch avoids sign extension and fixes a data conversion
bug in stpcifc. Both issues where found by Coverity.

Signed-off-by: Frank Blaschka 
---
 hw/s390x/s390-pci-inst.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 5ea13e5..c269184 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -784,10 +784,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, 
uint64_t fiba)
 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
 
-data = (pbdev->isc << 28) | (pbdev->noi << 16) |
-   (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
-   pbdev->routes.adapter.summary_offset;
-stw_p(&fib.data, data);
+data = ((uint32_t)pbdev->isc << 28) | ((uint32_t)pbdev->noi << 16) |
+   ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
+   ((uint32_t)pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
+stl_p(&fib.data, data);
 
 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
 fib.fc |= 0x80;
-- 
2.1.4

Re: [Qemu-devel] Can we make better use of Coverity?

[Peter Maydell]

On 21 January 2015 at 15:55, Markus Armbruster  wrote:
> I wasn't bold enough to suggest "daily", let alone "release blocker".

I think the Coverity FAQ says we can't do more than 2
scans a week for a project of QEMU's size anyway...

-- PMM

[Qemu-devel] [PATCHv2 4/7] target-ppc: move sdr1 value change detection logic to helper_store_sdr1()

[Mark Cave-Ayland]

Otherwise when cpu_post_load calls ppc_store_sdr1() when restoring a VM
snapshot the value is deemed unchanged and so the internal env->htab*
variables aren't set correctly.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Paolo Bonzini 
---
 target-ppc/misc_helper.c |7 ++-
 target-ppc/mmu_helper.c  |   35 +++
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index a577b3a..6b12ca8 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -77,8 +77,13 @@ void helper_msr_facility_check(CPUPPCState *env, uint32_t 
bit,
 
 void helper_store_sdr1(CPUPPCState *env, target_ulong val)
 {
+PowerPCCPU *cpu = ppc_env_get_cpu(env);
+
 if (!env->external_htab) {
-ppc_store_sdr1(env, val);
+if (env->spr[SPR_SDR1] != val) {
+ppc_store_sdr1(env, val);
+tlb_flush(CPU(cpu), 1);
+}
 }
 }
 
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 660be7f..527c6ad 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2036,31 +2036,26 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, 
target_ulong addr)
 /* Special registers manipulation */
 void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
 {
-PowerPCCPU *cpu = ppc_env_get_cpu(env);
-
 qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
 assert(!env->external_htab);
-if (env->spr[SPR_SDR1] != value) {
-env->spr[SPR_SDR1] = value;
+env->spr[SPR_SDR1] = value;
 #if defined(TARGET_PPC64)
-if (env->mmu_model & POWERPC_MMU_64) {
-target_ulong htabsize = value & SDR_64_HTABSIZE;
+if (env->mmu_model & POWERPC_MMU_64) {
+target_ulong htabsize = value & SDR_64_HTABSIZE;
 
-if (htabsize > 28) {
-fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
-" stored in SDR1\n", htabsize);
-htabsize = 28;
-}
-env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
-env->htab_base = value & SDR_64_HTABORG;
-} else
-#endif /* defined(TARGET_PPC64) */
-{
-/* FIXME: Should check for valid HTABMASK values */
-env->htab_mask = ((value & SDR_32_HTABMASK) << 16) | 0x;
-env->htab_base = value & SDR_32_HTABORG;
+if (htabsize > 28) {
+fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
+" stored in SDR1\n", htabsize);
+htabsize = 28;
 }
-tlb_flush(CPU(cpu), 1);
+env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
+env->htab_base = value & SDR_64_HTABORG;
+} else
+#endif /* defined(TARGET_PPC64) */
+{
+/* FIXME: Should check for valid HTABMASK values */
+env->htab_mask = ((value & SDR_32_HTABMASK) << 16) | 0x;
+env->htab_base = value & SDR_32_HTABORG;
 }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 1/7] macio.c: include parent PCIDevice state in VMStateDescription

[Mark Cave-Ayland]

This ensures that the macio PCI device is correctly configured when restoring
from a VM snapshot.

Signed-off-by: Mark Cave-Ayland 
---
 hw/misc/macio/macio.c |   24 
 1 file changed, 24 insertions(+)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index e0f1e88..9bc3f2d 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -336,20 +336,44 @@ static void macio_instance_init(Object *obj)
 memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
 }
 
+static const VMStateDescription vmstate_macio_oldworld = {
+.name = "macio-oldworld",
+.version_id = 0,
+.minimum_version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(parent_obj.parent, OldWorldMacIOState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void macio_oldworld_class_init(ObjectClass *oc, void *data)
 {
 PCIDeviceClass *pdc = PCI_DEVICE_CLASS(oc);
+DeviceClass *dc = DEVICE_CLASS(oc);
 
 pdc->init = macio_oldworld_initfn;
 pdc->device_id = PCI_DEVICE_ID_APPLE_343S1201;
+dc->vmsd = &vmstate_macio_oldworld;
 }
 
+static const VMStateDescription vmstate_macio_newworld = {
+.name = "macio-newworld",
+.version_id = 0,
+.minimum_version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(parent_obj.parent, NewWorldMacIOState),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void macio_newworld_class_init(ObjectClass *oc, void *data)
 {
 PCIDeviceClass *pdc = PCI_DEVICE_CLASS(oc);
+DeviceClass *dc = DEVICE_CLASS(oc);
 
 pdc->init = macio_newworld_initfn;
 pdc->device_id = PCI_DEVICE_ID_APPLE_UNI_N_KEYL;
+dc->vmsd = &vmstate_macio_newworld;
 }
 
 static Property macio_properties[] = {
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 0/7] ppc: loadvm/savevm fixups for -M g3beige and -M mac99

[Mark Cave-Ayland]

This patchset fixes up various bugs in loadvm/savevm for -M g3beige and
-M mac99 so that it is becomes possible to save and restore image snapshots.

The focus of this patchset is on -M g3beige since this matches the majority
of my test images, but there were some easy fixes to be made to -M mac99
at the same time.

With this patchset applied both -M g3beige and -M mac99 images can be
saved/restored whilst booted into OpenBIOS with no issues. I tested -M g3beige
with a paused, disk-inactive Darwin 6 image and was able to resume
successfully which was good enough for my needs.

I noticed some hangs can still occur when trying to restore an image
where the disk is active which makes me believe that there is still some
extra macio/dbdma state which needs to be included if someone is interested
enough to pursue this further.

Most of the patches are straightforward except for patch 4 which came out of
a discussion on-list between Alex and Paolo, and patch 5 which is a similar
error except this time for the MSR register. I suspect patch 5 can be
improved by someone with more PPC knowledge than myself.

Signed-off-by: Mark Cave-Ayland 

v2:
- Minor subject line changes for patches 4+5
- Update patches 4+5 based upon feedback from Paolo
- Fix line width exceeding 80 characters in patch 2


Mark Cave-Ayland (7):
  macio.c: include parent PCIDevice state in VMStateDescription
  adb.c: include ADBDevice parent state in KBDState and MouseState
  cuda.c: include adb_poll_timer in VMStateDescription
  target-ppc: move sdr1 value change detection logic to
helper_store_sdr1()
  target-ppc: force update of msr bits in cpu_post_load
  openpic: fix segfault on -M mac99 savevm
  openpic: fix up loadvm under -M mac99

 hw/input/adb.c   |   22 ++
 hw/intc/openpic.c|   10 --
 hw/misc/macio/cuda.c |5 +++--
 hw/misc/macio/macio.c|   24 
 target-ppc/machine.c |8 +++-
 target-ppc/misc_helper.c |7 ++-
 target-ppc/mmu_helper.c  |   35 +++
 7 files changed, 77 insertions(+), 34 deletions(-)

-- 
1.7.10.4