Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support

[gengdongjiu]

Michael,
   very sorry for the late response due to recently busy.


On 2017/5/13 7:59, Michael S. Tsirkin wrote:
> On Sun, Apr 30, 2017 at 01:35:03PM +0800, Dongjiu Geng wrote:
>> This implements APEI GHES Table by passing the error cper info
>> to the guest via a fw_cfg_blob. After a CPER info is added, an
>> SEA/SEI exception will be injected into the guest OS.
>>
>> Below is the table layout, the max number of error soure is 11,
>> which is classified by notification type.
>>
>> etc/acpi/tables etc/hardware_errors
>>  ==
>>  +---+
>> +--+ | address   | +-> +--+
>> |HEST  + | registers | |   | Error Status |
>> + ++ | +-+ |   | Data Block 1 |
>> | | GHES1  | --> | |address1 | +   | ++
>> | | GHES2  | --> | |address2 | --+ | |  CPER  |
>> | | GHES3  | --> | |address3 | + | | |  CPER  |
>> | |    | --> | | ... | | | | |  CPER  |
>> | | GHES10 | --> | |address10| -+  | | | |  CPER  |
>> +-++ +-+-+  |  | | +-++
>> |  | |
>> |  | +---> +--+
>> |  |   | Error Status |
>> |  |   | Data Block 2 |
>> |  |   | ++
>> |  |   | |  CPER  |
>> |  |   | |  CPER  |
>> |  |   +-++
>> |  |
>> |  +-> +--+
>> |  | Error Status |
>> |  | Data Block 3 |
>> |  | ++
>> |  | |  CPER  |
>> |  +-++
>> |...
>> +> +--+
>>| Error Status |
>>| Data Block 10|
>>| ++
>>| |  CPER  |
>>| |  CPER  |
>>| |  CPER  |
>>+-++
>>
>> Signed-off-by: Dongjiu Geng 
>> ---
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/acpi/Makefile.objs   |   1 +
>>  hw/acpi/aml-build.c |   2 +
>>  hw/acpi/hest_ghes.c | 203 +++
>>  hw/arm/virt-acpi-build.c|   6 ++
>>  include/hw/acpi/acpi-defs.h | 227 
>> 
>>  include/hw/acpi/aml-build.h |   1 +
>>  include/hw/acpi/hest_ghes.h |  43 
>>  8 files changed, 484 insertions(+)
>>  create mode 100644 hw/acpi/hest_ghes.c
>>  create mode 100644 include/hw/acpi/hest_ghes.h
>>
>> diff --git a/default-configs/arm-softmmu.mak 
>> b/default-configs/arm-softmmu.mak
>> index 1e3bd2b..d5f1552 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
>>  CONFIG_SMBIOS=y
>>  CONFIG_ASPEED_SOC=y
>>  CONFIG_GPIO_KEY=y
>> +CONFIG_ACPI_APEI_GENERATION=y
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 11c35bc..776b46e 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>>  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>>  common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>>  common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
>>  common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>  
>>  common-obj-y += acpi_interface.o
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index c6f2032..802b98d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1560,6 +1560,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
>>  tables->table_data = g_array_new(false, true /* clear */, 1);
>>  tables->tcpalog = g_array_new(false, true /* clear */, 1);
>>  tables->vmgenid = g_array_new(false, true /* clear */, 1);
>> +tables->hardware_errors = g_array_new(false, true /* clear */, 1);
>>  tables->linker = bios_linker_loader_init();
>>  }
>>  
>> @@ -1570,6 +1571,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables 
>> *tables, bool 

Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support

[gengdongjiu]

Laszlo,
   sorry for the late response.

On 2017/5/13 5:00, Laszlo Ersek wrote:
> On 04/30/17 07:35, Dongjiu Geng wrote:
>> This implements APEI GHES Table by passing the error cper info
>> to the guest via a fw_cfg_blob. After a CPER info is added, an
>> SEA/SEI exception will be injected into the guest OS.
>>
>> Below is the table layout, the max number of error soure is 11,
>> which is classified by notification type.
>>
>> etc/acpi/tables etc/hardware_errors
>>  ==
>>  +---+
>> +--+ | address   | +-> +--+
>> |HEST  + | registers | |   | Error Status |
>> + ++ | +-+ |   | Data Block 1 |
>> | | GHES1  | --> | |address1 | +   | ++
>> | | GHES2  | --> | |address2 | --+ | |  CPER  |
>> | | GHES3  | --> | |address3 | + | | |  CPER  |
>> | |    | --> | | ... | | | | |  CPER  |
>> | | GHES10 | --> | |address10| -+  | | | |  CPER  |
>> +-++ +-+-+  |  | | +-++
>> |  | |
>> |  | +---> +--+
>> |  |   | Error Status |
>> |  |   | Data Block 2 |
>> |  |   | ++
>> |  |   | |  CPER  |
>> |  |   | |  CPER  |
>> |  |   +-++
>> |  |
>> |  +-> +--+
>> |  | Error Status |
>> |  | Data Block 3 |
>> |  | ++
>> |  | |  CPER  |
>> |  +-++
>> |...
>> +> +--+
>>| Error Status |
>>| Data Block 10|
>>| ++
>>| |  CPER  |
>>| |  CPER  |
>>| |  CPER  |
>>+-++
>>
>> Signed-off-by: Dongjiu Geng 
>> ---
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/acpi/Makefile.objs   |   1 +
>>  hw/acpi/aml-build.c |   2 +
>>  hw/acpi/hest_ghes.c | 203 +++
>>  hw/arm/virt-acpi-build.c|   6 ++
>>  include/hw/acpi/acpi-defs.h | 227 
>> 
>>  include/hw/acpi/aml-build.h |   1 +
>>  include/hw/acpi/hest_ghes.h |  43 
>>  8 files changed, 484 insertions(+)
>>  create mode 100644 hw/acpi/hest_ghes.c
>>  create mode 100644 include/hw/acpi/hest_ghes.h
> 
> Disclaimer: I'm not an ACPI (or any kind of) QEMU maintainer, so I can
> only share my personal opinion.
  I know it, I appreciated that you can find your free time to review it.

> 
> (1) This patch is too big. It should be split in two parts at least.
> 
> The first patch should contain the new ACPI structures and macros. The
> second patch should contain the generation feature.
   OK, have splited it.

> 
> I'll reorder the diff in my response.
  thanks.

> 
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 4cc3630..27adede 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable 
>> AcpiMultipleApicTable;
>>  #define ACPI_APIC_GENERIC_TRANSLATOR15
>>  #define ACPI_APIC_RESERVED  16   /* 16 and greater are reserved 
>> */
>>
> 
> (2) Please add a comment above the following macros: they come from the
> UEFI Spec 2.6, "N.2.5 Memory Error Section".
  good suggestion.

> 
>> +#define CPER_MEM_VALID_ERROR_STATUS 0x0001
>> +#define CPER_MEM_VALID_PA   0x0002
>> +#define CPER_MEM_VALID_PA_MASK  0x0004
>> +#define CPER_MEM_VALID_NODE 0x0008
>> +#define CPER_MEM_VALID_CARD 0x0010
>> +#define CPER_MEM_VALID_MODULE   0x0020
>> +#define CPER_MEM_VALID_BANK 0x0040
>> +#define CPER_MEM_VALID_DEVICE   0x0080
>> +#define CPER_MEM_VALID_ROW  0x0100
>> +#define CPER_MEM_VALID_COLUMN   0x0200
>> +#define CPER_MEM_VALID_BIT_POSITION 0x0400
>> +#define 

[Qemu-devel] [Bug 888150] Re: qemu and qemu.git -> Migration + disk stress introduces qcow2 corruptions

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/888150

Title:
  qemu and qemu.git -> Migration + disk stress introduces qcow2
  corruptions

Status in QEMU:
  Expired

Bug description:
  Hi guys, here I am, reporting yet another issue with qemu. This time,
  it's something that was first reported in January, and Juan proposed a
  patch for it:

  http://comments.gmane.org/gmane.comp.emulators.qemu/89009

  [PATCH 4/5] Reopen files after migration

  The symptom is, when running disk stress or any intense IO operation
  in guest while migrating it causes a qcow2 corruption. We've seen this
  consistently on the daily test jobs, both for qemu and qemu-kvm. The
  test that triggers it is autotest stress test running on a VM with
  ping-pong background migration.

  The fix proposed by Juan is on our RHEL branch and such a problem does
  not happen on the RHEL branch. So, what about re-considering Juan's
  patch, or maybe work out a solution that is satisfactory for the
  upstream maintainers?

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/888150/+subscriptions

Re: [Qemu-devel] [PATCH] oslib: strip trailing '\n' from error_setg() string argument

[Philippe Mathieu-Daudé]

On 05/17/2017 09:48 AM, Stefan Hajnoczi wrote:

On Mon, May 15, 2017 at 09:11:49PM -0300, Philippe Mathieu-Daudé wrote:

spotted by Coccinelle script scripts/coccinelle/err-bad-newline.cocci

Signed-off-by: Philippe Mathieu-Daudé 
---
 util/oslib-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


Please send multi-patch series with a cover letter and proper patch
numbering ([PATCH 0/2], [PATCH 1/2], [PATCH 2/2]).  git-format-patch and
git-send-email normally do this.  It's important so that patch
management tools and continuous integration systems can track your
patches.


Hi Stefan, I intended one command to generate unrelated patches from the 
same branch at once and send them individually without cover letter. I 
didn't noticed they'd go as a serie. I'll double-check next time!



Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block


Hmm I can't see it there maybe this part was not for this patch, I'll 
resend it soon with r-b tags.




Stefan



Regards,

Phil.

Re: [Qemu-devel] [PATCH] coccinelle: fix typo in comment

[Philippe Mathieu-Daudé]

On 05/16/2017 11:35 AM, Eric Blake wrote:

On 05/15/2017 07:11 PM, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/coccinelle/return_directly.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


Did you intend for this to be threaded with your other 'oslib: strip
trailing '\n'...' patch? If so, where's the 0/2 cover letter and proper
subject lines?


Hi Eric, I think I did a mistake in my command, I intended to generate 
the patches in the same branch but wanted to send them as unrelated 
(without cover). I didn't noticed while sending until your remark, I'll 
double-check next time!




At any rate,
Reviewed-by: Eric Blake 


Thanks!





diff --git a/scripts/coccinelle/return_directly.cocci 
b/scripts/coccinelle/return_directly.cocci
index 48680f2c2a..4cf50e75ea 100644
--- a/scripts/coccinelle/return_directly.cocci
+++ b/scripts/coccinelle/return_directly.cocci
@@ -1,4 +1,4 @@
-// replace 'R = X; return R;' with 'return R;'
+// replace 'R = X; return R;' with 'return X;'
 @@
 identifier VAR;
 expression E;

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Philippe Mathieu-Daudé]

Hi Andrew,

On 05/19/2017 09:26 PM, Andrew Jeffery wrote:

This model implements enough behaviour to do basic functionality tests
such as device initialisation and read out of dummy sample values. The
sample value generation strategy is similar to the STM ADC already in
the tree.

Signed-off-by: Andrew Jeffery 
---
 hw/adc/Makefile.objs|   1 +
 hw/adc/aspeed_adc.c | 246 
 include/hw/adc/aspeed_adc.h |  33 ++
 3 files changed, 280 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

diff --git a/hw/adc/Makefile.objs b/hw/adc/Makefile.objs
index 3f6dfdedaec7..2bf9362ba3c4 100644
--- a/hw/adc/Makefile.objs
+++ b/hw/adc/Makefile.objs
@@ -1 +1,2 @@
 obj-$(CONFIG_STM32F2XX_ADC) += stm32f2xx_adc.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_adc.o
diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
new file mode 100644
index ..d08f1684f7bc
--- /dev/null
+++ b/hw/adc/aspeed_adc.c
@@ -0,0 +1,246 @@
+/*
+ * Aspeed ADC
+ *
+ * Andrew Jeffery 
+ *
+ * Copyright 2017 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/adc/aspeed_adc.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+
+#define ASPEED_ADC_ENGINE_CTRL  0x00
+#define  ASPEED_ADC_ENGINE_CH_EN_MASK   0x
+#define   ASPEED_ADC_ENGINE_CH_EN(x)((BIT(x)) << 16)
+#define  ASPEED_ADC_ENGINE_INIT BIT(8)
+#define  ASPEED_ADC_ENGINE_AUTO_COMPBIT(5)
+#define  ASPEED_ADC_ENGINE_COMP BIT(4)
+#define  ASPEED_ADC_ENGINE_MODE_MASK0x000e
+#define   ASPEED_ADC_ENGINE_MODE_OFF(0b000 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_STANDBY(0b001 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_NORMAL (0b111 << 1)
+#define  ASPEED_ADC_ENGINE_EN   BIT(0)
+
+#define ASPEED_ADC_L_MASK   ((1 << 10) - 1)
+#define ASPEED_ADC_L(x) ((x) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_H(x) (((x) >> 16) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_LH_MASK  (ASPEED_ADC_L_MASK << 16 | ASPEED_ADC_L_MASK)
+
+static inline uint32_t update_channels(uint32_t current)
+{
+const uint32_t next = (current + 7) & 0x3ff;
+
+return (next << 16) | next;
+}
+
+static bool breaks_threshold(AspeedADCState *s, int ch_off)
+{
+const uint32_t a = ASPEED_ADC_L(s->channels[ch_off]);
+const uint32_t a_lower = ASPEED_ADC_L(s->bounds[2 * ch_off]);
+const uint32_t a_upper = ASPEED_ADC_H(s->bounds[2 * ch_off]);
+const uint32_t b = ASPEED_ADC_H(s->channels[ch_off]);
+const uint32_t b_lower = ASPEED_ADC_L(s->bounds[2 * ch_off + 1]);
+const uint32_t b_upper = ASPEED_ADC_H(s->bounds[2 * ch_off + 1]);
+
+return ((a < a_lower || a > a_upper)) ||
+   ((b < b_lower || b > b_upper));
+}
+
+static uint32_t read_channel_sample(AspeedADCState *s, int ch_off)
+{
+uint32_t ret;
+
+/* Poor man's sampling */
+ret = s->channels[ch_off];
+s->channels[ch_off] = update_channels(s->channels[ch_off]);
+
+if (breaks_threshold(s, ch_off)) {
+qemu_irq_raise(s->irq);
+}
+
+return ret;
+}
+
+#define TO_INDEX(addr, base) (((addr) - (base)) >> 2)
+
+static uint64_t aspeed_adc_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+AspeedADCState *s = ASPEED_ADC(opaque);
+uint64_t ret;
+
+switch (addr) {
+case 0x00:
+ret = s->engine_ctrl;
+break;
+case 0x04:
+ret = s->irq_ctrl;
+break;
+case 0x08:
+ret = s->vga_detect_ctrl;
+break;
+case 0x0c:
+ret = s->adc_clk_ctrl;
+break;
+case 0x10 ... 0x2e:
+ret = read_channel_sample(s, TO_INDEX(addr, 0x10));
+break;
+case 0x30 ... 0x6e:
+ret = s->bounds[TO_INDEX(addr, 0x30)];
+break;
+case 0x70 ... 0xae:
+ret = s->hysteresis[TO_INDEX(addr, 0x70)];
+break;
+case 0xc0:
+ret = s->irq_src;
+break;
+case 0xc4:
+ret = s->comp_trim;
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "%s: addr: 0x%lx, size: %u\n", __func__, addr,
+size);
+ret = 0;
+break;
+}
+
+return ret;
+}
+
+static void aspeed_adc_write(void *opaque, hwaddr addr,
+   uint64_t val, unsigned int size)
+{
+AspeedADCState *s = ASPEED_ADC(opaque);
+
+switch (addr) {
+case 0x00:
+{
+uint32_t init;
+
+init = !!(val & ASPEED_ADC_ENGINE_EN);
+init *= ASPEED_ADC_ENGINE_INIT;
+
+val &= ~ASPEED_ADC_ENGINE_INIT;
+val |= init;
+}
+
+val &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
+s->engine_ctrl = val;
+
+break;
+case 0x04:
+s->irq_ctrl = val;
+break;
+

[Qemu-devel] [RFC 3/3] qmp: Include parent types on 'qom-list-types' output

[Eduardo Habkost]

Include list of parent types of each type on 'qom-list-types' output.

Without this, there's no way to figure out the parents of a given type
without making additional 'qom-list-types' queries.

In addition to the test case for the new feature, update the
abstract-interface test case to use the new field and avoid the
"qom-list-types implements=object" trick.

Signed-off-by: Eduardo Habkost 
---
I would to include a "interfaces" field in the future, too, but
this would require extending the core QOM API to allow that.
---
 qapi-schema.json   |  6 -
 qmp.c  | 16 +++
 tests/device-introspect-test.c | 61 +++---
 3 files changed, 72 insertions(+), 11 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index cbf1e2a837..169dfd2d7f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3037,12 +3037,16 @@
 # @abstract: the type is abstract and can't be directly instantiated
 #(since 2.10)
 #
+# @parent-types: parent types.  Parent types implemented by the type.  Note 
that
+#   this doesn't include interface names also implemented by the type.
+#   (since 2.10)
+#
 # Since: 1.1
 #
 # Notes: This command is experimental and may change syntax in future releases.
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str', 'abstract': 'bool' } }
+  'data': { 'name': 'str', 'abstract': 'bool', 'parent-types': [ 'str' ] } }
 
 ##
 # @qom-list-types:
diff --git a/qmp.c b/qmp.c
index 8066705dbe..5b50f6f650 100644
--- a/qmp.c
+++ b/qmp.c
@@ -436,6 +436,21 @@ void qmp_change(const char *device, const char *target,
 }
 }
 
+static strList *qom_type_get_parents(ObjectClass *klass)
+{
+ObjectClass *parent = object_class_get_parent(klass);
+strList *r = NULL;
+
+if (!parent) {
+return NULL;
+}
+
+r = g_new0(strList, 1);
+r->value = g_strdup(object_class_get_name(parent));
+r->next = qom_type_get_parents(parent);
+return r;
+}
+
 static void qom_list_types_tramp(ObjectClass *klass, void *data)
 {
 ObjectTypeInfoList *e, **pret = data;
@@ -444,6 +459,7 @@ static void qom_list_types_tramp(ObjectClass *klass, void 
*data)
 info = g_malloc0(sizeof(*info));
 info->name = g_strdup(object_class_get_name(klass));
 info->abstract = object_class_is_abstract(klass);
+info->parent_types = qom_type_get_parents(klass);
 
 e = g_malloc0(sizeof(*e));
 e->value = info;
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index d5aacb4ed1..350b862f3b 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -61,6 +61,20 @@ static QDict *type_list_find(QList *types, const char *name)
 return NULL;
 }
 
+static bool str_list_has(QList *strlist, const char *str)
+{
+QListEntry *e;
+
+QLIST_FOREACH_ENTRY(strlist, e) {
+const char *s = 
qstring_get_str(qobject_to_qstring(qlist_entry_obj(e)));
+if (!strcmp(s, str)) {
+return true;
+}
+}
+
+return false;
+}
+
 static QList *device_type_list(bool abstract)
 {
 return qom_list_types("device", abstract);
@@ -103,6 +117,31 @@ static void test_device_intro_list(void)
 qtest_end();
 }
 
+/*
+ * Ensure all entries returned by qom-list-types implements=
+ * have  in parent-types.
+ */
+static void test_qom_list_parents(const char *parent)
+{
+QList *types;
+QListEntry *e;
+
+types = qom_list_types(parent, true);
+
+QLIST_FOREACH_ENTRY(types, e) {
+QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+
+/* The parent type being queried is included in the list too, skip it 
*/
+if (!strcmp(qdict_get_str(d, "name"), parent)) {
+continue;
+}
+
+g_assert(str_list_has(qdict_get_qlist(d, "parent-types"), parent));
+}
+
+QDECREF(types);
+}
+
 static void test_qom_list_fields(void)
 {
 QList *all_types;
@@ -123,6 +162,10 @@ static void test_qom_list_fields(void)
 g_assert(abstract == expected_abstract);
 }
 
+test_qom_list_parents("object");
+test_qom_list_parents("device");
+test_qom_list_parents("sys-bus-device");
+
 QDECREF(all_types);
 QDECREF(non_abstract);
 qtest_end();
@@ -165,23 +208,22 @@ static void test_device_intro_concrete(void)
 static void test_abstract_interfaces(void)
 {
 QList *all_types;
-QList *obj_types;
 QListEntry *e;
 
 qtest_start(common_args);
-/*
- * qom-list-types implements=interface returns all types that
- * implement _any_ interface (not just interface types), but
- * we can filter them out because interfaces don't implement
- * the "object" type.
- */
+
 all_types = qom_list_types("interface", true);
-obj_types = qom_list_types("object", true);
 
 QLIST_FOREACH_ENTRY(all_types, e) {
 QDict *d = qobject_to_qdict(qlist_entry_obj(e));
 
-if (type_list_find(obj_types, 

[Qemu-devel] [RFC 2/3] qmp: Include 'abstract' field on 'qom-list-types' output

[Eduardo Habkost]

A client may be interested in getting the list of both abstract and
non-abstract types.  Instead of requiring them to make multiple queries
with different filter arguments, just return an 'abstract' field in
'qom-list-types'.

In addition to the new test code for validating this field, update the
abstract-interfaces test case to query for all 'interface' subtypes
(including abstract ones), and to look at the 'abstract' field directly.

Signed-off-by: Eduardo Habkost 
---
 qapi-schema.json   |  5 -
 qmp.c  |  1 +
 tests/device-introspect-test.c | 51 +++---
 3 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 80603cfc51..cbf1e2a837 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3034,12 +3034,15 @@
 #
 # @name: the type name found in the search
 #
+# @abstract: the type is abstract and can't be directly instantiated
+#(since 2.10)
+#
 # Since: 1.1
 #
 # Notes: This command is experimental and may change syntax in future releases.
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', 'abstract': 'bool' } }
 
 ##
 # @qom-list-types:
diff --git a/qmp.c b/qmp.c
index f656940769..8066705dbe 100644
--- a/qmp.c
+++ b/qmp.c
@@ -443,6 +443,7 @@ static void qom_list_types_tramp(ObjectClass *klass, void 
*data)
 
 info = g_malloc0(sizeof(*info));
 info->name = g_strdup(object_class_get_name(klass));
+info->abstract = object_class_is_abstract(klass);
 
 e = g_malloc0(sizeof(*e));
 e->value = info;
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index f1e6dddfa3..d5aacb4ed1 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -103,6 +103,31 @@ static void test_device_intro_list(void)
 qtest_end();
 }
 
+static void test_qom_list_fields(void)
+{
+QList *all_types;
+QList *non_abstract;
+QListEntry *e;
+
+qtest_start(common_args);
+
+all_types = qom_list_types(NULL, true);
+non_abstract = qom_list_types(NULL, false);
+
+QLIST_FOREACH_ENTRY(all_types, e) {
+QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+const char *name = qdict_get_str(d, "name");
+bool abstract = qdict_get_bool(d, "abstract");
+bool expected_abstract = !type_list_find(non_abstract, name);
+
+g_assert(abstract == expected_abstract);
+}
+
+QDECREF(all_types);
+QDECREF(non_abstract);
+qtest_end();
+}
+
 static void test_device_intro_none(void)
 {
 qtest_start(common_args);
@@ -144,25 +169,24 @@ static void test_abstract_interfaces(void)
 QListEntry *e;
 
 qtest_start(common_args);
-/* qom-list-types implements=interface would return any type
- * that implements _any_ interface (not just interface types),
- * so use a trick to find the interface type names:
- * - list all object types
- * - list all types, and look for items that are not
- *   on the first list
+/*
+ * qom-list-types implements=interface returns all types that
+ * implement _any_ interface (not just interface types), but
+ * we can filter them out because interfaces don't implement
+ * the "object" type.
  */
-all_types = qom_list_types(NULL, false);
-obj_types = qom_list_types("object", false);
+all_types = qom_list_types("interface", true);
+obj_types = qom_list_types("object", true);
 
 QLIST_FOREACH_ENTRY(all_types, e) {
 QDict *d = qobject_to_qdict(qlist_entry_obj(e));
 
-/*
- * An interface (incorrectly) marked as non-abstract would
- * appear on all_types, but not on obj_types:
- */
-g_assert(type_list_find(obj_types, qdict_get_str(d, "name")));
+if (type_list_find(obj_types, qdict_get_str(d, "name"))) {
+/* Not an interface type */
+continue;
+}
 
+g_assert(qdict_get_bool(d, "abstract"));
 }
 
 QDECREF(all_types);
@@ -175,6 +199,7 @@ int main(int argc, char **argv)
 g_test_init(, , NULL);
 
 qtest_add_func("device/introspect/list", test_device_intro_list);
+qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
 qtest_add_func("device/introspect/none", test_device_intro_none);
 qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
 qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 1/3] tests: Simplify abstract-interfaces check with a helper

[Eduardo Habkost]

Add a new type_list_find() helper to device-introspect-test.c, to
simplify the code at test_abstract_interfaces().

Signed-off-by: Eduardo Habkost 
---
 tests/device-introspect-test.c | 43 --
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index b1abb2ad89..f1e6dddfa3 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -45,6 +45,22 @@ static QList *qom_list_types(const char *implements, bool 
abstract)
 return ret;
 }
 
+/* Find an entry on a list returned by qom-list-types */
+static QDict *type_list_find(QList *types, const char *name)
+{
+QListEntry *e;
+
+QLIST_FOREACH_ENTRY(types, e) {
+QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+const char *ename = qdict_get_str(d, "name");
+if (!strcmp(ename, name)) {
+return d;
+}
+}
+
+return NULL;
+}
+
 static QList *device_type_list(bool abstract)
 {
 return qom_list_types("device", abstract);
@@ -125,7 +141,7 @@ static void test_abstract_interfaces(void)
 {
 QList *all_types;
 QList *obj_types;
-QListEntry *ae;
+QListEntry *e;
 
 qtest_start(common_args);
 /* qom-list-types implements=interface would return any type
@@ -138,24 +154,15 @@ static void test_abstract_interfaces(void)
 all_types = qom_list_types(NULL, false);
 obj_types = qom_list_types("object", false);
 
-QLIST_FOREACH_ENTRY(all_types, ae) {
-QDict *at = qobject_to_qdict(qlist_entry_obj(ae));
-const char *aname = qdict_get_str(at, "name");
-QListEntry *oe;
-const char *found = NULL;
-
-QLIST_FOREACH_ENTRY(obj_types, oe) {
-QDict *ot = qobject_to_qdict(qlist_entry_obj(oe));
-const char *oname = qdict_get_str(ot, "name");
-if (!strcmp(aname, oname)) {
-found = oname;
-break;
-}
-}
+QLIST_FOREACH_ENTRY(all_types, e) {
+QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+
+/*
+ * An interface (incorrectly) marked as non-abstract would
+ * appear on all_types, but not on obj_types:
+ */
+g_assert(type_list_find(obj_types, qdict_get_str(d, "name")));
 
-/* Using g_assert_cmpstr() will give more useful failure
- * messages than g_assert(found) */
-g_assert_cmpstr(aname, ==, found);
 }
 
 QDECREF(all_types);
-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 0/3] qmp: Return extra information on qom-list-types

[Eduardo Habkost]

This series adds 'abstract' and 'parent-types' fields to the
output of qom-list-types.

For reference, below are the sizes of the output of
"qom-list-types abstract=true" on qemu-system-x86_64, before and
after applying the patches:

* before: 11724 bytes
* with 'abstract' field: 20119 bytes
* with 'abstract' and 'parent-types': 44383 bytes

I'm not sure if extending qom-list-types with this info is the
right way to go, or if we should keep qom-list-types as-is and
add a new "query-qom-type" QMP command to query info on one QOM
type at a time.

Eduardo Habkost (3):
  tests: Simplify abstract-interfaces check with a helper
  qmp: Include 'abstract' field on 'qom-list-types' output
  qmp: Include parent types on 'qom-list-types' output

 qapi-schema.json   |   9 ++-
 qmp.c  |  17 ++
 tests/device-introspect-test.c | 131 -
 3 files changed, 127 insertions(+), 30 deletions(-)

-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 0/2] hw/adc: Implement a basic Aspeed ADC model

[Andrew Jeffery]

On Fri, 2017-05-19 at 17:51 -0700, no-re...@patchew.org wrote:
> In file included from /tmp/qemu-test/src/hw/adc/aspeed_adc.c:15:0:
> /tmp/qemu-test/src/hw/adc/aspeed_adc.c: In function 'aspeed_adc_read':
> /tmp/qemu-test/src/hw/adc/aspeed_adc.c:106:34: error: format '%lx' expects 
> argument of type 'long unsigned int', but argument 3 has type 'hwaddr {aka 
> long long unsigned int}' [-Werror=format=]
>  qemu_log_mask(LOG_UNIMP, "%s: addr: 0x%lx, size: %u\n", __func__, 
> addr,
>   ^
> /tmp/qemu-test/src/include/qemu/log.h:94:22: note: in definition of macro 
> 'qemu_log_mask'
>  qemu_log(FMT, ## __VA_ARGS__);  \
>   ^~~
> /tmp/qemu-test/src/hw/adc/aspeed_adc.c: In function 'aspeed_adc_write':
> /tmp/qemu-test/src/hw/adc/aspeed_adc.c:162:34: error: format '%lu' expects 
> argument of type 'long unsigned int', but argument 3 has type 'hwaddr {aka 
> long long unsigned int}' [-Werror=format=]
>  qemu_log_mask(LOG_UNIMP, "%s: %lu\n", __func__, addr);
>   ^
> /tmp/qemu-test/src/include/qemu/log.h:94:22: note: in definition of macro 
> 'qemu_log_mask'
>  qemu_log(FMT, ## __VA_ARGS__);  \
>   ^~~
> cc1: all warnings being treated as errors
> /tmp/qemu-test/src/rules.mak:69: recipe for target 'hw/adc/aspeed_adc.o' 
> failed
> make[1]: *** [hw/adc/aspeed_adc.o] Error 1
> make[1]: *** Waiting for unfinished jobs
>   CC  aarch64-softmmu/hw/scsi/virtio-scsi-dataplane.o
> Makefile:327: recipe for target 'subdir-aarch64-softmmu' failed
> make: *** [subdir-aarch64-softmmu] Error 2
> make: *** Waiting for unfinished jobs
>   GEN x86_64-softmmu/qemu-system-x86_64.exe
> tests/docker/Makefile.include:118: recipe for target 'docker-run' failed
> make[1]: *** [docker-run] Error 2
> make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-i73ef6w0/src'
> tests/docker/Makefile.include:149: recipe for target 
> 'docker-run-test-mingw@fedora' failed
> > make: *** [docker-run-test-mingw@fedora] Error 2
> === OUTPUT END ===
> 
> Test command exited with code: 2

I'll fix the format string issues in v2 along with any other issues
identified in review.

Separately, is it necessary for the bot to send the entirety of the
build output? Can we make it a bit smarter to send just the error with
some small amount of context? Maybe send the full log as an attachment?
Or host it and link to it from the email? It's a bit tedious having to
scroll through all that output to find the error at the bottom.

Cheers,

Andrew

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] Migration downtime more than 5s when migrating guest with massive disks

[Yang Hongyang]

On 2017/5/20 0:46, Paolo Bonzini wrote:
> 
> 
> On 19/05/2017 14:17, Gonglei (Arei) wrote:
>>> It would.  Right now memory_region_transaction_commit() is roughly
>>> O(n^2) (n devices * n BARs), and there are n of them.
>>>
>>> Reducing memory_region_transaction_commit to O(n) would be a large
>>> change.  One idea is to share the AddressSpaceDispatch for AddressSpaces
>>> that have the same root memory region (after resolving aliases).  The
>>> starting point would be to change mem_begin/mem_commit/mem_add from a
>>> MemoryListener to an loop on the FlatView, storing the
>>> AddressSpaceDispatch in the FlatView.
>>>
>> How about do O(1) for stopping stage of live migration? 
>> Because the cpu is stopped in this phase, it wouldn't cause
>> side effects IMHO, right? 
> 
> O(1) update is probably not quite possible, but it's possible to do one
> O(n) update.  We tried doing that, but something in virtio broke.

How were you do that? did you put extra begin/commit before/after destination
guest load, so that the destination guest only do one update(of n devs) 
throughout
the whole migration?

>  I
> don't remember the details.
> 
> Paolo
> 

-- 
Thanks,
Yang

[Qemu-devel] [PATCH 1/2] hw/adc: Add basic Aspeed ADC model

[Andrew Jeffery]

This model implements enough behaviour to do basic functionality tests
such as device initialisation and read out of dummy sample values. The
sample value generation strategy is similar to the STM ADC already in
the tree.

Signed-off-by: Andrew Jeffery 
---
 hw/adc/Makefile.objs|   1 +
 hw/adc/aspeed_adc.c | 246 
 include/hw/adc/aspeed_adc.h |  33 ++
 3 files changed, 280 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

diff --git a/hw/adc/Makefile.objs b/hw/adc/Makefile.objs
index 3f6dfdedaec7..2bf9362ba3c4 100644
--- a/hw/adc/Makefile.objs
+++ b/hw/adc/Makefile.objs
@@ -1 +1,2 @@
 obj-$(CONFIG_STM32F2XX_ADC) += stm32f2xx_adc.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_adc.o
diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
new file mode 100644
index ..d08f1684f7bc
--- /dev/null
+++ b/hw/adc/aspeed_adc.c
@@ -0,0 +1,246 @@
+/*
+ * Aspeed ADC
+ *
+ * Andrew Jeffery 
+ *
+ * Copyright 2017 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/adc/aspeed_adc.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+
+#define ASPEED_ADC_ENGINE_CTRL  0x00
+#define  ASPEED_ADC_ENGINE_CH_EN_MASK   0x
+#define   ASPEED_ADC_ENGINE_CH_EN(x)((BIT(x)) << 16)
+#define  ASPEED_ADC_ENGINE_INIT BIT(8)
+#define  ASPEED_ADC_ENGINE_AUTO_COMPBIT(5)
+#define  ASPEED_ADC_ENGINE_COMP BIT(4)
+#define  ASPEED_ADC_ENGINE_MODE_MASK0x000e
+#define   ASPEED_ADC_ENGINE_MODE_OFF(0b000 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_STANDBY(0b001 << 1)
+#define   ASPEED_ADC_ENGINE_MODE_NORMAL (0b111 << 1)
+#define  ASPEED_ADC_ENGINE_EN   BIT(0)
+
+#define ASPEED_ADC_L_MASK   ((1 << 10) - 1)
+#define ASPEED_ADC_L(x) ((x) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_H(x) (((x) >> 16) & ASPEED_ADC_L_MASK)
+#define ASPEED_ADC_LH_MASK  (ASPEED_ADC_L_MASK << 16 | ASPEED_ADC_L_MASK)
+
+static inline uint32_t update_channels(uint32_t current)
+{
+const uint32_t next = (current + 7) & 0x3ff;
+
+return (next << 16) | next;
+}
+
+static bool breaks_threshold(AspeedADCState *s, int ch_off)
+{
+const uint32_t a = ASPEED_ADC_L(s->channels[ch_off]);
+const uint32_t a_lower = ASPEED_ADC_L(s->bounds[2 * ch_off]);
+const uint32_t a_upper = ASPEED_ADC_H(s->bounds[2 * ch_off]);
+const uint32_t b = ASPEED_ADC_H(s->channels[ch_off]);
+const uint32_t b_lower = ASPEED_ADC_L(s->bounds[2 * ch_off + 1]);
+const uint32_t b_upper = ASPEED_ADC_H(s->bounds[2 * ch_off + 1]);
+
+return ((a < a_lower || a > a_upper)) ||
+   ((b < b_lower || b > b_upper));
+}
+
+static uint32_t read_channel_sample(AspeedADCState *s, int ch_off)
+{
+uint32_t ret;
+
+/* Poor man's sampling */
+ret = s->channels[ch_off];
+s->channels[ch_off] = update_channels(s->channels[ch_off]);
+
+if (breaks_threshold(s, ch_off)) {
+qemu_irq_raise(s->irq);
+}
+
+return ret;
+}
+
+#define TO_INDEX(addr, base) (((addr) - (base)) >> 2)
+
+static uint64_t aspeed_adc_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+AspeedADCState *s = ASPEED_ADC(opaque);
+uint64_t ret;
+
+switch (addr) {
+case 0x00:
+ret = s->engine_ctrl;
+break;
+case 0x04:
+ret = s->irq_ctrl;
+break;
+case 0x08:
+ret = s->vga_detect_ctrl;
+break;
+case 0x0c:
+ret = s->adc_clk_ctrl;
+break;
+case 0x10 ... 0x2e:
+ret = read_channel_sample(s, TO_INDEX(addr, 0x10));
+break;
+case 0x30 ... 0x6e:
+ret = s->bounds[TO_INDEX(addr, 0x30)];
+break;
+case 0x70 ... 0xae:
+ret = s->hysteresis[TO_INDEX(addr, 0x70)];
+break;
+case 0xc0:
+ret = s->irq_src;
+break;
+case 0xc4:
+ret = s->comp_trim;
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "%s: addr: 0x%lx, size: %u\n", __func__, addr,
+size);
+ret = 0;
+break;
+}
+
+return ret;
+}
+
+static void aspeed_adc_write(void *opaque, hwaddr addr,
+   uint64_t val, unsigned int size)
+{
+AspeedADCState *s = ASPEED_ADC(opaque);
+
+switch (addr) {
+case 0x00:
+{
+uint32_t init;
+
+init = !!(val & ASPEED_ADC_ENGINE_EN);
+init *= ASPEED_ADC_ENGINE_INIT;
+
+val &= ~ASPEED_ADC_ENGINE_INIT;
+val |= init;
+}
+
+val &= ~ASPEED_ADC_ENGINE_AUTO_COMP;
+s->engine_ctrl = val;
+
+break;
+case 0x04:
+s->irq_ctrl = val;
+break;
+case 0x08:
+s->vga_detect_ctrl = val;
+break;

[Qemu-devel] [PATCH 2/2] hw/arm: Integrate ADC model into Aspeed SoC

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery 
---
 hw/arm/aspeed_soc.c | 15 +++
 include/hw/arm/aspeed_soc.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 5c667d2c35b6..11f9588720d2 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -31,6 +31,7 @@
 #define ASPEED_SOC_VIC_BASE 0x1E6C
 #define ASPEED_SOC_SDMC_BASE0x1E6E
 #define ASPEED_SOC_SCU_BASE 0x1E6E2000
+#define ASPEED_SOC_ADC_BASE 0x1E6E9000
 #define ASPEED_SOC_SRAM_BASE0x1E72
 #define ASPEED_SOC_TIMER_BASE   0x1E782000
 #define ASPEED_SOC_WDT_BASE 0x1E785000
@@ -157,6 +158,10 @@ static void aspeed_soc_init(Object *obj)
 object_property_add_alias(obj, "hw-strap2", OBJECT(>scu),
   "hw-strap2", _abort);
 
+object_initialize(>adc, sizeof(s->adc), TYPE_ASPEED_ADC);
+object_property_add_child(obj, "adc", OBJECT(>adc), NULL);
+qdev_set_parent_bus(DEVICE(>adc), sysbus_get_default());
+
 object_initialize(>fmc, sizeof(s->fmc), sc->info->fmc_typename);
 object_property_add_child(obj, "fmc", OBJECT(>fmc), NULL);
 qdev_set_parent_bus(DEVICE(>fmc), sysbus_get_default());
@@ -256,6 +261,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 }
 sysbus_mmio_map(SYS_BUS_DEVICE(>scu), 0, ASPEED_SOC_SCU_BASE);
 
+/* ADC */
+object_property_set_bool(OBJECT(>adc), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>adc), 0, ASPEED_SOC_ADC_BASE);
+sysbus_connect_irq(SYS_BUS_DEVICE(>adc), 0,
+qdev_get_gpio_in(DEVICE(>vic), 31));
+
 /* UART - attach an 8250 to the IO space as our UART5 */
 if (serial_hds[0]) {
 qemu_irq uart5 = qdev_get_gpio_in(DEVICE(>vic), uart_irqs[4]);
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index d16205c66b5f..3b4d66d30f08 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -15,6 +15,7 @@
 #include "hw/arm/arm.h"
 #include "hw/intc/aspeed_vic.h"
 #include "hw/misc/aspeed_scu.h"
+#include "hw/adc/aspeed_adc.h"
 #include "hw/misc/aspeed_sdmc.h"
 #include "hw/timer/aspeed_timer.h"
 #include "hw/i2c/aspeed_i2c.h"
@@ -37,6 +38,7 @@ typedef struct AspeedSoCState {
 AspeedTimerCtrlState timerctrl;
 AspeedI2CState i2c;
 AspeedSCUState scu;
+AspeedADCState adc;
 AspeedSMCState fmc;
 AspeedSMCState spi[ASPEED_SPIS_NUM];
 AspeedSDMCState sdmc;
-- 
2.9.3

[Qemu-devel] [PATCH 0/2] hw/adc: Implement a basic Aspeed ADC model

[Andrew Jeffery]

Hello,

This short series introduces a basic model for the Aspeed ADC and glues it into
the generic Aspeed SoC definition. The register interface is enhanced slightly
from the AST2400 to the AST2500, but in a backwards-compatible way by making
use of reserved bits. As such I haven't made any effort to differentiate the
two.

The addition of a basic ADC model allows the Aspeed SDK kernel to boot under
QEMU's ast2500-evb machine. The upstream kernel driver doesn't test the
initialisation bit before completing its probe(), and thus doesn't get stuck if
the bit is not set. This is in contrast to the SDK kernel which spins on the
initialisation bit, never making forward progress in the absence of the ADC
model.

Tested with both Aspeed's SDK kernel and upstream Linux.

Cheers,

Andrew

Andrew Jeffery (2):
  hw/adc: Add basic Aspeed ADC model
  hw/arm: Integrate ADC model into Aspeed SoC

 hw/adc/Makefile.objs|   1 +
 hw/adc/aspeed_adc.c | 246 
 hw/arm/aspeed_soc.c |  15 +++
 include/hw/adc/aspeed_adc.h |  33 ++
 include/hw/arm/aspeed_soc.h |   2 +
 5 files changed, 297 insertions(+)
 create mode 100644 hw/adc/aspeed_adc.c
 create mode 100644 include/hw/adc/aspeed_adc.h

-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 0/9] QOM'ify work for sparc

[Philippe Mathieu-Daudé]

Hi Xiaoqiang,

On 04/29/2017 07:49 AM, xiaoqiang zhao wrote:

This patch set aims for QOM'ifying code relate with sparc.
It is part of my QOM'ify work of qemu code base.

changes since v1:
* rebased on the latest master

xiaoqiang zhao (9):
  hw/misc: QOM'ify eccmemctl.c
  hw/dma: QOM'ify sparc32_dma.c
  hw/dma: QOM'ify sun4m_iommu.c
  hw/misc: QOM'ify slavio_misc.c
  hw/timer: QOM'ify m48txx_sysbus (pass 1)
  hw/timer: QOM'ify m48txx_sysbus (pass 2)


I think you can squash those 2. Any particular reason you did pass 2 in 
another commit?



  hw/timer: QOM'ify slavio_timer
  hw/sparc: QOM'ify sun4m.c
  hw/sparc64: QOM'ify sun4u.c

 hw/dma/sparc32_dma.c| 25 ++-
 hw/dma/sun4m_iommu.c| 12 +--
 hw/misc/eccmemctl.c | 25 ++-
 hw/misc/slavio_misc.c   | 43 ---
 hw/sparc/sun4m.c| 54 +
 hw/sparc64/sun4u.c  | 20 +-
 hw/timer/m48t59.c   | 38 +-
 hw/timer/slavio_timer.c | 12 +--
 8 files changed, 105 insertions(+), 124 deletions(-)

Re: [Qemu-devel] [PATCH v2 6/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Philippe Mathieu-Daudé]

On 04/29/2017 07:49 AM, xiaoqiang zhao wrote:

assign DeviceClass::vmsd instead of using vmstate_register function

Signed-off-by: xiaoqiang zhao 


Reviewed-by: Philippe Mathieu-Daudé 


---
 hw/timer/m48t59.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index eff259ef96..3a9f541c9b 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -640,8 +640,6 @@ void m48t59_realize_common(M48t59State *s, Error **errp)
 s->wd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, _cb, s);
 }
 qemu_get_timedate(>alarm, 0);
-
-vmstate_register(NULL, -1, _m48t59, s);
 }

 static void m48t59_init1(Object *obj)
@@ -702,6 +700,7 @@ static void m48txx_sysbus_class_init(ObjectClass *klass, 
void *data)
 dc->realize = m48t59_realize;
 dc->reset = m48t59_reset_sysbus;
 dc->props = m48t59_sysbus_properties;
+dc->vmsd = _m48t59;
 nc->read = m48txx_sysbus_read;
 nc->write = m48txx_sysbus_write;
 nc->toggle_lock = m48txx_sysbus_toggle_lock;

Re: [Qemu-devel] [PATCH v2 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 1)

[Philippe Mathieu-Daudé]

On 04/29/2017 07:49 AM, xiaoqiang zhao wrote:

* split the old SysBus init function into an instance_init
  and a Device realize function
* use DeviceClass::realize instead of SysBusDeviceClass::init

Signed-off-by: xiaoqiang zhao 


Reviewed-by: Philippe Mathieu-Daudé 


---
 hw/timer/m48t59.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index 474981a6ac..eff259ef96 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -644,30 +644,31 @@ void m48t59_realize_common(M48t59State *s, Error **errp)
 vmstate_register(NULL, -1, _m48t59, s);
 }

-static int m48t59_init1(SysBusDevice *dev)
+static void m48t59_init1(Object *obj)
 {
-M48txxSysBusDeviceClass *u = M48TXX_SYS_BUS_GET_CLASS(dev);
-M48txxSysBusState *d = M48TXX_SYS_BUS(dev);
-Object *o = OBJECT(dev);
+M48txxSysBusDeviceClass *u = M48TXX_SYS_BUS_GET_CLASS(obj);
+M48txxSysBusState *d = M48TXX_SYS_BUS(obj);
+SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 M48t59State *s = >state;
-Error *err = NULL;

 s->model = u->info.model;
 s->size = u->info.size;
 sysbus_init_irq(dev, >IRQ);

-memory_region_init_io(>iomem, o, _ops, s, "m48t59.nvram",
+memory_region_init_io(>iomem, obj, _ops, s, "m48t59.nvram",
   s->size);
-memory_region_init_io(>io, o, _io_ops, s, "m48t59", 4);
-sysbus_init_mmio(dev, >iomem);
-sysbus_init_mmio(dev, >io);
-m48t59_realize_common(s, );
-if (err != NULL) {
-error_free(err);
-return -1;
-}
+memory_region_init_io(>io, obj, _io_ops, s, "m48t59", 4);
+}
+
+static void m48t59_realize(DeviceState *dev, Error **errp)
+{
+M48txxSysBusState *d = M48TXX_SYS_BUS(dev);
+M48t59State *s = >state;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);

-return 0;
+sysbus_init_mmio(sbd, >iomem);
+sysbus_init_mmio(sbd, >io);
+m48t59_realize_common(s, errp);
 }

 static uint32_t m48txx_sysbus_read(Nvram *obj, uint32_t addr)
@@ -696,10 +697,9 @@ static Property m48t59_sysbus_properties[] = {
 static void m48txx_sysbus_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 NvramClass *nc = NVRAM_CLASS(klass);

-k->init = m48t59_init1;
+dc->realize = m48t59_realize;
 dc->reset = m48t59_reset_sysbus;
 dc->props = m48t59_sysbus_properties;
 nc->read = m48txx_sysbus_read;
@@ -725,6 +725,7 @@ static const TypeInfo m48txx_sysbus_type_info = {
 .name = TYPE_M48TXX_SYS_BUS,
 .parent = TYPE_SYS_BUS_DEVICE,
 .instance_size = sizeof(M48txxSysBusState),
+.instance_init = m48t59_init1,
 .abstract = true,
 .class_init = m48txx_sysbus_class_init,
 .interfaces = (InterfaceInfo[]) {

[Qemu-devel] [PATCH] tests/libqtest: Print error instead of aborting when env variable is missing

[Thomas Huth]

When you currently try to run a test directly from the command line
without setting the QTEST_QEMU_BINARY environment variable first,
you are presented with an unhelpful assertion message like this:

 ERROR:tests/libqtest.c:163:qtest_init_without_qmp_handshake:
 assertion failed: (qemu_binary != NULL)
 Aborted (core dumped)

Let's replace the assert() with a more user friendly error message
instead.

Signed-off-by: Thomas Huth 
---
 tests/libqtest.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 84ecbd2..f0fd682 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -160,7 +160,11 @@ QTestState *qtest_init_without_qmp_handshake(const char 
*extra_args)
 const char *qemu_binary;
 
 qemu_binary = getenv("QTEST_QEMU_BINARY");
-g_assert(qemu_binary != NULL);
+if (!qemu_binary) {
+fprintf(stderr, "The environment variable QTEST_QEMU_BINARY has to be "
+"set before running the test.\n");
+exit(1);
+}
 
 s = g_malloc(sizeof(*s));
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 4/5] io: preserve ipv4/ipv6 flags when resolving InetSocketAddress

[Philippe Mathieu-Daudé]

On 05/19/2017 03:03 PM, Daniel P. Berrange wrote:

The original InetSocketAddress struct may have has_ipv4 and
has_ipv6 fields set, which will control both the ai_family
used during DNS resolution, and later use of the V6ONLY
flag.

Currently the standalone DNS resolver code drops the
has_ipv4 & has_ipv6 flags after resolving, which means
the later bind() code won't correctly set V6ONLY.

This fixes the following scenarios

  -vnc :0,ipv4=off
  -vnc :0,ipv6=on
  -vnc :::0,ipv4=off
  -vnc :::0,ipv6=on

which all mistakenly accepted IPv4 clients

Signed-off-by: Daniel P. Berrange 


Reviewed-by: Philippe Mathieu-Daudé 


---
 io/dns-resolver.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index 57a8896..c072d12 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -116,8 +116,10 @@ static int 
qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
 .numeric = true,
 .has_to = iaddr->has_to,
 .to = iaddr->to,
-.has_ipv4 = false,
-.has_ipv6 = false,
+.has_ipv4 = iaddr->has_ipv4,
+.ipv4 = iaddr->ipv4,
+.has_ipv6 = iaddr->has_ipv6,
+.ipv6 = iaddr->ipv6,
 };

 (*addrs)[i] = newaddr;

Re: [Qemu-devel] [PATCH v2 2/5] sockets: don't block IPv4 clients when listening on "::"

[Philippe Mathieu-Daudé]

On 05/19/2017 03:03 PM, Daniel P. Berrange wrote:

When inet_parse() parses the hostname, it is forcing the
has_ipv6 && ipv6 flags if the address contains a ":". This
means that if the user had set the ipv4=on flag, to try to
restrict the listener to just ipv4, an error would not have
been raised.  eg

   -incoming tcp:[::]:9000,ipv4

should have raised an error because listening for IPv4
on "::" is a non-sensical combination. With this removed,
we now call getaddrinfo() on "::" passing PF_INET and
so getaddrinfo reports an error about the hostname being
incompatible with the requested protocol.

Likewise it is explicitly setting the has_ipv4 & ipv4
flags when the address contains only digits + '.'. This
has no ill-effect, but also has no benefit, so is removed.

Signed-off-by: Daniel P. Berrange 


Reviewed-by: Philippe Mathieu-Daudé 


---
 util/qemu-sockets.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 397212b..b82412e 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -618,16 +618,12 @@ int inet_parse(InetSocketAddress *addr, const char *str, 
Error **errp)
 error_setg(errp, "error parsing IPv6 address '%s'", str);
 return -1;
 }
-addr->ipv6 = addr->has_ipv6 = true;
 } else {
 /* hostname or IPv4 addr */
 if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, ) != 2) {
 error_setg(errp, "error parsing address '%s'", str);
 return -1;
 }
-if (host[strspn(host, "0123456789.")] == '\0') {
-addr->ipv4 = addr->has_ipv4 = true;
-}
 }

 addr->host = g_strdup(host);

[Qemu-devel] [Bug 721825] Re: VDI block driver bugs

[Thomas Huth]

** Changed in: qemu
   Status: Incomplete => Triaged

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/721825

Title:
  VDI block driver bugs

Status in QEMU:
  Triaged

Bug description:
  Chunqiang Tang reports the following issues with the VDI block driver,
  these are present in QEMU 0.14:

  "Bug 1. The most serious bug is caused by race condition in updating a new 
  bmap entry in memory and on disk. Considering the following operation 
  sequence. 
O1: VM issues a write to sector X
O2: VDI allocates a new bmap entry and updates in-memory s->bmap
O3: VDI writes data to disk
O4: The disk I/O for writing sector X fails
O5: VDI reports error to VM and returns.

  Note that the bmap entry is updated in memory, but not persisted on disk. 
  Now consider another write that immediately follows:
P1: VM issues a write to sector X+1, which locates in the same block as 
  the previously used sector X.
P2: s->bmap already has one entry for the block, and hence VDI writes 
  data directly without persisting the new s->bmap entry on disk.
P3: The write disk I/O succeeds
P4: VDI report success to VM, but the bitmap entry is still not 
  persisted on disk.

  Now suppose the VM powers off gracefully (i.e., the QEMU process quits) 
  and reboots. The second write to sector X+1, which is reported as finished 
  successfully, is simply lost, because the corresponding in-memory s->bmap 
  entry is never persisted on disk. This is exactly what FVD's testing tool 
  discovers. After the block device is closed and then re-opened, disk 
  content verification fails.

  This is just one example of the problem. Race condition plus host crash 
  also causes problems. Consider another example below.
Q1: VM issues a write to sector X
Q2: VDI allocates a new bmap entry and updates in-memory s->bmap
Q3: VDI writes sector X to disk and waits for the callback
Q4: VM issues a write to another sector X+1, which is in the same block 
  as sector X.
Q5: VDI sees the bitmap entry in s->bmap is already allocated, and 
  writes sector X+1 to disk.
Q6: Write to sector X+1 finishes, and VDI's callback is invoked.
Q7: VDI acknowledges to the VM the completion of writing sector X+1
Q8: After observing the completion of writing sector X+1, VM issues a 
  flush to ensure that sector X+1 is persisted on disk.
Q9: VDI finishes the flush and acknowledge the completion of the 
  operation.
Q10: ... (some other arbitrary operations, but the disk I/O for writing 
  sector X is still not finished)
Q11: The host crashes

  Now the new bitmap entry is not persisted on disk, while both writing to 
  sector X+1 and the flush has been acknowledged as finished. Sector X+1 is 
  lost, which is a corruption. This problem exists even if it uses O_DSYNC. 
  The root cause of the problem is that, if a request updates in-memory 
  s->bmap, another request that sees this update assumes that the update is 
  already persisted on disk, which is not.

  Bug 2: Similar to the bugs the FVD testing tool found for QCOW2, there are 
  several cases of the code below on failure handling path without setting 
  error return code, which mistakenly reports failure as success. This 
  mistake is caught by FVD when doing image content validation.
 if (acb->hd_aiocb == NULL) {
 /* missing ret = -EIO; */
  goto done; 
  } 

  Bug 3: Similar to the bugs the FVD testing tool found for QCOW2, 
  vdi_aio_cancel does not perform a complete clean up and there are several 
  related bugs. First, memory buffer is not freed, acb->orig_buf and 
  acb->block_buffer. Second, acb->bh is not cancelled. Third, 
  vdi_aio_setup() does not initialize acb->bh to NULL so that when a request 
  acb is cancelled and then later reused for another request, its acb->bh != 
  NULL and the new request fails in  vdi_schedule_bh(). This is caught by 
  FVD's testing tool, when it observes that no I/O failure is injected but 
  VDI reports a failed I/O request, which indicates a bug in the driver."

  http://permalink.gmane.org/gmane.comp.emulators.qemu/94340

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/721825/+subscriptions

Re: [Qemu-devel] [PATCH v2 1/5] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Philippe Mathieu-Daudé]

On 05/19/2017 03:03 PM, Daniel P. Berrange wrote:

When binding to an IPv6 socket we currently force the
IPV6_V6ONLY flag to off. This means that the IPv6 socket
will accept both IPv4 & IPv6 sockets when QEMU is launched
with something like

  -vnc :::1

While this is good for that case, it is bad for other
cases. For example if an empty hostname is given,
getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::,
in that order. We will thus bind to 0.0.0.0 first, and
then fail to bind to :: on the same port. The same
problem can happen if any other hostname lookup causes
the IPv4 address to be reported before the IPv6 address.

When we get an IPv6 bind failure, we should re-try the
same port, but with IPV6_V6ONLY turned on again, to
avoid clash with any IPv4 listener.

This ensures that

  -vnc :1

will bind successfully to both 0.0.0.0 and ::, and also
avoid

  -vnc :1,to=2

from mistakenly using a 2nd port for the :: listener.

Signed-off-by: Daniel P. Berrange 


Reviewed-by: Philippe Mathieu-Daudé 


---
 util/qemu-sockets.c | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..397212b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -208,22 +208,37 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 }

 socket_set_fast_reuse(slisten);
-#ifdef IPV6_V6ONLY
-if (e->ai_family == PF_INET6) {
-/* listen on both ipv4 and ipv6 */
-const int off = 0;
-qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, ,
-sizeof(off));
-}
-#endif

 port_min = inet_getport(e);
 port_max = saddr->has_to ? saddr->to + port_offset : port_min;
 for (p = port_min; p <= port_max; p++) {
+#ifdef IPV6_V6ONLY
+/* listen on both ipv4 and ipv6 */
+int v6only = 0;
+#endif
 inet_setport(e, p);
+#ifdef IPV6_V6ONLY
+rebind:
+if (e->ai_family == PF_INET6) {
+qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, ,
+sizeof(v6only));
+}
+#endif
 if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
 goto listen;
 }
+
+#ifdef IPV6_V6ONLY
+/* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset,
+ * it could be that the IPv4 port is already claimed, so retry
+ * with V6ONLY set
+ */
+if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) {
+v6only = 1;
+goto rebind;
+}
+#endif
+
 if (p == port_max) {
 if (!e->ai_next) {
 error_setg_errno(errp, errno, "Failed to bind socket");

Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[John Snow]

On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/dirty-bitmap.c | 16 
>  include/block/dirty-bitmap.h |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 90af37287f..ab6a95cf41 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>  int64_t size;   /* Size of the bitmap (Number of sectors) */
>  bool disabled;  /* Bitmap is read-only */
>  int active_iterators;   /* How many iterators are active */
> +bool readonly;  /* Bitmap is read-only and may be changed 
> only
> +   by deserialize* functions */
>  QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  
> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int64_t nr_sectors)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));

Doesn't this change the nature of the assertion?

We're going from
return !(bitmap->disabled || bitmap->successor);
to
!bitmap->readonly

That makes me a little nervous to ACK this patch, because the
correctness depends on how readonly is updated and manipulated in the
future, which makes it more prone to error.

>  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>   int64_t cur_sector, int64_t nr_sectors)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));
>  hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));
>  if (!out) {
>  hbitmap_reset_all(bitmap->bitmap);
>  } else {
> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> cur_sector,
>  if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>  continue;
>  }
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));
>  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  }
> @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap 
> *bitmap)
>  {
>  return hbitmap_count(bitmap->meta);
>  }
> +
> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
> +{
> +return bitmap->readonly;
> +}
> +
> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
> +{
> +bitmap->readonly = true;
> +}
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 1e17729ac2..0aab5841f5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
> *bitmap,
>  bool finish);
>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>  
> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
> +
>  #endif
> 

Re: [Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd

[Eric Blake]

On 05/19/2017 09:30 AM, Greg Kurz wrote:
> Since chroot() doesn't change the current directory, it is indeed a good
> practice to chdir() to the target directory and then then chroot(), or
> to chroot() to the target directory and then chdir("/").
> 
> The current code does neither of them actually. Let's go for the latter.
> 
> This doesn't fix any security issue since all of this takes place before
> the helper begins to process requests.
> 
> Signed-off-by: Greg Kurz 
> ---
>  fsdev/virtfs-proxy-helper.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

You are correct that failing to sanitize the current working directory
alongside a chroot() can lead to escaped access outside of the new
smaller root.

Aside: chdir() is annoying in multi-threaded apps - it is global state,
rather than thread-local.  A multi-threaded app should therefore either
never change the current working directory, or else never rely on the
current working directory.  But if I'm not mistaken, virtfs-proxy-helper
is an independent helper app, not qemu proper, so the use of
chdir/chroot is not affecting other threads.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v0 0/6] move the tcg files into tcg directory.

[Eric Blake]

On 05/19/2017 02:30 AM, Yang Zhong wrote:
> Move the tcg relative files into tcg directory, which will make
> the code more clean in qemu.

Titling a patch series v0 is a bit unusual (typically, the first version
is untitled, and the second version is titled v2; 'git send-email -v2'
can help).  But since 0 is less than 1 or 2, I don't think it breaks any
tools if you use that as your explicit title for a first submission,
even if you have to respin.

> 
> Yang Zhong (6):
>   move tcg relative files into tcg directory
>   move tcg relative files into tcg directory
>   move tcg header file
>   move tcg relative files into tcg directory
>   move tcg relative files into tcg directory

None of your patches match the usual "topic: Short description" of other
patches.  Furthermore, having identical titles on 4 out of 6 distinct
patches is a nightmare for downstream backporters (which "move tcg
relative files into tcg directory" do I have to backport to fix the bug,
again?).  I highly suggest that every patch you submit have enough
details in the subject line that the subject is distinct (we don't
always succeed, but it's usually quite easy to avoid duplicates).

So, as an example, I might title a patch:

"tcg: Move tcg-runtime.c to tcg/ subdirectory"

>   change tcg relative file's compile definition
> 
>  Makefile.objs| 1 +
>  Makefile.target  | 8 ++--
>  tcg/Makefile.objs| 2 ++
>  tcg-runtime.c => tcg/tcg-runtime.c   | 0
>  tci.c => tcg/tci.c   | 0
>  tcg/trace-events | 6 ++
>  translate-all.c => tcg/translate-all.c   | 2 +-
>  translate-all.h => tcg/translate-all.h   | 0
>  translate-common.c => tcg/translate-common.c | 0
>  trace-events | 3 ---

Thankfully, you've got git rename detection turned on, which makes for
much nicer reviews.

>  10 files changed, 12 insertions(+), 10 deletions(-)
>  create mode 100644 tcg/Makefile.objs
>  rename tcg-runtime.c => tcg/tcg-runtime.c (100%)
>  rename tci.c => tcg/tci.c (100%)
>  create mode 100644 tcg/trace-events
>  rename translate-all.c => tcg/translate-all.c (99%)
>  rename translate-all.h => tcg/translate-all.h (100%)
>  rename translate-common.c => tcg/translate-common.c (100%)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] block: Tweak error message related to qemu-img convert

[Eric Blake]

On 05/19/2017 08:30 AM, Max Reitz wrote:
> On 2017-05-08 19:13, Eric Blake wrote:
>> When converting a 1.1 image down to 0.10, qemu-iotests 060 forces
>> a contrived failure where allocating a cluster used to replace a
>> zero cluster reads unaligned data.  Since it is a zero cluster
>> rather than a data cluster being converted, changing the error
>> message to match our earlier change in 'qcow2: Make distinction
>> bewteen zero cluster types obvious' is worthwhile.

I really had problems with that commit message. If you want to re-stage,
you could s/bewteen/between/

>>
>> Suggested-by: Max Reitz 
>> Signed-off-by: Eric Blake 
>> ---
>>
>> There's one more instance of "Data cluster offset" in qcow2-cluster.c,
>> but that one in handle_copied() is contained inside a
>> cluster_type == QCOW2_CLUSTER_NORMAL conditional.
>>
>>  block/qcow2-cluster.c  | 3 ++-
>>  tests/qemu-iotests/060.out | 2 +-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> Assuming no objection means consent:

Okay by me.

> 
> Thanks, fixed the commit title and applied to my block branch:
> 
> https://github.com/XanClic/qemu/commits/block
> 
> Max
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] migration: keep bytes_xfer_prev init'd to zero

[Felipe Franciosi]

The first time migration_bitmap_sync() is called, bytes_xfer_prev is set
to ram_state.bytes_transferred which is, at this point, zero. The next
time migration_bitmap_sync() is called, an iteration has happened and
bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second
has passed, so the auto converge logic will be triggered and
bytes_xfer_now will also be set to 'x' bytes.

This condition is currently masked by dirty_rate_high_cnt, which will
wait for a few iterations before throttling. It would otherwise always
assume zero bytes have been copied and therefore throttle the guest
(possibly) prematurely.

Given bytes_xfer_prev is only used by the auto convergence logic, it
makes sense to only set its value after a check has been made against
bytes_xfer_now.

Signed-off-by: Felipe Franciosi 
~
---
 migration/ram.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f59fdd4..793af39 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -670,10 +670,6 @@ static void migration_bitmap_sync(RAMState *rs)
 
 rs->bitmap_sync_count++;
 
-if (!rs->bytes_xfer_prev) {
-rs->bytes_xfer_prev = ram_bytes_transferred();
-}
-
 if (!rs->time_last_bitmap_sync) {
 rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
 }
-- 
1.9.5

[Qemu-devel] [PATCH] cpus: reset throttle_thread_scheduled after sleep

[Felipe Franciosi]

Currently, the throttle_thread_scheduled flag is reset back to 0 before
sleeping (as part of the throttling logic). Given that throttle_timer
(well, any timer) may tick with a slight delay, it so happens that under
heavy throttling (ie. close or on CPU_THROTTLE_PCT_MAX) the tick may
schedule a further cpu_throttle_thread() work item after the flag reset,
but before the previous sleep completed. This results on the vCPU thread
sleeping continuously for potentially several seconds in a row.

The chances of that happening can be drastically minimised by resetting
the flag after the sleep.

Signed-off-by: Felipe Franciosi 
Signed-off-by: Malcolm Crossley 
---
 cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 516e5cb..f42eebd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -677,9 +677,9 @@ static void cpu_throttle_thread(CPUState *cpu, 
run_on_cpu_data opaque)
 sleeptime_ns = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS);
 
 qemu_mutex_unlock_iothread();
-atomic_set(>throttle_thread_scheduled, 0);
 g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
 qemu_mutex_lock_iothread();
+atomic_set(>throttle_thread_scheduled, 0);
 }
 
 static void cpu_throttle_timer_tick(void *opaque)
-- 
1.9.5

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [PATCH v2 00/16] Vhost-pci for inter-VM communication

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 05:00:37PM +0800, Wei Wang wrote:
> > > 
> > > That being said, we compared to vhost-user, instead of vhost_net,
> > > because vhost-user is the one
> > > that is used in NFV, which we think is a major use case for vhost-pci.
> > 
> > If this is true, why not draft a pmd driver instead of a kernel one?
> 
> Yes, that's right. There are actually two directions of the vhost-pci driver
> implementation - kernel driver
> and dpdk pmd. The QEMU side device patches are first posted out for
> discussion, because when the device
> part is ready, we will be able to have the related team work on the pmd
> driver as well. As usual, the pmd
> driver would give a much better throughput.

For PMD to work though, the protocol will need to support vIOMMU.
Not asking you to add it right now since it's work in progress
for vhost user at this point, but something you will have to
keep in mind. Further, reviewing vhost user iommu patches might be
a good idea for you.

-- 
MST

Re: [Qemu-devel] [PATCH RFC] virtio-net: enable configurable tx queue size

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 10:32:19AM +0800, Wei Wang wrote:
> This patch enables the virtio-net tx queue size to be configurable
> between 256 (the default queue size) and 1024 by the user. The queue
> size specified by the user should be power of 2.
> 
> Setting the tx queue size to be 1024 requires the guest driver to
> support the VIRTIO_NET_F_MAX_CHAIN_SIZE feature.

This should be a generic ring feature, not one specific to virtio net.

> This feature restricts
> the guest driver from chaining 1024 vring descriptors, which may cause
> the device side implementation to send more than 1024 iov to writev.
> Currently, the max chain size allowed for the guest driver is set to
> 1023.
> 
> In the case that the tx queue size is set to 1024 and the
> VIRTIO_NET_F_MAX_CHAIN_SIZE feature is not supported by the guest driver,
> the default tx queue size (256) will be used.
> 
> Signed-off-by: Wei Wang 
> ---
>  hw/net/virtio-net.c | 71 
> +++--
>  include/hw/virtio/virtio-net.h  |  1 +
>  include/standard-headers/linux/virtio_net.h |  3 ++
>  3 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7d091c9..ef38cb1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -33,8 +33,12 @@
>  
>  /* previously fixed value */
>  #define VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE 256
> +#define VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE 256
>  /* for now, only allow larger queues; with virtio-1, guest can downsize */
>  #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> +#define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> +
> +#define VIRTIO_NET_MAX_CHAIN_SIZE 1023
>  
>  /*
>   * Calculate the number of bytes up to and including the given 'field' of
> @@ -57,6 +61,8 @@ static VirtIOFeature feature_sizes[] = {
>   .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>  {.flags = 1 << VIRTIO_NET_F_MTU,
>   .end = endof(struct virtio_net_config, mtu)},
> +{.flags = 1 << VIRTIO_NET_F_MAX_CHAIN_SIZE,
> + .end = endof(struct virtio_net_config, max_chain_size)},
>  {}
>  };
>  
> @@ -84,6 +90,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>  virtio_stw_p(vdev, , n->status);
>  virtio_stw_p(vdev, _virtqueue_pairs, n->max_queues);
>  virtio_stw_p(vdev, , n->net_conf.mtu);
> +virtio_stw_p(vdev, _chain_size, VIRTIO_NET_MAX_CHAIN_SIZE);
>  memcpy(netcfg.mac, n->mac, ETH_ALEN);
>  memcpy(config, , n->config_size);
>  }
> @@ -568,6 +575,7 @@ static uint64_t virtio_net_get_features(VirtIODevice 
> *vdev, uint64_t features,
>  features |= n->host_features;
>  
>  virtio_add_feature(, VIRTIO_NET_F_MAC);
> +virtio_add_feature(, VIRTIO_NET_F_MAX_CHAIN_SIZE);
>  
>  if (!peer_has_vnet_hdr(n)) {
>  virtio_clear_feature(, VIRTIO_NET_F_CSUM);
> @@ -603,6 +611,7 @@ static uint64_t virtio_net_bad_features(VirtIODevice 
> *vdev)
>  virtio_add_feature(, VIRTIO_NET_F_HOST_TSO4);
>  virtio_add_feature(, VIRTIO_NET_F_HOST_TSO6);
>  virtio_add_feature(, VIRTIO_NET_F_HOST_ECN);
> +virtio_add_feature(, VIRTIO_NET_F_MAX_CHAIN_SIZE);
>  
>  return features;
>  }
> @@ -635,6 +644,27 @@ static inline uint64_t 
> virtio_net_supported_guest_offloads(VirtIONet *n)
>  return virtio_net_guest_offloads_by_features(vdev->guest_features);
>  }
>  
> +static bool is_tx(int queue_index)
> +{
> +return queue_index % 2 == 1;
> +}
> +
> +static void virtio_net_change_tx_queue_size(VirtIONet *n)
> +{
> +VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +int i, num_queues = virtio_get_num_queues(vdev);
> +
> +if (n->net_conf.tx_queue_size == VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE) {
> +return;
> +}
> +
> +for (i = 0; i < num_queues; i++) {
> +if (is_tx(i)) {
> +virtio_queue_set_num(vdev, i, n->net_conf.tx_queue_size);
> +}
> +}
> +}
> +
>  static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>  {
>  VirtIONet *n = VIRTIO_NET(vdev);
> @@ -649,6 +679,16 @@ static void virtio_net_set_features(VirtIODevice *vdev, 
> uint64_t features)
> virtio_has_feature(features,
>VIRTIO_F_VERSION_1));
>  
> +/*
> + * Change the tx queue size if the guest supports
> + * VIRTIO_NET_F_MAX_CHAIN_SIZE. This will restrict the guest from sending
> + * a very large chain of vring descriptors (e.g. 1024), which may cause
> + * 1025 iov to be written to writev.
> + */
> +if (virtio_has_feature(features, VIRTIO_NET_F_MAX_CHAIN_SIZE)) {
> +virtio_net_change_tx_queue_size(n);
> +}
> +
>  if (n->has_vnet_hdr) {
>  n->curr_guest_offloads =
>  virtio_net_guest_offloads_by_features(features);
> @@ -1297,8 +1337,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>  
>   

Re: [Qemu-devel] [PATCH 3/6] vhost: Update rings information for IOTLB earlier

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 11:48:52AM +0200, Maxime Coquelin wrote:
> 
> 
> On 05/18/2017 05:24 PM, Michael S. Tsirkin wrote:
> > On Thu, May 18, 2017 at 04:45:23PM +0200, Maxime Coquelin wrote:
> > > Hi Michael,
> > > 
> > > On 05/18/2017 09:35 AM, Maxime Coquelin wrote:
> > > > 
> > > > 
> > > > On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote:
> > > > > On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote:
> > > > > > 
> > > > > > On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote:
> > > > > > > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote:
> > > > > > > > Vhost-kernel backend need to receive IOTLB entries for rings
> > > > > > > > information early, but vhost-user need the same information
> > > > > > > > earlier, before VHOST_USER_SET_VRING_ADDR is sent.
> > > > > > > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it?
> > > > > > > 
> > > > > > > According to
> > > > > > >  Starting and stopping rings
> > > > > > > in vhost user spec, vhost user does not access
> > > > > > > anything until ring is started and enabled.
> > > > > > > 
> > > > > > > 
> > > > > > > > This patch also trigger IOTLB miss for all rings informations
> > > > > > > > for robustness, even if in practice these adresses are on the
> > > > > > > > same page.
> > > > > > Actually, the DPDK vhost-user backend is compliant with the spec,
> > > > > > but when handling VHOST_USER_SET_VRING_ADDR request, it translates 
> > > > > > the
> > > > > > guest addresses into backend VAs, and check they are valid. I
> > > > > > will make the
> > > > > > commit message clearer about this in next revision.
> > > > > > 
> > > > > > The check could be done later, for example when the ring are 
> > > > > > started,
> > > > > > but it wouldn't change the need to trigger a miss at some point.
> > > > > I think it should be done later, yes. As long as ring is not
> > > > > started addresses should not be interpreted.
> > > > > 
> > > > 
> > > > Ok, then I'll move these addresses translations in the
> > > > VHOST_USER_SET_VRING_KICK handler.
> > > s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/
> > 
> > Note that when protocol features are off ring is started in
> > enabled state, but iommu requires protocol features.
> 
> OK, I will take care of this.
> 
> Note that currently in DPDK, the ring is created in enabled state,
> so it is enabled as soon as started even with protocol features.
> I have done the patch to fix this, will be posted with the patch that
> do the ring addresses translations only when starting/enabling the ring.
> 
> Also, note that disabling VHOST_USER_F_PROTOCOL_FEATURES with latest
> DPDK and QEMU seems broken. I'll add this to my todo list to understand
> where the problem is, but this is lower priority.

And hopefully add a unit test without this so we don't break
it in the future.

> > > I just looked at implementing this change, but I'm not convinced this is
> > > the right thing to do.
> > > 
> > > On backend side, it means saving temporarily the vhost_vring_addr struct
> > > into the vq struct, and moving all what is done currently in
> > > SET_VRING_ADDR handler to SET_VRING_ENABLE one.
> > 
> > Yes, and this is consistent with what the kernel does.
> > 
> > > My understanding of the "Starting and stopping rings" chapter of the
> > > spec is that the ring must not be processed as long as not started and
> > > enabled, not that the addresses passed should not be checked/translated
> > > as it is done today both in DPDK and libvhost-user.
> > > 
> > > If the addresses are invalid, isn't it better to know as soon as
> > > possible?
> > > 
> > > Cheers,
> > > Maxime
> > 
> > There could be valid reasons to set an invalid address temporarily.
> > For example to make sure connection is reset.
> 
> Ok.
> 
> Thanks,
> Maxime

Re: [Qemu-devel] [Bug 721825] Re: VDI block driver bugs

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 8:36 PM, Thomas Huth <721...@bugs.launchpad.net> wrote:
> Is this still an issue with the latest version of QEMU, or could we
> close this bug nowadays?

A quick check of block/vdi.c shows that error handling is still
lacking.  Updates to in-memory data structures are not reverted if the
write to disk fails.

Let's leave this in case someone is interested in fixing the bugs
sometime.  VDI is not used heavily and typically in read-only mode so
these bugs are not urgent.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/721825

Title:
  VDI block driver bugs

Status in QEMU:
  Incomplete

Bug description:
  Chunqiang Tang reports the following issues with the VDI block driver,
  these are present in QEMU 0.14:

  "Bug 1. The most serious bug is caused by race condition in updating a new 
  bmap entry in memory and on disk. Considering the following operation 
  sequence. 
O1: VM issues a write to sector X
O2: VDI allocates a new bmap entry and updates in-memory s->bmap
O3: VDI writes data to disk
O4: The disk I/O for writing sector X fails
O5: VDI reports error to VM and returns.

  Note that the bmap entry is updated in memory, but not persisted on disk. 
  Now consider another write that immediately follows:
P1: VM issues a write to sector X+1, which locates in the same block as 
  the previously used sector X.
P2: s->bmap already has one entry for the block, and hence VDI writes 
  data directly without persisting the new s->bmap entry on disk.
P3: The write disk I/O succeeds
P4: VDI report success to VM, but the bitmap entry is still not 
  persisted on disk.

  Now suppose the VM powers off gracefully (i.e., the QEMU process quits) 
  and reboots. The second write to sector X+1, which is reported as finished 
  successfully, is simply lost, because the corresponding in-memory s->bmap 
  entry is never persisted on disk. This is exactly what FVD's testing tool 
  discovers. After the block device is closed and then re-opened, disk 
  content verification fails.

  This is just one example of the problem. Race condition plus host crash 
  also causes problems. Consider another example below.
Q1: VM issues a write to sector X
Q2: VDI allocates a new bmap entry and updates in-memory s->bmap
Q3: VDI writes sector X to disk and waits for the callback
Q4: VM issues a write to another sector X+1, which is in the same block 
  as sector X.
Q5: VDI sees the bitmap entry in s->bmap is already allocated, and 
  writes sector X+1 to disk.
Q6: Write to sector X+1 finishes, and VDI's callback is invoked.
Q7: VDI acknowledges to the VM the completion of writing sector X+1
Q8: After observing the completion of writing sector X+1, VM issues a 
  flush to ensure that sector X+1 is persisted on disk.
Q9: VDI finishes the flush and acknowledge the completion of the 
  operation.
Q10: ... (some other arbitrary operations, but the disk I/O for writing 
  sector X is still not finished)
Q11: The host crashes

  Now the new bitmap entry is not persisted on disk, while both writing to 
  sector X+1 and the flush has been acknowledged as finished. Sector X+1 is 
  lost, which is a corruption. This problem exists even if it uses O_DSYNC. 
  The root cause of the problem is that, if a request updates in-memory 
  s->bmap, another request that sees this update assumes that the update is 
  already persisted on disk, which is not.

  Bug 2: Similar to the bugs the FVD testing tool found for QCOW2, there are 
  several cases of the code below on failure handling path without setting 
  error return code, which mistakenly reports failure as success. This 
  mistake is caught by FVD when doing image content validation.
 if (acb->hd_aiocb == NULL) {
 /* missing ret = -EIO; */
  goto done; 
  } 

  Bug 3: Similar to the bugs the FVD testing tool found for QCOW2, 
  vdi_aio_cancel does not perform a complete clean up and there are several 
  related bugs. First, memory buffer is not freed, acb->orig_buf and 
  acb->block_buffer. Second, acb->bh is not cancelled. Third, 
  vdi_aio_setup() does not initialize acb->bh to NULL so that when a request 
  acb is cancelled and then later reused for another request, its acb->bh != 
  NULL and the new request fails in  vdi_schedule_bh(). This is caught by 
  FVD's testing tool, when it observes that no I/O failure is injected but 
  VDI reports a failed I/O request, which indicates a bug in the driver."

  http://permalink.gmane.org/gmane.comp.emulators.qemu/94340

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/721825/+subscriptions

[Qemu-devel] [Bug 917824] Re: qemu loops/hangs on extending qcow2-diskspace

[Thomas Huth]

** Changed in: qemu
   Status: New => Invalid

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/917824

Title:
  qemu loops/hangs on extending qcow2-diskspace

Status in QEMU:
  Invalid
Status in qemu-kvm package in Ubuntu:
  Invalid

Bug description:
  system is ubuntu 11-10 with qemu-kvm 0.14.1 (standard dpkg)

  while trying to install a software in a winXP-guest on a nearly empty
  disk within a qcow2-file, qemu stops answering console and vnc.

  gdb on original binary shows, that it doesn't really hang, but seems
  to endless loop

  recompiling the binary with debugging-symbols shows following problem:

  in block/qcow2-cluster.c:777 there's a loop over an allocation-list, but 
instead of stopping somewhere, the "next" points to the current one.
   QLIST_FOREACH(old_alloc, >cluster_allocs, next_in_flight) { ... }

  because I'm not firm with qemu-development, I don't know, which
  behaviour would be correct, but simply break this loop doesn't seem to
  work: a few moments later the process is again at this point (tried so
  by setting the register for comparison to 0)

  this situation is continuosly reproducible, but it always take at
  least 10-15min to get there, so please, if you have suggestion which I
  should try and report, try to give as much suggestions as possible in
  one action.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/917824/+subscriptions

[Qemu-devel] [Bug 924943] Re: usb-host devices given by command line are routed incomplete to the guest

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/924943

Title:
  usb-host devices given by command line are routed incomplete to the
  guest

Status in QEMU:
  Incomplete

Bug description:
  affected qemus: qemu-1.0, qemu-kvm-1.0, qemu and qemu-kvm master branches 
(older versions not tested)
  affected guests: linux, windows
  test hardware: standard usb key (or any other piece of USB hardware) that 
works perfectly when plugged in after guest bootup

  Several Sequences have been tested:
  - start qemu with  -readconfig /etc/ich9-ehci-uhci.cfg -device usb-tablet 
-device usb-host,bus=ehci.0
  - start qemu with -readconfig /etc/ich9-ehci-uhci.cfg -device usb-tablet -S 
(to not start up the guest directly) + at the console prompt: "device_add 
usb-host" then "c" to start the guest.

  For the linux guest, I get a usb device listed and detected as /dev/sdb when 
plugging it in at runtime. At startup linux does NOT detect it.
  For the windows guest, I get a usb device listed and detected as "removable 
media" when plugging it in at runtime. At startup Windows does detect 
"something" that is listed in the device manager as Generic Mass Storage 
device, but with a yellow exclamation mark and there is no removable media 
listed in Explorer

  If you need further testings, just let me know.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/924943/+subscriptions

[Qemu-devel] [Bug 959992] Re: segfault in apic_report_irq_delivered when booting tinycore_3.3.iso

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/959992

Title:
  segfault in apic_report_irq_delivered when booting tinycore_3.3.iso

Status in QEMU:
  Incomplete

Bug description:
  it git head (33cf629) sometimes it segfaults in
  apic_report_irq_delivered() and backtrace is looping in
  apic_update_irq(#3-#4)

  Log:
  C:\msys\home\User\qemu\i386-softmmu>gdb --args qemu-system-i386.exe -L 
..\pc-bios -cdrom tinycore_3.3.iso
  GNU gdb (GDB) 7.3
  Copyright (C) 2011 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later 
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "mingw32".
  For bug reporting instructions, please see:
  ...
  Reading symbols from 
C:\msys\home\User\qemu\i386-softmmu/qemu-system-i386.exe...
  done.
  (gdb) r
  Starting program: C:\msys\home\User\qemu\i386-softmmu/qemu-system-i386.exe -L 
..\\pc-bios -cdrom tinycore_3.3.iso
  [New Thread 9012.0x2348]
  [New Thread 9012.0x2860]
  [New Thread 9012.0x2b64]

  Program received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 9012.0x2b64]
  0x0053cde8 in apic_report_irq_delivered (delivered=0)
  at C:/msys/home/User/qemu/hw/apic_common.c:110
  110 {
  (gdb) bt
  #0  0x0053cde8 in apic_report_irq_delivered (delivered=0)
  at C:/msys/home/User/qemu/hw/apic_common.c:110
  #1  0x0053b9eb in apic_set_irq (s=0x1d7aff8, vector_num=,
  trigger_mode=0) at C:/msys/home/User/qemu/hw/apic.c:390
  #2  0x0053b990 in apic_update_irq (s=0x1d7aff8)
  at C:/msys/home/User/qemu/hw/apic.c:376
  #3  apic_update_irq (s=0x1d7aff8) at C:/msys/home/User/qemu/hw/apic.c:367
  #4  0x0053b990 in apic_update_irq (s=0x1d7aff8)
  at C:/msys/home/User/qemu/hw/apic.c:376
  #5  apic_update_irq (s=0x1d7aff8) at C:/msys/home/User/qemu/hw/apic.c:367
  #6  0x0053b990 in apic_update_irq (s=0x1d7aff8)
  at C:/msys/home/User/qemu/hw/apic.c:376
  #7  apic_update_irq (s=0x1d7aff8) at C:/msys/home/User/qemu/hw/apic.c:367
  ...

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/959992/+subscriptions

[Qemu-devel] [Bug 1234179] Re: QEMU segfaults during Windows 7 unattended install

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1234179

Title:
  QEMU segfaults during Windows 7 unattended install

Status in QEMU:
  Incomplete

Bug description:
  During today's automated qemu.git testing, a segmentation fault while
  installing Windows 7 SP1 happened.

  qemu.git top commit: 
  10/02 01:30:24 INFO |   git:0150| git commit ID is 
a684f3cf9b9b9c3cb82be87aafc463de8974610c (tag v1.4.0-4237-ga684f3c)

  commit a684f3cf9b9b9c3cb82be87aafc463de8974610c
  Merge: 349cd52 1cf9412
  Author: Anthony Liguori 
  Date:   Mon Sep 30 17:15:27 2013 -0500

  Merge remote-tracking branch 'kraxel/seabios-1.7.3.2' into staging
  
  # By Gerd Hoffmann
  # Via Gerd Hoffmann
  * kraxel/seabios-1.7.3.2:
update seabios from 1.7.2.2 to 1.7.3.2
  
  Message-id: 1380533055-24960-1-git-send-email-kra...@redhat.com

  We have the core file saved in our test servers, we can make
  arrangements to transfer it if there's someone interested in
  investigating further. The framework saved the 'bt full' of the core
  file, that was missing some debug info:

  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib64/libthread_db.so.1".
  Core was generated by `/usr/local/autotest/tests/virt/qemu/qemu -S -name 
virt-tests-vm1 -M pc -nodefau'.
  Program terminated with signal 11, Segmentation fault.
  #0  0x7ffc8fb86cf0 in pixman_image_get_data () from 
/lib64/libpixman-1.so.0
  #0  0x7ffc8fb86cf0 in pixman_image_get_data () from 
/lib64/libpixman-1.so.0
  No symbol table info available.
  #1  0x7ffc9165b05c in ?? ()
  No symbol table info available.
  #2  0x7ffc9382b540 in ?? ()
  No symbol table info available.
  #3  0x7ffc8f359a8d in clock_gettime () from /lib64/libc.so.6
  No symbol table info available.
  #4  0x7ffc9382b5a8 in ?? ()
  No symbol table info available.
  #5  0x00019382b4c0 in ?? ()
  No symbol table info available.
  #6  0x in ?? ()
  No symbol table info available.

  Extra info:

  Commits for the submodules:

  10/02 01:30:29 DEBUG|base_utils:0134| [stdout] Submodule path 'dtc': checked 
out 'bc895d6d09695d05ceb8b52486ffe861d6cfbdde'
  10/02 01:30:51 DEBUG|base_utils:0134| [stdout] Submodule path 'pixman': 
checked out '97336fad32acf802003855cd8bd6477fa49a12e3'
  10/02 01:30:58 DEBUG|base_utils:0134| [stdout] Submodule path 'roms/SLOF': 
checked out '8cfdfc43f4c4c8c8dfa4b7cf16f7c19c84eee812'
  10/02 01:31:16 DEBUG|base_utils:0134| [stdout] Submodule path 'roms/ipxe': 
checked out '09c5109b8585178172c7608de8d52e9d9af0b680'
  10/02 01:31:20 DEBUG|base_utils:0134| [stdout] Submodule path 
'roms/openbios': checked out '0f3d51ef22ec9166beb3ed434d253029ed7cfe84'
  10/02 01:31:21 DEBUG|base_utils:0134| [stdout] Submodule path 
'roms/qemu-palcode': checked out 'c87a92639b28ac42bc8f6c67443543b405dc479b'
  10/02 01:31:27 DEBUG|base_utils:0134| [stdout] Submodule path 'roms/seabios': 
checked out 'ece025f5980bae88fa677bc9c0d24d2e580e205d'
  10/02 01:31:28 DEBUG|base_utils:0134| [stdout] Submodule path 'roms/sgabios': 
checked out '23d474943dcd55d0550a3d20b3d30e9040a4f15b'
  10/02 01:31:31 DEBUG|base_utils:0134| [stdout] Submodule path 'roms/vgabios': 
checked out '19ea12c230ded95928ecaef0db47a82231c2e485'

  Configure options:

  10/02 01:31:32 DEBUG|base_utils:0099| Running 
'/usr/local/autotest/tmp/virt/src/qemu/configure --target-list=x86_64-softmmu 
--disable-strip --prefix=/usr/local/autotest/tests/virt/qemu/install_root'
  10/02 01:31:35 DEBUG|env_proces:0829| (address cache) DHCP lease OK: 
00:30:48:c5:d6:e2 --> 10.16.72.38
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] Install prefix
/usr/local/autotest/tests/virt/qemu/install_root
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] BIOS directory
/usr/local/autotest/tests/virt/qemu/install_root/share/qemu
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] binary directory  
/usr/local/autotest/tests/virt/qemu/install_root/bin
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] library directory 
/usr/local/autotest/tests/virt/qemu/install_root/lib
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] libexec directory 
/usr/local/autotest/tests/virt/qemu/install_root/libexec
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] include directory 
/usr/local/autotest/tests/virt/qemu/install_root/include
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] config directory  
/usr/local/autotest/tests/virt/qemu/install_root/etc
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] local state directory   
/usr/local/autotest/tests/virt/qemu/install_root/var
  10/02 01:31:40 DEBUG|base_utils:0134| [stdout] Manual directory  

[Qemu-devel] [Bug 950692] Re: High CPU usage in Host (revisited)

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/950692

Title:
  High CPU usage in Host (revisited)

Status in QEMU:
  Incomplete

Bug description:
  Hi,

  last time QEMU(KVM) was working for us flawlessly was 2.6.35 kernel.

  Actually it still works flawlessly on that one single machine, that
  still has this kernel. Qemu version is meanwhile 1.0-r3, so the
  problem seems to be dependent on kernel version and not qemu version.

  We have several other machines, where the "high CPU usage in host"
  problem is present in various degrees of annoyingness.

  Both host and guest are Gentoo linux, at least that's what we test
  with. Several tested systems with other linux distributions and
  FreeBSD show similar - if not worse - behaviour. I will talk about 3
  hosts, machine A, machine B and machine C

  A:

  2.6.35-gentoo-r9 #2 SMP Sat Nov 6 22:32:28 CET 2010 x86_64 Intel(R) Xeon(R) 
CPU L5410 @ 2.33GHz
  32GB, runs about 15 KVM guests (all Gentoo, some 32bit, some 64bit, all SMP)
  no problems whatsoever, host CPU usage corresponds to Guest CPU usage + 1-2%, 
that's how we like it
  qemu 1.0-r3

  B:

  3.0.6-gentoo #1 SMP Sun Oct 16 18:57:31 CEST 2011 x86_64 Intel(R) Xeon(R) CPU 
L5630 @ 2.13GHz
  144GB, runs 1(!) KVM guest (Debian 6.x)
  /usr/bin/qemu-system-x86_64 --enable-kvm -daemonize -cpu host -k de -net tap 
-tdf -hda /data/vm/disk.raw -m 768 -smp 1 -vnc :5 -net 
nic,model=e1000,macaddr=...
  100% host CPU load always, therefore it got only "smp 1", if we gave it smp 
2, it would have 200%, smp 4 400% and so on.
  qemu 1.0-r3

  C:

  3.1.6-gentoo #5 SMP Tue Mar 6 20:34:44 CET 2012 x86_64 Intel(R) Xeon(R) CPU 
5148 @ 2.33GHz
  16GB, runs 1-4 KVM guests (mostly Gentoo machines from A, plus some SuSE, 
RedHat etc.)
  X00% CPU usage, where x corresponds to the smp X parameter, at startup as 
well as if someone "touches" the VM, like logging in, doing a "ls". If the 
machine is ABSOLUTELY IDLE, the process also exhibits 1-2% CPU load in host, 
but as soon as you do a simple ls, usage goes to - say - 400%, where it remains 
for some seconds, then slowly falls 280%, 120%, 60%, ... back to 1-2%
  qemu 1.0-r3

  
  B is no go, C tries to well-behave but ultimatively fails, A is golden.

  There seems to be REAL high CPU usage and not only an error in
  displaying it. Other processes get less CPU power and exhibit
  definitely a slower runtime. On B, definitely one CPU core is hogged
  all the time

  
  Some years ago we experienced something similar with ~2.6.26 and after a long 
and woeful period, we found out that compiling the host kernel as a tickless 
system caused the problem. Enabling high resolution timers made the problem go 
away and that is the situation on machine A until today. Since then no one 
dared to touch this production server. Unfortunately, this recipe didn't help 
with the other machines.

  I have scanned the net for similar problems and there are people
  complaining about high CPU usage. Unfortunately very often the devs or
  maintainers cannot reproduce it and the issue is dropped. Well - we
  cannot reproduce a "good behaviour"(tm) on any but one machine with
  any recent (read: post-2.6.35) linux kernel.

  Summary what we tried so far:

  * different linux kernels @ host, and @ guest

  -> no difference, especially there are guests @ A, that run newer
  kernels and there are Guests at B and C that run older kernels than is
  the host kernel

  * smp and non-smp, 32bit and 64bit guests

  -> 32/64bit in the guest makes no difference whatsoever. The smp just
  limits how much of the host CPU the guest hogs on non-well behaving
  systems (smp X -> X * 100%)

  * various linux guest OS, as well as FreeBSD

  -> no difference whatsoever

  * various options parameters in the host kernel (other schedulers,
  HRT, tickless,...)

  -> no difference whatsoever

  * various versions of qemu/kvm since 0.13

  -> no difference whatsoever

  * various qemu/kvm options, virtio and non-virtio configurations (most
  of the VMs @ A run blk-virtio but emulate an e1000)

  -> no difference whatsoever

  
  You could say, we've reached wits' end. We could try 2.6.35 @ machine C with 
the same configuration from A (they are identical except CPU and RAM size, but 
same RAID, mainboard, etc. plus A once had also the 5148 Xeons and an upgrade 
luckily made no difference in good behaviour, so I would exclude the CPU 
factor) but honestly that is not the way I'd like to go. The goal is to update 
A to something recent and not to loose it's VM-hosting well behaviour. Ideally 
to propagate this well beaviour to the other machines.

  
  Arjan Minski
PetaMem IT

To manage notifications 

[Qemu-devel] [Bug 721825] Re: VDI block driver bugs

[Thomas Huth]

Is this still an issue with the latest version of QEMU, or could we
close this bug nowadays?

** Changed in: qemu
   Status: Confirmed => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/721825

Title:
  VDI block driver bugs

Status in QEMU:
  Incomplete

Bug description:
  Chunqiang Tang reports the following issues with the VDI block driver,
  these are present in QEMU 0.14:

  "Bug 1. The most serious bug is caused by race condition in updating a new 
  bmap entry in memory and on disk. Considering the following operation 
  sequence. 
O1: VM issues a write to sector X
O2: VDI allocates a new bmap entry and updates in-memory s->bmap
O3: VDI writes data to disk
O4: The disk I/O for writing sector X fails
O5: VDI reports error to VM and returns.

  Note that the bmap entry is updated in memory, but not persisted on disk. 
  Now consider another write that immediately follows:
P1: VM issues a write to sector X+1, which locates in the same block as 
  the previously used sector X.
P2: s->bmap already has one entry for the block, and hence VDI writes 
  data directly without persisting the new s->bmap entry on disk.
P3: The write disk I/O succeeds
P4: VDI report success to VM, but the bitmap entry is still not 
  persisted on disk.

  Now suppose the VM powers off gracefully (i.e., the QEMU process quits) 
  and reboots. The second write to sector X+1, which is reported as finished 
  successfully, is simply lost, because the corresponding in-memory s->bmap 
  entry is never persisted on disk. This is exactly what FVD's testing tool 
  discovers. After the block device is closed and then re-opened, disk 
  content verification fails.

  This is just one example of the problem. Race condition plus host crash 
  also causes problems. Consider another example below.
Q1: VM issues a write to sector X
Q2: VDI allocates a new bmap entry and updates in-memory s->bmap
Q3: VDI writes sector X to disk and waits for the callback
Q4: VM issues a write to another sector X+1, which is in the same block 
  as sector X.
Q5: VDI sees the bitmap entry in s->bmap is already allocated, and 
  writes sector X+1 to disk.
Q6: Write to sector X+1 finishes, and VDI's callback is invoked.
Q7: VDI acknowledges to the VM the completion of writing sector X+1
Q8: After observing the completion of writing sector X+1, VM issues a 
  flush to ensure that sector X+1 is persisted on disk.
Q9: VDI finishes the flush and acknowledge the completion of the 
  operation.
Q10: ... (some other arbitrary operations, but the disk I/O for writing 
  sector X is still not finished)
Q11: The host crashes

  Now the new bitmap entry is not persisted on disk, while both writing to 
  sector X+1 and the flush has been acknowledged as finished. Sector X+1 is 
  lost, which is a corruption. This problem exists even if it uses O_DSYNC. 
  The root cause of the problem is that, if a request updates in-memory 
  s->bmap, another request that sees this update assumes that the update is 
  already persisted on disk, which is not.

  Bug 2: Similar to the bugs the FVD testing tool found for QCOW2, there are 
  several cases of the code below on failure handling path without setting 
  error return code, which mistakenly reports failure as success. This 
  mistake is caught by FVD when doing image content validation.
 if (acb->hd_aiocb == NULL) {
 /* missing ret = -EIO; */
  goto done; 
  } 

  Bug 3: Similar to the bugs the FVD testing tool found for QCOW2, 
  vdi_aio_cancel does not perform a complete clean up and there are several 
  related bugs. First, memory buffer is not freed, acb->orig_buf and 
  acb->block_buffer. Second, acb->bh is not cancelled. Third, 
  vdi_aio_setup() does not initialize acb->bh to NULL so that when a request 
  acb is cancelled and then later reused for another request, its acb->bh != 
  NULL and the new request fails in  vdi_schedule_bh(). This is caught by 
  FVD's testing tool, when it observes that no I/O failure is injected but 
  VDI reports a failed I/O request, which indicates a bug in the driver."

  http://permalink.gmane.org/gmane.comp.emulators.qemu/94340

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/721825/+subscriptions

[Qemu-devel] [Bug 726619] Re: loadvm does not load (offline) snapshot anymore

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/726619

Title:
  loadvm does not load (offline) snapshot anymore

Status in QEMU:
  Incomplete

Bug description:
  qemu Version: 0.14.0
  The problem is present in the current code from git master as well.

  Loading a snapshot that was created while qemu was not running (using
  qemu-img) does not seem to work anymore.

  Using "loadvm " in the qemu monitor does not have the
  desired effect. Not even an error message is displayed.

  I was able to track down the problem (using git bisect) to this commit:
  
http://git.qemu.org/qemu.git/commit/?id=f0aa7a8b2d518c54430e4382309281b93e51981a

  After reverting that commit in my local git checkout everything is
  workin as expected again.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/726619/+subscriptions

[Qemu-devel] [Bug 796202] Re: Doing a 64 bit load from a 32 bit local APIC register is allowed

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: Confirmed => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/796202

Title:
  Doing a 64 bit load from a 32 bit local APIC register is allowed

Status in QEMU:
  Incomplete

Bug description:
  Doing

  u64 lapic_idregister = (u64) fix_to_virt(FIX_APIC_BASE) + 0x20;

  and later in an interrupt handler

  movq (lapic_idregister), %rcx
  movq (%rcx), %rcx

  in a linux kernel module works in qemu 0.13.91 but not on real hardware (it 
simply reboots).
  On real hardware only

  movl (%rcx), %ecx

  works (also in qemu).

  Commandline:
  qemu-system-x86_64 \
-kernel $LINUXDIR/arch/x86_64/boot/bzImage \
-hda $BUILDROOTDIR/output/images/rootfs.ext2 \
-append "root=/dev/sda rw rootfstype=ext2 maxcpus=4" \
-cpu phenom \
-smp 4 \
-gdb tcp::1234 \
-net nic -net user

  Guest:
  Vanilla Linux Kernel 2.6.37.6 64-bit with buildroot

  Mikael Pettersson from the linux kernel mailinglist told me it's an
  accepts-invalid bug in qemu.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/796202/+subscriptions

[Qemu-devel] [Bug 757702] Re: ARM: singlestepping insn which UNDEFs should stop at UNDEF vector insn, not after it

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/757702

Title:
  ARM: singlestepping insn which UNDEFs should stop at UNDEF vector
  insn, not after it

Status in QEMU:
  Incomplete

Bug description:
  ARMv7a has lot of undefined instruction from its instruction opcode
  space. This undefined instructions are very useful for replacing
  sensitive non-priviledged instructions of guest operating systems
  (virtualization). The undefined instruction exception executes at
   + 0x4, where  can be 0x0 or
  0xfff0. Currently, in qemu 0.14.0 undefined instruction fault at
  0x8 offset instead of 0x4. This was not a problem with qemu 0.13.0,
  seems like this is a new bug. As as example, if we try to execute
  value "0xec019800" in qemu 0.14.0 then it should cause undefined
  exception at +0x4 since "0xec019800" is an undefined
  instruction.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/757702/+subscriptions

[Qemu-devel] [Bug 752476] Re: monitor command mouse_button 1 moves mouse

[Thomas Huth]

Triaging old bug tickets ... can you somehow still reproduce this
problem with the latest version of QEMU (currently v2.9), or could we
close this ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/752476

Title:
  monitor command mouse_button 1 moves mouse

Status in QEMU:
  Incomplete

Bug description:
  via the qemu -monitor interface, it is possible to move and click the mouse 
using
  mouse_move 2 1
  mouse_button 1
  but the mouse_button command always moves the mouse to (0,0) making it rather 
unusable to (auto-)trigger any widgets in the VM from the outside.

  Would be nice to have this available for my qemu/kvm based os-autoinst
  testing framework.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/752476/+subscriptions

Re: [Qemu-devel] [PATCH v4 0/2] Support CPUID signature for TCG

[Richard W.M. Jones]

On Fri, May 19, 2017 at 04:41:46PM -0300, Eduardo Habkost wrote:
> On Tue, May 09, 2017 at 07:18:07AM -0700, Richard Henderson wrote:
> > On 05/09/2017 07:13 AM, Richard W.M. Jones wrote:
> > > On Tue, May 09, 2017 at 07:05:51AM -0700, Richard Henderson wrote:
> > > > > Daniel P. Berrange (2):
> > > > >i386: rewrite way CPUID index is validated
> > > > >i386: expose "TCGTCGTCGTCG" in the 0x4000 CPUID leaf
> > > > 
> > > > I probably should have commented earlier but...  what's the point?
> > > > 
> > > > If you want the guest os to actually do anything with this, what do
> > > > you gain for advertising TCG over KVM?
> > > 
> > > I can see this being useful from virt-what, since it would allow
> > > vendors to diagnose problems being caused by TCG ...
> > 
> > Thanks, that's an excellent point.
> 
> Richard, Paolo: I would like to merge this. Do you agree?

Ah, other Richard, OK!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH v4 0/2] Support CPUID signature for TCG

[Richard W.M. Jones]

On Fri, May 19, 2017 at 04:41:46PM -0300, Eduardo Habkost wrote:
> On Tue, May 09, 2017 at 07:18:07AM -0700, Richard Henderson wrote:
> > On 05/09/2017 07:13 AM, Richard W.M. Jones wrote:
> > > On Tue, May 09, 2017 at 07:05:51AM -0700, Richard Henderson wrote:
> > > > > Daniel P. Berrange (2):
> > > > >i386: rewrite way CPUID index is validated
> > > > >i386: expose "TCGTCGTCGTCG" in the 0x4000 CPUID leaf
> > > > 
> > > > I probably should have commented earlier but...  what's the point?
> > > > 
> > > > If you want the guest os to actually do anything with this, what do
> > > > you gain for advertising TCG over KVM?
> > > 
> > > I can see this being useful from virt-what, since it would allow
> > > vendors to diagnose problems being caused by TCG ...
> > 
> > Thanks, that's an excellent point.
> 
> Richard, Paolo: I would like to merge this. Do you agree?

It's fine by me but note that I didn't review the patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v4 1/2] i386: rewrite way CPUID index is validated

[Eduardo Habkost]

On Fri, May 19, 2017 at 04:58:23PM +0200, Kashyap Chamarthy wrote:
> On Tue, May 09, 2017 at 02:27:35PM +0100, Daniel P. Berrange wrote:
> > Change the nested if statements into a flat format, to make
> > it clearer what validation / capping is being performed on
> > different CPUID index values.
> > 
> > NB this changes behaviour when "index > env->cpuid_xlevel2".
> > This won't have any guest-visible effect because no there is
> > no CPUID[0xC001]
> 
> Nit: When applying, maybe the maintainer could fix the typo:
> 
> "because no there is no" -> "because there is no"

This was already merged, unfortunately.  Thanks for noting that,
anyway.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[anonym]

Gentle ping! :)

I've lost the Message-IDs of my previous posts on this topic but its about this 
patch submission:

* https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03433.html
* https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03434.html

Cheers!

> Hi, list!
> 
> I guess you can take this as a ping for this patch submission. We'd very much 
> like this to be dealt with soon, so the fix can land into Debian Stretch 
> before 
> it is released, otherwise some of us will have to work around this issue for 
> the next two years. :/
> 
> Hi, Gerd!
> 
> You are the author of QEMU commit 05f43d4 which fixed CVE-2016-8576, which I 
> believe introduced the regression described below. Do you think my patch's 
> approach is safe and makes sense?
> 
> At the moment this issue is causing trouble for Tails [1] since its installer 
> hits this bug, which affects both users of QEMU + Tails, and our automated QA 
> infrastructure, which uses QEMU.
> 
> Cheers!
> 
> [1] https://tails.boum.org/
> 
> address@hidden:
>> From: anonym 
>> 
>> While formatting partitions (on virtual USB drives and the nec-xhci
>> virtual USB controller) to EXT4, I have observed errors like these:
>> 
>> kernel: sd 8:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ABORT
>> driverbyte=DRIVER_OK
>> kernel: sd 8:0:0:0: [sda] tag#0 CDB: Write(10) 2a 00 00 66 49 86
>> 00 08 00 00
>> kernel: blk_update_request: I/O error, dev sda, sector 6703494
>> kernel: Buffer I/O error on dev dm-0, logical block 1573254, lost
>> async page write
>> 
>> Raising TRB_LINK_LIMIT fixes the limit, but the new value was
>> admittedly arbitrarily chosen.
>> 
>> Regarding cycle detection in general, allowing at most 4 levels of
>> links seems pretty low. This bump should be safe: a high number only
>> means that we get a performance hit when encountering cycles but then
>> we should have a fatal error any way; a low number instead means that
>> we'll incorrectly identify cycles and abort operations that otherwise
>> would succeed, like in the case above.
>> 
>> Signed-off-by: anonym 
>> ---
>>  hw/usb/hcd-xhci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 4acf0c6dd8..d14ce126a2 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -53,7 +53,7 @@
>>   * to the specs when it gets them */
>>  #define ER_FULL_HACK
>>  
>> -#define TRB_LINK_LIMIT  4
>> +#define TRB_LINK_LIMIT  32
>>  
>>  #define LEN_CAP 0x40
>>  #define LEN_OPER(0x400 + 0x10 * MAXPORTS)

Re: [Qemu-devel] [PATCH v4 0/2] Support CPUID signature for TCG

[Eduardo Habkost]

On Tue, May 09, 2017 at 07:18:07AM -0700, Richard Henderson wrote:
> On 05/09/2017 07:13 AM, Richard W.M. Jones wrote:
> > On Tue, May 09, 2017 at 07:05:51AM -0700, Richard Henderson wrote:
> > > > Daniel P. Berrange (2):
> > > >i386: rewrite way CPUID index is validated
> > > >i386: expose "TCGTCGTCGTCG" in the 0x4000 CPUID leaf
> > > 
> > > I probably should have commented earlier but...  what's the point?
> > > 
> > > If you want the guest os to actually do anything with this, what do
> > > you gain for advertising TCG over KVM?
> > 
> > I can see this being useful from virt-what, since it would allow
> > vendors to diagnose problems being caused by TCG ...
> 
> Thanks, that's an excellent point.

Richard, Paolo: I would like to merge this. Do you agree?

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
> 
> Moving the cleanup of from_src_file there too.
> 
> Signed-off-by: Peter Xu 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/migration.c| 8 +++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 92617fc..a4006b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>  struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
>  if (mis->to_src_file) {
> +/* Tell source that we are done */
> +migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 
> 0);
>  qemu_fclose(mis->to_src_file);
>  mis->to_src_file = NULL;
>  }
>  
> +if (mis->from_src_file) {
> +qemu_fclose(mis->from_src_file);
> +mis->from_src_file = NULL;
> +}
> +
>  qemu_event_destroy(>main_thread_load_event);
>  loadvm_free_handlers(mis);
>  }
> @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
>  exit(EXIT_FAILURE);
>  }
>  
> -qemu_fclose(f);
>  free_xbzrle_decoded_buf();
>  
>  mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..57aa208 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState 
> *mis)
>  }
>  
>  postcopy_state_set(POSTCOPY_INCOMING_END);
> -migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>  
>  if (mis->postcopy_tmp_page) {
>  munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH V5 8/9] migration: add postcopy total blocktime into query-migrate

[Dr. David Alan Gilbert]

* Alexey Perevalov (a.pereva...@samsung.com) wrote:
> Postcopy total blocktime is available on destination side only.
> But query-migrate was possible only for source. This patch
> adds ability to call query-migrate on destination. To distinguish
> src/dst, state of the MigrationState is using, query-migrate prepares
> MigrationInfo for source machine only in case of migration's state is 
> different
> than MIGRATION_STATUS_NONE.
> 
> To be able to see postcopy blocktime, need to request postcopy-blocktime
> capability.
> 
> The query-migrate command will show following sample result:
> {"return":
> "postcopy_vcpu_blocktime": [115, 100],
> "status": "completed",
> "postcopy_blocktime": 100
> }}
> 
> postcopy_vcpu_blocktime contains list, where the first item is the first
> vCPU in QEMU.

Lets just check Eric is happy with the qapi side.
Please also update hmp.c:hmp_info_migrate.

A few comments below.

> Signed-off-by: Alexey Perevalov 
> ---
>  include/migration/migration.h |  4 +++
>  migration/migration.c | 47 ++--
>  migration/postcopy-ram.c  | 73 
> +++
>  migration/trace-events|  1 +
>  qapi-schema.json  |  6 +++-
>  5 files changed, 127 insertions(+), 4 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 7e69a2d..aba0535 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -135,6 +135,10 @@ struct MigrationIncomingState {
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
>  void migration_incoming_state_destroy(void);
> +/*
> + * Functions to work with blocktime context
> + */
> +void fill_destination_postcopy_migration_info(MigrationInfo *info);
>  
>  struct MigrationState
>  {
> diff --git a/migration/migration.c b/migration/migration.c
> index c0443ce..7a4f33f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -666,9 +666,15 @@ static void populate_ram_info(MigrationInfo *info, 
> MigrationState *s)
>  }
>  }
>  
> -MigrationInfo *qmp_query_migrate(Error **errp)
> +/* TODO improve this assumption */
> +static bool is_source_migration(void)
> +{
> +MigrationState *ms = migrate_get_current();
> +return ms->state != MIGRATION_STATUS_NONE;
> +}
> +
> +static void fill_source_migration_info(MigrationInfo *info)
>  {
> -MigrationInfo *info = g_malloc0(sizeof(*info));
>  MigrationState *s = migrate_get_current();
>  
>  switch (s->state) {
> @@ -759,10 +765,45 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>  break;
>  }
>  info->status = s->state;
> +}
> +
> +static void fill_destination_migration_info(MigrationInfo *info)
> +{
> +MigrationIncomingState *mis = migration_incoming_get_current();
>  
> -return info;
> +switch (mis->state) {
> +case MIGRATION_STATUS_NONE:
> +break;
> +case MIGRATION_STATUS_SETUP:
> +case MIGRATION_STATUS_CANCELLING:
> +case MIGRATION_STATUS_CANCELLED:
> +case MIGRATION_STATUS_ACTIVE:
> +case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> +case MIGRATION_STATUS_FAILED:
> +case MIGRATION_STATUS_COLO:
> +info->has_status = true;
> +break;
> +case MIGRATION_STATUS_COMPLETED:
> +info->has_status = true;
> +fill_destination_postcopy_migration_info(info);
> +break;
> +}
> +info->status = mis->state;
>  }
>  
> +MigrationInfo *qmp_query_migrate(Error **errp)
> +{
> +MigrationInfo *info = g_malloc0(sizeof(*info));
> +
> +if (is_source_migration()) {
> +fill_source_migration_info(info);
> +} else {
> +fill_destination_migration_info(info);
> +}

A VM that was migated in can then later get migrated out;
so I think you need to give both sets of data.
Which probably means you need a second status field
since existing stuff might get confused if it's watching
an outbound migration after an inbound one.

Dave
 
> +
> + return info;
> + }
> +
>  void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>Error **errp)
>  {
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index e2660ae..fe047c8 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -129,6 +129,71 @@ static struct PostcopyBlocktimeContext 
> *blocktime_context_new(void)
>  return ctx;
>  }
>  
> +static int64List *get_vcpu_blocktime_list(PostcopyBlocktimeContext *ctx)
> +{
> +int64List *list = NULL, *entry = NULL;
> +int i;
> +
> +for (i = smp_cpus - 1; i >= 0; i--) {
> +entry = g_new0(int64List, 1);
> +entry->value = ctx->vcpu_blocktime[i];
> +entry->next = list;
> +list = entry;
> +}
> +
> +return list;
> +}
> +
> +/*
> + * This function just provide calculated blocktime per cpu and trace it.
> + * Total blocktime is calculated in 

Re: [Qemu-devel] [RFC] qmp: Return 'user_creatable' & 'hotpluggable' fields on qom-list-types

[Eduardo Habkost]

On Thu, May 18, 2017 at 01:59:53PM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > Currently there's no way for QMP clients to get a list of device types
> > that are really usable with -device.  This information would be useful
> > for management software and test scripts (like the device-crash-test
> > script I have submitted recently).  Interestingly, the "info qdm" HMP
> > command provides this information, but no QMP command does.
> >
> > Add two new fields to the return value of qom-list-types:
> > "user-creatable-device" and "hotpluggable-device".
> 
> Does the combination
> 
> "user-creatable-device": false,
> "hotpluggable-device": true
> 
> make any sense?

It doesn't, and the code ensures this won't happen.

> 
> Exposing information on user-creatable/hot-pluggable via QMP makes
> sense.  The question is how.  This is a design question, and as so often
> with design questions, I need more words to make my case than I'd like
> to.  Please bear with me.
> 
> > Also, add extra
> > arguments for filtering the list of types based on the new fields.
> 
> I consider the case for filtering weak.  Let's ignore this part for now.

I considered sending a version that didn't include filtering.  I
will be happy to ignore it.  :)

> 
> We have a number of commands to introspect static information,
> e.g. query-version, query-commands, query-qmp-schema, query-target,
> query-machines, query-cpu-definitions, query-chardev-backends,
> device-list-properties, qom-list-types.
> 
> Aside: us abandoning the convention to name such commands query-FOO is
> regrettable.
> 
> In its basic form, i.e. without arguments, qom-list-types does what its
> name suggests: it lists the QOM types.
> 
> It also permits finding out whether a type is abstract, but only in a
> roundabout way: subtract the result of running it without arguments (or
> with 'abstract':false) from the result with 'abstract':true.
> 
> It also permits finding the "implements" relation, but only in an even
> more roundabout way: run it with 'implements':T for every abstract T,
> then solve the resulting puzzle.
> 
> Unless there's a direct way to find both (and I'm not aware of any),
> this is bad design.  The obvious fix is to extend its return type to
> include the information.

Agreed.

> 
> With qom-list-types fixed that way, there's precedence for exposing
> additional information on QOM types there.
> 
> Note that both the "abstract" bit and the "implements directly" list
> apply to any QOM type, not just to certain subtypes.  As proposed,
> "user-creatable-device' and "hotpluggable-device" apply only to the
> "device" subtype.  There's no precedence for exposing information
> specific to certain subtypes.
> 
> If we want to do it anyway, then qom-list-type should perhaps return a
> union.  Taken to the logical conclusion, this becomes a nest of unions
> mirroring the "direct subtype of" relation.  Hmm.

I don't think that's the logical conclusion.  The differences
between "object" and "device" are hardcoded in our interfaces: we
already have -object and -device.  Modelling our data to take
care of thoes differences doesn't mean we will also have to treat
the differences between other QOM types (e.g. between
"pci-device" and "usb-device") the same way.

Now, we could choose to encode that in different ways.  We could
have a single command for all QOM types (like qom-list-types or a
new query-qom-type command) that return an union, or have
separate commands for object types and device types.  I'm not
sure what's the better option here (see below for additional
comments on that).

> 
> However, we already have a command to introspect device types:
> device-list-properties.  Can we expose the information as read-only
> property/ies of type "device" and be done?

I don't think we should.  device-list-properties has a very
specific purpose: listing QOM properties that can be read or
written using qom-get and qom-set, or set in -device.  If there's
no use case for qom-get/qom-set on a property like "hotpluggable"
or "user-creatable", we shouldn't make them QOM properties (hence
they shouldn't appear on device-list-properties).

But that doesn't mean we can't have something like a
"query-device-type" command that returns other information about
a given device type, in addition to the property list.

(In my specific use case (the device-crash-test script), I would
prefer to avoid having to fetch the full list of properties of
every single device type, just to find out which ones are
user-creatable.  But this is not really performance-critical
code, so I can live with that.)


So, I see a few options here:

1) Including DeviceClass::user_creatable and
   DeviceClass::hotpluggable in qom-list-types output.  Probably
   using an union.

2) Adding a new "query-device-type" command, returning
   DeviceClass::user_creatable and DeviceClass::hotpluggable, and
   

Re: [Qemu-devel] [PATCH V5 7/9] migration: calculate vCPU blocktime on dst side

[Dr. David Alan Gilbert]

* Alexey (a.pereva...@samsung.com) wrote:
> On Tue, May 16, 2017 at 12:34:16PM +0100, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > This patch provides blocktime calculation per vCPU,
> > > as a summary and as a overlapped value for all vCPUs.
> > > 
> > > This approach was suggested by Peter Xu, as an improvements of
> > > previous approch where QEMU kept tree with faulted page address and cpus 
> > > bitmask
> > > in it. Now QEMU is keeping array with faulted page address as value and 
> > > vCPU
> > > as index. It helps to find proper vCPU at UFFD_COPY time. Also it keeps
> > > list for blocktime per vCPU (could be traced with page_fault_addr)
> > > 
> > > Blocktime will not calculated if postcopy_blocktime field of
> > > MigrationIncomingState wasn't initialized.
> > > 
> > > Signed-off-by: Alexey Perevalov 
> > 
> > I have some multi-threading/ordering worries still.
> > 
> > The fault thread receives faults over the ufd and calls
> > mark_postcopy_blocktime_being.  That's fine.
> > 
> > The receiving thread receives pages, calls place page, and
> > calls mark_postcopy_blocktime_end.  That's also fine.
> > 
> > However, remember that we send pages from the source without
> > them being requested as background transfers; consider:
> > 
> > 
> > Source   receive-thread  fault-thread
> > 
> >   1  Send A
> >   2  Receive A
> >   3Access A
> >   4Report on UFD
> >   5  Place
> >   6Read UFD entry
> > 
> > 
> >  Placing and reading UFD race - and up till now that's been fine;
> > so we can read off the ufd an address that's already on it's way from
> > the source, and which we might just be receiving, or that we might
> > have already placed.
> > 
> > In this code at (6) won't you call mark_postcopy_blocktime_start
> > even though it's already been placed at (5) ? Then that blocktime
> > will stay set until the end of the run?
> > 
> > Perhaps that's not a problem; if mark_postcopy_blocktime_end is called
> > for a different address it wont count the blocktime; and when
> > mark_postcopy_blocktime_start is called for a different address it'll
> > remove the addres that was a problem above - so perhaps that's fine?
> It's not 100% fine, but I'm going to clarify my previous answer to that
> email where I wrote "forever". That mechanism will think vCPU is blocked
> until the same vCPU will block/page copied again.
> Unfortunately we don't know vCPU index at *_end time, and I don't want
> to extend struct uffdio_copy and add pid into it.

You couldn't anyway, one uffdio_copy might wake up multiple PIDs.

> But now I have only
> expensive and robust or not expensive and not robust solution, like
> keeping list of page addressed which was faulted (or just one page
> address, the latest, taking into account _end, _start sequence should be
> quick, and no other pages interpose, but it's assumption).
> 
> BTW with tree based solution, proposed in the first version, was possible to
> lookup node by pageaddr in _end and mark it as populated.

Yes , sorry, I hadn't realised at the time that this solution wasn't
robust.
Would this be fixed by a 'received' pages bitmap? i.e. a bitmap with one
bit per page (fixed 0.003% RAM overhead - tiny) that gets set by
mark_postcopy_blocktime_end (called before the 'place' operation)
and checked in mark_postcopy_blocktime_start?
That would be interesting because that bitmap is potentially needed by
other projects (recovery from network failure in particular).
However, I'm not sure it really helps - you'd have to get the
ordering just-right, and I'm not sure it's possible.
My thoughts are something like:

blocktime_end:
   set bitmap entry for 'arrived'
   read CPU stall address, if none-0 then zero it and update stats

blocktime_start:
   set CPU stall address
   check bitmap entry
 if set then zero stall-address

is that safe?

Dave

> 
> 
> > 
> > 
> > > ---
> > >  migration/postcopy-ram.c | 87 
> > > +++-
> > >  migration/trace-events   |  5 ++-
> > >  2 files changed, 90 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > index a1f1705..e2660ae 100644
> > > --- a/migration/postcopy-ram.c
> > > +++ b/migration/postcopy-ram.c
> > > @@ -23,6 +23,7 @@
> > >  #include "migration/postcopy-ram.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "sysemu/balloon.h"
> > > +#include 
> > >  #include "qemu/error-report.h"
> > >  #include "trace.h"
> > >  
> > > @@ -542,6 +543,86 @@ static int ram_block_enable_notify(const char 
> > > *block_name, void *host_addr,
> > >  return 0;
> > >  }
> > >  
> > > +static int get_mem_fault_cpu_index(uint32_t pid)
> > > +{
> > > +CPUState *cpu_iter;
> > > +
> > > +

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

[Dr. David Alan Gilbert]

* Alexey (a.pereva...@samsung.com) wrote:
> On Tue, May 16, 2017 at 11:32:51AM +0100, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (a.pereva...@samsung.com) wrote:
> > > This modification is necessary for userfault fd features which are
> > > required to be requested from userspace.
> > > UFFD_FEATURE_THREAD_ID is a one of such "on demand" feature, which will
> > > be introduced in the next patch.
> > > 
> > > QEMU need to use separate userfault file descriptor, due to
> > > userfault context has internal state, and after first call of
> > > ioctl UFFD_API it changes its state to UFFD_STATE_RUNNING (in case of
> > > success), but
> > > kernel while handling ioctl UFFD_API expects UFFD_STATE_WAIT_API. So
> > > only one ioctl with UFFD_API is possible per ufd.
> > > 
> > > Signed-off-by: Alexey Perevalov 
> > > ---
> > >  migration/postcopy-ram.c | 82 
> > > ++--
> > >  1 file changed, 73 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > index 0f75700..c96d5f5 100644
> > > --- a/migration/postcopy-ram.c
> > > +++ b/migration/postcopy-ram.c
> > > @@ -60,32 +60,96 @@ struct PostcopyDiscardState {
> > >  #include 
> > >  #include 
> > >  
> > > -static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
> > > +
> > > +/*
> > > + * Check userfault fd features, to request only supported features in
> > > + * future.
> > > + * __NR_userfaultfd - should be checked before
> > > + * Return obtained features
> > 
> > That's not quite right;
> >  * Returns: True on success, sets *features to supported features
> > False on failure or if kernel doesn't support ufd
> > 
> yes, obtained features is out parameter,
> but I want to keep false uncommented and just add error_report into
> syscall check, because the possible reason of failure is:
> 1. No syscall userfaultfd, but function expects that syscall, it reflects in
> comment
> 2  Within syscall:  exhausted fd or out of memory (file in kernel
> is allocating)
> 3. Problem in ioctl due to internal state of UFFD, as example
> UFFDIO_API after UFFDIO_REGISTER

I don't think we're allowed to depend on error pointers, but either
way we should comment it to make sure it's clear, so if you have a
boolean return at least say it's true for success and explain features
etc.

> Also I would prefer follow migration/ram.c comment style.

Yes, that's fine - it's the content of the comment I was more
worried about (and the one below).

Dave

> > > + */
> > > +static bool receive_ufd_features(uint64_t *features)
> > >  {
> > > -struct uffdio_api api_struct;
> > > -uint64_t ioctl_mask;
> > > +struct uffdio_api api_struct = {0};
> > > +int ufd;
> > > +bool ret = true;
> > > +
> > > +/* if we are here __NR_userfaultfd should exists */
> > > +ufd = syscall(__NR_userfaultfd, O_CLOEXEC);
> > > +if (ufd == -1) {
> > > +return false;
> > > +}
> > >  
> > > +/* ask features */
> > >  api_struct.api = UFFD_API;
> > >  api_struct.features = 0;
> > >  if (ioctl(ufd, UFFDIO_API, _struct)) {
> > > -error_report("%s: UFFDIO_API failed: %s", __func__
> > > +error_report("%s: UFFDIO_API failed: %s", __func__,
> > >   strerror(errno));
> > > +ret = false;
> > > +goto release_ufd;
> > > +}
> > > +
> > > +*features = api_struct.features;
> > > +
> > > +release_ufd:
> > > +close(ufd);
> > > +return ret;
> > > +}
> > 
> > Needs a comment; perhaps something like:
> >   * Called once on a newly opened ufd, can request specific features.
> >   * Returns: True on success
> > 
> > > +static bool request_ufd_features(int ufd, uint64_t features)
> > > +{
> > > +struct uffdio_api api_struct = {0};
> > > +uint64_t ioctl_mask;
> > > +
> > > +api_struct.api = UFFD_API;
> > > +api_struct.features = features;
> > > +if (ioctl(ufd, UFFDIO_API, _struct)) {
> > > +error_report("%s failed: UFFDIO_API failed: %s", __func__,
> > > +strerror(errno));
> > >  return false;
> > >  }
> > >  
> > > -ioctl_mask = (__u64)1 << _UFFDIO_REGISTER |
> > > - (__u64)1 << _UFFDIO_UNREGISTER;
> > > +ioctl_mask = 1 << _UFFDIO_REGISTER |
> > > + 1 << _UFFDIO_UNREGISTER;
> > >  if ((api_struct.ioctls & ioctl_mask) != ioctl_mask) {
> > >  error_report("Missing userfault features: %" PRIx64,
> > >   (uint64_t)(~api_struct.ioctls & ioctl_mask));
> > >  return false;
> > >  }
> > >  
> > > +return true;
> > > +}
> > > +
> > > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> > > +{
> > > +uint64_t asked_features = 0;
> > > +uint64_t supported_features;
> > > +
> > > +/*
> > > + * it's not possible to
> > > + * request UFFD_API twice per one fd
> > > + */
> > > +if 

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Fri, May 19, 2017 at 03:33:12PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > shutdown() is safe, in that it stops any other threads accessing the fd
> > > > but doesn't allow it's reallocation until the close;  We perform the
> > > > close only when we've joined all other threads that were using the fd.
> > > > Any of the threads that do new calls on the fd get an error and quickly
> > > > fall down their error paths.
> > > 
> > > Ahh that's certainly an interesting scenario. That would certainly be
> > > a problem with the migration code when this was originally written.
> > > It had two QEMUFile structs each with an 'int fd' field, so when you
> > > close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> > > used by another thread.
> > > 
> > > Since we switched over to use QIOChannel though, I think the thread
> > > scenario you describe should be avoided entirely. When you have multiple
> > > QEMUFile objects, they each have a reference counted pointer to the same
> > > underlying QIOChannel object instance. So when QEMUFile triggers a call
> > > to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> > > Since the other threads have a reference to the same QIOChannel object,
> > > they'll now see this fd == -1 straightaway.
> > > 
> > > So, IIUC, this should make the need for shutdown() redundant (at least
> > > for the thread race conditions you describe).
> > 
> > That's not thread safe unless you're doing some very careful locking.
> > Consider:
> >   T1  T2   
> >  oldfd=fd   tmp=fd
> >  fd=-1
> >  close(oldfd)
> >  unrelated open()
> > read(tmp,...
> > 
> > In practice every use of fd will be a copy into a tmp and then the
> > syscall; the unrelated open() could happen in another thread.
> > (OK, the gap between the tmp and the read is tiny, although if we're
> > doing multiple operations chances are the compiler will optimise
> > it to the top of a loop).
> > 
> > There's no way to make that code safe.
> 
> Urgh, yes, I see what you mean.
> 
> Currently the QIOChannelCommand implementation, uses a pair of anonymous
> pipes for stdin/out to the child process. I wonder if we could switch
> that to use socketpair() instead, thus letting us shutdown() on it too.
> 
> Though I guess it would be sufficient for qio_channel_shutdown() to
> merely kill the child PID, while leaving the FDs open, as then you'd
> get EOF and/or EPIPE on the read/writes.

Yes, I guess it's a question of which one is more likely to actually
kill the exec child off; the socketpair is more likely to cause the
source side migration code to cancel cleanly, although a kill -9 
should sort out a wayward exec child.

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote:
> >> To sum it up although I'm currently leaning towards abandoning my idea
> >> of two sections for two devices, I'm not comfortable with making the
> >> call myself. I'm hoping for some maintainer guidance (s390x, virtio
> >> and migration). 
> >  > dhildenb etc>
> > 
> > OK, so I think:
> >   a) First split the series into two separate series; one that
> > VMStatifies the existing stuff without breaking compatibility;
> > and one that adds the new stuff.  Lets get the first of those
> > going in while we think about the second.
> > 
> > a.1) I'd do this with patches that convert one chunk into
> >  vmstate and remove the corresponding C code at the same time.
> > 
> >   b) While the way PCI devices are done is weird, I think it'll
> > be a lot simpler if you can stick to a structure that's similar
> > to them while diverging.  It's hard enough for those of us
> > who don't do Virtio every day to get our minds around virtio-pci
> > without it being different for virtio-ccw/css.
> > 
> >   c) To do (b) I suggest:
> >   c.1) you *don't* add a vmsd to your virtio_ccw_device
> > 
> >   c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any
> >  non-virtio devices you migrate (like each of the PCI devices
> >   have)
> > 
> >   c.3) You can add extra state for CSS to the ->save_extra_state
> >handle on virtio devices or to the config
> > 
> >   d) vmstatifying the config is OK as well.
> > 
> > I should say I'm no virtio expert, so if any of that's truly
> > mad say so.
> > 
> > Dave
> > 
> 
> Agreed (a,b,c,d). Reorganizing my patch set according to a) is
> going to require some effort, but it should not be too bad. 
> About c.2): I don't think we have any migratable non virtio devices
> yet, but I'll keep it in mind.
> 
> I do not understand what you mean by c.3) and extra_sate. Maybe
> it will settle with time. My idea of extending VirtioCcwDevice
> is just adding subsections to it's VMStateDescription. The
> call vmstate_save_state(f, _virtio_ccw_dev, dev, NULL)
> in save_config should take care of compatibility. Maybe some
> staring at virtio-pci is going to help, but right now I can't tell
> what's the extra_state for, and how/why is it 'extra'.

Yes adding extra subsections into the 'config' is probably fine;
but there's also another hook that Jason added, see a6df8adf3,
it's an existing subsection at the end of the virtio state
that can be linked for transport specific data.

Dave

> Thanks for your patience!
> 
> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 5/5] tests: add functional test validating ipv4/ipv6 address flag handling

[Daniel P. Berrange]

The semantics around handling ipv4=on|off & ipv6=on|off are quite
subtle to understand in combination with the various hostname addresses
and backend types. Introduce a massive test matrix that launches QEMU
and validates the ability to connect a client on each protocol as
appropriate.

The test requires that the host has ability to bind to both :: and
0.0.0.0, on port 9000. If either protocol is not available, or if
something is already listening on that port the test will skip.

Although it isn't using the QTest APIs, it expects the
QTEST_QEMU_BINARY env variable to be set.

Signed-off-by: Daniel P. Berrange 
---
 tests/.gitignore   |   1 +
 tests/Makefile.include |   4 +
 tests/test-sockets-proto.c | 855 +
 3 files changed, 860 insertions(+)
 create mode 100644 tests/test-sockets-proto.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 40c2e3e..9fa14e5 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -76,6 +76,7 @@ test-qobject-output-visitor
 test-rcu-list
 test-replication
 test-shift128
+test-sockets-proto
 test-string-input-visitor
 test-string-output-visitor
 test-thread-pool
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 16ff8f3..c3b487e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -261,6 +261,7 @@ check-qtest-i386-y += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/postcopy-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
+check-qtest-i386-y += tests/test-sockets-proto$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst 
i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -287,6 +288,7 @@ check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-ppc64-y += tests/pnv-xscom-test$(EXESUF)
 check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
 check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
+check-qtest-ppc64-y += tests/test-sockets-proto$(EXESUF)
 check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
 check-qtest-ppc64-y += tests/rtas-test$(EXESUF)
 check-qtest-ppc64-y += tests/pxe-test$(EXESUF)
@@ -741,6 +743,8 @@ tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o 
$(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
 tests/postcopy-test$(EXESUF): tests/postcopy-test.o
+tests/test-sockets-proto$(EXESUF): tests/test-sockets-proto.o \
+   $(test-io-obj-y)
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
$(qtest-obj-y) $(test-io-obj-y) $(libqos-virtio-obj-y) 
$(libqos-pc-obj-y) \
$(chardev-obj-y)
diff --git a/tests/test-sockets-proto.c b/tests/test-sockets-proto.c
new file mode 100644
index 000..f058a50
--- /dev/null
+++ b/tests/test-sockets-proto.c
@@ -0,0 +1,855 @@
+/*
+ * QTest for IPv4/IPv6 protocol setup
+ *
+ * Copyright (c) 2017 Red Hat, Inc. and/or its affiliates
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+
+#include "io/channel-socket.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+
+typedef struct {
+const char *name;
+const char *args;
+bool ipv4;
+bool ipv6;
+bool error;
+} QSocketsData;
+
+/*
+ * This is the giant matrix of combinations we need to consider.
+ * There are 3 axes we deal with
+ *
+ * Axis 1: Protocol flags:
+ *
+ *  ipv4=unset, ipv6=unset  -> v4 & v6 clients ([1]
+ *  ipv4=unset, ipv6=off-> v4 clients only
+ *  ipv4=unset, ipv6=on -> v6 clients only
+ *  ipv4=off, ipv6=unset-> v6 clients only
+ *  ipv4=off, ipv6=off  -> error - can't disable both [2]
+ *  ipv4=off, ipv6=on   -> v6 clients only
+ *  ipv4=on, ipv6=unset -> v4 clients only
+ *  ipv4=on, ipv6=off   -> v4 clients only
+ *  ipv4=on, ipv6=on-> v4 & v6 clients [3]
+ *
+ * Depending on the listening address, some of those combinations
+ * may result in errors. eg ipv4=off,ipv6=on combined with 0.0.0.0
+ * is nonsensical.
+ *
+ * [1] Some backends only support a single socket listener, so
+ * will actually only allow v4 clients
+ * [2] QEMU should fail to startup in this case
+ * [3] If hostname is "" or "::", then we get a single listener
+ * on IPv6 and thus can also accept v4 clients. For all other
+ * hostnames, have same problem as [1].
+ *
+ * Axis 2: Listening address:
+ *
+ *  ""- resolves to 0.0.0.0 and ::, in that order
+ *  "0.0.0.0" - v4 clients only
+ *  "::"  - Mostly v6 clients only. Some scenarios should
+ *  permit v4 clients too.
+ *
+ * Axis 3: Backend type:
+ *
+ *  Migration - restricted to a single listener. Also relies
+ *  on buggy inet_parse() which can't accept
+ *

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Halil Pasic]

On 05/19/2017 07:47 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
 We could also consider making WITH_TMP act as a normal field. 
 Working on the whole state looks like a bit like a corner case:
 we have some stuff adjacent in the migration stream, and we have
 to map it on multiple fields (and vice-versa). Getting the whole
 state with a pointer to a certain field could work via container_of.
>>> You do need to know which field you're working on to be able to safely
>>> use container_of, so I'm not sure how it would work for you in this
>>> case.
>>
>>
>> Well, if you have to write to just one field you are good because you
>> already have a pointer to that field (.offset was added).
>>
>> If you need to write to multiple fields in post_load then you just pick
>> one of the fields you are going to write to (probably the first) and use
>> container_of to gain access to the whole state. The logic is specific to
>> the bunch of the fields you are going to touch anyway.
>>
>> In fact any member of the state struct will do it's only important that
>> you use the same when creating the VMStateField and when trying to get a
>> pointer to the parent in pre_save and post_load.
>>
>> I haven't tried, so I'm not 100% sure, but if you like I can try, and send
>> you a patch if it's viable. 
>>
>> I think the key to a good solution is really what is intended and typical
>> usage, and what is corner case. My patch in the other reply shows that we
>> can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
>> what I'm trying to do here a bit prettier at the expense of making what
>> you are doing in virtio-net a bit uglier, but whether it's a good idea to
>> do so, I cant tell.
> 
> Lets go with what you put in the other patch (I replied to it); I hadn't
> realised that was possible (hence my comment below).
> Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code
> base, I'll step back and see how to tidy them up.
> 
> Dave
> 

Sounds very reasonable. Let's do it like that!

Halil

>>>
>>> The other thought I'd had was that perhaps we could change the temporary
>>> structure in VMSTATE_WITH_TMP to:
>>>
>>>   struct foo {
>>>  struct whatever **parent;
>>>
>>> so now you could write to *parent in cases like these.
>>>
>>
>> Sorry, I do not get your idea. If you have some WIP patch in this
>> direction I would be happy to provide some feedback.
>>
>>  
 Btw, I would rather call it get_indicator a factory method or even a
 constructor than an allocator, but I think we understand each-other
 anyway.
>>> Yes; I'm not too worried about the actual name as long as it's short
>>> and obvious.
>>>
>>> I'd thought of 'allocator' since in most cases it's used where the
>>> load-time code allocates memory for the object being loaded.
>>> A constructor is normally something I think of as happening after
>>> allocation; and a factory, hmm maybe.  However, if it does the right
>>> thing I wouldn't object to any of those names.
>>>
>>
>> I think we are on the same page.
>>
>> Cheers,
>> Halil
>>
>>> Dave
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 

[Qemu-devel] [PATCH v2 4/5] io: preserve ipv4/ipv6 flags when resolving InetSocketAddress

[Daniel P. Berrange]

The original InetSocketAddress struct may have has_ipv4 and
has_ipv6 fields set, which will control both the ai_family
used during DNS resolution, and later use of the V6ONLY
flag.

Currently the standalone DNS resolver code drops the
has_ipv4 & has_ipv6 flags after resolving, which means
the later bind() code won't correctly set V6ONLY.

This fixes the following scenarios

  -vnc :0,ipv4=off
  -vnc :0,ipv6=on
  -vnc :::0,ipv4=off
  -vnc :::0,ipv6=on

which all mistakenly accepted IPv4 clients

Signed-off-by: Daniel P. Berrange 
---
 io/dns-resolver.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index 57a8896..c072d12 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -116,8 +116,10 @@ static int 
qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
 .numeric = true,
 .has_to = iaddr->has_to,
 .to = iaddr->to,
-.has_ipv4 = false,
-.has_ipv6 = false,
+.has_ipv4 = iaddr->has_ipv4,
+.ipv4 = iaddr->ipv4,
+.has_ipv6 = iaddr->has_ipv6,
+.ipv6 = iaddr->ipv6,
 };
 
 (*addrs)[i] = newaddr;
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/5] sockets: ensure we don't accept IPv4 clients when IPv4 is disabled

[Daniel P. Berrange]

Currently if you disable listening on IPv4 addresses, via the
CLI flag ipv4=off, we still mistakenly accept IPv4 clients via
the IPv6 listener socket due to IPV6_V6ONLY flag being unset.

We must ensure IPV6_V6ONLY is always set if ipv4=off

This fixes the following scenarios

  -incoming tcp::9000,ipv6=on
  -incoming tcp:[::]:9000,ipv6=on
  -chardev socket,id=cdev0,host=,port=9000,server,nowait,ipv4=off
  -chardev socket,id=cdev0,host=,port=9000,server,nowait,ipv6=on
  -chardev socket,id=cdev0,host=::,port=9000,server,nowait,ipv4=off
  -chardev socket,id=cdev0,host=::,port=9000,server,nowait,ipv6=on

which all mistakenly accepted IPv4 clients

Signed-off-by: Daniel P. Berrange 
---
 util/qemu-sockets.c | 40 +++-
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b82412e..c0f2d92 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -104,17 +104,16 @@ NetworkAddressFamily inet_netfamily(int family)
  *   f t   PF_INET6
  *   t -   PF_INET
  *   t f   PF_INET
- *   t t   PF_INET6
+ *   t t   PF_INET6/PF_UNSPEC
  *
  * NB, this matrix is only about getting the necessary results
  * from getaddrinfo(). Some of the cases require further work
  * after reading results from getaddrinfo in order to fully
- * apply the logic the end user wants. eg with the last case
- * ipv4=t + ipv6=t + PF_INET6, getaddrinfo alone can only
- * guarantee the ipv6=t part of the request - we need more
- * checks to provide ipv4=t part of the guarantee. This is
- * outside scope of this method and not currently handled by
- * callers at all.
+ * apply the logic the end user wants.
+ *
+ * In the first and last cases, we must set IPV6_V6ONLY=0
+ * when binding, to allow a single listener to potentially
+ * accept both IPv4+6 addresses.
  */
 int inet_ai_family_from_address(InetSocketAddress *addr,
 Error **errp)
@@ -124,6 +123,23 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
 error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
 return PF_UNSPEC;
 }
+if ((addr->has_ipv6 && addr->ipv6) && (addr->has_ipv4 && addr->ipv4)) {
+/*
+ * Some backends can only do a single listener. In that case
+ * we want empty hostname to resolve to "::" and then use the
+ * flag IPV6_V6ONLY==0 to get both protocols on 1 socket. This
+ * doesn't work for addresses other than "", so they're just
+ * inevitably broken until multiple listeners can be used,
+ * and thus we honour getaddrinfo automatic protocol detection
+ * Once all backends do multi-listener, remove the PF_INET6
+ * branch entirely.
+ */
+if (!addr->host || g_str_equal(addr->host, "")) {
+return PF_INET6;
+} else {
+return PF_UNSPEC;
+}
+}
 if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
 return PF_INET6;
 }
@@ -213,8 +229,14 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 port_max = saddr->has_to ? saddr->to + port_offset : port_min;
 for (p = port_min; p <= port_max; p++) {
 #ifdef IPV6_V6ONLY
-/* listen on both ipv4 and ipv6 */
-int v6only = 0;
+/*
+ * Deals with first & last cases in matrix in comment
+ * for inet_ai_family_from_address().
+ */
+int v6only =
+((!saddr->has_ipv4 && !saddr->has_ipv6) ||
+ (saddr->has_ipv4 && saddr->ipv4 &&
+  saddr->has_ipv6 && saddr->ipv6)) ? 0 : 1;
 #endif
 inet_setport(e, p);
 #ifdef IPV6_V6ONLY
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/5] sockets: don't block IPv4 clients when listening on "::"

[Daniel P. Berrange]

When inet_parse() parses the hostname, it is forcing the
has_ipv6 && ipv6 flags if the address contains a ":". This
means that if the user had set the ipv4=on flag, to try to
restrict the listener to just ipv4, an error would not have
been raised.  eg

   -incoming tcp:[::]:9000,ipv4

should have raised an error because listening for IPv4
on "::" is a non-sensical combination. With this removed,
we now call getaddrinfo() on "::" passing PF_INET and
so getaddrinfo reports an error about the hostname being
incompatible with the requested protocol.

Likewise it is explicitly setting the has_ipv4 & ipv4
flags when the address contains only digits + '.'. This
has no ill-effect, but also has no benefit, so is removed.

Signed-off-by: Daniel P. Berrange 
---
 util/qemu-sockets.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 397212b..b82412e 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -618,16 +618,12 @@ int inet_parse(InetSocketAddress *addr, const char *str, 
Error **errp)
 error_setg(errp, "error parsing IPv6 address '%s'", str);
 return -1;
 }
-addr->ipv6 = addr->has_ipv6 = true;
 } else {
 /* hostname or IPv4 addr */
 if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, ) != 2) {
 error_setg(errp, "error parsing address '%s'", str);
 return -1;
 }
-if (host[strspn(host, "0123456789.")] == '\0') {
-addr->ipv4 = addr->has_ipv4 = true;
-}
 }
 
 addr->host = g_strdup(host);
-- 
2.9.3

[Qemu-devel] [PATCH v2 0/5] Fix handling of IPv4/IPv6 dual stack

[Daniel P. Berrange]

This is a (much larger) followup to:

  v1: https://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05659.html

This series aims to fix a lot of bugs related to handling of IPv4 / IPv6
dual stack.

 - The VNC server mistakenly listened on two separate ports 5900+5901
   when the to= parameter was given
 - IPv6 sockets are accepting IPv4 clients even when IPv4 is set to
   be disabled
 - IPv6 sockets are failing to accept IPv4 clients when IPv4 is not set
   to be disabled
 - The VNC server was loosing the ipv4=/ipv6= settings due to a bug
   in the DNS resolver

The behaviour of all this is really subtle and hard to get working correctly
across all the different network backends. Thus, the most important part of
this patch series is the last patch which adds a test case covering the
backends for -vnc, -chardev tcp, -net socket, and -incoming socket, with
a 120 entry matrix.

IOW, if you think any of the first 4 patches are applying the wrong logic,
then take a look at the last patch and indicate which test matrix entries
are believed to be defining wrong behaviour :-)

Daniel P. Berrange (5):
  sockets: ensure we can bind to both ipv4 & ipv6 separately
  sockets: don't block IPv4 clients when listening on "::"
  sockets: ensure we don't accept IPv4 clients when IPv4 is disabled
  io: preserve ipv4/ipv6 flags when resolving InetSocketAddress
  tests: add functional test validating ipv4/ipv6 address flag handling

 io/dns-resolver.c  |   6 +-
 tests/.gitignore   |   1 +
 tests/Makefile.include |   4 +
 tests/test-sockets-proto.c | 855 +
 util/qemu-sockets.c|  71 +++-
 5 files changed, 916 insertions(+), 21 deletions(-)
 create mode 100644 tests/test-sockets-proto.c

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/5] sockets: ensure we can bind to both ipv4 & ipv6 separately

[Daniel P. Berrange]

When binding to an IPv6 socket we currently force the
IPV6_V6ONLY flag to off. This means that the IPv6 socket
will accept both IPv4 & IPv6 sockets when QEMU is launched
with something like

  -vnc :::1

While this is good for that case, it is bad for other
cases. For example if an empty hostname is given,
getaddrinfo resolves it to 2 addresses 0.0.0.0 and ::,
in that order. We will thus bind to 0.0.0.0 first, and
then fail to bind to :: on the same port. The same
problem can happen if any other hostname lookup causes
the IPv4 address to be reported before the IPv6 address.

When we get an IPv6 bind failure, we should re-try the
same port, but with IPV6_V6ONLY turned on again, to
avoid clash with any IPv4 listener.

This ensures that

  -vnc :1

will bind successfully to both 0.0.0.0 and ::, and also
avoid

  -vnc :1,to=2

from mistakenly using a 2nd port for the :: listener.

Signed-off-by: Daniel P. Berrange 
---
 util/qemu-sockets.c | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..397212b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -208,22 +208,37 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 }
 
 socket_set_fast_reuse(slisten);
-#ifdef IPV6_V6ONLY
-if (e->ai_family == PF_INET6) {
-/* listen on both ipv4 and ipv6 */
-const int off = 0;
-qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, ,
-sizeof(off));
-}
-#endif
 
 port_min = inet_getport(e);
 port_max = saddr->has_to ? saddr->to + port_offset : port_min;
 for (p = port_min; p <= port_max; p++) {
+#ifdef IPV6_V6ONLY
+/* listen on both ipv4 and ipv6 */
+int v6only = 0;
+#endif
 inet_setport(e, p);
+#ifdef IPV6_V6ONLY
+rebind:
+if (e->ai_family == PF_INET6) {
+qemu_setsockopt(slisten, IPPROTO_IPV6, IPV6_V6ONLY, ,
+sizeof(v6only));
+}
+#endif
 if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
 goto listen;
 }
+
+#ifdef IPV6_V6ONLY
+/* If we got EADDRINUSE from an IPv6 bind & V6ONLY is unset,
+ * it could be that the IPv4 port is already claimed, so retry
+ * with V6ONLY set
+ */
+if (e->ai_family == PF_INET6 && errno == EADDRINUSE && !v6only) {
+v6only = 1;
+goto rebind;
+}
+#endif
+
 if (p == port_max) {
 if (!e->ai_next) {
 error_setg_errno(errp, errno, "Failed to bind socket");
-- 
2.9.3

Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration

[Halil Pasic]

On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote:
>> To sum it up although I'm currently leaning towards abandoning my idea
>> of two sections for two devices, I'm not comfortable with making the
>> call myself. I'm hoping for some maintainer guidance (s390x, virtio
>> and migration). 
>  dhildenb etc>
> 
> OK, so I think:
>   a) First split the series into two separate series; one that
> VMStatifies the existing stuff without breaking compatibility;
> and one that adds the new stuff.  Lets get the first of those
> going in while we think about the second.
> 
> a.1) I'd do this with patches that convert one chunk into
>  vmstate and remove the corresponding C code at the same time.
> 
>   b) While the way PCI devices are done is weird, I think it'll
> be a lot simpler if you can stick to a structure that's similar
> to them while diverging.  It's hard enough for those of us
> who don't do Virtio every day to get our minds around virtio-pci
> without it being different for virtio-ccw/css.
> 
>   c) To do (b) I suggest:
>   c.1) you *don't* add a vmsd to your virtio_ccw_device
> 
>   c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any
>  non-virtio devices you migrate (like each of the PCI devices
>   have)
> 
>   c.3) You can add extra state for CSS to the ->save_extra_state
>handle on virtio devices or to the config
> 
>   d) vmstatifying the config is OK as well.
> 
> I should say I'm no virtio expert, so if any of that's truly
> mad say so.
> 
> Dave
> 

Agreed (a,b,c,d). Reorganizing my patch set according to a) is
going to require some effort, but it should not be too bad. 
About c.2): I don't think we have any migratable non virtio devices
yet, but I'll keep it in mind.

I do not understand what you mean by c.3) and extra_sate. Maybe
it will settle with time. My idea of extending VirtioCcwDevice
is just adding subsections to it's VMStateDescription. The
call vmstate_save_state(f, _virtio_ccw_dev, dev, NULL)
in save_config should take care of compatibility. Maybe some
staring at virtio-pci is going to help, but right now I can't tell
what's the extra_state for, and how/why is it 'extra'.

Thanks for your patience!

Regards,
Halil

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> >> We could also consider making WITH_TMP act as a normal field. 
> >> Working on the whole state looks like a bit like a corner case:
> >> we have some stuff adjacent in the migration stream, and we have
> >> to map it on multiple fields (and vice-versa). Getting the whole
> >> state with a pointer to a certain field could work via container_of.
> > You do need to know which field you're working on to be able to safely
> > use container_of, so I'm not sure how it would work for you in this
> > case.
> 
> 
> Well, if you have to write to just one field you are good because you
> already have a pointer to that field (.offset was added).
> 
> If you need to write to multiple fields in post_load then you just pick
> one of the fields you are going to write to (probably the first) and use
> container_of to gain access to the whole state. The logic is specific to
> the bunch of the fields you are going to touch anyway.
> 
> In fact any member of the state struct will do it's only important that
> you use the same when creating the VMStateField and when trying to get a
> pointer to the parent in pre_save and post_load.
> 
> I haven't tried, so I'm not 100% sure, but if you like I can try, and send
> you a patch if it's viable. 
> 
> I think the key to a good solution is really what is intended and typical
> usage, and what is corner case. My patch in the other reply shows that we
> can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
> what I'm trying to do here a bit prettier at the expense of making what
> you are doing in virtio-net a bit uglier, but whether it's a good idea to
> do so, I cant tell.

Lets go with what you put in the other patch (I replied to it); I hadn't
realised that was possible (hence my comment below).
Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code
base, I'll step back and see how to tidy them up.

Dave

> > 
> > The other thought I'd had was that perhaps we could change the temporary
> > structure in VMSTATE_WITH_TMP to:
> > 
> >   struct foo {
> >  struct whatever **parent;
> > 
> > so now you could write to *parent in cases like these.
> >
> 
> Sorry, I do not get your idea. If you have some WIP patch in this
> direction I would be happy to provide some feedback.
> 
>  
> >> Btw, I would rather call it get_indicator a factory method or even a
> >> constructor than an allocator, but I think we understand each-other
> >> anyway.
> > Yes; I'm not too worried about the actual name as long as it's short
> > and obvious.
> > 
> > I'd thought of 'allocator' since in most cases it's used where the
> > load-time code allocates memory for the object being loaded.
> > A constructor is normally something I think of as happening after
> > allocation; and a factory, hmm maybe.  However, if it does the right
> > thing I wouldn't object to any of those names.
> > 
> 
> I think we are on the same page.
> 
> Cheers,
> Halil
> 
> > Dave
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
>  On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >> As a preparation for switching to a vmstate based migration let us
> >> introduce vmstate entities (e.g. VMStateDescription) for the css 
> >> entities
> >> to be migrated. Alongside some comments explaining or indicating the 
> >> not
> >> migration of certain members are introduced too.
> >>
> >> No changes in behavior, we just added some dead code -- which should
> >> rise to life soon.
> >>
> >> Signed-off-by: Halil Pasic 
> >> ---
> >>  hw/s390x/css.c | 276 
> >> +
> >>  include/hw/s390x/css.h |  10 +-
> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index c03bb20..2bda7d0 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -20,29 +20,231 @@
> >>  #include "hw/s390x/css.h"
> >>  #include "trace.h"
> >>  #include "hw/s390x/s390_flic.h"
> >> +#include "hw/s390x/s390-virtio-ccw.h"
> >>  
> 
>  [..]
> 
> >> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> +VMStateField *field)
> >> +{
> >> +int32_t len;
> >> +IndAddr **ind_addr = pv;
> >> +
> >> +len = qemu_get_be32(f);
> >> +if (len != 0) {
> >> +*ind_addr = get_indicator(qemu_get_be64(f), len);
> >> +} else {
> >> +qemu_get_be64(f);
> >> +*ind_addr = NULL;
> >> +}
> >> +return 0;
> >> +}
> >> +
> >> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> +VMStateField *field, QJSON *vmdesc)
> >> +{
> >> +IndAddr *ind_addr = *(IndAddr **) pv;
> >> +
> >> +if (ind_addr != NULL) {
> >> +qemu_put_be32(f, ind_addr->len);
> >> +qemu_put_be64(f, ind_addr->addr);
> >> +} else {
> >> +qemu_put_be32(f, 0);
> >> +qemu_put_be64(f, 0UL);
> >> +}
> >> +return 0;
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_ind_addr = {
> >> +.name = "s390_ind_addr",
> >> +.get = css_get_ind_addr,
> >> +.put = css_put_ind_addr
> >> +};
> >
> > You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> > declare a temporary struct something like:
> >   struct tmp_ind_addr {
> >  IndAddr *parent;
> >  uint32_t  len;
> >  uint64_t  addr;
> >   }
> >
> > and then your .get/.put routines turn into pre_save/post_load
> > routines to just setup the len/addr.
> >
> 
>  I don't think this is going to work -- unfortunately! You can see below,
>  how this IndAddr* migration stuff is supposed to be used:
>  the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
>  field when describing state which needs and IndAddr* migrated.
> 
>  The problem is, we do not know in what state will this field
>  be embedded, the pre_save/post_load called by put_tmp/get_tmp
>  is however copying the pointer to this state into the parent.
>  So instead of having a pointer to IndAddr* in those functions
>  and updating it accordingly, I would have to find the IndAddr*
>  in some arbitrary state (in our case VirtioCcwDevice) first,
>  and I lack information for that.
> 
>  If it's hard to follow I can give you the patch I was debugging
>  to come to this conclusion. (By the way I ended up with 10
>  lines of code more than in this version, and although I think
>  it looks nicer, it's simpler only if one knows how WITH_TMP
>  works. My plan was to ask you which version do you like more
>  and go with that before I realized it ain't gonna work.)
> 
> >>>
> >>> Yes, I see - I've got some similar other cases; the challenge
> >>> is it's a custom allocator - 'get_indicator' - and it's used
> >>> as fields in a few places.  Hmm.
> >>>
> >>>
> >>
> >> The problem can be worked around by wrapping the WITH_TMP into a another
> >> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
> >> quite some boilerplate (+16 lines). Should I post the patch here?
> > 
> > Yes please.
> > 
> 8<--
> 
> From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001
> From: Halil Pasic 
> Date: Tue, 9 May 2017 

Re: [Qemu-devel] [PATCH v12 2/2] migration: spapr: migrate pending_events of spapr state

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-05-19 09:27:50)
> From: Jianjun Duan 
> 
> In racing situations between hotplug events and migration operation,
> a rtas hotplug event could have not yet be delivered to the source
> guest when migration is started. In this case the pending_events of
> spapr state need be transmitted to the target so that the hotplug
> event can be finished on the target.
> 
> All the different fields of the events are encoded as defined by
> PAPR. We can migrate them as a binary stream inside VBUFFER without
> any concerns about data padding or endianess.
> 
> pending_events is put in a subsection in the spapr state VMSD to make
> sure migration across different versions is not broken.
> 
> Signed-off-by: Jianjun Duan 
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Michael Roth 

> ---
>  hw/ppc/spapr.c | 32 
>  hw/ppc/spapr_events.c  | 12 
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..5afd328 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1444,6 +1444,37 @@ static bool version_before_3(void *opaque, int 
> version_id)
>  return version_id < 3;
>  }
> 
> +static bool spapr_pending_events_needed(void *opaque)
> +{
> +sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +return !QTAILQ_EMPTY(>pending_events);
> +}
> +
> +static const VMStateDescription vmstate_spapr_event_entry = {
> +.name = "spapr_event_log_entry",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_INT32(log_type, sPAPREventLogEntry),
> +VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +VMSTATE_VBUFFER_ALLOC_UINT32(data, sPAPREventLogEntry, 0,
> + NULL, data_size),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_events = {
> +.name = "spapr_pending_events",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_pending_events_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
> + vmstate_spapr_event_entry, sPAPREventLogEntry, 
> next),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>  sPAPRMachineState *spapr = opaque;
> @@ -1542,6 +1573,7 @@ static const VMStateDescription vmstate_spapr = {
>  .subsections = (const VMStateDescription*[]) {
>  _spapr_ov5_cas,
>  _spapr_patb_entry,
> +_spapr_pending_events,
>  NULL
>  }
>  };
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 73e2a18..a509c46 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -350,6 +350,18 @@ static void rtas_event_log_queue(int log_type, void 
> *data)
>  g_assert(data);
>  entry->log_type = log_type;
>  entry->data = data;
> +
> +switch (log_type) {
> +case RTAS_LOG_TYPE_EPOW:
> +entry->data_size = sizeof(struct epow_log_full);
> +break;
> +case RTAS_LOG_TYPE_HOTPLUG:
> +entry->data_size = sizeof(struct hp_log_full);
> +break;
> +default:
> +g_assert(false);
> +}
> +
>  QTAILQ_INSERT_TAIL(>pending_events, entry, next);
>  }
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 02239a5..0554e11 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -597,8 +597,9 @@ struct sPAPRTCETable {
>  sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
> 
>  struct sPAPREventLogEntry {
> -int log_type;
> +int32_t log_type;
>  void *data;
> +uint32_t data_size;
>  QTAILQ_ENTRY(sPAPREventLogEntry) next;
>  };
> 
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
>  On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
> [..]
> 
>  Why not use virtio oddities? Because they are oddities. I have
>  figured, it's a good idea to separate the migration of the 
>  proxy form the rest: we have two QEMU Device objects and it
>  should be good practice, that these are migrating themselves via
>  DeviceClass.vmsd. That's what I get with this patch set, 
>  for new machine versions (since we can not fix the past), and
>  with the notable difference of config_vector, because it is
>  defined as a common infrastructure (struct VirtIODevice) but
>  ain't migrated as a common virtio infrastructure.
> >>>
> >>> Have you got a bit of a description of your classes/structure - it's
> >>> a little hard to get my head around.
> >>>
> >>
> >> Unfortunately I do not have any extra description besides the comments
> >> and the commit messages. What exactly do you mean  by 'my
> >> classes/structure'?  I would like to provide some helpful developer
> >> documentation on how migration works for s390x. There were voices on the
> >> internal mailing list too requesting something like that, but I find it
> >> hard, because for me, the most challenging part was understanding how
> >> qemu migration works in general and the virtio oddities come next. 
> > 
> > Yes, there are only about 2 people who have the overlap of understanding
> > migration AND s390 IO.
> > 
> >> Fore example, I still don't understand why is is (virtio) load_config
> >> called like that, when what it mainly does is loading state of the proxy
> >> which is basically the reification of the device side of the virtio spec
> >> calls the transport within QOM. (I say mainly, because of this
> >> config_vector which resides in core but is migrated by via a callback for
> >> some strange reason I do not understand).
> > 
> > I think the idea is that virtio_load is trying to act as a generic
> > save/load with a number of virtual components that are specialised for:
> >   a) The device (e.g. rng, serial, gpu, net, blk)
> >   b) The transport (PCI, MMIO, CCW etc)
> >   c) The virtio queue content
> >   d) But has a load of core stuff (features, the virtio ring management)
> > 
> > (a) & (b) are very much virtual-function like that doesn't fit that
> > well with the migration macro structure.
> > 
> > The split between (a) & (c) isn't necessary clean - gpu does it a
> > different way.
> > And the order of a/b/c/d is very random (aka wrong).
> > 
> 
> I mostly agree with your analysis. Honestly I have forgot abut this
> load_queue callback (I think its c)), but it's a strange one too. What it
> does is handling the vector of the queue which is again common
> infrastructure in a sense that it reside within VirtIODevice, but it may
> need some proxy specific handling.
> 
> In my understanding the virtio migration and the migration subsystem
> (lets call it vmstate) are a misfit in the following aspect. Most
> importantly it separation of concerns. In my understanding, for vmstate,
> each device is supposed to load/save itself, and loading state and doing
> stuff with the state we have loaded are separate concerns. I'm not sure
> whats the vmstate place for code which is supposed to run as a part of
> the migration logic, but requires cooperation of devices (e.g. notify in
> virtio_load which basically generates an interrupt). 
> 
> 
> >> Could tell me to which (specific) questions should I provide an answer?
> >> It would make my job much easier.
> >>
> >> About the general approach. First step was to provide VMStateDescription
> >> for the entities which have migration relevant state but no
> >> VMStateDescription (patches 3, 4 and 5).  This is done so that
> >> lots of qemu_put/qem_get calls can be replaced with few
> >> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand,
> >> and that state not migrated yet but needed is also included, if the
> >> compat. switch (property) added in patch 2 is on. Then in patch 8, I add
> >> ORB which is a state we wanted to add for some time now, but we needed
> >> vmstate to add it without breaking migration. So we waited.
> > 
> > I'm most interested at this point in understanding which bits aren't
> > changing behaviour - if we've got stuff that's just converting qemu_get
> > to vmstate then lets go for it, no problem; easy to check.
> 
> The commit messages should be helpful. Up to patch 8 all I do is
> converting qemu_get to vmstate as you said. 
> 
> > I'm just trying to make sure I understand the bit where you're
> > converting from being a virtio device.
> > 
> 
> By converting from being a virtio device you mean factoring out the
> 

Re: [Qemu-devel] [PATCH v4 10/10] vhost: iommu: cache static mapping if there is

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 11:19:49AM +0800, Peter Xu wrote:
> This patch pre-heat vhost iotlb cache when passthrough mode enabled.
> 
> Sometimes, even if user specified iommu_platform for vhost devices,
> IOMMU might still be disabled. One case is passthrough mode in VT-d
> implementation. We can detect this by observing iommu_list. If it's
> empty, it means IOMMU translation is disabled, then we can actually
> pre-heat the translation (it'll be static mapping then) by first
> invalidating all IOTLB, then cache existing memory ranges into vhost
> backend iotlb using 1:1 mapping.
> 
> Signed-off-by: Peter Xu 

I don't really understand. Is this a performance optimization?
Can you post some #s please?

Also, if it's PT, can't we bypass iommu altogether? That would be
even faster ...

> ---
>  hw/virtio/trace-events |  4 
>  hw/virtio/vhost.c  | 49 +
>  2 files changed, 53 insertions(+)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 1f7a7c1..54dcbb3 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t 
> gpa) "section name: %s g
>  virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: 
> %d actual: %d"
>  virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d 
> oldactual: %d"
>  virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon 
> target: %"PRIx64" num_pages: %d"
> +
> +# hw/virtio/vhost.c
> +vhost_iommu_commit(void) ""
> +vhost_iommu_static_preheat(void) ""
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 03a46a7..8069135 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -27,6 +27,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "migration/blocker.h"
>  #include "sysemu/dma.h"
> +#include "trace.h"
>  
>  /* enabled until disconnected backend stabilizes */
>  #define _VHOST_DEBUG 1
> @@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, 
> IOMMUTLBEntry *iotlb)
>  }
>  }
>  
> +static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
> +{
> +return !QLIST_EMPTY(>iommu_list);
> +}
> +
>  static void vhost_iommu_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
>  {
> @@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener 
> *listener,
>  }
>  }
>  
> +static void vhost_iommu_commit(MemoryListener *listener)
> +{
> +struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> + iommu_listener);
> +struct vhost_memory_region *r;
> +int i;
> +
> +trace_vhost_iommu_commit();
> +
> +if (!vhost_iommu_mr_enabled(dev)) {
> +/*
> +* This means iommu_platform is enabled, however iommu memory
> +* region is disabled, e.g., when device passthrough is setup.
> +* Then, no translation is needed any more.
> +*
> +* Let's first invalidate the whole IOTLB, then pre-heat the
> +* static mapping by looping over vhost memory ranges.
> +*/
> +
> +if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
> +  UINT64_MAX)) {
> +error_report("%s: flush existing IOTLB failed", __func__);
> +return;
> +}
> +
> +for (i = 0; i < dev->mem->nregions; i++) {
> +r = >mem->regions[i];
> +/* Vhost regions are writable RAM, so IOMMU_RW suites. */
> +if (dev->vhost_ops->vhost_update_device_iotlb(dev,
> +  r->guest_phys_addr,
> +  r->userspace_addr,
> +  r->memory_size,
> +  IOMMU_RW)) {
> +error_report("%s: pre-heat static mapping failed", __func__);
> +return;
> +}
> +}
> +
> +trace_vhost_iommu_static_preheat();
> +}
> +}
> +
>  static void vhost_region_nop(MemoryListener *listener,
>   MemoryRegionSection *section)
>  {
> @@ -1298,6 +1346,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>  hdev->iommu_listener = (MemoryListener) {
>  .region_add = vhost_iommu_region_add,
>  .region_del = vhost_iommu_region_del,
> +.commit = vhost_iommu_commit,
>  };
>  
>  if (hdev->migration_blocker == NULL) {
> -- 
> 2.7.4

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [PATCH v2 00/16] Vhost-pci for inter-VM communication

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 11:10:33AM +0800, Jason Wang wrote:
> 
> 
> On 2017年05月18日 11:03, Wei Wang wrote:
> > On 05/17/2017 02:22 PM, Jason Wang wrote:
> > > 
> > > 
> > > On 2017年05月17日 14:16, Jason Wang wrote:
> > > > 
> > > > 
> > > > On 2017年05月16日 15:12, Wei Wang wrote:
> > > > > > > 
> > > > > > 
> > > > > > Hi:
> > > > > > 
> > > > > > Care to post the driver codes too?
> > > > > > 
> > > > > OK. It may take some time to clean up the driver code before
> > > > > post it out. You can first
> > > > > have a check of the draft at the repo here:
> > > > > https://github.com/wei-w-wang/vhost-pci-driver
> > > > > 
> > > > > Best,
> > > > > Wei
> > > > 
> > > > Interesting, looks like there's one copy on tx side. We used to
> > > > have zerocopy support for tun for VM2VM traffic. Could you
> > > > please try to compare it with your vhost-pci-net by:
> > > > 
> > We can analyze from the whole data path - from VM1's network stack to
> > send packets -> VM2's
> > network stack to receive packets. The number of copies are actually the
> > same for both.
> 
> That's why I'm asking you to compare the performance. The only reason for
> vhost-pci is performance. You should prove it.
> 
> > 
> > vhost-pci: 1-copy happen in VM1's driver xmit(), which copes packets
> > from its network stack to VM2's
> > RX ring buffer. (we call it "zerocopy" because there is no intermediate
> > copy between VMs)
> > zerocopy enabled vhost-net: 1-copy happen in tun's recvmsg, which copies
> > packets from VM1's TX ring
> > buffer to VM2's RX ring buffer.
> 
> Actually, there's a major difference here. You do copy in guest which
> consumes time slice of vcpu thread on host. Vhost_net do this in its own
> thread. So I feel vhost_net is even faster here, maybe I was wrong.

Yes but only if you have enough CPUs. The point of vhost-pci
is to put the switch in a VM and scale better with # of VMs.

> > 
> > That being said, we compared to vhost-user, instead of vhost_net,
> > because vhost-user is the one
> > that is used in NFV, which we think is a major use case for vhost-pci.
> 
> If this is true, why not draft a pmd driver instead of a kernel one? And do
> you use virtio-net kernel driver to compare the performance? If yes, has OVS
> dpdk optimized for kernel driver (I think not)?
> 
> What's more important, if vhost-pci is faster, I think its kernel driver
> should be also faster than virtio-net, no?

If you have a vhost CPU per VCPU and can give a host CPU to each using
that will be faster.  But not everyone has so many host CPUs.


> > 
> > 
> > > > - make sure zerocopy is enabled for vhost_net
> > > > - comment skb_orphan_frags() in tun_net_xmit()
> > > > 
> > > > Thanks
> > > > 
> > > 
> > > You can even enable tx batching for tun by ethtool -C tap0 rx-frames
> > > N. This will greatly improve the performance according to my test.
> > > 
> > 
> > Thanks, but would this hurt latency?
> > 
> > Best,
> > Wei
> 
> I don't see this in my test.
> 
> Thanks

Re: [Qemu-devel] [PATCH 6/6] spec/vhost-user spec: Add IOMMU support

[Michael S. Tsirkin]

On Fri, May 19, 2017 at 03:46:36PM +0800, Jason Wang wrote:
> 
> 
> On 2017年05月18日 16:43, Maxime Coquelin wrote:
> > > > 
> > > > +When the VHOST_USER_PROTOCOL_F_SLAVE_REQ is supported by the
> > > > slave, and the
> > > > +master initiated the slave to master communication channel using the
> > > > +VHOST_USER_SET_SLAVE_REQ_FD request, the slave can send IOTLB
> > > > miss and access
> > > > +failure events by sending VHOST_USER_SLAVE_IOTLB_MSG requests
> > > > to the master
> > > > +with a struct vhost_iotlb_msg payload. For miss events, the
> > > > iotlb payload has
> > > > +to be filled with the miss message type (1), the I/O virtual
> > > > address and the
> > > > +permissions flags. For access failure event, the iotlb payload
> > > > has to be
> > > > +filled with the access failure message type (4), the I/O
> > > > virtual address and
> > > > +the permissions flags.
> > > 
> > > I don't think slave should cache invalid entries. If it does not,
> > > how can it detect access failure as opposed to a miss?
> > 
> > Of course, invalid cache entries should not be cached.
> > The VHOST_IOTLB_ACCESS_FAIL has been specified for the Kernel backend,
> > even if the latter does not implement it yet.
> 
> Yes, I leave this for future use e.g reporting copy_to_user() failure to
> userspace.
> 
> Thanks

Interesting. And it's not handled now.
So let's add a text "reserved for reporting internal access
errors in the future. Should not be used for now.".

-- 
MST

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Halil Pasic]

On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
>> We could also consider making WITH_TMP act as a normal field. 
>> Working on the whole state looks like a bit like a corner case:
>> we have some stuff adjacent in the migration stream, and we have
>> to map it on multiple fields (and vice-versa). Getting the whole
>> state with a pointer to a certain field could work via container_of.
> You do need to know which field you're working on to be able to safely
> use container_of, so I'm not sure how it would work for you in this
> case.


Well, if you have to write to just one field you are good because you
already have a pointer to that field (.offset was added).

If you need to write to multiple fields in post_load then you just pick
one of the fields you are going to write to (probably the first) and use
container_of to gain access to the whole state. The logic is specific to
the bunch of the fields you are going to touch anyway.

In fact any member of the state struct will do it's only important that
you use the same when creating the VMStateField and when trying to get a
pointer to the parent in pre_save and post_load.

I haven't tried, so I'm not 100% sure, but if you like I can try, and send
you a patch if it's viable. 

I think the key to a good solution is really what is intended and typical
usage, and what is corner case. My patch in the other reply shows that we
can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
what I'm trying to do here a bit prettier at the expense of making what
you are doing in virtio-net a bit uglier, but whether it's a good idea to
do so, I cant tell.

> 
> The other thought I'd had was that perhaps we could change the temporary
> structure in VMSTATE_WITH_TMP to:
> 
>   struct foo {
>  struct whatever **parent;
> 
> so now you could write to *parent in cases like these.
>

Sorry, I do not get your idea. If you have some WIP patch in this
direction I would be happy to provide some feedback.

 
>> Btw, I would rather call it get_indicator a factory method or even a
>> constructor than an allocator, but I think we understand each-other
>> anyway.
> Yes; I'm not too worried about the actual name as long as it's short
> and obvious.
> 
> I'd thought of 'allocator' since in most cases it's used where the
> load-time code allocates memory for the object being loaded.
> A constructor is normally something I think of as happening after
> allocation; and a factory, hmm maybe.  However, if it does the right
> thing I wouldn't object to any of those names.
> 

I think we are on the same page.

Cheers,
Halil

> Dave

[Qemu-devel] [PULL] Update OpenBIOS images

[Mark Cave-Ayland]

Hi Stefan,

This update contains the OpenBIOS VGA driver updates required to enable Ben's 
QemuMacDrivers
for Mac guests. Please pull.


ATB,

Mark.


The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:

  Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging 
(2017-05-18 13:36:15 +0100)

are available in the git repository at:


  https://github.com/mcayland/qemu.git tags/qemu-openbios-signed

for you to fetch changes up to 415c3824836d3b65a5796a81b17fccd1ad575ff8:

  Update OpenBIOS images to 3ebaaa2 built from submodule. (2017-05-19 16:52:40 
+0100)


Update OpenBIOS images


Mark Cave-Ayland (1):
  Update OpenBIOS images to 3ebaaa2 built from submodule.

 pc-bios/openbios-ppc |  Bin 750840 -> 750840 bytes
 pc-bios/openbios-sparc32 |  Bin 382048 -> 382048 bytes
 pc-bios/openbios-sparc64 |  Bin 1593408 -> 1593408 bytes
 roms/openbios|2 +-
 4 files changed, 1 insertion(+), 1 deletion(-)

Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19

[Paolo Bonzini]

On 19/05/2017 17:51, Stefan Hajnoczi wrote:
>> This series seems to have some coding style problems. See output below for
>> more information:
> Yikes, on second thought I've dropped the pull request for now.
> 
> Please look at these coding style violations.

These are just a sample program, so I didn't really care much.  But
these three aren't:

Checking PATCH 17/20: vhost-user-scsi: Introduce vhost-user-scsi host
device...
ERROR: do not use C99 // comments
#216: FILE: hw/scsi/vhost-user-scsi.c:145:
+// Turn on predefined features supported by this device

ERROR: do not use C99 // comments
#261: FILE: hw/scsi/vhost-user-scsi.c:190:
+// Add the bootindex property for this object

ERROR: do not use C99 // comments
#265: FILE: hw/scsi/vhost-user-scsi.c:194:
+// Set boot index according the the device config

total: 3 errors, 0 warnings, 382 lines checked

so I guess I'll fix the sample program too.

Paolo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Halil Pasic]

On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:


 On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>> As a preparation for switching to a vmstate based migration let us
>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
>> to be migrated. Alongside some comments explaining or indicating the not
>> migration of certain members are introduced too.
>>
>> No changes in behavior, we just added some dead code -- which should
>> rise to life soon.
>>
>> Signed-off-by: Halil Pasic 
>> ---
>>  hw/s390x/css.c | 276 
>> +
>>  include/hw/s390x/css.h |  10 +-
>>  2 files changed, 285 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index c03bb20..2bda7d0 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -20,29 +20,231 @@
>>  #include "hw/s390x/css.h"
>>  #include "trace.h"
>>  #include "hw/s390x/s390_flic.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>>  

 [..]

>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
>> +VMStateField *field)
>> +{
>> +int32_t len;
>> +IndAddr **ind_addr = pv;
>> +
>> +len = qemu_get_be32(f);
>> +if (len != 0) {
>> +*ind_addr = get_indicator(qemu_get_be64(f), len);
>> +} else {
>> +qemu_get_be64(f);
>> +*ind_addr = NULL;
>> +}
>> +return 0;
>> +}
>> +
>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
>> +VMStateField *field, QJSON *vmdesc)
>> +{
>> +IndAddr *ind_addr = *(IndAddr **) pv;
>> +
>> +if (ind_addr != NULL) {
>> +qemu_put_be32(f, ind_addr->len);
>> +qemu_put_be64(f, ind_addr->addr);
>> +} else {
>> +qemu_put_be32(f, 0);
>> +qemu_put_be64(f, 0UL);
>> +}
>> +return 0;
>> +}
>> +
>> +const VMStateInfo vmstate_info_ind_addr = {
>> +.name = "s390_ind_addr",
>> +.get = css_get_ind_addr,
>> +.put = css_put_ind_addr
>> +};
>
> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> declare a temporary struct something like:
>   struct tmp_ind_addr {
>  IndAddr *parent;
>  uint32_t  len;
>  uint64_t  addr;
>   }
>
> and then your .get/.put routines turn into pre_save/post_load
> routines to just setup the len/addr.
>

 I don't think this is going to work -- unfortunately! You can see below,
 how this IndAddr* migration stuff is supposed to be used:
 the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
 field when describing state which needs and IndAddr* migrated.

 The problem is, we do not know in what state will this field
 be embedded, the pre_save/post_load called by put_tmp/get_tmp
 is however copying the pointer to this state into the parent.
 So instead of having a pointer to IndAddr* in those functions
 and updating it accordingly, I would have to find the IndAddr*
 in some arbitrary state (in our case VirtioCcwDevice) first,
 and I lack information for that.

 If it's hard to follow I can give you the patch I was debugging
 to come to this conclusion. (By the way I ended up with 10
 lines of code more than in this version, and although I think
 it looks nicer, it's simpler only if one knows how WITH_TMP
 works. My plan was to ask you which version do you like more
 and go with that before I realized it ain't gonna work.)

>>>
>>> Yes, I see - I've got some similar other cases; the challenge
>>> is it's a custom allocator - 'get_indicator' - and it's used
>>> as fields in a few places.  Hmm.
>>>
>>>
>>
>> The problem can be worked around by wrapping the WITH_TMP into a another
>> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
>> quite some boilerplate (+16 lines). Should I post the patch here?
> 
> Yes please.
> 
8<--

>From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001
From: Halil Pasic 
Date: Tue, 9 May 2017 12:06:42 +0200
Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP

Convert s VMSatateInfo based solution manipulating the migration stream
directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
separate.

Signed-off-by: Halil Pasic 
---
 hw/s390x/css.c | 56 

Re: [Qemu-devel] [PULL 0/3] audio patch queue.

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 01:24:12PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> Smallish audio patch queue, renaming moving soundhw init code.
> 
> please pull,
>   Gerd
> 
> The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:
> 
>   Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging 
> (2017-05-18 13:36:15 +0100)
> 
> are available in the git repository at:
> 
>   git://git.kraxel.org/qemu tags/pull-audio-20170519-1
> 
> for you to fetch changes up to 8a824e4d74213a2da39323304f949c5b4243e1fb:
> 
>   audio: Rename hw/audio/audio.h to hw/audio/soundhw.h (2017-05-19 10:48:54 
> +0200)
> 
> 
> audio: move & rename soundhw init code.
> 
> 
> Eduardo Habkost (3):
>   audio: Move arch_init audio code to hw/audio/soundhw.c
>   audio: Rename audio_init() to soundhw_init()
>   audio: Rename hw/audio/audio.h to hw/audio/soundhw.h
> 
>  include/hw/audio/{audio.h => soundhw.h} |   3 +
>  include/sysemu/arch_init.h  |   2 -
>  arch_init.c | 126 +-
>  hw/audio/ac97.c |   2 +-
>  hw/audio/adlib.c|   2 +-
>  hw/audio/cs4231a.c  |   2 +-
>  hw/audio/es1370.c   |   2 +-
>  hw/audio/gus.c  |   2 +-
>  hw/audio/intel-hda.c|   2 +-
>  hw/audio/pcspk.c|   2 +-
>  hw/audio/sb16.c |   2 +-
>  hw/audio/soundhw.c  | 156 
> 
>  hw/ppc/prep.c   |   3 +-
>  vl.c|   3 +-
>  hw/audio/Makefile.objs  |   2 +
>  15 files changed, 174 insertions(+), 137 deletions(-)
>  rename include/hw/audio/{audio.h => soundhw.h} (81%)
>  create mode 100644 hw/audio/soundhw.c
> 

Coding style violations are pre-existing.  That's fine.

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 05:41:28AM -0700, no-re...@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:

Yikes, on second thought I've dropped the pull request for now.

Please look at these coding style violations.

Thanks,
Stefan

> 
> Message-id: 1495192872-27667-1-git-send-email-pbonz...@redhat.com
> Type: series
> Subject: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 6fce4cd target/i386: use multiple CPU AddressSpaces
> a04ba9d target/i386: enable A20 automatically in system management mode
> 87c38d5 vhost-user-scsi: Introduce a vhost-user-scsi sample application
> a63728e vhost-user-scsi: Introduce vhost-user-scsi host device
> bda4194 virtio-scsi: Unset hotplug handler when unrealize
> ca14443 exec: simplify phys_page_find() params
> 7eee4fd nbd/client.c: use errp instead of LOG
> 388beda nbd: add errp to read_sync, write_sync and drop_sync
> 0032273 nbd: add errp parameter to nbd_wr_syncv()
> bdf25c9 nbd: read_sync and friends: return 0 on success
> b61d7d1 nbd: strict nbd_wr_syncv
> cc100d3 Check the return value of fcntl in qemu_set_cloexec
> 94297c6 kvm: irqchip: skip update msi when disabled
> f8f04f1 msix: trace control bit write op
> 11bfe30 kvm: irqchip: trace changes on msi add/remove
> 192c432 mc146818rtc: embrace all x86 specific code
> 6e1b003 mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
> cb9a45b mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on 
> TARGET_I386
> 98a508b mc146818rtc: precisely count the clock for periodic timer
> b9744f3 mc146818rtc: update periodic timer only if it is needed
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/20: mc146818rtc: update periodic timer only if it is 
> needed...
> Checking PATCH 2/20: mc146818rtc: precisely count the clock for periodic 
> timer...
> ERROR: braces {} are necessary for all arms of this statement
> #129: FILE: hw/timer/mc146818rtc.c:216:
> +if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> [...]
> +} else
> [...]
> 
> total: 1 errors, 0 warnings, 181 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/20: mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only 
> enabled on TARGET_I386...
> Checking PATCH 4/20: mc146818rtc: drop unnecessary '#ifdef TARGET_I386'...
> Checking PATCH 5/20: mc146818rtc: embrace all x86 specific code...
> Checking PATCH 6/20: kvm: irqchip: trace changes on msi add/remove...
> Checking PATCH 7/20: msix: trace control bit write op...
> Checking PATCH 8/20: kvm: irqchip: skip update msi when disabled...
> Checking PATCH 9/20: Check the return value of fcntl in qemu_set_cloexec...
> Checking PATCH 10/20: nbd: strict nbd_wr_syncv...
> Checking PATCH 11/20: nbd: read_sync and friends: return 0 on success...
> Checking PATCH 12/20: nbd: add errp parameter to nbd_wr_syncv()...
> Checking PATCH 13/20: nbd: add errp to read_sync, write_sync and drop_sync...
> Checking PATCH 14/20: nbd/client.c: use errp instead of LOG...
> ERROR: code indent should never use tabs
> #126: FILE: nbd/client.c:729:
> +^I Error **errp)$
> 
> total: 1 errors, 0 warnings, 146 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 15/20: exec: simplify phys_page_find() params...
> Checking PATCH 16/20: virtio-scsi: Unset hotplug handler when unrealize...
> Checking PATCH 17/20: vhost-user-scsi: Introduce vhost-user-scsi host 
> device...
> ERROR: do not use C99 // comments
> #216: FILE: hw/scsi/vhost-user-scsi.c:145:
> +// Turn on predefined features supported by this device
> 
> ERROR: do not use C99 // comments
> #261: FILE: hw/scsi/vhost-user-scsi.c:190:
> +// Add the bootindex property for this object
> 
> ERROR: do not use C99 // comments
> #265: FILE: hw/scsi/vhost-user-scsi.c:194:
> +// Set boot index according the the device config
> 
> total: 3 errors, 0 warnings, 382 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 18/20: vhost-user-scsi: Introduce 

Re: [Qemu-devel] [PULL 00/20] Misc patches for 2017-05-19

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 01:20:52PM +0200, Paolo Bonzini wrote:
> The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:
> 
>   Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging 
> (2017-05-18 13:36:15 +0100)
> 
> are available in the git repository at:
> 
> 
>   git://github.com/bonzini/qemu.git tags/for-upstream
> 
> for you to fetch changes up to e10dc0ca6854c4f47cc5e9d47e20c62aa875f518:
> 
>   target/i386: use multiple CPU AddressSpaces (2017-05-19 13:01:32 +0200)
> 
> 
> * virtio-scsi use-after-free fix (Fam)
> * vhost-user-scsi support (Felipe)
> * SMM fixes and improvements for TCG (myself)
> * irqchip and AddressSpaceDispatch cleanups and fixes (Peter)
> * Coverity fix (Stefano)
> * NBD cleanups (Vladimir)
> * RTC accuracy improvements and code cleanups (Guangrong+Yunfang)
> 
> 
> Fam Zheng (1):
>   virtio-scsi: Unset hotplug handler when unrealize
> 
> Felipe Franciosi (2):
>   vhost-user-scsi: Introduce vhost-user-scsi host device
>   vhost-user-scsi: Introduce a vhost-user-scsi sample application
> 
> Paolo Bonzini (2):
>   target/i386: enable A20 automatically in system management mode
>   target/i386: use multiple CPU AddressSpaces
> 
> Peter Xu (4):
>   kvm: irqchip: trace changes on msi add/remove
>   msix: trace control bit write op
>   kvm: irqchip: skip update msi when disabled
>   exec: simplify phys_page_find() params
> 
> Stefano Stabellini (1):
>   Check the return value of fcntl in qemu_set_cloexec
> 
> Tai Yunfang (1):
>   mc146818rtc: precisely count the clock for periodic timer
> 
> Vladimir Sementsov-Ogievskiy (5):
>   nbd: strict nbd_wr_syncv
>   nbd: read_sync and friends: return 0 on success
>   nbd: add errp parameter to nbd_wr_syncv()
>   nbd: add errp to read_sync, write_sync and drop_sync
>   nbd/client.c: use errp instead of LOG
> 
> Xiao Guangrong (4):
>   mc146818rtc: update periodic timer only if it is needed
>   mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on TARGET_I386
>   mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
>   mc146818rtc: embrace all x86 specific code
> 
>  .gitignore|   1 +
>  Makefile  |   3 +
>  Makefile.objs |   4 +
>  block/nbd-client.c|  11 +-
>  contrib/vhost-user-scsi/Makefile.objs |   1 +
>  contrib/vhost-user-scsi/vhost-user-scsi.c | 886 
> ++
>  default-configs/pci.mak   |   1 +
>  default-configs/s390x-softmmu.mak |   1 +
>  exec.c|  13 +-
>  hw/pci/msix.c |  11 +-
>  hw/pci/trace-events   |   3 +
>  hw/scsi/Makefile.objs |   1 +
>  hw/scsi/vhost-user-scsi.c | 215 
>  hw/scsi/virtio-scsi.c |   3 +
>  hw/timer/mc146818rtc.c| 206 ---
>  hw/virtio/virtio-pci.c|  54 ++
>  hw/virtio/virtio-pci.h|  11 +
>  include/block/nbd.h   |   8 +-
>  include/hw/virtio/vhost-user-scsi.h   |  35 ++
>  include/hw/virtio/virtio-scsi.h   |   3 +
>  kvm-all.c |   4 +-
>  nbd/client.c  | 125 ++---
>  nbd/common.c  |  23 +-
>  nbd/nbd-internal.h|  40 +-
>  nbd/server.c  |  92 ++--
>  qemu-nbd.c|   3 +-
>  target/i386/arch_memory_mapping.c |  18 +-
>  target/i386/cpu.c |  15 +-
>  target/i386/cpu.h |  20 +-
>  target/i386/helper.c  |  96 ++--
>  target/i386/kvm.c |  12 +-
>  target/i386/machine.c |   4 -
>  target/i386/smm_helper.c  |  18 -
>  trace-events  |   3 +-
>  util/oslib-posix.c|   4 +-
>  35 files changed, 1642 insertions(+), 306 deletions(-)
>  create mode 100644 contrib/vhost-user-scsi/Makefile.objs
>  create mode 100644 contrib/vhost-user-scsi/vhost-user-scsi.c
>  create mode 100644 hw/scsi/vhost-user-scsi.c
>  create mode 100644 include/hw/virtio/vhost-user-scsi.h
> -- 
> 1.8.3.1
> 
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PULL 0/1] ui: egl-headless requires dmabuf support

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 10:48:29AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> Little single-patch pull request to fix a build issue.
> 
> please pull,
>   Gerd
> 
> The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:
> 
>   Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging 
> (2017-05-18 13:36:15 +0100)
> 
> are available in the git repository at:
> 
>   git://git.kraxel.org/qemu tags/pull-ui-20170519-1
> 
> for you to fetch changes up to 371ec54e9f8415cd74af45acdcf67b413f50cce5:
> 
>   ui: egl-headless requires dmabuf support (2017-05-19 10:46:00 +0200)
> 
> 
> ui: egl-headless requires dmabuf support
> 
> 
> Gerd Hoffmann (1):
>   ui: egl-headless requires dmabuf support
> 
>  vl.c | 4 ++--
>  ui/Makefile.objs | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PULL 00/18] Migration pull request

[Stefan Hajnoczi]

On Thu, May 18, 2017 at 07:24:44PM +0200, Juan Quintela wrote:
> Hi
> 
> This include the following series:
> - Fix non-multiple of page size migraition (dave)
> - Remove use of old MigrationParms (a.k.a. now block migration is a 
> capability)
> - Cleanups of headers in migration
> - Make savevm.c target independent
> 
> Please, apply.
> 
> Thanks, Juan.
> 
> The following changes since commit 56821559f0ba682fe6b367815572e6f974d329ab:
> 
>   Merge remote-tracking branch 'dgilbert/tags/pull-hmp-20170517' into staging 
> (2017-05-18 13:36:15 +0100)
> 
> are available in the git repository at:
> 
>   git://github.com/juanquintela/qemu.git tags/migration/20170518
> 
> for you to fetch changes up to 46d702b106d20beda2fcd0f96ddc44855ba262b3:
> 
>   migration: Make savevm.c target independent (2017-05-18 19:21:00 +0200)
> 
> 
> migration/next for 20170518
> 
> 
> Dr. David Alan Gilbert (3):
>   migration: Fix non-multiple of page size migration
>   postcopy: Require RAMBlocks that are whole pages
>   block migration: Allow compile time disable
> 
> Juan Quintela (15):
>   hmp: Use visitor api for hmp_migrate_set_parameter()
>   migration: Create block capability
>   migration: Remove use of old MigrationParams
>   migration: Remove old MigrationParams
>   migration: Create migration/xbzrle.h
>   migration: Split migration/channel.c for channel operations
>   migration: Export qemu-file-channel.c functions in its own file
>   migration: Remove migration.h from colo.h
>   migration: Move qjson.h to migration/
>   migration: Split vmstate-types.c from vmstate.c
>   migration: Remove qemu-file.h from vmstate.h
>   migration: Remove vmstate.h from migration.h
>   migration: migration.h was not needed
>   exec: Create include for target_page_size()
>   migration: Make savevm.c target independent
> 
>  Makefile.target  |   2 +-
>  block/qed.c  |   1 -
>  configure|  11 +
>  exec.c   |  10 +
>  hmp.c|  23 +-
>  hw/i386/pc_q35.c |   1 -
>  hw/virtio/vhost-user.c   |   1 -
>  hw/virtio/vhost-vsock.c  |   1 -
>  hw/virtio/virtio.c   |   1 -
>  include/exec/target_page.h   |  21 +
>  include/hw/hw.h  |   1 +
>  include/migration/block.h|  24 ++
>  include/migration/colo.h |   1 -
>  include/migration/migration.h|  30 +-
>  include/migration/qemu-file.h|   4 -
>  include/migration/vmstate.h  |   4 -
>  include/qemu/typedefs.h  |   1 -
>  include/sysemu/sysemu.h  |   4 +-
>  migration/Makefile.objs  |   6 +-
>  migration/block.c|  19 +-
>  migration/channel.c  |  67 
>  migration/channel.h  |  27 ++
>  migration/colo-comm.c|   4 +-
>  migration/colo.c |   9 +-
>  migration/exec.c |   1 +
>  migration/fd.c   |   1 +
>  migration/migration.c| 149 ---
>  migration/postcopy-ram.c |  18 +-
>  migration/qemu-file-channel.c|   1 +
>  migration/qemu-file-channel.h|  32 ++
>  migration/qjson.c|   2 +-
>  {include/migration => migration}/qjson.h |   0
>  migration/ram.c  |   8 +-
>  migration/rdma.c |   1 +
>  migration/savevm.c   |  40 +-
>  migration/socket.c   |   1 +
>  migration/tls.c  |   1 +
>  migration/vmstate-types.c| 661 
> +++
>  migration/vmstate.c  | 656 +-
>  migration/xbzrle.c   |   2 +-
>  migration/xbzrle.h   |  21 +
>  monitor.c|   1 -
>  qapi-schema.json |  28 +-
>  tests/Makefile.include   |   2 +-
>  tests/test-vmstate.c |   2 +
>  tests/test-xbzrle.c  |   2 +-
>  46 files changed, 1102 insertions(+), 801 deletions(-)
>  create mode 100644 include/exec/target_page.h
>  create mode 100644 migration/channel.c
>  create mode 100644 migration/channel.h
>  create mode 100644 migration/qemu-file-channel.h
>  rename {include/migration => migration}/qjson.h (100%)
>  create mode 100644 migration/vmstate-types.c
>  create mode 100644 migration/xbzrle.h
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan

Re: [Qemu-devel] [PATCH v2 0/9] QOM'ify work for sparc

[Mark Cave-Ayland]

On 29/04/17 11:49, xiaoqiang zhao wrote:

> This patch set aims for QOM'ifying code relate with sparc.
> It is part of my QOM'ify work of qemu code base.
> 
> changes since v1: 
> * rebased on the latest master
> 
> xiaoqiang zhao (9):
>   hw/misc: QOM'ify eccmemctl.c
>   hw/dma: QOM'ify sparc32_dma.c
>   hw/dma: QOM'ify sun4m_iommu.c
>   hw/misc: QOM'ify slavio_misc.c
>   hw/timer: QOM'ify m48txx_sysbus (pass 1)
>   hw/timer: QOM'ify m48txx_sysbus (pass 2)
>   hw/timer: QOM'ify slavio_timer
>   hw/sparc: QOM'ify sun4m.c
>   hw/sparc64: QOM'ify sun4u.c
> 
>  hw/dma/sparc32_dma.c| 25 ++-
>  hw/dma/sun4m_iommu.c| 12 +--
>  hw/misc/eccmemctl.c | 25 ++-
>  hw/misc/slavio_misc.c   | 43 ---
>  hw/sparc/sun4m.c| 54 
> +
>  hw/sparc64/sun4u.c  | 20 +-
>  hw/timer/m48t59.c   | 38 +-
>  hw/timer/slavio_timer.c | 12 +--
>  8 files changed, 105 insertions(+), 124 deletions(-)

I've finally found time to take a look at these and they look good to me.

I'll wait a few days to see if anyone has any comments (particularly
relating to the m48t59 device since that it appears that device is also
used by PReP) but if I hear nothing then I'll apply to my qemu-sparc branch.

And please do accept my apologies that it has taken so long to review
these patches, I will aspire to do much better in future.


ATB,

Mark.

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [PATCH v2 00/16] Vhost-pci for inter-VM communication

[Stefan Hajnoczi]

On Fri, May 19, 2017 at 11:10:33AM +0800, Jason Wang wrote:
> On 2017年05月18日 11:03, Wei Wang wrote:
> > On 05/17/2017 02:22 PM, Jason Wang wrote:
> > > On 2017年05月17日 14:16, Jason Wang wrote:
> > > > On 2017年05月16日 15:12, Wei Wang wrote:
> > > > > > Hi:
> > > > > > 
> > > > > > Care to post the driver codes too?
> > > > > > 
> > > > > OK. It may take some time to clean up the driver code before
> > > > > post it out. You can first
> > > > > have a check of the draft at the repo here:
> > > > > https://github.com/wei-w-wang/vhost-pci-driver
> > > > > 
> > > > > Best,
> > > > > Wei
> > > > 
> > > > Interesting, looks like there's one copy on tx side. We used to
> > > > have zerocopy support for tun for VM2VM traffic. Could you
> > > > please try to compare it with your vhost-pci-net by:
> > > > 
> > We can analyze from the whole data path - from VM1's network stack to
> > send packets -> VM2's
> > network stack to receive packets. The number of copies are actually the
> > same for both.
> 
> That's why I'm asking you to compare the performance. The only reason for
> vhost-pci is performance. You should prove it.

There is another reason for vhost-pci besides maximum performance:

vhost-pci makes it possible for end-users to run networking or storage
appliances in compute clouds.  Cloud providers do not allow end-users to
run custom vhost-user processes on the host so you need vhost-pci.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Halil Pasic]

On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> Dave
> P.S. I'm out for about a week.

Thanks for the info! Could you say something about our 'two
devices two sections vs two devices one section' dilemma
form PATCH 06/10 before leaving? I do not want to be pushy,
but I'm also eager to make progress :).

Have a good whatever it is next week!

Halil 

Re: [Qemu-devel] [PATCH v4 1/2] i386: rewrite way CPUID index is validated

[Kashyap Chamarthy]

On Tue, May 09, 2017 at 02:27:35PM +0100, Daniel P. Berrange wrote:
> Change the nested if statements into a flat format, to make
> it clearer what validation / capping is being performed on
> different CPUID index values.
> 
> NB this changes behaviour when "index > env->cpuid_xlevel2".
> This won't have any guest-visible effect because no there is
> no CPUID[0xC001]

Nit: When applying, maybe the maintainer could fix the typo:

"because no there is no" -> "because there is no"

> feature supported by TCG, and KVM code
> will never call cpu_x86_cpuid() with such an index value.
> 
> Reviewed-by: Eduardo Habkost 
> Signed-off-by: Daniel P. Berrange 
> ---
>  target/i386/cpu.c | 35 +++
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 

[...]

-- 
/kashyap

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>  As a preparation for switching to a vmstate based migration let us
>  introduce vmstate entities (e.g. VMStateDescription) for the css entities
>  to be migrated. Alongside some comments explaining or indicating the not
>  migration of certain members are introduced too.
> 
>  No changes in behavior, we just added some dead code -- which should
>  rise to life soon.
> 
>  Signed-off-by: Halil Pasic 
>  ---
>   hw/s390x/css.c | 276 
>  +
>   include/hw/s390x/css.h |  10 +-
>   2 files changed, 285 insertions(+), 1 deletion(-)
> 
>  diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>  index c03bb20..2bda7d0 100644
>  --- a/hw/s390x/css.c
>  +++ b/hw/s390x/css.c
>  @@ -20,29 +20,231 @@
>   #include "hw/s390x/css.h"
>   #include "trace.h"
>   #include "hw/s390x/s390_flic.h"
>  +#include "hw/s390x/s390-virtio-ccw.h"
>   
> >>
> >> [..]
> >>
>  +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
>  +VMStateField *field)
>  +{
>  +int32_t len;
>  +IndAddr **ind_addr = pv;
>  +
>  +len = qemu_get_be32(f);
>  +if (len != 0) {
>  +*ind_addr = get_indicator(qemu_get_be64(f), len);
>  +} else {
>  +qemu_get_be64(f);
>  +*ind_addr = NULL;
>  +}
>  +return 0;
>  +}
>  +
>  +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
>  +VMStateField *field, QJSON *vmdesc)
>  +{
>  +IndAddr *ind_addr = *(IndAddr **) pv;
>  +
>  +if (ind_addr != NULL) {
>  +qemu_put_be32(f, ind_addr->len);
>  +qemu_put_be64(f, ind_addr->addr);
>  +} else {
>  +qemu_put_be32(f, 0);
>  +qemu_put_be64(f, 0UL);
>  +}
>  +return 0;
>  +}
>  +
>  +const VMStateInfo vmstate_info_ind_addr = {
>  +.name = "s390_ind_addr",
>  +.get = css_get_ind_addr,
>  +.put = css_put_ind_addr
>  +};
> >>>
> >>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> >>> declare a temporary struct something like:
> >>>   struct tmp_ind_addr {
> >>>  IndAddr *parent;
> >>>  uint32_t  len;
> >>>  uint64_t  addr;
> >>>   }
> >>>
> >>> and then your .get/.put routines turn into pre_save/post_load
> >>> routines to just setup the len/addr.
> >>>
> >>
> >> I don't think this is going to work -- unfortunately! You can see below,
> >> how this IndAddr* migration stuff is supposed to be used:
> >> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> >> field when describing state which needs and IndAddr* migrated.
> >>
> >> The problem is, we do not know in what state will this field
> >> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> >> is however copying the pointer to this state into the parent.
> >> So instead of having a pointer to IndAddr* in those functions
> >> and updating it accordingly, I would have to find the IndAddr*
> >> in some arbitrary state (in our case VirtioCcwDevice) first,
> >> and I lack information for that.
> >>
> >> If it's hard to follow I can give you the patch I was debugging
> >> to come to this conclusion. (By the way I ended up with 10
> >> lines of code more than in this version, and although I think
> >> it looks nicer, it's simpler only if one knows how WITH_TMP
> >> works. My plan was to ask you which version do you like more
> >> and go with that before I realized it ain't gonna work.)
> >>
> > 
> > Yes, I see - I've got some similar other cases; the challenge
> > is it's a custom allocator - 'get_indicator' - and it's used
> > as fields in a few places.  Hmm.
> > 
> > 
> 
> The problem can be worked around by wrapping the WITH_TMP into a another
> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
> quite some boilerplate (+16 lines). Should I post the patch here?

Yes please.

> We could also consider making WITH_TMP act as a normal field. 
> Working on the whole state looks like a bit like a corner case:
> we have some stuff adjacent in the migration stream, and we have
> to map it on multiple fields (and vice-versa). Getting the whole
> state with a pointer to a certain field could work via container_of.

You do need to know which field you're working on to be able to safely
use container_of, so I'm not sure how it would work for you in this
case.

The other thought I'd had was that perhaps we could change the temporary
structure in VMSTATE_WITH_TMP to:

  struct foo 

[Qemu-devel] [PATCH 2/2] util: drop old utimensat() compat code

[Greg Kurz]

Now that 9pfs and virtfs-proxy-helper have been converted to utimensat(),
we don't need to keep qemu_utimens() anymore.

Signed-off-by: Greg Kurz 
---
 configure |   22 -
 include/sysemu/os-posix.h |   11 ---
 util/oslib-posix.c|   47 -
 3 files changed, 80 deletions(-)

diff --git a/configure b/configure
index 139638e922e0..1dea17ed2e73 100755
--- a/configure
+++ b/configure
@@ -3623,25 +3623,6 @@ if compile_prog "" "" ; then
   inotify1=yes
 fi
 
-# check if utimensat and futimens are supported
-utimens=no
-cat > $TMPC << EOF
-#define _ATFILE_SOURCE
-#include 
-#include 
-#include 
-
-int main(void)
-{
-utimensat(AT_FDCWD, "foo", NULL, 0);
-futimens(0, NULL);
-return 0;
-}
-EOF
-if compile_prog "" "" ; then
-  utimens=yes
-fi
-
 # check if pipe2 is there
 pipe2=no
 cat > $TMPC << EOF
@@ -5427,9 +5408,6 @@ fi
 if test "$curses" = "yes" ; then
   echo "CONFIG_CURSES=y" >> $config_host_mak
 fi
-if test "$utimens" = "yes" ; then
-  echo "CONFIG_UTIMENSAT=y" >> $config_host_mak
-fi
 if test "$pipe2" = "yes" ; then
   echo "CONFIG_PIPE2=y" >> $config_host_mak
 fi
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 900bdcb45ad0..629c8c648b7a 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -51,17 +51,6 @@ int os_mlock(void);
 typedef struct timeval qemu_timeval;
 #define qemu_gettimeofday(tp) gettimeofday(tp, NULL)
 
-#ifndef CONFIG_UTIMENSAT
-#ifndef UTIME_NOW
-# define UTIME_NOW ((1l << 30) - 1l)
-#endif
-#ifndef UTIME_OMIT
-# define UTIME_OMIT((1l << 30) - 2l)
-#endif
-#endif
-typedef struct timespec qemu_timespec;
-int qemu_utimens(const char *path, const qemu_timespec *times);
-
 bool is_daemonized(void);
 
 /**
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 4d9189e9efcf..7e28c161b257 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -207,53 +207,6 @@ int qemu_pipe(int pipefd[2])
 return ret;
 }
 
-int qemu_utimens(const char *path, const struct timespec *times)
-{
-struct timeval tv[2], tv_now;
-struct stat st;
-int i;
-#ifdef CONFIG_UTIMENSAT
-int ret;
-
-ret = utimensat(AT_FDCWD, path, times, AT_SYMLINK_NOFOLLOW);
-if (ret != -1 || errno != ENOSYS) {
-return ret;
-}
-#endif
-/* Fallback: use utimes() instead of utimensat() */
-
-/* happy if special cases */
-if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) {
-return 0;
-}
-if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) {
-return utimes(path, NULL);
-}
-
-/* prepare for hard cases */
-if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
-gettimeofday(_now, NULL);
-}
-if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
-stat(path, );
-}
-
-for (i = 0; i < 2; i++) {
-if (times[i].tv_nsec == UTIME_NOW) {
-tv[i].tv_sec = tv_now.tv_sec;
-tv[i].tv_usec = tv_now.tv_usec;
-} else if (times[i].tv_nsec == UTIME_OMIT) {
-tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
-tv[i].tv_usec = 0;
-} else {
-tv[i].tv_sec = times[i].tv_sec;
-tv[i].tv_usec = times[i].tv_nsec / 1000;
-}
-}
-
-return utimes(path, [0]);
-}
-
 char *
 qemu_get_local_state_pathname(const char *relative_pathname)
 {

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 03:33:12PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > shutdown() is safe, in that it stops any other threads accessing the fd
> > > but doesn't allow it's reallocation until the close;  We perform the
> > > close only when we've joined all other threads that were using the fd.
> > > Any of the threads that do new calls on the fd get an error and quickly
> > > fall down their error paths.
> > 
> > Ahh that's certainly an interesting scenario. That would certainly be
> > a problem with the migration code when this was originally written.
> > It had two QEMUFile structs each with an 'int fd' field, so when you
> > close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> > used by another thread.
> > 
> > Since we switched over to use QIOChannel though, I think the thread
> > scenario you describe should be avoided entirely. When you have multiple
> > QEMUFile objects, they each have a reference counted pointer to the same
> > underlying QIOChannel object instance. So when QEMUFile triggers a call
> > to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> > Since the other threads have a reference to the same QIOChannel object,
> > they'll now see this fd == -1 straightaway.
> > 
> > So, IIUC, this should make the need for shutdown() redundant (at least
> > for the thread race conditions you describe).
> 
> That's not thread safe unless you're doing some very careful locking.
> Consider:
>   T1  T2   
>  oldfd=fd   tmp=fd
>  fd=-1
>  close(oldfd)
>  unrelated open()
> read(tmp,...
> 
> In practice every use of fd will be a copy into a tmp and then the
> syscall; the unrelated open() could happen in another thread.
> (OK, the gap between the tmp and the read is tiny, although if we're
> doing multiple operations chances are the compiler will optimise
> it to the top of a loop).
> 
> There's no way to make that code safe.

Urgh, yes, I see what you mean.

Currently the QIOChannelCommand implementation, uses a pair of anonymous
pipes for stdin/out to the child process. I wonder if we could switch
that to use socketpair() instead, thus letting us shutdown() on it too.

Though I guess it would be sufficient for qio_channel_shutdown() to
merely kill the child PID, while leaving the FDs open, as then you'd
get EOF and/or EPIPE on the read/writes.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 1/2] 9pfs: assume utimensat() and futimens() are present

[Greg Kurz]

The utimensat() and futimens() syscalls have been around for ages (ie,
glibc 2.6 and linux 2.6.22), and the decision was already taken to
switch to utimensat() anyway when fixing CVE-2016-9602 in 2.9.

Signed-off-by: Greg Kurz 
---
 fsdev/virtfs-proxy-helper.c |3 ++-
 hw/9pfs/9p-handle.c |5 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 54f7ad1c48f0..617e19cd0b88 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -945,7 +945,8 @@ static int process_requests(int sock)
  [0].tv_sec, [0].tv_nsec,
  [1].tv_sec, [1].tv_nsec);
 if (retval > 0) {
-retval = qemu_utimens(path.data, spec);
+retval = utimensat(AT_FDCWD, path.data, spec,
+   AT_SYMLINK_NOFOLLOW);
 if (retval < 0) {
 retval = -errno;
 }
diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 1687661bc95a..9875f1894cc5 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -378,7 +378,6 @@ static int handle_utimensat(FsContext *ctx, V9fsPath 
*fs_path,
 const struct timespec *buf)
 {
 int ret;
-#ifdef CONFIG_UTIMENSAT
 int fd;
 struct handle_data *data = (struct handle_data *)ctx->private;
 
@@ -388,10 +387,6 @@ static int handle_utimensat(FsContext *ctx, V9fsPath 
*fs_path,
 }
 ret = futimens(fd, buf);
 close(fd);
-#else
-ret = -1;
-errno = ENOSYS;
-#endif
 return ret;
 }
 

[Qemu-devel] [PATCH 0/2] get rid of qemu_utimens()

[Greg Kurz]

It is currently only used by 9pfs and virtfs-proxy-helper. This series convert
them to utimensat() and futimens().

--
Greg

---

Greg Kurz (2):
  9pfs: assume utimensat() and futimens() are present
  util: drop old utimensat() compat code


 configure   |   22 
 fsdev/virtfs-proxy-helper.c |3 ++-
 hw/9pfs/9p-handle.c |5 -
 include/sysemu/os-posix.h   |   11 --
 util/oslib-posix.c  |   47 ---
 5 files changed, 2 insertions(+), 86 deletions(-)

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Fri, May 19, 2017 at 02:02:00PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > On Fri, May 19, 2017 at 01:51:43PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > > > > We don't really have a return path for the other types yet. Let's 
> > > > > > check
> > > > > > this when .get_return_path() is called.
> > > > > > 
> > > > > > For this, we introduce a new feature bit, and set it up only for 
> > > > > > socket
> > > > > > typed IO channels.
> > > > > > 
> > > > > > This will help detect earlier failure for postcopy, e.g., logically
> > > > > > speaking postcopy cannot work with "exec:". Before this patch, when 
> > > > > > we
> > > > > > try to migrate with "migrate -d exec:cat>out", we'll hang the 
> > > > > > system.
> > > > > > With this patch, we'll get:
> > > > > > 
> > > > > > (qemu) migrate -d exec:cat>out
> > > > > > Unable to open return-path for postcopy
> > > > > 
> > > > > This is wrong - post-copy migration *can* work with exec: - it just 
> > > > > entirely
> > > > > depends on what command you are running. Your example ran a command 
> > > > > which is
> > > > > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > > > > bidirectional channel. Actually the channel is always bi-directional, 
> > > > > but
> > > > > 'cat' simply won't ever send data back to QEMU.
> > > > 
> > > > The thing is it didn't used to be able to; prior to your conversion to
> > > > channel, postcopy would reject being started with exec: because it
> > > > couldn't open a return path, so it was safe.
> > > 
> > > It safe but functionally broken because it is valid to want to use
> > > exec migration with post-copy.
> > > 
> > > > > If QEMU hangs when the other end doesn't send data back, that 
> > > > > actually seems
> > > > > like a potentially serious bug in migration code. Even if using the 
> > > > > normal
> > > > > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > > > > send data to QEMU on the return path, the source QEMU must never hang.
> > > > 
> > > > Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
> > > > return path; but it does depend on how the exec: behaves on the
> > > > destination.
> > > > If the destination discards data written to it, then I think the
> > > > behaviour would be:
> > > >a) Page requests would just get dropped, they'd eventually get
> > > > fulfilled by the background page transmissions, but that could mean that
> > > > a page request would wait for minutes for the page.
> > > >b) The qemu main thread on the destination can be blocked by that, so
> > > > the monitor might not respond until the page request is fulfilled.
> > > >c) I'm not quite sure what would happen to the source return-path
> > > > thread
> > > > 
> > > > The behaviour seems to have changed between 2.9.0 (f26 package) and my
> > > > reasonably recent head build.
> > > 
> > > That's due to the bug we just fixed where we mistakenly didn't
> > > allow bi-directional I/O for exec
> > > 
> > >   commit 062d81f0e968fe1597474735f3ea038065027372
> > >   Author: Daniel P. Berrange 
> > >   Date:   Fri Apr 21 12:12:20 2017 +0100
> > > 
> > > migration: setup bi-directional I/O channel for exec: protocol
> > > 
> > > Historically the migration data channel has only needed to be
> > > unidirectional. Thus the 'exec:' protocol was requesting an
> > > I/O channel with O_RDONLY on incoming side, and O_WRONLY on
> > > the outgoing side.
> > > 
> > > This is fine for classic migration, but if you then try to run
> > > TLS over it, this fails because the TLS handshake requires a
> > > bi-directional channel.
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > Reviewed-by: Juan Quintela 
> > > Signed-off-by: Juan Quintela 
> > > 
> > > 
> > > > A migration_cancel also doesn't work for 'exec' because it doesn't
> > > > support shutdown() - it just sticks in 'cancelling'.
> > > > On a socket that was broken like this the cancel would work because
> > > > it issues a shutdown() which causes the socket to cleanup.
> > > 
> > > I'm curious why migration_cancel requires shutdown() to work ? Why
> > > isn't it sufficient from the source POV to just close the socket
> > > entirely straight away.
> > 
> > close() closes the fd so that any other uses of the fd get an
> > error and you're at risk of the fd getting reallocated by something
> > else.  So consider if the main thread calls close(), the migration
> > thread and the return thread both carry on using that fd, which might
> > have been reallocated to something different.  Worse I think we came to the
> > consolution that on some OSs a 

[Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd

[Greg Kurz]

Since chroot() doesn't change the current directory, it is indeed a good
practice to chdir() to the target directory and then then chroot(), or
to chroot() to the target directory and then chdir("/").

The current code does neither of them actually. Let's go for the latter.

This doesn't fix any security issue since all of this takes place before
the helper begins to process requests.

Signed-off-by: Greg Kurz 
---
 fsdev/virtfs-proxy-helper.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 54f7ad1c48f0..4c4238f62e53 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -1129,14 +1129,14 @@ int main(int argc, char **argv)
 }
 }
 
-if (chdir("/") < 0) {
-do_perror("chdir");
-goto error;
-}
 if (chroot(rpath) < 0) {
 do_perror("chroot");
 goto error;
 }
+if (chdir("/") < 0) {
+do_perror("chdir");
+goto error;
+}
 
 get_version = false;
 #ifdef FS_IOC_GETVERSION

[Qemu-devel] [PATCH v12 0/2] pseries: migrate pending_events of spapr state

[Daniel Henrique Barboza]

NOTE: At the moment I am sending this v12, patch 1 isn't available in
dgibson/ppc-for-2.10 branch yet. I am resending it here, unchanged,
just to allow patch 2 to be applied cleanly.


v12:
- patch 2: added a switch statement to get the proper data_size based on
the log_type

v11:
- patch 1 (new): cleanup of spapr_events.c:
* removed the 'exception' boolean from the sPAPREventLogEntry
* simplified the 'event_scan' function
- patch 2:
* data_size is now calculated inside rtas_event_log_queue()
* using VBUFFER instead of VARRAY to avoid casts
* log_type changed to int32_t

v10: detached from DRC patch set

v9: no changes

v8: no changes

v7: no changes

v6: - Rebased with QEMU master after 6+ months.
class and minor improvements.
- Added clarifications from the previous v5 discussions in the commit 
messages.

v5: - Rebased on David's ppc-for-2.8.

v4: - Rebased on David's ppc-for-2.7. 

v3: - Simplify overall design followng discussion with Paolo. No longer need
  metadata to migrate QTAILQ.
- Extend VMStateInfo instead of adding similar fields to VMStateField.

v2: - Put the newly added migrating fields in subsections so that backward 
  migration is not broken.  
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)

v1: - Inital version.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)


This patch was detached from the patchset:

"[PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events"

Because it is independent and has use outside of the scope of the
pseries DRC migration patchset.

Daniel Henrique Barboza (1):
  hw/ppc/spapr_events.c: removing 'exception' from sPAPREventLogEntry

Jianjun Duan (1):
  migration: spapr: migrate pending_events of spapr state

 hw/ppc/spapr.c | 32 +
 hw/ppc/spapr_events.c  | 64 +++---
 include/hw/ppc/spapr.h |  4 ++--
 3 files changed, 53 insertions(+), 47 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v12 2/2] migration: spapr: migrate pending_events of spapr state

[Daniel Henrique Barboza]

From: Jianjun Duan 

In racing situations between hotplug events and migration operation,
a rtas hotplug event could have not yet be delivered to the source
guest when migration is started. In this case the pending_events of
spapr state need be transmitted to the target so that the hotplug
event can be finished on the target.

All the different fields of the events are encoded as defined by
PAPR. We can migrate them as a binary stream inside VBUFFER without
any concerns about data padding or endianess.

pending_events is put in a subsection in the spapr state VMSD to make
sure migration across different versions is not broken.

Signed-off-by: Jianjun Duan 
Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr.c | 32 
 hw/ppc/spapr_events.c  | 12 
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0980d73..5afd328 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1444,6 +1444,37 @@ static bool version_before_3(void *opaque, int 
version_id)
 return version_id < 3;
 }
 
+static bool spapr_pending_events_needed(void *opaque)
+{
+sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+return !QTAILQ_EMPTY(>pending_events);
+}
+
+static const VMStateDescription vmstate_spapr_event_entry = {
+.name = "spapr_event_log_entry",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_INT32(log_type, sPAPREventLogEntry),
+VMSTATE_UINT32(data_size, sPAPREventLogEntry),
+VMSTATE_VBUFFER_ALLOC_UINT32(data, sPAPREventLogEntry, 0,
+ NULL, data_size),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static const VMStateDescription vmstate_spapr_pending_events = {
+.name = "spapr_pending_events",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = spapr_pending_events_needed,
+.fields = (VMStateField[]) {
+VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
+ vmstate_spapr_event_entry, sPAPREventLogEntry, next),
+VMSTATE_END_OF_LIST()
+},
+};
+
 static bool spapr_ov5_cas_needed(void *opaque)
 {
 sPAPRMachineState *spapr = opaque;
@@ -1542,6 +1573,7 @@ static const VMStateDescription vmstate_spapr = {
 .subsections = (const VMStateDescription*[]) {
 _spapr_ov5_cas,
 _spapr_patb_entry,
+_spapr_pending_events,
 NULL
 }
 };
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 73e2a18..a509c46 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -350,6 +350,18 @@ static void rtas_event_log_queue(int log_type, void *data)
 g_assert(data);
 entry->log_type = log_type;
 entry->data = data;
+
+switch (log_type) {
+case RTAS_LOG_TYPE_EPOW:
+entry->data_size = sizeof(struct epow_log_full);
+break;
+case RTAS_LOG_TYPE_HOTPLUG:
+entry->data_size = sizeof(struct hp_log_full);
+break;
+default:
+g_assert(false);
+}
+
 QTAILQ_INSERT_TAIL(>pending_events, entry, next);
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 02239a5..0554e11 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -597,8 +597,9 @@ struct sPAPRTCETable {
 sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
 
 struct sPAPREventLogEntry {
-int log_type;
+int32_t log_type;
 void *data;
+uint32_t data_size;
 QTAILQ_ENTRY(sPAPREventLogEntry) next;
 };
 
-- 
2.9.4

[Qemu-devel] [PATCH v12 1/2] hw/ppc/spapr_events.c: removing 'exception' from sPAPREventLogEntry

[Daniel Henrique Barboza]

Currenty we do not have any RTAS event that is reported by the
event-scan interface. The existing events, RTAS_LOG_TYPE_EPOW and
RTAS_LOG_TYPE_HOTPLUG, are being reported by the check-exception
interface and, as such, marked as 'exception=true'.

Commit 79853e18d9, 'spapr_events: event-scan RTAS interface', added
the event_scan interface because the guest kernel requires it to
initialize other required interfaces. It is acting since then as
a stub because no events that would be reported by it were added
since then. However, the existence of the 'exception' boolean adds
an unnecessary load in the future migration of the pending_events,
sPAPREventLogEntry QTAILQ that hosts the pending RTAS events.

To make the code cleaner and ease the future migration changes, this
patch makes the following changes:

- remove the 'exception' boolean that filter these events. There is
nothing to filter since all events are reported by check-exception;

- functions rtas_event_log_queue, rtas_event_log_dequeue and
rtas_event_log_contains don't receive the 'exception' boolean
as parameter;

- event_scan function was simplified. It was calling
'rtas_event_log_dequeue(mask, false)' that was always returning
'NULL' because we have no events that are created with
exception=false, thus in the end it would execute a jump to
'out_no_events' all the time. The function now assumes that
this will always be the case and all the remaining logic were
deleted.

In the future, when or if we add new RTAS events that should
be reported with the event_scan interface, we can refer to
the changes made in this patch to add the event_scan logic
back.

Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr_events.c  | 52 +++---
 include/hw/ppc/spapr.h |  1 -
 2 files changed, 7 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index f0b28d8..73e2a18 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -342,20 +342,18 @@ static int rtas_event_log_to_irq(sPAPRMachineState 
*spapr, int log_type)
 return source->irq;
 }
 
-static void rtas_event_log_queue(int log_type, void *data, bool exception)
+static void rtas_event_log_queue(int log_type, void *data)
 {
 sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
 
 g_assert(data);
 entry->log_type = log_type;
-entry->exception = exception;
 entry->data = data;
 QTAILQ_INSERT_TAIL(>pending_events, entry, next);
 }
 
-static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask,
-  bool exception)
+static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask)
 {
 sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 sPAPREventLogEntry *entry = NULL;
@@ -364,10 +362,6 @@ static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t 
event_mask,
 const sPAPREventSource *source =
 rtas_event_log_to_source(spapr, entry->log_type);
 
-if (entry->exception != exception) {
-continue;
-}
-
 if (source->mask & event_mask) {
 break;
 }
@@ -380,7 +374,7 @@ static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t 
event_mask,
 return entry;
 }
 
-static bool rtas_event_log_contains(uint32_t event_mask, bool exception)
+static bool rtas_event_log_contains(uint32_t event_mask)
 {
 sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 sPAPREventLogEntry *entry = NULL;
@@ -389,10 +383,6 @@ static bool rtas_event_log_contains(uint32_t event_mask, 
bool exception)
 const sPAPREventSource *source =
 rtas_event_log_to_source(spapr, entry->log_type);
 
-if (entry->exception != exception) {
-continue;
-}
-
 if (source->mask & event_mask) {
 return true;
 }
@@ -479,7 +469,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
 epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
 epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
 
-rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true);
+rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow);
 
 qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
  rtas_event_log_to_irq(spapr,
@@ -572,7 +562,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t 
hp_action,
 cpu_to_be32(drc_id->count_indexed.index);
 }
 
-rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
+rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp);
 
 qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
  rtas_event_log_to_irq(spapr,
@@ -667,7 +657,7 @@ static void check_exception(PowerPCCPU *cpu, 
sPAPRMachineState *spapr,
 xinfo |= (uint64_t)rtas_ld(args, 6) << 32;
 }
 
-

Re: [Qemu-devel] Migration downtime more than 5s when migrating guest with massive disks

[Gonglei (Arei)]

Oops, forgot to CC qemu-devel, add it.


> -Original Message-
> From: Gonglei (Arei)
> Sent: Friday, May 19, 2017 8:17 PM
> To: 'Paolo Bonzini'; yanghongyang; m...@redhat.com
> Cc: quint...@redhat.com; Dr. David Alan Gilbert; Huangzhichao
> Subject: RE: Migration downtime more than 5s when migrating guest with
> massive disks
> 
> 
> > -Original Message-
> > From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> > Sent: Friday, May 19, 2017 6:19 PM
> >
> > On 19/05/2017 12:00, Yang Hongyang wrote:
> > > We found that migration downtime is unacceptable when migrating guest
> > with
> > > 60 disks, more than 5.5 seconds.
> > > By debugging, we find out the problem is there's too many
> > > memory_region_transaction_commit() operations during guest load, about
> > > 31w+ times.
> > > Any idea to optimize the migration downtime in this scenario?
> > > maybe reduce the times of memory_region_transaction_commit() call, but
> > how?
> > > or we could optimize the time cost of
> memory_region_transaction_commit()
> > call,
> > > but I think that wouldn't help much.
> >
> > It would.  Right now memory_region_transaction_commit() is roughly
> > O(n^2) (n devices * n BARs), and there are n of them.
> >
> > Reducing memory_region_transaction_commit to O(n) would be a large
> > change.  One idea is to share the AddressSpaceDispatch for AddressSpaces
> > that have the same root memory region (after resolving aliases).  The
> > starting point would be to change mem_begin/mem_commit/mem_add from
> a
> > MemoryListener to an loop on the FlatView, storing the
> > AddressSpaceDispatch in the FlatView.
> >
> How about do O(1) for stopping stage of live migration?
> Because the cpu is stopped in this phase, it wouldn't cause
> side effects IMHO, right?
> 
> Thanks,
> -Gonglei
> 
> > One bandaid solution is to use virtio-scsi in the guest, with multiple
> > disks behind one controller.
> >
> > Thanks,
> >
> > Paolo

[Qemu-devel] A problem of IRQchip in QEMU and KVM for ARM

[Li Zhang]

Hi,

I am looking into QEMU code in ARM recently and trying to add add_hot_cpu
in QEMU for ARM,
but it doesn't work when enabling KVM. It reports error:

"kvm_init_vcpu failed: Device or resourc busy."

By debugging QEMU with gdb, it failed on ioctl. In kernel soruce code
arch/arm/kvm/arm.c,
vcpu is created by this following function, it will report -EBUSY if
irqchip_in_kernel.

struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
{
int err;
struct kvm_vcpu *vcpu;

if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) {
err = -EBUSY;
goto out;
}

  
}

I set virt machine with kernel_irqchip = off, it can execute cpu-add
interface correctly with qmp-shell
commands. But VMs still can't work well with kernel_irqchip=off when
executing "info cpus" in qemu monitor.

My question is that:
1) Can we change this error status in kvm_arch_vcpu_create?
2) Is it that irqchip_kernel=off  isn't supported with KVM enabled on ARM?

-- 

Best Regards
-Li

Re: [Qemu-devel] [PATCH] e1000e: Fix a bug where guest hangs upon migration

[Sameeh Jubran]

On Fri, May 19, 2017 at 9:25 AM, Jason Wang  wrote:

>
>
> On 2017年05月17日 19:46, Sameeh Jubran wrote:
>
>> The bug was caused by the "receive overrun" (bit #6 of the ICR register)
>> interrupt
>> which would be triggered post migration in a heavy traffic environment.
>> Even though the
>> "receive overrun" bit (#6) is masked out by the IMS register (refer to
>> the log below)
>> the driver still receives an interrupt as the "receive overrun" bit (#6)
>> causes the
>> "Other" - bit #24 of the ICR register - bit to be set as documented
>> below. The driver
>> handles the interrupt and clears the "Other" bit (#24) but doesn't clear
>> the
>> "receive overrun" bit (#6) which leads to an infinite loop. Apparently
>> the Windows
>> driver expects that the "receive overrun" bit and other ones - documented
>> below - to be
>> cleared when the "Other" bit (#24) is cleared.
>>
>> So to sum that up:
>> 1. Bit #6 of the ICR register is set by heavy traffic
>> 2. As a results of setting bit #6, bit #24 is set
>> 3. The driver receives an interrupt for bit 24 (it doesn't receieve an
>> interrupt for bit #6 as it is masked out by IMS)
>> 4. The driver handles and clears the interrupt of bit #24
>> 5. Bit #6 is still set.
>> 6. 2 happens all over again
>>
>> The Interrupt Cause Read - ICR register:
>>
>> The ICR has the "Other" bit - bit #24 - that is set when one or more of
>> the following
>> ICR register's bits are set:
>>
>> LSC - bit #2, RXO - bit #6, MDAC - bit #9, SRPD - bit #16, ACK - bit #17,
>> MNG - bit #18
>>
>> Log sample of the storm:
>>
>> 27563@1494850819.411877:e1000e_irq_pending_interrupts ICR PENDING:
>> 0x100 (ICR: 0x815000c2, IMS: 0x1a4)
>> 27563@1494850819.411900:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.411915:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.412380:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.412395:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.412436:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.412441:e1000e_irq_pending_interrupts ICR PENDING: 0x0
>> (ICR: 0x815000c2, IMS: 0xa4)
>> 27563@1494850819.412998:e1000e_irq_pending_interrupts ICR PENDING:
>> 0x100 (ICR: 0x815000c2, IMS: 0x1a4)
>>
>> This commit solves:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1447935
>> https://bugzilla.redhat.com/show_bug.cgi?id=1449490
>>
>> Signed-off-by: Sameeh Jubran 
>> ---
>>   hw/net/e1000e_core.c | 7 +--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index 28c5be1..8174b53 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -2454,14 +2454,17 @@ e1000e_set_ics(E1000ECore *core, int index,
>> uint32_t val)
>>   static void
>>   e1000e_set_icr(E1000ECore *core, int index, uint32_t val)
>>   {
>> +uint32_t icr = 0;
>>   if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>>   (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>>   trace_e1000e_irq_icr_process_iame();
>>   e1000e_clear_ims_bits(core, core->mac[IAM]);
>>   }
>>   -trace_e1000e_irq_icr_write(val, core->mac[ICR], core->mac[ICR] &
>> ~val);
>> -core->mac[ICR] &= ~val;
>> +icr = core->mac[ICR] & ~val;
>> +icr = (val & E1000_ICR_OTHER) ? (icr & ~E1000_ICR_OTHER_CAUSES) :
>> icr;
>> +trace_e1000e_irq_icr_write(val, core->mac[ICR], icr);
>> +core->mac[ICR] = icr;
>>   e1000e_update_interrupt_state(core);
>>   }
>>
>>
>
> Thanks for the patch.
>
> So this is an undocumented behavior, we must be careful on this. Several
> question below:
>
> - have you verified this on real hardware?
>
No I haven't

> - is MSIX enabled in this case?
>
Yes it is, I have tested the patch with msi disabled too.

> - according to the steps you've summed up above, it's not specific to
> migration?
>
True

>
> Thanks
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin *
*Software Engineer @ Daynix .*

Re: [Qemu-devel] [PATCH] linux-user: remove all traces of qemu from /proc/self/cmdline

[Riku Voipio]

On Mon, Mar 20, 2017 at 12:31:55PM +0100, Andreas Schwab wrote:
> Instead of post-processing the real contents use the remembered target
> argv.  That removes all traces of qemu, including command line options,
> and handles QEMU_ARGV0.

Applied to Linux-user, thanks

Riku
 
> Signed-off-by: Andreas Schwab 
> ---
>  linux-user/syscall.c | 47 +++
>  1 file changed, 7 insertions(+), 40 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index cec8428589..ec1fd20386 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7358,52 +7358,19 @@ int host_to_target_waitstatus(int status)
>  
>  static int open_self_cmdline(void *cpu_env, int fd)
>  {
> -int fd_orig = -1;
> -bool word_skipped = false;
> -
> -fd_orig = open("/proc/self/cmdline", O_RDONLY);
> -if (fd_orig < 0) {
> -return fd_orig;
> -}
> +CPUState *cpu = ENV_GET_CPU((CPUArchState *)cpu_env);
> +struct linux_binprm *bprm = ((TaskState *)cpu->opaque)->bprm;
> +int i;
>  
> -while (true) {
> -ssize_t nb_read;
> -char buf[128];
> -char *cp_buf = buf;
> +for (i = 0; i < bprm->argc; i++) {
> +size_t len = strlen(bprm->argv[i]) + 1;
>  
> -nb_read = read(fd_orig, buf, sizeof(buf));
> -if (nb_read < 0) {
> -int e = errno;
> -fd_orig = close(fd_orig);
> -errno = e;
> +if (write(fd, bprm->argv[i], len) != len) {
>  return -1;
> -} else if (nb_read == 0) {
> -break;
> -}
> -
> -if (!word_skipped) {
> -/* Skip the first string, which is the path to qemu-*-static
> -   instead of the actual command. */
> -cp_buf = memchr(buf, 0, nb_read);
> -if (cp_buf) {
> -/* Null byte found, skip one string */
> -cp_buf++;
> -nb_read -= cp_buf - buf;
> -word_skipped = true;
> -}
> -}
> -
> -if (word_skipped) {
> -if (write(fd, cp_buf, nb_read) != nb_read) {
> -int e = errno;
> -close(fd_orig);
> -errno = e;
> -return -1;
> -}
>  }
>  }
>  
> -return close(fd_orig);
> +return 0;
>  }
>  
>  static int open_self_maps(void *cpu_env, int fd)
> -- 
> 2.12.0
> 
> 
> -- 
> Andreas Schwab, SUSE Labs, sch...@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [Qemu-devel] [PATCH] linux-user: Fix TARGET_MAP* and TARGET_F_??LCK for hppa arch

[Riku Voipio]

On Sun, Mar 12, 2017 at 08:17:46AM +1000, Richard Henderson wrote:
> On 03/12/2017 03:50 AM, Helge Deller wrote:
> >TARGET_MAP_TYPE needs to be 0x03 instead of 0x0f on the hppa
> >architecture, otherwise it conflicts with MAP_FIXED which is 0x04.
> >
> >Add missing TARGET_MAP_STACK and TARGET_MAP_HUGETLB values.
> >
> >Fix TARGET_F_RDLCK, TARGET_F_WRLCK and TARGET_F_UNLCK.
> >
> >Signed-off-by: Helge Deller 
> 
> I applied the MAP_FIXED and TARGET_F_* parts separately in my tree.  I'd
> like to see what others think about the other MAP_* defines before including
> that.

What's the current state of these patches? Are these patches still waiting for
opinions?

Riku

Re: [Qemu-devel] [PATCH 0/2] linux-user: fix eventfd()

[Riku Voipio]

On Tue, Apr 25, 2017 at 06:32:30PM +0200, Laurent Vivier wrote:
> Ping?

Applied, thanks. 

> Laurent
> 
> Le 01/03/2017 à 10:37, Laurent Vivier a écrit :
> > This patch series byte-swap the uint64_t data stream
> > of a file-descriptor opened with eventfd().
> > 
> > It allows to pass more LTP test cases:
> > 
> > eventfd011  TPASS  :  counter value matches required
> > eventfd012  TPASS  :  read failed with EAGAIN as expected
> > eventfd013  TPASS  :  counter value matches required
> > eventfd014  TPASS  :  write failed with EAGAIN as expected
> > eventfd015  TPASS  :  read failed with EINVAL as expected
> > eventfd016  TPASS  :  write failed with EINVAL as expected
> > eventfd017  TPASS  :  write failed with EINVAL as expected
> > eventfd018  TPASS  :  fd is set in readfds
> > eventfd019  TPASS  :  fd is not set in readfds
> > eventfd01   10  TPASS  :  fd is set in writefds
> > eventfd01   11  TPASS  :  fd is not set in writefds
> > eventfd011  TPASS  :  counter value matches required
> > eventfd012  TPASS  :  read failed with EAGAIN as expected
> > eventfd013  TPASS  :  counter value matches required
> > eventfd014  TPASS  :  write failed with EAGAIN as expected
> > eventfd015  TPASS  :  read failed with EINVAL as expected
> > eventfd016  TPASS  :  write failed with EINVAL as expected
> > eventfd017  TPASS  :  write failed with EINVAL as expected
> > eventfd018  TPASS  :  fd is set in readfds
> > eventfd019  TPASS  :  fd is not set in readfds
> > eventfd01   10  TPASS  :  fd is set in writefds
> > eventfd01   11  TPASS  :  fd is not set in writefds
> > eventfd01   12  TPASS  :  counter value write from child successful
> > eventfd01   13  TCONF  :  eventfd01.c:642: eventfd support is not available 
> > in AIO subsystem
> > eventfd01   14  TCONF  :  eventfd01.c:647: eventfd support is not available 
> > in AIO subsystem
> > eventfd01   15  TCONF  :  eventfd01.c:652: eventfd support is not available 
> > in AIO subsystem
> > 
> > Laurent Vivier (2):
> >   linux-user: call fd_trans_target_to_host_data() for write()
> >   linux-user: fix eventfd
> > 
> >  linux-user/syscall.c | 38 +++---
> >  1 file changed, 35 insertions(+), 3 deletions(-)
> > 
> 

Re: [Qemu-devel] [PATCH] block: Tweak error message related to qemu-img convert

[Max Reitz]

On 2017-05-08 19:13, Eric Blake wrote:
> When converting a 1.1 image down to 0.10, qemu-iotests 060 forces
> a contrived failure where allocating a cluster used to replace a
> zero cluster reads unaligned data.  Since it is a zero cluster
> rather than a data cluster being converted, changing the error
> message to match our earlier change in 'qcow2: Make distinction
> bewteen zero cluster types obvious' is worthwhile.
> 
> Suggested-by: Max Reitz 
> Signed-off-by: Eric Blake 
> ---
> 
> There's one more instance of "Data cluster offset" in qcow2-cluster.c,
> but that one in handle_copied() is contained inside a
> cluster_type == QCOW2_CLUSTER_NORMAL conditional.
> 
>  block/qcow2-cluster.c  | 3 ++-
>  tests/qemu-iotests/060.out | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)

Assuming no objection means consent:

Thanks, fixed the commit title and applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature