Re: [Qemu-devel] a tiny tool for checking and repairing damaged qcow2 image

[Markus Armbruster]

Cc'ing QCOW2 maintainer.

ÕÅÓÑ¼Ó  writes:

> Hi,
>
> I want to introduce a tiny tool(https://github.com/zhangyoujia/qcow2-dump) 
> for checking and repairing damaged qcow2 image.
>
> I met many damaged qcow2 images when I work.
>
> It does't always work by using the qemu-img command to check and repair these 
> damaged qcow2 images.
>
> The functions of the qemu-img check command are too limited for developers.
>
> It's difficult to known where is damaged, and difficult to repair the damage 
> too.
>
> For example: more than one active L2 entries point to the same active 
> cluster(mostly caused by refcount error), such as:
>
> Active Cluster:
> 
> REUSED cluster offset: 0x4400 | l1_index:2, l2_index: 1017 | 
> refcount: 2
> REUSED cluster offset: 0x4400 | l1_index:   12, l2_index: 7780
>
> REUSED cluster offset: 0x4500 | l1_index:2, l2_index: 1273 | 
> refcount: 2
> REUSED cluster offset: 0x4500 | l1_index:   12, l2_index: 6742
>
> REUSED cluster offset: 0x4a00 | l1_index:2, l2_index: 2553 | 
> refcount: 2
> REUSED cluster offset: 0x4a00 | l1_index:   12, l2_index: 7068
>
> The results of qemu-img check command are wrong which only show refcount 
> errors and copied errors, but do not show the active cluster reused errors. 
> so if you use qemu-img check -r sub-command to repair this qcow2 image, it 
> only increases refcount of the reused cluster, but does not rebuild the 
> reused active cluster. that does't work. (eg: reused_active_cluster.png)
>
> That's why I want to make a tool has more functions.
>
> qcow2-dump has some improvements compare with qemu-img check command:
>
> 1. -m check: output the status of qcow2 image and statistics of metadata. 
> (eg: rebuild_refcount_table.png)
>
> 2. -m error: output of -m check + the damaged positions of qcow2 image. (eg: 
> reused_active_cluster.png & error.tgz)
>
> 3. -m dump: output of -m error + dump all metadata of qcow2 image. (eg: 
> dump.tgz)
>
>
> qcow2-dump also has some functions for repairing damaged qcow2 image:
>
> 1. rebuild damaged refcount table. (eg: rebuild_refcount_table.png)
>qcow2-dump -R all rebuild_refcount_table.qcow2
>
> 2. rebuild reused active cluster. (eg: reused_active_cluster.png)
>qcow2-dump -R all -r reused_active_cluster.qcow2
>
> 3. delete damaged qcow2 snapshot header. (eg: invalid_snapshot_header.png)
>qcow2-dump -D snapshot[0] vm-disk-2.qcow2
>
> 4. -m edit / -m copy: for repairing some special damage.  (eg: comments of 
> qcow2_edit_modify is a instance)
>
> ...
>
> But this tool is not good enough yet, hope to receive your advices to improve 
> it.
>
> Best wishes
> youplus

[Qemu-devel] [PATCH] hw/s390x/ipl: The s390-ipl device is not hot-pluggable

[Thomas Huth]

The s390-ipl device can not be created by the user, since it is meant only
to  be instantiated once internally to load the ROMs and kernel. If the user
tries to do a "device_add s390-ipl" via the monitor later, QEMU aborts with
a "ROM images must be loaded at startup" error message.

Signed-off-by: Thomas Huth 
---
 hw/s390x/ipl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index cc36003..0d06fc1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -442,6 +442,8 @@ static void s390_ipl_class_init(ObjectClass *klass, void 
*data)
 dc->reset = s390_ipl_reset;
 dc->vmsd = _ipl;
 set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+/* Reason: Loads the ROMs and thus can only be used one time - internally 
*/
+dc->user_creatable = false;
 }
 
 static const TypeInfo s390_ipl_info = {
-- 
1.8.3.1

[Qemu-devel] Crash when deleting the diag288 watchdog

[Thomas Huth]

 Hi,

I recently noticed that QEMU abort()s if you try to remove the diag288
watchdog. For example:

$ qemu-system-s390x -nographic -nodefaults -S -monitor stdio
QEMU 2.9.92 monitor - type 'help' for more information
(qemu) device_add diag288,id=x
(qemu) device_del x
**
ERROR:/home/thuth/devel/qemu/qdev-monitor.c:872:qdev_unplug: assertion
failed: (hotplug_ctrl)
Aborted (core dumped)

This is ugly, can we fix this somehow? For example, should the diag288
device be hot-pluggable at all, or can it only be used via the
"-watchdog" parameter instead? In the latter case, we could simply mark
the device with "user_creatable = false", I guess?

 Thomas

Re: [Qemu-devel] [PATCH qemu v4 1/3] spapr_iommu: Realloc guest visible TCE table when hot(un)plugging vfio-pci

[Alexey Kardashevskiy]

On 09/08/17 17:33, David Gibson wrote:
> On Mon, Jul 24, 2017 at 08:48:45PM +1000, Alexey Kardashevskiy wrote:
>> On 24/07/17 15:53, David Gibson wrote:
>>> On Thu, Jul 20, 2017 at 05:22:29PM +1000, Alexey Kardashevskiy wrote:
 This replaces g_malloc() with spapr_tce_alloc_table() as this is
 the standard way of allocating tables and this allows moving the table
 back to KVM when unplugging a VFIO PCI device and VFIO TCE acceleration
 support is not present in the KVM.
>>>
>>> So, I like the idea here, and I think it will work for now, but one
>>> thing concerns me.
>>>
>>> AIUI, your future plans include allowing in-kernel accelerated TCE
>>> tables even with VFIO devices.  When that happens, we might have an
>>> in-kernel table both before and after a change in the "need_vfio"
>>> state.
>>
>> The in-kernel table just stays there, mappings will be replayed but in the
>> end of the replay only the hardware table will change.
>>
>>>
>>> But you won't be able to have two in-kernel tables allocated at once,
>>> whereas this code relies on having both the old and new tables at once
>>> so it can copy the contents over.
>>>
>>> How do you intend to handle that?
>>
>> I intend to make this function no-op when cap_spapr_vfio is present.
>>
>>
>>>
 Although spapr_tce_alloc_table() is expected to fail with EBUSY
 if called when previous fd is not closed yet, in practice we will not
 see it because cap_spapr_vfio is false at the moment.
>>>
>>> I don't follow this.  How would cap_spapr_vfio be changing at any
>>> point?
>>
>> It depends on the version of the host kernel.
> 
> Ok.  I'm still a bit dubious about the line of reasoning here, but the
> patch is correct for now, so we just have to make sure subsequent
> changes work with it.
> 
> Applied to ppc-for-2.11.

Thanks, what about the other two (2/3, 3/3)?



-- 
Alexey



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 0/4] i386/kvm: advertise Hyper-V frequency MSRs

[Konrad Rzeszutek Wilk]

On Tue, Aug 08, 2017 at 09:50:53PM -0300, Marcelo Tosatti wrote:
> On Mon, Aug 07, 2017 at 10:56:59AM +0200, Ladi Prosek wrote:
> > This is the QEMU part of the changes required for nested Hyper-V to read
> > timestamps with RDTSC + TSC page. Without exposing the frequency MSRs,
> > Windows with the Hyper-V role enabled use the much slower
> > HV_X64_MSR_TIME_REF_COUNT (0x4020) RDMSR to read timestamps.
> > 
> > The new registers are exposed only if the TSC frequency is stable across
> > migration and known, as suggested by Paolo.
> > 
> > v1->v2:
> > * deleted an extra empty line in patch 1
> > * added patch 3 introducing a helper function for the "TSC is stable and
> >   known" check (David)
> > 
> > Ladi Prosek (4):
> >   i386/kvm: use a switch statement for MSR detection
> >   i386/kvm: set tsc_khz before configuring Hyper-V CPUID
> >   i386/kvm: introduce tsc_is_stable_and_known()
> >   i386/kvm: advertise Hyper-V frequency MSRs
> > 
> >  target/i386/kvm.c | 138 
> > --
> >  1 file changed, 71 insertions(+), 67 deletions(-)
> > 
> > -- 
> > 2.9.3
> 
> 
> Signed-off-by: Marcelo Tosatti 

Come again please?

> 

[Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0

[Dou Liyang]

Currently, Using the fisrt node without memory on the machine makes
QEMU unhappy. With this example command line:
  ... \
  -m 1024M,slots=4,maxmem=32G \
  -numa node,nodeid=0 \
  -numa node,mem=1024M,nodeid=1 \
  -numa node,nodeid=2 \
  -numa node,nodeid=3 \
Guest reports "No NUMA configuration found" and the NUMA topology is
wrong.

This is because when QEMU builds ACPI SRAT, it regards node0 as the
default node to deal with the memory hole(640K-1M). this means the
node0 must have some memory(>1M), but, actually it can have no
memory.

Fix this problem by replace the node0 with the first node which has
memory on it. Add a new function for each node. Also do some cleanup.

Signed-off-by: Dou Liyang 
---
V3 --> V2
  -Modify the title
V2 --> V1:
  -Fix a coding style problem
Replace
for (node = 0;
node < pcms->numa_nodes && pcms->node_mem[node] == 0;
node++);

with
for (node = 0; node < pcms->numa_nodes; node++) {
   if (pcms->node_mem[node] != 0) {
break;
 }

 hw/i386/acpi-build.c | 78 +---
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424..f93d712 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
  (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
 }
 
+static uint64_t
+build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
+int i, uint64_t mem_base, uint64_t mem_len)
+{
+AcpiSratMemoryAffinity *numamem;
+uint64_t next_base;
+
+next_base = mem_base + mem_len;
+
+/* Cut out the ACPI_PCI hole */
+if (mem_base <= pcms->below_4g_mem_size &&
+next_base > pcms->below_4g_mem_size) {
+mem_len -= next_base - pcms->below_4g_mem_size;
+if (mem_len > 0) {
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, mem_base, mem_len, i,
+  MEM_AFFINITY_ENABLED);
+}
+mem_base = 1ULL << 32;
+mem_len = next_base - pcms->below_4g_mem_size;
+next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+}
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, mem_base, mem_len, i,
+  MEM_AFFINITY_ENABLED);
+return next_base;
+}
+
 static void
 build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 {
 AcpiSystemResourceAffinityTable *srat;
 AcpiSratMemoryAffinity *numamem;
 
-int i;
+int i, node;
 int srat_start, numa_start, slots;
-uint64_t mem_len, mem_base, next_base;
+uint64_t mem_len, mem_base;
 MachineClass *mc = MACHINE_GET_CLASS(machine);
 const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
 PCMachineState *pcms = PC_MACHINE(machine);
@@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
MachineState *machine)
 /* the memory map is a bit tricky, it contains at least one hole
  * from 640k-1M and possibly another one from 3.5G-4G.
  */
-next_base = 0;
+
 numa_start = table_data->len;
 
-numamem = acpi_data_push(table_data, sizeof *numamem);
-build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
-next_base = 1024 * 1024;
-for (i = 1; i < pcms->numa_nodes + 1; ++i) {
-mem_base = next_base;
-mem_len = pcms->node_mem[i - 1];
-if (i == 1) {
-mem_len -= 1024 * 1024;
+/* get the first node which has memory and map the hole from 640K-1M */
+for (node = 0; node < pcms->numa_nodes; node++) {
+if (pcms->node_mem[node] != 0) {
+break;
 }
-next_base = mem_base + mem_len;
-
-/* Cut out the ACPI_PCI hole */
-if (mem_base <= pcms->below_4g_mem_size &&
-next_base > pcms->below_4g_mem_size) {
-mem_len -= next_base - pcms->below_4g_mem_size;
-if (mem_len > 0) {
-numamem = acpi_data_push(table_data, sizeof *numamem);
-build_srat_memory(numamem, mem_base, mem_len, i - 1,
-  MEM_AFFINITY_ENABLED);
-}
-mem_base = 1ULL << 32;
-mem_len = next_base - pcms->below_4g_mem_size;
-next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+}
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
+
+/* map the rest of memory from 1M */
+mem_base = 1024 * 1024;
+mem_len = pcms->node_mem[node] - mem_base;
+mem_base = build_srat_node_entry(table_data, pcms, node,
+mem_base, mem_len);
+
+for (i = 0; i < pcms->numa_nodes; i++) {
+if (i == node) {
+

[Qemu-devel] [PATCH v2] hw/acpi: Select an node with memory for mapping memory hole to

[Dou Liyang]

Currently, Using the fisrt node without memory on the machine makes
QEMU unhappy. With this example command line:
  ... \
  -m 1024M,slots=4,maxmem=32G \
  -numa node,nodeid=0 \
  -numa node,mem=1024M,nodeid=1 \
  -numa node,nodeid=2 \
  -numa node,nodeid=3 \
Guest reports "No NUMA configuration found" and the NUMA topology is
wrong.

This is because when QEMU builds ACPI SRAT, it regards node0 as the
default node to deal with the memory hole(640K-1M). this means the
node0 must have some memory(>1M), but, actually it can have no
memory.

Fix this problem by replace the node0 with the first node which has
memory on it. Add a new function for each node. Also do some cleanup.

Signed-off-by: Dou Liyang 
---
V2 --> V1:
  -Fix a coding style problem
Replace
for (node = 0;
node < pcms->numa_nodes && pcms->node_mem[node] == 0;
node++);

with
for (node = 0; node < pcms->numa_nodes; node++) {
   if (pcms->node_mem[node] != 0) {
break;
 }

 hw/i386/acpi-build.c | 78 +---
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424..f93d712 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
  (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
 }
 
+static uint64_t
+build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
+int i, uint64_t mem_base, uint64_t mem_len)
+{
+AcpiSratMemoryAffinity *numamem;
+uint64_t next_base;
+
+next_base = mem_base + mem_len;
+
+/* Cut out the ACPI_PCI hole */
+if (mem_base <= pcms->below_4g_mem_size &&
+next_base > pcms->below_4g_mem_size) {
+mem_len -= next_base - pcms->below_4g_mem_size;
+if (mem_len > 0) {
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, mem_base, mem_len, i,
+  MEM_AFFINITY_ENABLED);
+}
+mem_base = 1ULL << 32;
+mem_len = next_base - pcms->below_4g_mem_size;
+next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+}
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, mem_base, mem_len, i,
+  MEM_AFFINITY_ENABLED);
+return next_base;
+}
+
 static void
 build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 {
 AcpiSystemResourceAffinityTable *srat;
 AcpiSratMemoryAffinity *numamem;
 
-int i;
+int i, node;
 int srat_start, numa_start, slots;
-uint64_t mem_len, mem_base, next_base;
+uint64_t mem_len, mem_base;
 MachineClass *mc = MACHINE_GET_CLASS(machine);
 const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
 PCMachineState *pcms = PC_MACHINE(machine);
@@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
MachineState *machine)
 /* the memory map is a bit tricky, it contains at least one hole
  * from 640k-1M and possibly another one from 3.5G-4G.
  */
-next_base = 0;
+
 numa_start = table_data->len;
 
-numamem = acpi_data_push(table_data, sizeof *numamem);
-build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
-next_base = 1024 * 1024;
-for (i = 1; i < pcms->numa_nodes + 1; ++i) {
-mem_base = next_base;
-mem_len = pcms->node_mem[i - 1];
-if (i == 1) {
-mem_len -= 1024 * 1024;
+/* get the first node which has memory and map the hole from 640K-1M */
+for (node = 0; node < pcms->numa_nodes; node++) {
+if (pcms->node_mem[node] != 0) {
+break;
 }
-next_base = mem_base + mem_len;
-
-/* Cut out the ACPI_PCI hole */
-if (mem_base <= pcms->below_4g_mem_size &&
-next_base > pcms->below_4g_mem_size) {
-mem_len -= next_base - pcms->below_4g_mem_size;
-if (mem_len > 0) {
-numamem = acpi_data_push(table_data, sizeof *numamem);
-build_srat_memory(numamem, mem_base, mem_len, i - 1,
-  MEM_AFFINITY_ENABLED);
-}
-mem_base = 1ULL << 32;
-mem_len = next_base - pcms->below_4g_mem_size;
-next_base += (1ULL << 32) - pcms->below_4g_mem_size;
+}
+numamem = acpi_data_push(table_data, sizeof *numamem);
+build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
+
+/* map the rest of memory from 1M */
+mem_base = 1024 * 1024;
+mem_len = pcms->node_mem[node] - mem_base;
+mem_base = build_srat_node_entry(table_data, pcms, node,
+mem_base, mem_len);
+
+for (i = 0; i < pcms->numa_nodes; i++) {
+if (i == node) {
+continue;
 }
-

Re: [Qemu-devel] [PATCH for-2.10] mmio-interface: Mark as not user creatable

[Edgar E. Iglesias]

On Tue, Aug 15, 2017 at 06:16:22PM +0200, Thomas Huth wrote:
> On 15.08.2017 16:30, Peter Maydell wrote:
> > The mmio-interface device is not something we want to allow
> > users to create on the command line:
> >  * it is intended as an implementation detail of the memory
> >subsystem, which gets created and deleted by that
> >subsystem on demand; it makes no sense to create it
> >by hand on the command line
> >  * it uses a pointer property 'host_ptr' which can't be
> >set on the command line
> > 
> > Mark the device as not user_creatable to avoid confusion.
> > 
> > Signed-off-by: Peter Maydell 
> > ---
> >  hw/misc/mmio_interface.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
> > index da154e5..894e980 100644
> > --- a/hw/misc/mmio_interface.c
> > +++ b/hw/misc/mmio_interface.c
> > @@ -111,6 +111,11 @@ static void mmio_interface_class_init(ObjectClass *oc, 
> > void *data)
> >  dc->realize = mmio_interface_realize;
> >  dc->unrealize = mmio_interface_unrealize;
> >  dc->props = mmio_interface_properties;
> > +/* Reason: pointer property "host_ptr", and this device
> > + * is an implementation detail of the memory subsystem,
> > + * not intended to be created directly by the user.
> > + */
> > +dc->user_creatable = false;
> >  }
> >  
> >  static const TypeInfo mmio_interface_info = {
> > 
> 
> Reviewed-by: Thomas Huth 

Reviewed-by: Edgar E. Iglesias 

Re: [Qemu-devel] Help with Windows NT 4.0

[Paolo Bonzini]

On 15/08/2017 20:46, Programmingkid wrote:
> 
>> On Aug 14, 2017, at 2:51 AM, Paolo Bonzini  wrote:
>>
>> On 13/08/2017 21:13, Programmingkid wrote:
>>> Lately I found out that Windows NT 4.0 seems to work well with the
>>> 486 and pentium processors. Using "-cpu 486" made installing it
>>> actually work. Now I am seeing another issue. When I boot Windows NT
>>> 4.0 I see this error message:
>>>
>>> *** STOP: 0x007B (0x807A8610,0x,0x,0x) 
>>> INACESSIBLE_BOOT_DEVICE
>>>
>>> Would anyone know a way to solve this issue?
>>
>> Hervé is probably the best person to answer this question.  Maybe try
>> installing it with SCSI disks ("-drive if=scsi,id=hd,file=... -drive
>> if=scsi,id=cd,file=... -device lsi -device scsi-hd,drive=hd -device
>> scsi-cd,drive=cd").
>>
>> Thanks,
>>
>> Paolo
> 
> Thanks for the help. Unfortunately trying to boot from the install CD leads 
> to the INACCESSIBLE_BOOT_DEVICE error when using SCSI. 

Try with 0.12.

Paolo

Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command

[Eric Blake]

On 08/15/2017 02:45 AM, Manos Pitsidianakis wrote:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
> 
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
> 
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  block.c|  192 
>  blockdev.c |   44 ++
>  include/block/block.h  |6 +
>  qapi/block-core.json   |   60 +++
>  tests/qemu-iotests/193 |  241 ++
>  tests/qemu-iotests/193.out | 1116 
> 
>  tests/qemu-iotests/group   |1 +
>  7 files changed, 1660 insertions(+)
>  create mode 100755 tests/qemu-iotests/193
>  create mode 100644 tests/qemu-iotests/193.out

You may want to look at using scripts/git.orderfile, to rearrange your
patch so that interface changes (.json, .h) occur before implementation
(.c).  For now, I'm just focusing on the interface:


> +++ b/qapi/block-core.json
> @@ -3947,3 +3947,63 @@
>'data' : { 'parent': 'str',
>   '*child': 'str',
>   '*node': 'str' } }
> +
> +##
> +# @block-insert-node:
> +#
> +# Insert a filter node between a specific edge in the block driver state 
> graph.
> +# @parent:  the name of the parent node or device
> +# @node:the name of the node to insert under parent
> +# @child:   the name of the child of both node and parent

Is this always going to be between two existing nodes, or can this
command also be used to insert at the end of the chain (for example, if
parent or child is omitted)?


> +#}
> +# <- { 'return': {} }
> +#
> +##

Missing 'Since: 2.11'.

> +{ 'command': 'block-insert-node',
> +  'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }

For now, it looks like you require all arguments, and therefore this is
always insertion in the middle.

> +##
> +# @block-remove-node:
> +#
> +# Remove a filter node between two other nodes in the block driver state 
> graph.
> +# @parent:  the name of the parent node or device
> +# @node:the name of the node to remove from parent
> +# @child:   the name of the child of node which will go under parent
> +##
> +{ 'command': 'block-remove-node',
> +  'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }

Likewise missing 2.11.

Overall I'm not seeing problems with the interface from the UI
perspective, but I have not been paying close attention to your larger
efforts on throttling nodes, so I hope other reviewers will chime in.

-- 
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 v3 1/1] PPC: KVM: Support machine option to set VSMT mode

[Greg Kurz]

On Tue, 15 Aug 2017 14:42:21 +1000
Sam Bobroff  wrote:

> KVM now allows writing to KVM_CAP_PPC_SMT which has previously been
> read only. Doing so causes KVM to act, for that VM, as if the host's
> SMT mode was the given value. This is particularly important on Power
> 9 systems because their default value is 1, but they are able to
> support values up to 8.
> 
> This patch introduces a way to control this capability via a new
> machine property called VSMT ("Virtual SMT"). If the value is not set
> on the command line a default is chosen that is, when possible,
> compatible with legacy systems.
> 
> Note that the intialization of KVM_CAP_PPC_SMT has changed slightly
> because it has changed (in KVM) from a global capability to a
> VM-specific one. This won't cause a problem on older KVMs because VM
> capabilities fall back to global ones.
> 
> Signed-off-by: Sam Bobroff 
> ---

Reviewed-by: Greg Kurz 

> Changes in v3:
> 
> * Suggested by Greg Kurz:
> * Comment on show_vsmt_possible() converted in to an assert().
> * Better use of g_string_new().
> * Removed trailing periods from several error_report() strings.
> * Shortened overly long line.
> * Converted some helpful error_report() calls into error_printf().
> * Simplified error handling in spapr_set_vsmt().
> * Use error_abort when adding properties (instead of ignoring errors), as 
> it's a bug if it fails.
> * Moved the explanation about KVM_CAP_PPC_SMT into the commit so that 
> it's not lost with the cover letter.
> 
>  hw/ppc/spapr.c  | 96 
> +
>  include/hw/ppc/spapr.h  |  1 +
>  target/ppc/kvm.c| 20 +-
>  target/ppc/kvm_ppc.h| 12 ++
>  target/ppc/translate_init.c | 14 ---
>  5 files changed, 128 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cd6eb2d4a9..e266cdd36a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -26,6 +26,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/numa.h"
>  #include "hw/hw.h"
> @@ -2140,6 +2141,82 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>  g_free(type);
>  }
>  
> +static void show_vsmt_possible(void)
> +{
> +int i;
> +GString *g;
> +char *s;
> +
> +assert(kvm_enabled());
> +if (kvmppc_smt_possible()) {
> +g = g_string_new("Available VSMT modes:");
> +for (i = 63; i >= 0; i--) {
> +if ((1UL << i) & kvmppc_smt_possible()) {
> +g_string_append_printf(g, " %lu", (1UL << i));
> +}
> +}
> +s = g_string_free(g, false);
> +error_printf("%s.\n", s);
> +g_free(s);
> +} else {
> +error_printf("This KVM seems to be too old to support VSMT.\n");
> +}
> +}
> +
> +static void spapr_set_vsmt_mode(sPAPRMachineState *spapr)
> +{
> +bool vsmt_user = !!spapr->vsmt;
> +int kvm_smt = kvmppc_smt_threads();
> +int ret;
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +const char *machine_name = object_class_get_name(OBJECT_CLASS(smc));
> +
> +if (!kvm_enabled() && (smp_threads > 1)) {
> +error_report("TCG cannot support more than 1 thread/core "
> + "on a %s", machine_name);
> +exit(1);
> +}
> +if (!is_power_of_2(smp_threads)) {
> +error_report("Cannot support %d threads/core on a %s because "
> + "it must be a power of 2", smp_threads, machine_name);
> +exit(1);
> +}
> +
> +/* Detemine the VSMT mode to use: */
> +if (vsmt_user) {
> +if (spapr->vsmt < smp_threads) {
> +error_report("Cannot support VSMT mode %d"
> + " because it must be >= threads/core (%d)",
> + spapr->vsmt, smp_threads);
> +exit(1);
> +}
> +/* In this case, spapr->vsmt has been set by the command line */
> +} else {
> +/* Choose a VSMT mode that may be higher than necessary but is
> + * likely to be compatible with hosts that don't have VSMT. */
> +spapr->vsmt = MAX(kvm_smt, smp_threads);
> +}
> +
> +/* KVM: If necessary, set the SMT mode: */
> +if (kvm_enabled() && (spapr->vsmt != kvm_smt)) {
> +ret = kvmppc_set_smt_threads(spapr->vsmt);
> +if (ret) {
> +error_report("Failed to set KVM's VSMT mode to %d (errno %d)",
> + spapr->vsmt, ret);
> +if (!vsmt_user) {
> +error_printf("On PPC, a VM with %d threads/core on a host"
> + " with %d threads/core requires the use of"
> + " VSMT mode %d.\n",
> + smp_threads, kvm_smt, spapr->vsmt);
> +}
> +

[Qemu-devel] [ANNOUNCE] QEMU 2.10.0-rc3 is now available

[Michael Roth]

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
fourth release candidate for the QEMU 2.10 release.  This release is meant
for testing purposes and should not be used in a production environment.

  http://download.qemu-project.org/qemu-2.10.0-rc3.tar.xz
  http://download.qemu-project.org/qemu-2.10.0-rc3.tar.xz.sig

A note from the maintainer:

  We don't currently know of any outstanding release critical bugs,
  so unless any last minute showstoppers appear next week, we hope
  to be able to release 2.10.0 on schedule on the 22nd. If any
  further release critical bugs are fixed we'll release an rc4 on
  the 22nd and the final release a week after that.

You can help improve the quality of the QEMU 2.10 release by testing this
release and reporting bugs on Launchpad:

  https://bugs.launchpad.net/qemu/

The release plan, as well a documented known issues for release
candidates, are available at:

  http://wiki.qemu.org/Planning/2.10

Please add entries to the ChangeLog for the 2.10 release below:

  http://wiki.qemu.org/ChangeLog/2.10

Changes since rc2:

1f29673: Update version for v2.10.0-rc3 release (Peter Maydell)
72b384f: mmio-interface: Mark as not user creatable (Peter Maydell)
4a2fdb7: target/arm: Require alignment for load exclusive (Alistair Francis)
19514cd: target/arm: Correct load exclusive pair atomicity (Richard Henderson)
955fd0a: target/arm: Correct exclusive store cmpxchg memop mask (Alistair 
Francis)
72b6ffc: nbd-client: Fix regression when server sends garbage (Eric Blake)
dd7fdaa: iotests: Add non-shared storage migration case 192 (Fam Zheng)
5f7772c: block-backend: Defer shared_perm tightening migration completion (Fam 
Zheng)
3dff24f: nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache (Kevin Wolf)
80adf54: stubs: Add vm state change handler stubs (Fam Zheng)
cbaddb2: qemu-iotests: step clock after each test iteration (Stefan Hajnoczi)
dad3946: nbd: Fix trace message for disconnect (Eric Blake)
47025a0: qxl: call qemu_spice_display_init_common for secondary devices (Paolo 
Bonzini)
840d835: simpletrace: fix flight recorder --no-header option (Stefan Hajnoczi)
d6b76d6: trace: use static event ID mapping in simpletrace.stp (Stefan Hajnoczi)
a8132a2: docker: add centos7 image (Philippe Mathieu-Daudé)
06f3c7b: docker: install more packages on CentOS to extend code coverage 
(Philippe Mathieu-Daudé)
18a2b7a: docker: add Xen libs to centos6 image (Philippe Mathieu-Daudé)
6aef2ad: docker: use one package per line in CentOS config (Philippe 
Mathieu-Daudé)
e45eaef: Makefile: Let "make check-help" work without running ./configure (Fam 
Zheng)
88b3739: pc-bios/s390-ccw: Use rm command during make clean (Eric Farman)
83c3a1f: xlnx-qspi: add a property for mmio-execution (KONRAD Frederic)
bd7adc8: qemu-doc: Mention host_net_add/-remove in the deprecation chapter 
(Thomas Huth)
a808c08: hw/misc/mmio_interface: Return after error_setg() to avoid crash 
(Thomas Huth)
a3e08c2: qemu-iotests: remove comment about root privileges requirement (Cleber 
Rosa)
80758ec: qemu-iotests: remove commented out variables (Cleber Rosa)
657c572: qemu-iotests: get rid of _full_imgproto_details() (Cleber Rosa)
88552b5: qemu-doc: Fix "-net van" typo (Thomas Huth)
e8ec011: libqtest: Fix typo in comments (Eric Blake)
0ac241b: unicore32: abort when entering "x 0" on the monitor (Eduardo Otubo)
6900191: qemu-doc: Fix "-net van" typo (Thomas Huth)
480bc11: boot-serial-test: fallback to kvm accelerator (Cornelia Huck)
8565c3a: qemu-iotests: fix 185 (Vladimir Sementsov-Ogievskiy)
2b218f5: file-posix: Do runtime check for ofd lock API (Fam Zheng)
ca74995: osdep: Add runtime OFD lock detection (Fam Zheng)
d0d5d0e: qcow2: Check failure of bdrv_getlength() (Eric Blake)
c40fe9c: qcow2: Drop debugging dump_refcounts() (Eric Blake)
81caa3c: vpc: Check failure of bdrv_getlength() (Eric Blake)
01a02ec: tests/multiboot: Fix whitespace failure (Eric Blake)
17d0bc0: virtio-blk: handle blk_getlength() errors (Stefan Hajnoczi)
ce317e8: IDE: test flush on empty CDROM (Kevin Wolf)
4da9712: IDE: Do not flush empty CDROM drives (Stefan Hajnoczi)
4751fd5: 9pfs: local: fix fchmodat_nofollow() limitations (Greg Kurz)
f57467e: spapr: Fix bug in h_signal_sys_reset() (Sam Bobroff)
325837c: spapr_drc: abort if object_property_add_child() fails (Greg Kurz)
b8af5b2: target/ppc: Add stub implementation of the PSSCR (David Gibson)
650f328: target/ppc: Implement TIDR (David Gibson)
e7bab9a: ppc: fix double-free in cpu_post_load() (Greg Kurz)
89fca22: booke206: fix MAS update on tlb miss (KONRAD Frederic)
24dd1e1: libqtest: always set up signal handler for SIGABRT (Jens Freimann)
2566378: libvhost-user: quit when no more data received (Jens Freimann)
0f8c289: net: fix -netdev socket,fd= for UDP sockets (Jens Freimann)
cde0a63: Revert "cpu: add APIs to allocate/free CPU environment" (Michael S. 
Tsirkin)
114e0af: acpi-test: update expected DSDT files (Michael S. Tsirkin)

Re: [Qemu-devel] [PATCH for-2.11 v2 4/5] qmp-shell: Accept QMP command as argument

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 12:03:53PM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> Suggest to insert here:
> 
>   If additional arguments QMP-COMMAND ARG=VAL... are given, run just
>   that QMP command instead of the REPL.
> 
> Question: is this limited to simple arguments?  If no, how would I write
> an object argument?  For instance, how would I do
> 
> { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": 
> "nbd", "server": { "type": "inet", "host": "localhost", "port": "12345" } } }
> 
> ?

Exactly the same way you would write it when running qmp-shell in
interactive mode.  e.g.:

  $ ./scripts/qmp/qmp-shell /tmp/qmp blockdev-add driver=qcow2 node-name=node-E 
'file={"driver":"file","filename":"/path/to/file.qcow2"}'


> 
> > This is useful for testing QMP commands in scripts.
> >
> > Example usage, combined with 'jq' for filtering the results:
> >
> >   $ ./scripts/qmp/qmp-shell /tmp/qmp qom-list path=/ | jq -r .return[].name
> >   machine
> >   type
> >   chardevs
> >   backend
> 
> What's jq?

https://stedolan.github.io/jq/

"like sed for JSON data"

> 
> >   $
> 
> Let's drop this line.

Will do it.

> 
> >
> > Signed-off-by: Eduardo Habkost 
> > ---
> > Changes v1 -> v2:
> > * Rewritten using optparse module
> > ---
> >  scripts/qmp/qmp-shell | 14 +-
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> > index 4b9a420..4b7374e 100755
> > --- a/scripts/qmp/qmp-shell
> > +++ b/scripts/qmp/qmp-shell
> > @@ -400,7 +400,7 @@ def die(msg):
> >  
> >  def main():
> >  parser = optparse.OptionParser(description='QMP shell utility')
> > -parser.set_usage("%prog [options]  |  > address:port>")
> > +parser.set_usage("%prog [options]  |  > address:port> [COMMAND [ARG=VALUE]...]")
> >  parser.add_option('-v', action='store_true', dest='verbose',
> >  help='Verbose (echo command sent and received)')
> >  parser.add_option('-p', action='store_true', dest='pretty',
> > @@ -411,10 +411,11 @@ def main():
> >  default=True, help='Skip negotiate (for qemu-ga)')
> >  opts,args = parser.parse_args()
> >  
> > -if len(args) != 1:
> > +if len(args) < 1:
> >  parser.print_help(sys.stderr)
> >  sys.exit(1)
> >  addr = args[0]
> > +cmdargs = args[1:]
> >  
> >  try:
> >  if opts.hmp:
> > @@ -433,10 +434,13 @@ def main():
> >  except qemu.error:
> >  die('Could not connect to %s' % addr)
> >  
> > -qemu.show_banner()
> >  qemu.set_verbosity(opts.verbose)
> > -while qemu.read_exec_command(qemu.get_prompt()):
> > -pass
> > +if len(cmdargs):
> 
> Superfluous len().

Will fix it in the next version.

> 
> > +qemu.execute_cmdargs(cmdargs)
> > +else:
> > +qemu.show_banner()
> > +while qemu.read_exec_command(qemu.get_prompt()):
> > +pass
> >  qemu.close()
> >  
> >  if __name__ == '__main__':

-- 
Eduardo

[Qemu-devel] [PATCH] hw/ppc: disable hotplug before CAS is completed

[Daniel Henrique Barboza]

This patch is a follow up on the discussions that started with
Laurent's patch series "spapr: disable hotplugging without OS" [1]
and discussions made at patch "spapr: reset DRCs on migration
pre_load" [2].

At this moment, we do not support CPU/memory hotplug in early
boot stages, before CAS. The reason is that the hotplug event
can't be handled at SLOF level (or even in PRELAUNCH runstate) and
at the same time can't be canceled. This leads to devices being
unable to be hot unplugged and, in some cases, guest kernel Ooops.
After CAS, with the FDT in place, the guest can handle the hotplug
events and everything works as usual.

An attempt to try to support hotplug before CAS was made, but not
successful. The key difference in the current code flow between a
coldplugged and a hotplugged device, in the PRELAUNCH state, is that
the coldplugged device is registered at the base FDT, allowing its
DRC to go straight to CONFIGURED state. In theory, this can also be
done with a hotplugged device if we can add it to the base of the
existing FDT. However, tampering with the FDT after writing in the
guest memory, besides being a dubitable idea, is also not
possible. The FDT is written in ppc_spapr_reset and there is no
way to retrieve it - we can calculate the fdt_address but the
fdt_size isn't stored. Storing the fdt_size to allow for
later retrieval is yet another state that would need to be
migrated. In short, it is not worth the trouble.

All this said, this patch opted to disable CPU/mem hotplug at early
boot stages. CAS detection is made by checking if there are
any bits set in ov5_cas to avoid adding an extra state that
would need tracking/migration. The patch also makes sure that
it doesn't interfere with hotplug in the INMIGRATE state.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05226.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01989.html

Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr.c  | 26 ++
 hw/ppc/spapr_ovec.c |  7 +++
 include/hw/ppc/spapr_ovec.h |  1 +
 3 files changed, 34 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a1972..bdcc813 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2803,6 +2803,19 @@ out:
 error_propagate(errp, local_err);
 }
 
+/*
+ * 'h_client_architecture_support' will set at least OV5_FORM1_AFFINITY
+ * in ov5_cas when intersecting it with spapr->ov5 and ov5_guest. It's safe
+ * then to assume that CAS ov5_cas will have something set after CAS.
+ */
+static bool spapr_cas_completed(sPAPRMachineState *spapr)
+{
+if (spapr->ov5_cas == NULL) {
+return false;
+}
+return !spapr_ovec_is_unset(spapr->ov5_cas);
+}
+
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState 
*dev,
   Error **errp)
 {
@@ -3256,6 +3269,19 @@ static void 
spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
   DeviceState *dev, Error **errp)
 {
+sPAPRMachineState *spapr;
+Error *local_err = NULL;
+
+if (dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE)) {
+spapr = SPAPR_MACHINE(hotplug_dev);
+if (!spapr_cas_completed(spapr)) {
+error_setg(_err,
+   "CPU/memory hotplug not supported at early boot");
+error_propagate(errp, local_err);
+return;
+}
+}
+
 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
 spapr_memory_pre_plug(hotplug_dev, dev, errp);
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index 41df4c3..fe7bc85 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -134,6 +134,13 @@ bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr)
 return test_bit(bitnr, ov->bitmap) ? true : false;
 }
 
+bool spapr_ovec_is_unset(sPAPROptionVector *ov)
+{
+unsigned long lastbit;
+lastbit = find_last_bit(ov->bitmap, OV_MAXBITS);
+return (lastbit == OV_MAXBITS);
+}
+
 static void guest_byte_to_bitmap(uint8_t entry, unsigned long *bitmap,
  long bitmap_offset)
 {
diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
index 9edfa5f..8126374 100644
--- a/include/hw/ppc/spapr_ovec.h
+++ b/include/hw/ppc/spapr_ovec.h
@@ -71,6 +71,7 @@ void spapr_ovec_cleanup(sPAPROptionVector *ov);
 void spapr_ovec_set(sPAPROptionVector *ov, long bitnr);
 void spapr_ovec_clear(sPAPROptionVector *ov, long bitnr);
 bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr);
+bool spapr_ovec_is_unset(sPAPROptionVector *ov);
 sPAPROptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int 
vector);
 int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
sPAPROptionVector *ov, const char *name);
-- 

[Qemu-devel] [PATCH v2] net: rtl8139: do not use old_mmio accesses

[Matt Parker]

Both io and memory use the same mmio functions in the rtl8139 device.
This patch removes the separate MemoryRegionOps and old_mmio accessors
for memory, and replaces it with an alias to the io memory region.

Signed-off-by: Matt Parker 
---
 hw/net/rtl8139.c | 53 +++--
 1 file changed, 3 insertions(+), 50 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 671c7e48c6..3be24bbee7 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3132,38 +3132,6 @@ static uint32_t rtl8139_io_readl(void *opaque, uint8_t 
addr)
 
 /* */
 
-static void rtl8139_mmio_writeb(void *opaque, hwaddr addr, uint32_t val)
-{
-rtl8139_io_writeb(opaque, addr & 0xFF, val);
-}
-
-static void rtl8139_mmio_writew(void *opaque, hwaddr addr, uint32_t val)
-{
-rtl8139_io_writew(opaque, addr & 0xFF, val);
-}
-
-static void rtl8139_mmio_writel(void *opaque, hwaddr addr, uint32_t val)
-{
-rtl8139_io_writel(opaque, addr & 0xFF, val);
-}
-
-static uint32_t rtl8139_mmio_readb(void *opaque, hwaddr addr)
-{
-return rtl8139_io_readb(opaque, addr & 0xFF);
-}
-
-static uint32_t rtl8139_mmio_readw(void *opaque, hwaddr addr)
-{
-uint32_t val = rtl8139_io_readw(opaque, addr & 0xFF);
-return val;
-}
-
-static uint32_t rtl8139_mmio_readl(void *opaque, hwaddr addr)
-{
-uint32_t val = rtl8139_io_readl(opaque, addr & 0xFF);
-return val;
-}
-
 static int rtl8139_post_load(void *opaque, int version_id)
 {
 RTL8139State* s = opaque;
@@ -3344,22 +3312,6 @@ static const MemoryRegionOps rtl8139_io_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static const MemoryRegionOps rtl8139_mmio_ops = {
-.old_mmio = {
-.read = {
-rtl8139_mmio_readb,
-rtl8139_mmio_readw,
-rtl8139_mmio_readl,
-},
-.write = {
-rtl8139_mmio_writeb,
-rtl8139_mmio_writew,
-rtl8139_mmio_writel,
-},
-},
-.endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static void rtl8139_timer(void *opaque)
 {
 RTL8139State *s = opaque;
@@ -3422,8 +3374,9 @@ static void pci_rtl8139_realize(PCIDevice *dev, Error 
**errp)
 
 memory_region_init_io(>bar_io, OBJECT(s), _io_ops, s,
   "rtl8139", 0x100);
-memory_region_init_io(>bar_mem, OBJECT(s), _mmio_ops, s,
-  "rtl8139", 0x100);
+memory_region_init_alias(>bar_mem, OBJECT(s), "rtl8139-mem", >bar_io,
+ 0, 0x100);
+
 pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >bar_io);
 pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, >bar_mem);
 
-- 
2.13.2

Re: [Qemu-devel] [PATCH for-2.11 v2 1/5] qmp-shell: Use optparse module

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 10:56:07AM +0100, Daniel P. Berrange wrote:
> On Tue, Aug 15, 2017 at 11:47:28AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost  writes:
> > 
> > > It makes command-line parsing and generation of help text much
> > > simpler.
> > 
> > There's really no excuse for parsing command line arguments by hand in
> > Python.
> > 
> > > The optparse module is deprecated since Python 2.7, but argparse
> > > is not available in Python 2.6 (the minimum Python version
> > > required for building QEMU).
> > 
> > We have a few uses of argparse in the tree.  Are they okay?
> > 
> > We also use getopt in places.  Perhaps we should pick one way to parse
> > command lines and stick to it.
> 
> I just came up against this problem with keycodemapdb which I'm adding as
> a submodule to qemu.  I used argparse there because it is the modern
> recommended API for python >= 2.7.   I examined possibilty of using
> optparse instead, but it lacks key features - in particular the idea
> of 'subcommands' is entirely missing from optparse.
> 
> The 'argparse' module is part of python core, but is *also* available
> as a standalone module that is implemented in terms of optparse.
> 
> So, I would suggest that we just copy the 'argparse' module into the
> QEMU 'scripts/thirdparty' directory.
> 
> Then in any files which need argparse, you can do this
> 
>   try:
>   import argparse
>   except:
>   import os, sys
>   sys.path.append(os.path.join(os.path.dirname(__file__), "thirdparty"))
>   import argparse

What about:

  try:
  import argparse
  except:
  from thirdparty import argparse

(I think we could move all our Python modules [qemu.py, qmp.py]
to ./scripts/qemu/, so this would become "qemu.thirdparty").

> 
> so that it tries to use the argparse provided by python, and falls back
> to pulling in the one in our scripts/thirdparty directory.
> 
> When we finally bump our min python to 2.7, we can simply drop this
> compat code for the import statement.  This avoids need for us to
> re-write code to use a deprecated API, with a worse feature set.

Sounds good to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] tests: switch tests to accel=kvm:tcg

[Philippe Mathieu-Daudé]

On 08/15/2017 12:47 PM, Richard Henderson wrote:

On 08/14/2017 08:33 AM, Cornelia Huck wrote:

On Mon, 14 Aug 2017 17:34:15 +0300
"Michael S. Tsirkin"  wrote:


Speed up tests on host systems with kvm support.
In particular, this fixes tests with --disable-tcg.

Cc: Paolo Bonzini 
Cc: Thomas Huth 
Cc: Laurent Vivier 
Suggested-by: Cornelia Huck 
Signed-off-by: Michael S. Tsirkin 
---

Tested on x86 only.

  tests/boot-serial-test.c | 2 +-
  tests/pnv-xscom-test.c   | 4 ++--
  tests/prom-env-test.c| 2 +-
  3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index 11f48b0..c3b2e4e 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -78,7 +78,7 @@ static void test_machine(const void *data)
  fd = mkstemp(tmpname);
  g_assert(fd != -1);
  
-args = g_strdup_printf("-M %s,accel=tcg -chardev file,id=serial0,path=%s"

+args = g_strdup_printf("-M %s,accel=kvm:tcg -chardev 
file,id=serial0,path=%s"
 " -no-shutdown -serial chardev:serial0 %s",
 test->machine, tmpname, test->extra);


This has already been changed upstream.


Ouch.  This is the only real smoke test we have for the tcg backend for the
host.  While it is still going to test tcg for whatever machines do not run
natively on the host, I can't help think we've lost testing.

Can we use accel=tcg:kvm instead?


can we use the following in this test main()?

/* This test is fast fast enough to use to test the host TCG backend,
 * so use "tcg" as default accel if available. */
#ifdef CONFIG_TCG
static const char *accel = "tcg";
#else
static const char *accel = "kvm";
#endif




r~

Re: [Qemu-devel] [PATCH v6 3/7] qemu.py: use python logging system

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 10:10:12AM +0200, Markus Armbruster wrote:
> Please spell "Python" with a capital "P" (it's a proper name).
> 
> Amador Pahim  writes:
> 
> > Let's provide extra control and flexibility by using python logging
> > system instead of print and/or sys.std*.write().
> >
> > Signed-off-by: Amador Pahim 
> 
> How exactly does this change error messages?
> 
> Is logging the right tool to report errors to the human user?  I'm
> asking because logging and error reporting are generally separate
> things.  Example: a program runs into a recoverable error.  It logs the
> error, but does not report it.
> 
> Is reporting errors to stderr the right thing to do for library class
> QEMUMachine?  I doubt it.  Libraries throw exceptions and let their
> users decide how to handle them.

I believe the "qemu received signal" event is supposed to be
logged, not necessarily reported.  Callers can then choose where
the log messages should go (scripts could choose to send them
directly to stderr if verbose or debugging mode is enabled).  We
don't even need an exception for it: we can let callers check
exitcode() and decide what to do about the QEMU exit code.

The send_fd_scm() messages, on the other hand, could become
exceptions, and don't need the logging system at all.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] tests: switch tests to accel=kvm:tcg

[Thomas Huth]

On 15.08.2017 17:47, Richard Henderson wrote:
> On 08/14/2017 08:33 AM, Cornelia Huck wrote:
>> On Mon, 14 Aug 2017 17:34:15 +0300
>> "Michael S. Tsirkin"  wrote:
>>
>>> Speed up tests on host systems with kvm support.
>>> In particular, this fixes tests with --disable-tcg.
>>>
>>> Cc: Paolo Bonzini 
>>> Cc: Thomas Huth 
>>> Cc: Laurent Vivier 
>>> Suggested-by: Cornelia Huck 
>>> Signed-off-by: Michael S. Tsirkin 
>>> ---
>>>
>>> Tested on x86 only.
>>>
>>>  tests/boot-serial-test.c | 2 +-
>>>  tests/pnv-xscom-test.c   | 4 ++--
>>>  tests/prom-env-test.c| 2 +-
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
>>> index 11f48b0..c3b2e4e 100644
>>> --- a/tests/boot-serial-test.c
>>> +++ b/tests/boot-serial-test.c
>>> @@ -78,7 +78,7 @@ static void test_machine(const void *data)
>>>  fd = mkstemp(tmpname);
>>>  g_assert(fd != -1);
>>>  
>>> -args = g_strdup_printf("-M %s,accel=tcg -chardev 
>>> file,id=serial0,path=%s"
>>> +args = g_strdup_printf("-M %s,accel=kvm:tcg -chardev 
>>> file,id=serial0,path=%s"
>>> " -no-shutdown -serial chardev:serial0 %s",
>>> test->machine, tmpname, test->extra);
>>
>> This has already been changed upstream.
> 
> Ouch.  This is the only real smoke test we have for the tcg backend for the
> host.  While it is still going to test tcg for whatever machines do not run
> natively on the host, I can't help think we've lost testing.

Ah, well, I didn't think of that when I suggested to use "kvm:tcg"
here... I rather thought that kvm would be preferable to speed up the
test. But since the test is already quite fast anyway, it's likely also
OK to use "tcg:kvm" here. (But we should keep "kvm:tcg" in the pxe-test
since that is quite slow with TCG, at least on ppc64).

 Thomas

Re: [Qemu-devel] [PATCH] tests: switch tests to accel=kvm:tcg

[Richard Henderson]

On 08/15/2017 12:28 PM, Michael S. Tsirkin wrote:
>> Ouch.  This is the only real smoke test we have for the tcg backend for the
>> host.  While it is still going to test tcg for whatever machines do not run
>> natively on the host, I can't help think we've lost testing.
>>
>> Can we use accel=tcg:kvm instead?
>>
>>
>> r~
> 
> We can add an environment to disable kvm on qemu side,
> for e.g. CI systems who have spare time.

Hum.  I'm not especially keen on an environment variable that wouldn't be
useful for anything else.  I'd much rather adjust the test case so that plain
"make check" tests all that we need to test.


r~

Re: [Qemu-devel] [RFC v4 00/13] qmp: query-device-slots command

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 01:57:50PM -0500, Eric Blake wrote:
> On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> > Changelog
> > -
> > 
> > Changes v3 -> v4:
> > * New compact representation of slot sets.
> > * New generic code to automatically merge similar slots
> >   into a single entry in the command output while keeping
> >   implementations of the method simpler.
> > * Example implementation of IDE and USB bus enumeration
> 
> > 
> > Slot sets are represented by a list of option names and sets of
> > possible values for each of those options.  The command uses a
> > compact representation for the set of valid values for an option.
> > For example, the following set of 5 PCI functions:
> > 
> >   bus: pcie.0
> >   device-number: 31
> >   functions: 1,4,5,6,7
> > 
> > would be represented in the JSON data as:
> > 
> >   {"available":false,"count":5,
> >"device-types":["pci-device"],"hotpluggable":false,
> >"opts":[
> >   {"option":"function","values":[1,[4,7]]},
> 
> A list (and not just a single-type list, but a list that mixes scalar
> and sublist),
> 
> >   {"option":"device-number","values":31},
> 
> vs. a scalar.  Why not a one-element array?

It was just to keep the representation as compact as possible, in
the common case of single-value sets.  Probably we can drop that
feature as it saves only 2 bytes in the JSON representation.

> 
> >   {"option":"bus","values":"pcie.0"}],
> >"opts-complete":true}
> > 
> > I planned to use QAPI alternates to model/document that in the
> > schema, but it would require implementing a few missing features
> > in QAPI alternate support.
> 
> Yeah, I can see how existing QAPI alternates do not yet support arrays,
> which becomes important to your representation.  Do you need help
> getting the QAPI generator improved to support a particular feature that
> you found to be lacking?

I think the lack of support for lists on alternates was the main
obstacle.

Probably we would also need to remove the restriction against
alternates with ambiguous string representations, to allow a
list/number/string/bool alternate to be defined.

Being able to set constraints on the number of elements of a list
would be nice to have, but not required.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] tests: switch tests to accel=kvm:tcg

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 08:47:35AM -0700, Richard Henderson wrote:
> On 08/14/2017 08:33 AM, Cornelia Huck wrote:
> > On Mon, 14 Aug 2017 17:34:15 +0300
> > "Michael S. Tsirkin"  wrote:
> > 
> >> Speed up tests on host systems with kvm support.
> >> In particular, this fixes tests with --disable-tcg.
> >>
> >> Cc: Paolo Bonzini 
> >> Cc: Thomas Huth 
> >> Cc: Laurent Vivier 
> >> Suggested-by: Cornelia Huck 
> >> Signed-off-by: Michael S. Tsirkin 
> >> ---
> >>
> >> Tested on x86 only.
> >>
> >>  tests/boot-serial-test.c | 2 +-
> >>  tests/pnv-xscom-test.c   | 4 ++--
> >>  tests/prom-env-test.c| 2 +-
> >>  3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> >> index 11f48b0..c3b2e4e 100644
> >> --- a/tests/boot-serial-test.c
> >> +++ b/tests/boot-serial-test.c
> >> @@ -78,7 +78,7 @@ static void test_machine(const void *data)
> >>  fd = mkstemp(tmpname);
> >>  g_assert(fd != -1);
> >>  
> >> -args = g_strdup_printf("-M %s,accel=tcg -chardev 
> >> file,id=serial0,path=%s"
> >> +args = g_strdup_printf("-M %s,accel=kvm:tcg -chardev 
> >> file,id=serial0,path=%s"
> >> " -no-shutdown -serial chardev:serial0 %s",
> >> test->machine, tmpname, test->extra);
> > 
> > This has already been changed upstream.
> 
> Ouch.  This is the only real smoke test we have for the tcg backend for the
> host.  While it is still going to test tcg for whatever machines do not run
> natively on the host, I can't help think we've lost testing.
> 
> Can we use accel=tcg:kvm instead?
> 
> 
> r~

We can add an environment to disable kvm on qemu side,
for e.g. CI systems who have spare time.

-- 
MST

Re: [Qemu-devel] Help with Windows NT 4.0

[Programmingkid]

> On Aug 14, 2017, at 3:18 AM, Mark Cave-Ayland  
> wrote:
> 
> On 14/08/17 07:51, Paolo Bonzini wrote:
> 
>> On 13/08/2017 21:13, Programmingkid wrote:
>>> Lately I found out that Windows NT 4.0 seems to work well with the
>>> 486 and pentium processors. Using "-cpu 486" made installing it
>>> actually work. Now I am seeing another issue. When I boot Windows NT
>>> 4.0 I see this error message:
>>> 
>>> *** STOP: 0x007B (0x807A8610,0x,0x,0x) 
>>> INACESSIBLE_BOOT_DEVICE
>>> 
>>> Would anyone know a way to solve this issue?
>> 
>> Hervé is probably the best person to answer this question.  Maybe try
>> installing it with SCSI disks ("-drive if=scsi,id=hd,file=... -drive
>> if=scsi,id=cd,file=... -device lsi -device scsi-hd,drive=hd -device
>> scsi-cd,drive=cd").
> 
> FWIW I used to run NT 4.0 in QEMU 0.9.1 without too many problems,
> (although every so often the mouse would go crazy and it would require a
> reboot). The only thing I needed to do was install updated VGA drivers
> from http://www.bearwindows.boot-land.net/vbemp.htm in order to prevent
> issues with the mouse pointer not displaying correctly.
> 
> I'm also fairly sure that was with a single -hda on the command line, so
> it might be useful to go back quite far back in time to figure out what
> changed.
> 
> 
> ATB,
> 
> Mark.

Thanks for the info. Trying to build 0.9.1 is going to be challenging. It 
requires GCC 3.x. 

Re: [Qemu-devel] [PATCH for-2.10 v2 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> On Tue, 15 Aug 2017 12:15:48 +0100
> Anthony PERARD  wrote:
> 
> > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > set, but this was done only when ACPI tables are built which is not
> > needed for a Xen guest. The need for the property starts with commit
> > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > 
> > Set pci info before checking for the needs to build ACPI tables.
> > 
> > Assign bsel=0 property only to the root bus on Xen as there is no
> > support in the Xen ACPI tables for a different value.
> 
> looking at hw/acpi/pcihp.c and bsel usage there it looks like
> bsel property is owned by it and not by ACPI tables, so instead of
> shuffling it in acpi_setup(), how about moving bsel initialization
> to hw/acpi/pcihp.c and initialize it there unconditionally?
> 
> It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> there and calling it from acpi_pcihp_reset().
> 
> Then there won't be need for Xen specific branches, as root bus
> will have bsel set automatically which is sufficient for Xen and
> the rest of bsel-s (bridges) will be just unused by Xen,
> which could later extend its ACPI table implementation to utilize them. 

Later is exactly what I'd like to try to avoid.
Whoever wants acpi hotplug for bridges needs to get
the bsel info from qemu supplied acpi tables.

> 
> > Reported-by: Sander Eikelenboom 
> > Signed-off-by: Anthony PERARD 
> > 
> > ---
> > Changes in V2:
> >   - check for acpi_enabled before calling acpi_set_pci_info.
> >   - set the property on the root bus only.
> > 
> > This patch would be a canditade to backport to 2.9.
> > 
> > CC: Stefano Stabellini 
> > CC: Bruce Rogers 
> > ---
> >  hw/i386/acpi-build.c | 25 -
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 98dd424678..c0483b96cf 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -46,6 +46,7 @@
> >  #include "sysemu/tpm_backend.h"
> >  #include "hw/timer/mc146818rtc_regs.h"
> >  #include "sysemu/numa.h"
> > +#include "hw/xen/xen.h"
> >  
> >  /* Supported chipsets: */
> >  #include "hw/acpi/piix4.h"
> > @@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
> >  unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> >  
> >  if (bus) {
> > -/* Scan all PCI buses. Set property to enable acpi based hotplug. 
> > */
> > -pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, 
> > _alloc);
> > +if (xen_enabled()) {
> > +/* Assign BSEL property to root bus only. */
> > +acpi_set_bsel(bus, _alloc);
> > +} else {
> > +/* Scan all PCI buses. Set property to enable acpi based 
> > hotplug. */
> > +pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, 
> > _alloc);
> > +}
> >  }
> >  }
> >  
> > @@ -2871,6 +2877,14 @@ void acpi_setup(void)
> >  AcpiBuildState *build_state;
> >  Object *vmgenid_dev;
> >  
> > +if (!acpi_enabled) {
> > +ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > +return;
> > +}
> > +
> > +/* Assign BSEL property on hotpluggable PCI buses. */
> > +acpi_set_pci_info();
> > +
> >  if (!pcms->fw_cfg) {
> >  ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >  return;
> > @@ -2881,15 +2895,8 @@ void acpi_setup(void)
> >  return;
> >  }
> >  
> > -if (!acpi_enabled) {
> > -ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > -return;
> > -}
> > -
> >  build_state = g_malloc0(sizeof *build_state);
> >  
> > -acpi_set_pci_info();
> > -
> >  acpi_build_tables_init();
> >  acpi_build(, MACHINE(pcms));
> >  

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:39 PM, Peter Maydell wrote:

On 15 August 2017 at 19:27, Philippe Mathieu-Daudé  wrote:

On 08/15/2017 03:13 PM, Eric Blake wrote:


On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:


only warn once about it.

- kernel without kvm:

  # make check-qtest-x86_64
GTESTER check-qtest-x86_64
  Could not access KVM kernel module: No such device
  qemu-system-x86_64: failed to initialize KVM: No such device
  qemu-system-x86_64: Back to tcg accelerator



How does this differ from what commit 2f6b38d1 was trying to do?



I'd say 2f6b38d1 is for common end-user usage while this is for QA/testers
usage. If you run N qtests with the same machine arguments, having this
warning displayed only once is enough and allow you to focus on the tests
output.


I think I'd rather we just straightforwardly didn't complain at
all in the 'make check' output. There's no benefit to this
for all these tests, especially given that they pretty much
don't care whether they run under KVM or TCG because they're


Oh you mean checking qtest_enabled() within configure_accelerator()? Ok, 
clever :)



not trying to test that. (If they *are* trying to test KVM
specific functionality then they should either skip the
test quietly if KVM isn't present or they should fail it;
printing useless waffle to the logs isn't helpful IMHO.)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:40 PM, Eric Blake wrote:

On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:

On 08/15/2017 03:13 PM, Eric Blake wrote:

On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:

only warn once about it.

- kernel without kvm:

  # make check-qtest-x86_64
GTESTER check-qtest-x86_64
  Could not access KVM kernel module: No such device
  qemu-system-x86_64: failed to initialize KVM: No such device
  qemu-system-x86_64: Back to tcg accelerator


How does this differ from what commit 2f6b38d1 was trying to do?


I'd say 2f6b38d1 is for common end-user usage while this is for
QA/testers usage. If you run N qtests with the same machine arguments,
having this warning displayed only once is enough and allow you to focus
on the tests output.


I guess my question is: Are we still getting a warning output even with
2f6b38d1 applied?  If so, why, because the point of that commit was to
avoid the warning.


The function this patch adds is called by gtester to prepare the QEMU 
command line options, usually this is done once in main(), then each 
qtest is run with the same QEMU command line.


Once started QEMU calls accel_init_machine() displaying warnings from 
303d4e865b7.


Having 3 lines of warnings for each test is way too verbose.

Re: [Qemu-devel] [RFC v4 00/13] qmp: query-device-slots command

[Eric Blake]

On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> Changelog
> -
> 
> Changes v3 -> v4:
> * New compact representation of slot sets.
> * New generic code to automatically merge similar slots
>   into a single entry in the command output while keeping
>   implementations of the method simpler.
> * Example implementation of IDE and USB bus enumeration

> 
> Slot sets are represented by a list of option names and sets of
> possible values for each of those options.  The command uses a
> compact representation for the set of valid values for an option.
> For example, the following set of 5 PCI functions:
> 
>   bus: pcie.0
>   device-number: 31
>   functions: 1,4,5,6,7
> 
> would be represented in the JSON data as:
> 
>   {"available":false,"count":5,
>"device-types":["pci-device"],"hotpluggable":false,
>"opts":[
>   {"option":"function","values":[1,[4,7]]},

A list (and not just a single-type list, but a list that mixes scalar
and sublist),

>   {"option":"device-number","values":31},

vs. a scalar.  Why not a one-element array?

>   {"option":"bus","values":"pcie.0"}],
>"opts-complete":true}
> 
> I planned to use QAPI alternates to model/document that in the
> schema, but it would require implementing a few missing features
> in QAPI alternate support.

Yeah, I can see how existing QAPI alternates do not yet support arrays,
which becomes important to your representation.  Do you need help
getting the QAPI generator improved to support a particular feature that
you found to be lacking?

-- 
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] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

On 08/15/2017 01:40 PM, Eric Blake wrote:
> On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
 only warn once about it.

 - kernel without kvm:

  # make check-qtest-x86_64
GTESTER check-qtest-x86_64
  Could not access KVM kernel module: No such device
  qemu-system-x86_64: failed to initialize KVM: No such device
  qemu-system-x86_64: Back to tcg accelerator
>>>
>>> How does this differ from what commit 2f6b38d1 was trying to do?
>>
>> I'd say 2f6b38d1 is for common end-user usage while this is for
>> QA/testers usage. If you run N qtests with the same machine arguments,
>> having this warning displayed only once is enough and allow you to focus
>> on the tests output.
> 
> I guess my question is: Are we still getting a warning output even with
> 2f6b38d1 applied?  If so, why, because the point of that commit was to
> avoid the warning.

And answering my own question: yes, the builds are still verbose when
done in a VM that lacks nested kvm.  :(

-- 
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] Help with Windows NT 4.0

[Programmingkid]

> On Aug 14, 2017, at 2:51 AM, Paolo Bonzini  wrote:
> 
> On 13/08/2017 21:13, Programmingkid wrote:
>> Lately I found out that Windows NT 4.0 seems to work well with the
>> 486 and pentium processors. Using "-cpu 486" made installing it
>> actually work. Now I am seeing another issue. When I boot Windows NT
>> 4.0 I see this error message:
>> 
>> *** STOP: 0x007B (0x807A8610,0x,0x,0x) 
>> INACESSIBLE_BOOT_DEVICE
>> 
>> Would anyone know a way to solve this issue?
> 
> Hervé is probably the best person to answer this question.  Maybe try
> installing it with SCSI disks ("-drive if=scsi,id=hd,file=... -drive
> if=scsi,id=cd,file=... -device lsi -device scsi-hd,drive=hd -device
> scsi-cd,drive=cd").
> 
> Thanks,
> 
> Paolo

Thanks for the help. Unfortunately trying to boot from the install CD leads to 
the INACCESSIBLE_BOOT_DEVICE error when using SCSI. 

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> On 08/15/2017 03:13 PM, Eric Blake wrote:
>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>> only warn once about it.
>>>
>>> - kernel without kvm:
>>>
>>>  # make check-qtest-x86_64
>>>GTESTER check-qtest-x86_64
>>>  Could not access KVM kernel module: No such device
>>>  qemu-system-x86_64: failed to initialize KVM: No such device
>>>  qemu-system-x86_64: Back to tcg accelerator
>>
>> How does this differ from what commit 2f6b38d1 was trying to do?
> 
> I'd say 2f6b38d1 is for common end-user usage while this is for
> QA/testers usage. If you run N qtests with the same machine arguments,
> having this warning displayed only once is enough and allow you to focus
> on the tests output.

I guess my question is: Are we still getting a warning output even with
2f6b38d1 applied?  If so, why, because the point of that commit was to
avoid the warning.

-- 
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] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Peter Maydell]

On 15 August 2017 at 19:27, Philippe Mathieu-Daudé  wrote:
> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>
>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>>
>>> only warn once about it.
>>>
>>> - kernel without kvm:
>>>
>>>  # make check-qtest-x86_64
>>>GTESTER check-qtest-x86_64
>>>  Could not access KVM kernel module: No such device
>>>  qemu-system-x86_64: failed to initialize KVM: No such device
>>>  qemu-system-x86_64: Back to tcg accelerator
>>
>>
>> How does this differ from what commit 2f6b38d1 was trying to do?
>
>
> I'd say 2f6b38d1 is for common end-user usage while this is for QA/testers
> usage. If you run N qtests with the same machine arguments, having this
> warning displayed only once is enough and allow you to focus on the tests
> output.

I think I'd rather we just straightforwardly didn't complain at
all in the 'make check' output. There's no benefit to this
for all these tests, especially given that they pretty much
don't care whether they run under KVM or TCG because they're
not trying to test that. (If they *are* trying to test KVM
specific functionality then they should either skip the
test quietly if KVM isn't present or they should fail it;
printing useless waffle to the logs isn't helpful IMHO.)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:13 PM, Eric Blake wrote:

On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:

only warn once about it.

- kernel without kvm:

 # make check-qtest-x86_64
   GTESTER check-qtest-x86_64
 Could not access KVM kernel module: No such device
 qemu-system-x86_64: failed to initialize KVM: No such device
 qemu-system-x86_64: Back to tcg accelerator


How does this differ from what commit 2f6b38d1 was trying to do?


I'd say 2f6b38d1 is for common end-user usage while this is for 
QA/testers usage. If you run N qtests with the same machine arguments, 
having this warning displayed only once is enough and allow you to focus 
on the tests output.

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
> only warn once about it.
> 
> - kernel without kvm:
> 
> # make check-qtest-x86_64
>   GTESTER check-qtest-x86_64
> Could not access KVM kernel module: No such device
> qemu-system-x86_64: failed to initialize KVM: No such device
> qemu-system-x86_64: Back to tcg accelerator

How does this differ from what commit 2f6b38d1 was trying to do?

-- 
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] How to implement different endian accesses per MMU page?

[Richard Henderson]

[CC Peter re MemTxAttrs below]

On 08/15/2017 09:38 AM, Mark Cave-Ayland wrote:
> Working through an incorrect endian issue on qemu-system-sparc64, it has
> become apparent that at least one OS makes use of the IE (Invert Endian)
> bit in the SPARCv9 MMU TTE to map PCI memory space without the
> programmer having to manually endian-swap accesses.
> 
> In other words, to quote the UltraSPARC specification: "if this bit is
> set, accesses to the associated page are processed with inverse
> endianness from what is specified by the instruction (big-for-little and
> little-for-big)".
> 
> Looking through various bits of code, I'm trying to get a feel for the
> best way to implement this in an efficient manner. From what I can see
> this could be solved using an additional MMU index, however I'm not
> overly familiar with the memory and softmmu subsystems.

No, it can't be solved with an MMU index.

> Can anyone point me in the right direction as to what would be the best
> way to implement this feature within QEMU?

It's definitely tricky.

We definitely need some TLB_FLAGS_MASK bit set so that we're forced through the
memory slow path.  There is no other way to bypass the endianness that we've
already encoded from the target instruction.

Given the tlb_set_page_with_attrs interface, I would think that we need a new
bit in MemTxAttrs, so that the target/sparc tlb_fill (and subroutines) can pass
along the TTE bit for the given page.

We have an existing problem in softmmu_template.h,

/* ??? Note that the io helpers always read data in the target
   byte ordering.  We should push the LE/BE request down into io.  */
res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr);
res = TGT_BE(res);

We do not want to add a third(!) byte swap along the i/o path.  We need to
collapse the two that we have already before considering this one.

This probably takes the form of:

(1) Replacing the "int size" argument with "TCGMemOp memop" for
  a) io_{read,write}x in accel/tcg/cputlb.c,
  b) memory_region_dispatch_{read,write} in memory.c,
  c) adjust_endianness in memory.c.
This carries size+sign+endianness down to the next level.

(2) In memory.c, adjust_endianness,

 if (memory_region_wrong_endianness(mr)) {
-switch (size) {
+memop ^= MO_BSWAP;
+}
+if (memop & MO_BSWAP) {

For extra credit, re-arrange memory_region_wrong_endianness
to something more explicit -- "wrong" isn't helpful.

(3) In tlb_set_page_with_attrs, notice attrs.byte_swap and set
a new TLB_FORCE_SLOW bit within TLB_FLAGS_MASK.

(4) In io_{read,write}x, if iotlbentry->attrs.byte_swap is set,
then memop ^= MO_BSWAP.



r~

Re: [Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 11:16:57AM -0500, Eric Blake wrote:
> On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> > The helper function will be useful when writing support code to
> > deal with device slot information.
> > 
> > TODO: documentation is incomplete and unclear, needs to be
> > improved.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  include/qapi/util.h| 39 +
> >  qapi/qapi-util.c   | 66 
> > ++
> >  tests/test-qapi-util.c | 53 
> >  3 files changed, 158 insertions(+)
> > 
> 
> > +/**
> > + * qobject_compare:
> > + *
> > + * Compare the value of @a and @b.
> > + *
> > + * If @a and @b have the same type and the same value (see list
> > + * of supported types below), return 0.
> > + *
> > + * If @a and @b are both strings, return strcmp(a, b).
> > + *
> > + * If @a and @b are numbers, return a negative value if a < b,
> > + * and a positive value if a > b.
> > + *
> > + * Otherwise (if @a and @b are not the same, have different types,
> > + * are of an unsupported type, or are different), return a non-zero value.
> 
> Is this number going to be commutative and distributive, in order to
> provide stable qsort()ing?  That is, if comparing a and b gives a
> positive number, then comparing b and a should give a negative number;
> and if comparing a and b then b and c results in two positive numbers,
> then comparing a and c should also give a positive number.  It is
> unclear from the documentation whether you are able to make this
> guarantee; and without it, it is unsafe to use this comparator in places
> that require stability.

No, I don't make that guarantee when the types don't match or in
the case of unsupported types.  That's a limitation of this
implementation.

Guaranteeing it when types don't match should be easy.
Guaranteeing it in the case of QTYPE_QDICT looks a bit harder.
Probably it's easier to simply implement full dictionary
comparison.

> 
> > + *
> > + * Note that this function doesn't support some types, and may
> > + * return false if the types are unsupported, or if the types don't
> > + * match exactly.
> 
> How is a return of false (== 0, which also means equivalent) correct?

This is a documentation bug.  Leftover from a version that
returned a boolean value (true for equal, false for not equal) in
the past.

> 
> > + *
> > + * Supported types:
> > + * - QTYPE_QNULL
> > + * - QTYPE_QSTRING
> > + * - QTYPE_QBOOL
> > + * - QTYPE_QNUM (integers only)
> > + * - QTYPE_QLIST
> > + *
> > + * Unsupported (always return false):
> > + * - QTYPE_QNUM (non-integer values)
> > + * - QTYPE_QDICT
> > + *
> > + * TODO: rewrite documentation to be clearer.
> > + * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.
> 
> There's another patch series pending for qobject_is_equal(); should
> these two patches share approaches or even code?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01134.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02459.html

I will take a look.  Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PULL for 2.10-rc3 0/7] NBD changes for 2.10-rc3

[Peter Maydell]

On 15 August 2017 at 16:09, Eric Blake  wrote:
> The following changes since commit 47025a0193f1f910300adfa443305ccf8482ef87:
>
>   qxl: call qemu_spice_display_init_common for secondary devices (2017-08-15 
> 15:04:51 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2017-08-15
>
> for you to fetch changes up to 72b6ffc76653214b69a94a7b1643ff80df134486:
>
>   nbd-client: Fix regression when server sends garbage (2017-08-15 10:03:28 
> -0500)
>
> 
> nbd patches for 2017-08-15
>
> - Eric Blake: nbd: Fix trace message for disconnect
> - Stefan Hajnoczi: qemu-iotests: step clock after each test iteration
> - Fam Zheng: 0/4 block: Fix non-shared storage migration
> - Eric Blake: nbd-client: Fix regression when server sends garbage
>
> 
> Eric Blake (2):
>   nbd: Fix trace message for disconnect
>   nbd-client: Fix regression when server sends garbage
>
> Fam Zheng (3):
>   stubs: Add vm state change handler stubs
>   block-backend: Defer shared_perm tightening migration completion
>   iotests: Add non-shared storage migration case 192
>
> Kevin Wolf (1):
>   nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache
>
> Stefan Hajnoczi (1):
>   qemu-iotests: step clock after each test iteration

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Philippe Mathieu-Daudé]

On 08/15/2017 01:32 PM, Richard Henderson wrote:

On 08/15/2017 08:56 AM, Philippe Mathieu-Daudé wrote:

@@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt,
int rt2,
   tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
   }
   } else {
-memop |= size;
+memop |= size | MO_ALIGN;


MO_ALIGN_8 here too?


   tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
   tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);


Peter already pointed out that MO_ALIGN_N should be reserved for those cases
where an explicit size really needed.  You should note that using MO_ALIGN_8
would be actively wrong here -- it would incorrectly require 8 byte alignment
for the single byte access of LDXRB.


Indeed I didn't think of that, thanks!




r~

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Philippe Mathieu-Daudé]

On 08/15/2017 01:14 PM, Peter Maydell wrote:

On 15 August 2017 at 16:56, Philippe Mathieu-Daudé  wrote:

Hi Richard,

On 08/15/2017 11:57 AM, Richard Henderson wrote:


From: Alistair Francis 

Acording to the ARM ARM exclusive loads require the same allignment as
exclusive stores. Let's update the memops used for the load to match
that of the store. This adds the alignment requirement to the memops.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Alistair Francis 
[rth: Require 16-byte alignment for 64-bit LDXP.]
Signed-off-by: Richard Henderson 
---
   target/arm/translate-a64.c | 11 ++-
   1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index eac545e4f2..2200e25be0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int
rt, int rt2,
   g_assert(size >= 2);
   if (size == 2) {
   /* The pair must be single-copy atomic for the doubleword.
*/
-memop |= MO_64;
+memop |= MO_64 | MO_ALIGN;



isn't MO_ALIGN_8 enough?


MO_ALIGN is "align to size of access", ie to 8 bytes in this case.
MO_ALIGN_8 is "align to a specified size (8 bytes) which isn't the
  size of the access".
In this case the access size is 8 bytes so we don't need MO_ALIGN_8.

Alignments to something other than the access size are the oddball
case, so I think it makes sense to stick with MO_ALIGN for the
common case of "just aligned to the access size" so you can
spot the odd cases when you're reading the source.


Ok, I misunderstood "MO_ALIGN supposes the alignment size is the size of 
a memory access."


thanks!



thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 for-2.10 0/3] Fixup logic for exclusive pair

[Peter Maydell]

On 15 August 2017 at 15:57, Richard Henderson
 wrote:
> In reviewing my previous patch, Peter pointed out that it is
> CONSTRAINED UNPREDICTABLE what happens when you mix the number
> of registers in a LDX[PR] + STX[RP] pair.  So most of the bug
> that I thought that I was fixing isn't a bug at all.
>
> That said, the patch does still fix a real bug wrt single-copy
> semantics, so patch 2 is largely unchanged; the commit message
> is re-worded.
>
> I also un-squashed Alistair's original patches and dropped the
> tcg/tcg-op.c change, to be revisited for 2.11.

Applied to master (with various reviewed-by etc tags, and
the typos in the commit message for 3/3 fixed).

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.10] mmio-interface: Mark as not user creatable

[Peter Maydell]

On 15 August 2017 at 17:16, Thomas Huth  wrote:
> On 15.08.2017 16:30, Peter Maydell wrote:
>> The mmio-interface device is not something we want to allow
>> users to create on the command line:
>>  * it is intended as an implementation detail of the memory
>>subsystem, which gets created and deleted by that
>>subsystem on demand; it makes no sense to create it
>>by hand on the command line
>>  * it uses a pointer property 'host_ptr' which can't be
>>set on the command line
>>
>> Mark the device as not user_creatable to avoid confusion.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>>  hw/misc/mmio_interface.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
>> index da154e5..894e980 100644
>> --- a/hw/misc/mmio_interface.c
>> +++ b/hw/misc/mmio_interface.c
>> @@ -111,6 +111,11 @@ static void mmio_interface_class_init(ObjectClass *oc, 
>> void *data)
>>  dc->realize = mmio_interface_realize;
>>  dc->unrealize = mmio_interface_unrealize;
>>  dc->props = mmio_interface_properties;
>> +/* Reason: pointer property "host_ptr", and this device
>> + * is an implementation detail of the memory subsystem,
>> + * not intended to be created directly by the user.
>> + */
>> +dc->user_creatable = false;
>>  }
>>
>>  static const TypeInfo mmio_interface_info = {
>>
>
> Reviewed-by: Thomas Huth 

Applied to master, thanks.

-- PMM

[Qemu-devel] [PATCH v3] target-i386/cpu: Add new EPYC CPU model

[Brijesh Singh]

Add a new base CPU model called 'EPYC' to model processors from AMD EPYC
family (which includes EPYC 76xx,75xx,74xx, 73xx and 72xx).

The following features bits have been added/removed compare to Opteron_G5

Added: monitor, movbe, rdrand, mmxext, ffxsr, rdtscp, cr8legacy, osvw,
   fsgsbase, bmi1, avx2, smep, bmi2, rdseed, adx, smap, clfshopt, sha
   xsaveopt, xsavec, xgetbv1, arat

Removed: xop, fma4, tbm

Cc: Paolo Bonzini 
Cc: Richard Henderson 
Cc: Eduardo Habkost 
Cc: Tom Lendacky 
Signed-off-by: Brijesh Singh 
---

Changes since v2:
 * limit the xlevel to 0x800a

Changes since v1:
 * fix typo EYPC -> EPYC to reflect the correct branding name

 target/i386/cpu.c | 44 
 1 file changed, 44 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ddc45ab..6617e01 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1522,6 +1522,50 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .xlevel = 0x801A,
 .model_id = "AMD Opteron 63xx class CPU",
 },
+{
+.name = "EPYC",
+.level = 0xd,
+.vendor = CPUID_VENDOR_AMD,
+.family = 23,
+.model = 1,
+.stepping = 2,
+.features[FEAT_1_EDX] =
+CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | CPUID_CLFLUSH |
+CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | CPUID_PGE |
+CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | CPUID_MCE |
+CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | CPUID_DE |
+CPUID_VME | CPUID_FP87,
+.features[FEAT_1_ECX] =
+CPUID_EXT_RDRAND | CPUID_EXT_F16C | CPUID_EXT_AVX |
+CPUID_EXT_XSAVE | CPUID_EXT_AES |  CPUID_EXT_POPCNT |
+CPUID_EXT_MOVBE | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
+CPUID_EXT_CX16 | CPUID_EXT_FMA | CPUID_EXT_SSSE3 |
+CPUID_EXT_MONITOR | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
+.features[FEAT_8000_0001_EDX] =
+CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_PDPE1GB |
+CPUID_EXT2_FFXSR | CPUID_EXT2_MMXEXT | CPUID_EXT2_NX |
+CPUID_EXT2_SYSCALL,
+.features[FEAT_8000_0001_ECX] =
+CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
+CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
+CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
+.features[FEAT_7_0_EBX] =
+CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
+CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
+CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
+CPUID_7_0_EBX_SHA_NI,
+/* Missing: XSAVES (not supported by some Linux versions,
+ * including v4.1 to v4.12).
+ * KVM doesn't yet expose any XSAVES state save component.
+ */
+.features[FEAT_XSAVE] =
+CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+CPUID_XSAVE_XGETBV1,
+.features[FEAT_6_EAX] =
+CPUID_6_EAX_ARAT,
+.xlevel = 0x800A,
+.model_id = "AMD EPYC Processor",
+},
 };
 
 typedef struct PropValue {
-- 
2.9.4

Re: [Qemu-devel] [PULL 7/7] nbd-client: Fix regression when server sends garbage

[Eric Blake]

On 08/15/2017 10:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.08.2017 18:09, Eric Blake wrote:
>> When we switched NBD to use coroutines for qemu 2.9 (in particular,
>> commit a12a712a), we introduced a regression: if a server sends us
>> garbage (such as a corrupted magic number), we quit the read loop
>> but do not stop sending further queued commands, resulting in the
>> client hanging when it never reads the response to those additional
>> commands.  In qemu 2.8, we properly detected that the server is no
>> longer reliable, and cancelled all existing pending commands with
>> EIO, then tore down the socket so that all further command attempts
>> get EPIPE.
>>

>> +++ b/block/nbd-client.c
>> @@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void
>> *opaque)
>>   int ret;
>>   Error *local_err = NULL;
>>
>> -for (;;) {
>> +while (!s->quit) {
>>   assert(s->reply.handle == 0);
>>   ret = nbd_receive_reply(s->ioc, >reply, _err);
>>   if (ret < 0) {
> 
> I think we should check quit here, if it is true, we should not continue
> normal path of handling reply

I don't think it matters.  If nbd_receive_reply() correctly got data off
the wire for this particular coroutine's request, we might as well act
on that data, regardless of what other coroutines have learned in the
meantime.

This is already in the pull request for -rc3, but if you can come up
with a scenario that still behaves incorrectly, we can do a followup
patch for -rc4 (although I'm hoping we don't have to change it any
further for 2.10).  Otherwise, I'm fine if your refactoring work for
2.11 addresses the issue as part of making the code easier to read.

>> @@ -154,6 +161,9 @@ static int nbd_co_send_request(BlockDriverState *bs,
>>   } else {
>>   rc = nbd_send_request(s->ioc, request);
>>   }
>> +if (rc < 0) {
>> +s->quit = true;
>> +}
>>   qemu_co_mutex_unlock(>send_mutex);
> 
> and here, if rc == 0 and quite is true, we should not return 0
> 
>>   return rc;

We don't - we return rc, which is negative.

>>   }
>> @@ -168,8 +178,7 @@ static void nbd_co_receive_reply(NBDClientSession *s,
>>   /* Wait until we're woken up by nbd_read_reply_entry.  */
>>   qemu_coroutine_yield();
>>   *reply = s->reply;
>> -if (reply->handle != request->handle ||
>> -!s->ioc) {
>> +if (reply->handle != request->handle || !s->ioc || s->quit) {
>>   reply->error = EIO;
> 
> here, if s->quit is false, we should set it to inform other coroutines

We can't get into nbd_co_receive_reply() unless the two handles were
once equal, and the only code that changes them to be not equal is when
we are shutting down.  Checking s->quit is a safety valve if some other
coroutine detects corruption first, but this coroutine does not need to
set s->quit because it is either already set, or we are already shutting
down.

> 
>>   } else {
>>   if (qiov && reply->error == 0) {
> 
> and here follows a call to nbd_rwv(), where s->quit should be
> appropriately handled..

Reading from a corrupt server is not as bad as writing to the corrupt
server; the patch for 2.10 is solely focused on preventing writes where
we need a followup read (because once we know the server is corrupt, we
can't guarantee the followup reads will come).

Again, if you can prove we have a scenario that is still buggy (client
can crash or hang), then it is -rc4 material; if not, then this is all
the more that 2.10 needs, and your refactoring work for 2.11 should
clean up a lot of this mess in the first place as you make the
coroutines easier to follow.

-- 
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 2/2] tests: use qtest_accel()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/bios-tables-test.c |  2 +-
 tests/boot-serial-test.c |  5 +++--
 tests/postcopy-test.c| 10 ++
 tests/pxe-test.c |  5 +++--
 tests/vmgenid-test.c |  4 ++--
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 564da45f65..621bd8d95d 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -627,7 +627,7 @@ static void test_acpi_one(const char *params, test_data 
*data)
"-net none -display none %s "
"-drive id=hd0,if=none,file=%s,format=raw "
"-device ide-hd,drive=hd0 ",
-   data->machine, "kvm:tcg",
+   data->machine, qtest_accel("kvm:tcg"),
params ? params : "", disk);
 
 qtest_start(args);
diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index a8ca877168..f986ea51b5 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -78,10 +78,11 @@ static void test_machine(const void *data)
 fd = mkstemp(tmpname);
 g_assert(fd != -1);
 
-args = g_strdup_printf("-M %s,accel=kvm:tcg "
+args = g_strdup_printf("-M %s,accel=%s "
"-chardev file,id=serial0,path=%s "
"-no-shutdown -serial chardev:serial0 %s",
-   test->machine, tmpname, test->extra);
+   test->machine, qtest_accel("kvm:tcg"),
+   tmpname, test->extra);
 
 qtest_start(args);
 unlink(tmpname);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 8142f2ab90..781b955a59 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -367,18 +367,20 @@ static void test_migrate(void)
 got_stop = false;
 
 if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+const char *accel = qtest_accel("kvm:tcg");
+
 init_bootfile_x86(bootpath);
-cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
   " -name pcsource,debug-threads=on"
   " -serial file:%s/src_serial"
   " -drive file=%s,format=raw",
-  tmpfs, bootpath);
-cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+  accel, tmpfs, bootpath);
+cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
   " -name pcdest,debug-threads=on"
   " -serial file:%s/dest_serial"
   " -drive file=%s,format=raw"
   " -incoming %s",
-  tmpfs, bootpath, uri);
+  accel, tmpfs, bootpath, uri);
 } else if (strcmp(arch, "ppc64") == 0) {
 const char *accel;
 
diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index cf6e225330..531e0d5506 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -25,9 +25,10 @@ static void test_pxe_one(const char *params, bool ipv6)
 {
 char *args;
 
-args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n "
+args = g_strdup_printf("-machine accel=%s -nodefaults -boot order=n "
"-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,"
-   "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
+   "ipv4=%s,ipv6=%s %s", qtest_accel("kvm:tcg"),
+   disk, ipv6 ? "off" : "on",
ipv6 ? "on" : "off", params);
 
 qtest_start(args);
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 3d5c1c3615..82b52aa44c 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -132,11 +132,11 @@ static char disk[] = "tests/vmgenid-test-disk-XX";
 
 static char *guid_cmd_strdup(const char *guid)
 {
-return g_strdup_printf("-machine accel=kvm:tcg "
+return g_strdup_printf("-machine accel=%s "
"-device vmgenid,id=testvgid,guid=%s "
"-drive id=hd0,if=none,file=%s,format=raw "
"-device ide-hd,drive=hd0 ",
-   guid, disk);
+   qtest_accel("kvm:tcg"), guid, disk);
 }
 
 
-- 
2.14.1

[Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

only warn once about it.

- kernel without kvm:

# make check-qtest-x86_64
  GTESTER check-qtest-x86_64
Could not access KVM kernel module: No such device
qemu-system-x86_64: failed to initialize KVM: No such device
qemu-system-x86_64: Back to tcg accelerator

- tests ran as user:

$ make check-qtest-x86_64
  GTESTER check-qtest-x86_64
Could not access KVM kernel module: Permission denied
qemu-system-x86_64: failed to initialize KVM: Permission denied
qemu-system-x86_64: Back to tcg accelerator

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/libqtest.h |  8 
 tests/libqtest.c | 18 ++
 2 files changed, 26 insertions(+)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9953..24e03148eb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -927,4 +927,12 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
  */
 void qtest_cb_for_every_machine(void (*cb)(const char *machine));
 
+/**
+ * qtest_accel:
+ * @accel: List of accelerators
+ *
+ *  Filter accelerators accessible on the host.
+ */
+const char *qtest_accel(const char *accel);
+
 #endif
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f180e1..d2dfca35a3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -987,3 +987,21 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
*machine))
 qtest_end();
 QDECREF(response);
 }
+
+const char *qtest_accel(const char *accel)
+{
+static bool kvm_accessible = true;
+
+if (strlen(accel) <= 4 || strncmp(accel, "kvm:", 4)) {
+return accel; /* no match */
+}
+
+if (!kvm_accessible || !access("/dev/kvm", W_OK)) {
+accel += 4; /* skip "kvm:" */
+if (kvm_accessible) {
+kvm_accessible = false; /* warn once */
+g_printerr("kvm not accessible, using %s\n", accel);
+}
+}
+return accel;
+}
-- 
2.14.1

[Qemu-devel] [RFC PATCH 0/2] tests: avoid kvm accel when it is not accessible

[Philippe Mathieu-Daudé]

This series reduce warnings when kvm is not accessible:

- kernel without kvm:

# make check-qtest-x86_64
  GTESTER check-qtest-x86_64
Could not access KVM kernel module: No such device
qemu-system-x86_64: failed to initialize KVM: No such device
qemu-system-x86_64: Back to tcg accelerator
[... many of these ...]

- tests ran as user:

$ make check-qtest-x86_64
  GTESTER check-qtest-x86_64
Could not access KVM kernel module: Permission denied
qemu-system-x86_64: failed to initialize KVM: Permission denied
qemu-system-x86_64: Back to tcg accelerator
[... many of these ...]

Once applied:

$ make check-qtest-x86_64
  GTESTER check-qtest-x86_64
kvm not accessible, using tcg
kvm not accessible, using tcg
kvm not accessible, using tcg
kvm not accessible, using tcg
kvm not accessible, using tcg

Philippe Mathieu-Daudé (2):
  libqtest: add qtest_accel() to avoid warnings when kvm is not
available
  tests: use qtest_accel()

 tests/libqtest.h |  8 
 tests/bios-tables-test.c |  2 +-
 tests/boot-serial-test.c |  5 +++--
 tests/libqtest.c | 18 ++
 tests/postcopy-test.c| 10 ++
 tests/pxe-test.c |  5 +++--
 tests/vmgenid-test.c |  4 ++--
 7 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.14.1

[Qemu-devel] How to implement different endian accesses per MMU page?

[Mark Cave-Ayland]

Hi all,

Working through an incorrect endian issue on qemu-system-sparc64, it has
become apparent that at least one OS makes use of the IE (Invert Endian)
bit in the SPARCv9 MMU TTE to map PCI memory space without the
programmer having to manually endian-swap accesses.

In other words, to quote the UltraSPARC specification: "if this bit is
set, accesses to the associated page are processed with inverse
endianness from what is specified by the instruction (big-for-little and
little-for-big)".

Looking through various bits of code, I'm trying to get a feel for the
best way to implement this in an efficient manner. From what I can see
this could be solved using an additional MMU index, however I'm not
overly familiar with the memory and softmmu subsystems.

Can anyone point me in the right direction as to what would be the best
way to implement this feature within QEMU?


Many thanks,

Mark.

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Richard Henderson]

On 08/15/2017 08:56 AM, Philippe Mathieu-Daudé wrote:
>> @@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt,
>> int rt2,
>>   tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
>>   }
>>   } else {
>> -memop |= size;
>> +memop |= size | MO_ALIGN;
> 
> MO_ALIGN_8 here too?
> 
>>   tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
>>   tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);

Peter already pointed out that MO_ALIGN_N should be reserved for those cases
where an explicit size really needed.  You should note that using MO_ALIGN_8
would be actively wrong here -- it would incorrectly require 8 byte alignment
for the single byte access of LDXRB.


r~

Re: [Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

[Eric Blake]

On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> The helper function will be useful when writing support code to
> deal with device slot information.
> 
> TODO: documentation is incomplete and unclear, needs to be
> improved.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  include/qapi/util.h| 39 +
>  qapi/qapi-util.c   | 66 
> ++
>  tests/test-qapi-util.c | 53 
>  3 files changed, 158 insertions(+)
> 

> +/**
> + * qobject_compare:
> + *
> + * Compare the value of @a and @b.
> + *
> + * If @a and @b have the same type and the same value (see list
> + * of supported types below), return 0.
> + *
> + * If @a and @b are both strings, return strcmp(a, b).
> + *
> + * If @a and @b are numbers, return a negative value if a < b,
> + * and a positive value if a > b.
> + *
> + * Otherwise (if @a and @b are not the same, have different types,
> + * are of an unsupported type, or are different), return a non-zero value.

Is this number going to be commutative and distributive, in order to
provide stable qsort()ing?  That is, if comparing a and b gives a
positive number, then comparing b and a should give a negative number;
and if comparing a and b then b and c results in two positive numbers,
then comparing a and c should also give a positive number.  It is
unclear from the documentation whether you are able to make this
guarantee; and without it, it is unsafe to use this comparator in places
that require stability.

> + *
> + * Note that this function doesn't support some types, and may
> + * return false if the types are unsupported, or if the types don't
> + * match exactly.

How is a return of false (== 0, which also means equivalent) correct?

> + *
> + * Supported types:
> + * - QTYPE_QNULL
> + * - QTYPE_QSTRING
> + * - QTYPE_QBOOL
> + * - QTYPE_QNUM (integers only)
> + * - QTYPE_QLIST
> + *
> + * Unsupported (always return false):
> + * - QTYPE_QNUM (non-integer values)
> + * - QTYPE_QDICT
> + *
> + * TODO: rewrite documentation to be clearer.
> + * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.

There's another patch series pending for qobject_is_equal(); should
these two patches share approaches or even code?

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01134.html
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02459.html

-- 
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 v2 for-2.10 1/3] target/arm: Correct exclusive store cmpxchg memop mask

[Alistair Francis]

On Tue, Aug 15, 2017 at 8:41 AM, Philippe Mathieu-Daudé  wrote:
> On 08/15/2017 11:57 AM, Richard Henderson wrote:
>>
>> From: Alistair Francis 
>>
>> When we perform the atomic_cmpxchg operation we want to perform the
>> operation on a pair of 32-bit registers. Previously we were just passing
>> the register size in which was set to MO_32. This would result in the
>> high register to be ignored. To fix this issue we hardcode the size to
>> be 64-bits long when operating on 32-bit pairs.
>>
>> Reviewed-by: Edgar E. Iglesias 
>> Signed-off-by: Alistair Francis 
>> Message-Id:
>> 
>> Signed-off-by: Richard Henderson 
>
>
> Reviewed-by: Philippe Mathieu-Daudé 

Can we keep this as well, it was posted for my entire series:

Tested-by: Portia Stephens 

Thanks,
Alistair

>
>
>> ---
>>   target/arm/translate-a64.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index 58ed4c6d05..113e2e172b 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int
>> rd, int rt, int rt2,
>>   tcg_gen_concat32_i64(val, cpu_exclusive_val,
>> cpu_exclusive_high);
>>   tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
>>  get_mem_index(s),
>> -   size | MO_ALIGN | s->be_data);
>> +   MO_64 | MO_ALIGN | s->be_data);
>>   tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
>>   tcg_temp_free_i64(val);
>>   } else if (s->be_data == MO_LE) {
>>
>

Re: [Qemu-devel] [PATCH for-2.10] mmio-interface: Mark as not user creatable

[Thomas Huth]

On 15.08.2017 16:30, Peter Maydell wrote:
> The mmio-interface device is not something we want to allow
> users to create on the command line:
>  * it is intended as an implementation detail of the memory
>subsystem, which gets created and deleted by that
>subsystem on demand; it makes no sense to create it
>by hand on the command line
>  * it uses a pointer property 'host_ptr' which can't be
>set on the command line
> 
> Mark the device as not user_creatable to avoid confusion.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/misc/mmio_interface.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
> index da154e5..894e980 100644
> --- a/hw/misc/mmio_interface.c
> +++ b/hw/misc/mmio_interface.c
> @@ -111,6 +111,11 @@ static void mmio_interface_class_init(ObjectClass *oc, 
> void *data)
>  dc->realize = mmio_interface_realize;
>  dc->unrealize = mmio_interface_unrealize;
>  dc->props = mmio_interface_properties;
> +/* Reason: pointer property "host_ptr", and this device
> + * is an implementation detail of the memory subsystem,
> + * not intended to be created directly by the user.
> + */
> +dc->user_creatable = false;
>  }
>  
>  static const TypeInfo mmio_interface_info = {
> 

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Peter Maydell]

On 15 August 2017 at 16:56, Philippe Mathieu-Daudé  wrote:
> Hi Richard,
>
> On 08/15/2017 11:57 AM, Richard Henderson wrote:
>>
>> From: Alistair Francis 
>>
>> Acording to the ARM ARM exclusive loads require the same allignment as
>> exclusive stores. Let's update the memops used for the load to match
>> that of the store. This adds the alignment requirement to the memops.
>>
>> Reviewed-by: Edgar E. Iglesias 
>> Signed-off-by: Alistair Francis 
>> [rth: Require 16-byte alignment for 64-bit LDXP.]
>> Signed-off-by: Richard Henderson 
>> ---
>>   target/arm/translate-a64.c | 11 ++-
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index eac545e4f2..2200e25be0 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int
>> rt, int rt2,
>>   g_assert(size >= 2);
>>   if (size == 2) {
>>   /* The pair must be single-copy atomic for the doubleword.
>> */
>> -memop |= MO_64;
>> +memop |= MO_64 | MO_ALIGN;
>
>
> isn't MO_ALIGN_8 enough?

MO_ALIGN is "align to size of access", ie to 8 bytes in this case.
MO_ALIGN_8 is "align to a specified size (8 bytes) which isn't the
 size of the access".
In this case the access size is 8 bytes so we don't need MO_ALIGN_8.

Alignments to something other than the access size are the oddball
case, so I think it makes sense to stick with MO_ALIGN for the
common case of "just aligned to the access size" so you can
spot the odd cases when you're reading the source.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Igor Mammedov]

On Tue, 15 Aug 2017 17:43:00 +0200
Vadim Galitsyn  wrote:

> Hi Guys,
> 
> Thank you for the input!
> 
> > "hotunpluggable" is ugly.  What about just "pluggable"?  
> 
> Markus, I think "pluggable" is a bit misleading here. Some people can take
> it like a maximum amount of memory that can be hot-added to a guest (i.e.,
> difference between static memory size and value specified with "-m
> ...,maxmem=XXX"). I would say:
> 
>   mem_info->plugged_memory = get_plugged_memory_size();
> 
> ..since it reflects actual amount which was already hot-added. Agree?
> 
> > this could be done without wrapper  
> 
> Igor, your approach changes command behaviour a little. First of all it
> turns it in command which can fail. As far as I understand we wanted to
> avoid this.
> 
> Wrapper is needed in order to "error_abort" command only in case if (a)
> CONFIG_MEM_HOTPLUG  is enabled, but (b) QOM data is somehow corrupted.
> If CONFIG_MEM_HOTPLUG
> is disabled for target, it does not mean to report an error -- in this case
> value simply not reported and no error path triggered.
Ok,
keeping current approach is also fine.

> 
> I agree with the rest of comments and will provide changes with the next
> version.
> Could you please leave a comment(s) if you agree or not (before I submit
> next version)?
> 
> Thank you in advance,
> Vadim
> 
> 
> On Tue, Aug 15, 2017 at 9:51 AM, Igor Mammedov  wrote:
> 
> > On Fri, 28 Jul 2017 14:10:43 +0200
> > Vadim Galitsyn  wrote:
> >  
> > > Command above provides the following memory information in bytes:
> > >
> > >   * base-memory - size of "base" memory specified with command line  
> > option -m.  
> > >
> > >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> > > If target does not have CONFIG_MEM_HOTPLUG enabled, no
> > > value is reported.
> > >
> > > Signed-off-by: Vasilis Liaskovitis  > >
> > > Signed-off-by: Mohammed Gamal 
> > > Signed-off-by: Eduardo Otubo 
> > > Signed-off-by: Vadim Galitsyn 
> > > Reviewed-by: Eugene Crosser 
> > > Cc: Dr. David Alan Gilbert 
> > > Cc: Markus Armbruster 
> > > Cc: Igor Mammedov 
> > > Cc: Eric Blake 
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > >  hw/mem/pc-dimm.c   |  5 +
> > >  include/hw/mem/pc-dimm.h   |  1 +
> > >  qapi-schema.json   | 25  
> > ++  
> > >  qmp.c  | 13 +++
> > >  stubs/Makefile.objs|  2 +-
> > >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +
> > >  6 files changed, 50 insertions(+), 1 deletion(-)
> > >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
> > >
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index ea67b461c2..1df8b7ee57 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
> > >  return cap.size;
> > >  }
> > >
> > > +uint64_t get_existing_hotpluggable_memory_size(void)
> > > +{
> > > +return pc_existing_dimms_capacity(_abort);
> > > +}
> > > +
> > >  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > >  {
> > >  MemoryDeviceInfoList ***prev = opaque;
> > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > > index 1e483f2670..52c6b5e641 100644
> > > --- a/include/hw/mem/pc-dimm.h
> > > +++ b/include/hw/mem/pc-dimm.h
> > > @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int  
> > max_slots, Error **errp);  
> > >
> > >  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
> > >  uint64_t pc_existing_dimms_capacity(Error **errp);
> > > +uint64_t get_existing_hotpluggable_memory_size(void);
> > >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > >   MemoryRegion *mr, uint64_t align, Error  
> > **errp);  
> > >  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 9c6c3e1a53..bbedf1a7bc 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4407,6 +4407,31 @@
> > >'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
> > >  '*unavailable-features': [ 'str' ], 'typename': 'str' } }
> > >
> > > +##
> > > +# @MemoryInfo:
> > > +#
> > > +# Actual memory information in bytes.
> > > +#
> > > +# @base-memory: size of "base" memory specified with command line
> > > +#   option -m.
> > > +#
> > > +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> > > +#
> > > +# Since: 

Re: [Qemu-devel] [PATCH] target-i386/cpu: Add new EYPC CPU model

[Brijesh Singh]

Hi Eduardo,


On 08/15/2017 06:35 AM, Eduardo Habkost wrote:

Hi,

Thanks for the patch.

On Mon, Aug 14, 2017 at 10:52:17AM -0500, Brijesh Singh wrote:

Add a new base CPU model called 'EPYC' to model processors from AMD EPYC
family (which includes EPYC 76xx,75xx,74xx,73xx and 72xx).


I suggest enumerating in the commit message which features were
added to the CPU model in comparison to Opteron_G5.



Will do.

[snip]


+/* Missing: XSAVES (not supported by some Linux versions,
+ * including v4.1 to v4.12).
+ * KVM doesn't yet expose any XSAVES state save component.
+ */


Do you know which supervisor state components are available in
EPYC CPUs?  Do you have a pointer to public AMD documentation
about XSAVES?



The available state components that may be saved by XSAVES are solely
determined by XCR0, and are therefore the same set of components that
can be used in XSAVE.

AMD APM vol4 [1] provides some information about XSAVES

[1] http://support.amd.com/TechDocs/26568.pdf#search=XSAVES

Note:
Looking at the document, I see that it says state elements saved are
determined by logical OR of XCRO with the IA32_XSS MSR. But the IA32_XSS MSR
definition is missing from the document and I have flagged it, it should be
updated with correct definition. But in the meantime, CPU team has confirmed
that the EPYC does not support any bits in IA32_XSS MSR.




+.features[FEAT_XSAVE] =
+CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+CPUID_XSAVE_XGETBV1,
+.features[FEAT_6_EAX] =
+CPUID_6_EAX_ARAT,
+.xlevel = 0x801F,


All CPUID leaves from 0x800B to 0x801F return all-zeroes
today.  If we set xlevel to 0x801F before we actually
implement those CPUID leaves, we will be forced to add extra
machine-type compat code when we finally implement them.

I suggest setting it to 0x800A, and increasing it only after
we actually implement the new CPUID leaves.




Sure, I will limit xlevel to 0x8000_000A.

-Brijesh

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Philippe Mathieu-Daudé]

Hi Richard,

On 08/15/2017 11:57 AM, Richard Henderson wrote:

From: Alistair Francis 

Acording to the ARM ARM exclusive loads require the same allignment as
exclusive stores. Let's update the memops used for the load to match
that of the store. This adds the alignment requirement to the memops.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Alistair Francis 
[rth: Require 16-byte alignment for 64-bit LDXP.]
Signed-off-by: Richard Henderson 
---
  target/arm/translate-a64.c | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index eac545e4f2..2200e25be0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
  g_assert(size >= 2);
  if (size == 2) {
  /* The pair must be single-copy atomic for the doubleword.  */
-memop |= MO_64;
+memop |= MO_64 | MO_ALIGN;


isn't MO_ALIGN_8 enough?


  tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
  if (s->be_data == MO_LE) {
  tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 0, 32);
@@ -1871,10 +1871,11 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
  tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 0, 
32);
  }
  } else {
-/* The pair must be single-copy atomic for *each* doubleword,
-   but not the entire quadword.  */
+/* The pair must be single-copy atomic for *each* doubleword, not
+   the entire quadword, however it must be quadword aligned.  */
  memop |= MO_64;
-tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
+tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx,
+memop | MO_ALIGN_16);


ok

  
  TCGv_i64 addr2 = tcg_temp_new_i64();

  tcg_gen_addi_i64(addr2, addr, 8);
@@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
  tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
  }
  } else {
-memop |= size;
+memop |= size | MO_ALIGN;


MO_ALIGN_8 here too?


  tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
  tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
  }



Regards,

Phil.

[Qemu-devel] [PATCH] PCI: PCIe access should always be little endian

[Matt Redfearn]

PCIe busses are always little endian, so set the endianness of the
memory region to little endian rather than native such that operations
work as expected on big endian targets.

Signed-off-by: Matt Redfearn 
---

 hw/pci/pcie_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index dcebf57ed45e..553db56778b6 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -81,7 +81,7 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
 static const MemoryRegionOps pcie_mmcfg_ops = {
 .read = pcie_mmcfg_data_read,
 .write = pcie_mmcfg_data_write,
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void pcie_host_init(Object *obj)
-- 
2.7.4

Re: [Qemu-devel] [PULL 7/7] nbd-client: Fix regression when server sends garbage

[Vladimir Sementsov-Ogievskiy]

15.08.2017 18:09, Eric Blake wrote:

When we switched NBD to use coroutines for qemu 2.9 (in particular,
commit a12a712a), we introduced a regression: if a server sends us
garbage (such as a corrupted magic number), we quit the read loop
but do not stop sending further queued commands, resulting in the
client hanging when it never reads the response to those additional
commands.  In qemu 2.8, we properly detected that the server is no
longer reliable, and cancelled all existing pending commands with
EIO, then tore down the socket so that all further command attempts
get EPIPE.

Restore the proper behavior of quitting (almost) all communication
with a broken server: Once we know we are out of sync or otherwise
can't trust the server, we must assume that any further incoming
data is unreliable and therefore end all pending commands with EIO,
and quit trying to send any further commands.  As an exception, we
still (try to) send NBD_CMD_DISC to let the server know we are going
away (in part, because it is easier to do that than to further
refactor nbd_teardown_connection, and in part because it is the
only command where we do not have to wait for a reply).

Based on a patch by Vladimir Sementsov-Ogievskiy.

A malicious server can be created with the following hack,
followed by setting NBD_SERVER_DEBUG to a non-zero value in the
environment when running qemu-nbd:

| --- a/nbd/server.c
| +++ b/nbd/server.c
| @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply 
*reply, Error **errp)
|  stl_be_p(buf + 4, reply->error);
|  stq_be_p(buf + 8, reply->handle);
|
| +static int debug;
| +static int count;
| +if (!count++) {
| +const char *str = getenv("NBD_SERVER_DEBUG");
| +if (str) {
| +debug = atoi(str);
| +}
| +}
| +if (debug && !(count % debug)) {
| +buf[0] = 0;
| +}
|  return nbd_write(ioc, buf, sizeof(buf), errp);
|  }

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
Message-Id: <20170814213426.24681-1-ebl...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
---
  block/nbd-client.h |  1 +
  block/nbd-client.c | 17 +
  2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..1935ffbcaa 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {

  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
  NBDReply reply;
+bool quit;
  } NBDClientSession;

  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..422ecb4307 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  int ret;
  Error *local_err = NULL;

-for (;;) {
+while (!s->quit) {
  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, >reply, _err);
  if (ret < 0) {


I think we should check quit here, if it is true, we should not continue 
normal path of handling reply



@@ -107,6 +107,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  qemu_coroutine_yield();
  }

+if (ret < 0) {
+s->quit = true;
+}
  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }
@@ -135,6 +138,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
  assert(i < MAX_NBD_REQUESTS);
  request->handle = INDEX_TO_HANDLE(s, i);


not bad to check s->quit at start of the function, but it will 
complicate things like in my patch.



+if (s->quit) {
+qemu_co_mutex_unlock(>send_mutex);
+return -EIO;
+}
  if (!s->ioc) {
  qemu_co_mutex_unlock(>send_mutex);
  return -EPIPE;
@@ -143,7 +150,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
  if (qiov) {
  qio_channel_set_cork(s->ioc, true);
  rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->quit) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
NULL);
  if (ret != request->len) {
@@ -154,6 +161,9 @@ static int nbd_co_send_request(BlockDriverState *bs,
  } else {
  rc = nbd_send_request(s->ioc, request);
  }
+if (rc < 0) {
+s->quit = true;
+}
  qemu_co_mutex_unlock(>send_mutex);


and here, if rc == 0 and quite is true, we should not return 0


  return rc;
  }
@@ -168,8 +178,7 @@ static void nbd_co_receive_reply(NBDClientSession *s,
  /* Wait until we're woken up by nbd_read_reply_entry.  */
  qemu_coroutine_yield();
  *reply = s->reply;
-if (reply->handle != request->handle ||
-!s->ioc) {
+if (reply->handle != request->handle || !s->ioc || s->quit) {
  reply->error = EIO;


here, if 

Re: [Qemu-devel] [PATCH] tests: switch tests to accel=kvm:tcg

[Richard Henderson]

On 08/14/2017 08:33 AM, Cornelia Huck wrote:
> On Mon, 14 Aug 2017 17:34:15 +0300
> "Michael S. Tsirkin"  wrote:
> 
>> Speed up tests on host systems with kvm support.
>> In particular, this fixes tests with --disable-tcg.
>>
>> Cc: Paolo Bonzini 
>> Cc: Thomas Huth 
>> Cc: Laurent Vivier 
>> Suggested-by: Cornelia Huck 
>> Signed-off-by: Michael S. Tsirkin 
>> ---
>>
>> Tested on x86 only.
>>
>>  tests/boot-serial-test.c | 2 +-
>>  tests/pnv-xscom-test.c   | 4 ++--
>>  tests/prom-env-test.c| 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
>> index 11f48b0..c3b2e4e 100644
>> --- a/tests/boot-serial-test.c
>> +++ b/tests/boot-serial-test.c
>> @@ -78,7 +78,7 @@ static void test_machine(const void *data)
>>  fd = mkstemp(tmpname);
>>  g_assert(fd != -1);
>>  
>> -args = g_strdup_printf("-M %s,accel=tcg -chardev 
>> file,id=serial0,path=%s"
>> +args = g_strdup_printf("-M %s,accel=kvm:tcg -chardev 
>> file,id=serial0,path=%s"
>> " -no-shutdown -serial chardev:serial0 %s",
>> test->machine, tmpname, test->extra);
> 
> This has already been changed upstream.

Ouch.  This is the only real smoke test we have for the tcg backend for the
host.  While it is still going to test tcg for whatever machines do not run
natively on the host, I can't help think we've lost testing.

Can we use accel=tcg:kvm instead?


r~

Re: [Qemu-devel] [PATCH v5 3/3] hmp: introduce 'info memory-size-summary' command

[Vadim Galitsyn]

Hi Eric,

Thank you for the input. I will update it with the next version. Btw, most
of HMP "info *" commands use '-' instead of '_' in names =)

Best regards,
Vadim

On Fri, Jul 28, 2017 at 8:27 PM, Eric Blake  wrote:

> On 07/28/2017 07:10 AM, Vadim Galitsyn wrote:
> > This command is an equivalent of QMP command query-memory-size-summary.
> > It provides the following memory information in bytes:
> >
> >   * base-memory - size of "base" memory specified with command line
> option -m.
> >
> >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> > If target does not have CONFIG_MEM_HOTPLUG enabled, no
> > value is reported.
>
> Most of our HMP commands use underscores between words; for consistency,
> you might want to name it 'info memory_size_summary'.  Also, between the
> new QMP and HMP parameters, do you have any testsuite coverage?  I know
> we don't have many existing QMP tests to copy from, but where possible,
> we want to avoid adding new QMP features that don't have some sort of
> coverage.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Vadim Galitsyn]

Hi Guys,

Thank you for the input!

> "hotunpluggable" is ugly.  What about just "pluggable"?

Markus, I think "pluggable" is a bit misleading here. Some people can take
it like a maximum amount of memory that can be hot-added to a guest (i.e.,
difference between static memory size and value specified with "-m
...,maxmem=XXX"). I would say:

  mem_info->plugged_memory = get_plugged_memory_size();

..since it reflects actual amount which was already hot-added. Agree?

> this could be done without wrapper

Igor, your approach changes command behaviour a little. First of all it
turns it in command which can fail. As far as I understand we wanted to
avoid this.

Wrapper is needed in order to "error_abort" command only in case if (a)
CONFIG_MEM_HOTPLUG  is enabled, but (b) QOM data is somehow corrupted.
If CONFIG_MEM_HOTPLUG
is disabled for target, it does not mean to report an error -- in this case
value simply not reported and no error path triggered.

I agree with the rest of comments and will provide changes with the next
version.
Could you please leave a comment(s) if you agree or not (before I submit
next version)?

Thank you in advance,
Vadim


On Tue, Aug 15, 2017 at 9:51 AM, Igor Mammedov  wrote:

> On Fri, 28 Jul 2017 14:10:43 +0200
> Vadim Galitsyn  wrote:
>
> > Command above provides the following memory information in bytes:
> >
> >   * base-memory - size of "base" memory specified with command line
> option -m.
> >
> >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> > If target does not have CONFIG_MEM_HOTPLUG enabled, no
> > value is reported.
> >
> > Signed-off-by: Vasilis Liaskovitis  >
> > Signed-off-by: Mohammed Gamal 
> > Signed-off-by: Eduardo Otubo 
> > Signed-off-by: Vadim Galitsyn 
> > Reviewed-by: Eugene Crosser 
> > Cc: Dr. David Alan Gilbert 
> > Cc: Markus Armbruster 
> > Cc: Igor Mammedov 
> > Cc: Eric Blake 
> > Cc: qemu-devel@nongnu.org
> > ---
> >  hw/mem/pc-dimm.c   |  5 +
> >  include/hw/mem/pc-dimm.h   |  1 +
> >  qapi-schema.json   | 25
> ++
> >  qmp.c  | 13 +++
> >  stubs/Makefile.objs|  2 +-
> >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +
> >  6 files changed, 50 insertions(+), 1 deletion(-)
> >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
> >
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index ea67b461c2..1df8b7ee57 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
> >  return cap.size;
> >  }
> >
> > +uint64_t get_existing_hotpluggable_memory_size(void)
> > +{
> > +return pc_existing_dimms_capacity(_abort);
> > +}
> > +
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> >  {
> >  MemoryDeviceInfoList ***prev = opaque;
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index 1e483f2670..52c6b5e641 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int
> max_slots, Error **errp);
> >
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
> >  uint64_t pc_existing_dimms_capacity(Error **errp);
> > +uint64_t get_existing_hotpluggable_memory_size(void);
> >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> >   MemoryRegion *mr, uint64_t align, Error
> **errp);
> >  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 9c6c3e1a53..bbedf1a7bc 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4407,6 +4407,31 @@
> >'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
> >  '*unavailable-features': [ 'str' ], 'typename': 'str' } }
> >
> > +##
> > +# @MemoryInfo:
> > +#
> > +# Actual memory information in bytes.
> > +#
> > +# @base-memory: size of "base" memory specified with command line
> > +#   option -m.
> > +#
> > +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'struct': 'MemoryInfo',
> > +  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' }
> }
> > +
> > +##
> > +# @query-memory-size-summary:
> > +#
> > +# Return the amount of initially allocated and hot-plugged (if
> > +# enabled) memory in bytes.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> > +
> >  

Re: [Qemu-devel] [PATCH v2 for-2.10 1/3] target/arm: Correct exclusive store cmpxchg memop mask

[Philippe Mathieu-Daudé]

On 08/15/2017 11:57 AM, Richard Henderson wrote:

From: Alistair Francis 

When we perform the atomic_cmpxchg operation we want to perform the
operation on a pair of 32-bit registers. Previously we were just passing
the register size in which was set to MO_32. This would result in the
high register to be ignored. To fix this issue we hardcode the size to
be 64-bits long when operating on 32-bit pairs.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Alistair Francis 
Message-Id: 

Signed-off-by: Richard Henderson 


Reviewed-by: Philippe Mathieu-Daudé 


---
  target/arm/translate-a64.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 58ed4c6d05..113e2e172b 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, 
int rt, int rt2,
  tcg_gen_concat32_i64(val, cpu_exclusive_val, cpu_exclusive_high);
  tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
 get_mem_index(s),
-   size | MO_ALIGN | s->be_data);
+   MO_64 | MO_ALIGN | s->be_data);
  tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
  tcg_temp_free_i64(val);
  } else if (s->be_data == MO_LE) {

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester

[Cornelia Huck]

On Fri, 11 Aug 2017 07:57:54 +0200
Thomas Huth  wrote:

> The first patch improves the buffer handling in the pxe tester a
> little bit by allocating a separate buffer on the heap for each
> architecture. This also gets rid of the huge pre-initialized
> array in the tester, shrinking the size of the executable by
> half of a megabyte!
> The second patch adds s390x support to the pxe tester. Starting
> with QEMU 2.10, the guest firmware on s390x can now net-boot via
> TFTP, too, so we can automatically test this code in the pxe tester.
> 
> v2: Adressed Cornelia's review feedback from the first version, e.g.:
>  - Use g_malloc0() instead of g_malloc()
>  - Use sizeof with parentheses
> 
> Thomas Huth (2):
>   tests/boot-sector: Do not overwrite the x86 buffer on other
> architectures
>   tests/pxe: Check virtio-net-ccw on s390x
> 
>  tests/Makefile.include |  1 +
>  tests/boot-sector.c| 61 
> +-
>  tests/pxe-test.c   |  7 ++
>  3 files changed, 54 insertions(+), 15 deletions(-)
> 

It's that question again: Who picks this up? :)

I can take it through the s390 tree if nobody else wants it.

Re: [Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Eric Blake]

On 08/15/2017 09:57 AM, Richard Henderson wrote:
> From: Alistair Francis 
> 
> Acording to the ARM ARM exclusive loads require the same allignment as

s/Acording/According/
s/allignmnet/alignment/

> exclusive stores. Let's update the memops used for the load to match
> that of the store. This adds the alignment requirement to the memops.
> 
> Reviewed-by: Edgar E. Iglesias 
> Signed-off-by: Alistair Francis 
> [rth: Require 16-byte alignment for 64-bit LDXP.]
> Signed-off-by: Richard Henderson 
> ---

-- 
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 v2 for-2.11 2/2] tests/pxe: Check virtio-net-ccw on s390x

[Cornelia Huck]

On Fri, 11 Aug 2017 07:57:56 +0200
Thomas Huth  wrote:

> Now that we've got a firmware that can do TFTP booting on s390x (i.e.
> the pc-bios/s390-netboot.img), we can enable the PXE tester for this
> architecture, too.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/Makefile.include |  1 +
>  tests/boot-sector.c| 20 
>  tests/pxe-test.c   |  7 +++
>  3 files changed, 28 insertions(+)

Reviewed-by: Cornelia Huck 

[Qemu-devel] [PULL 6/7] iotests: Add non-shared storage migration case 192

[Eric Blake]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Message-Id: <20170815130740.31229-5-f...@redhat.com>
Tested-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/192 | 63 ++
 tests/qemu-iotests/192.out |  7 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 71 insertions(+)
 create mode 100755 tests/qemu-iotests/192
 create mode 100644 tests/qemu-iotests/192.out

diff --git a/tests/qemu-iotests/192 b/tests/qemu-iotests/192
new file mode 100755
index 00..b50a2c0c8e
--- /dev/null
+++ b/tests/qemu-iotests/192
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# Test NBD export with -incoming (non-shared storage migration use case from
+# libvirt)
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=f...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
+size=64M
+_make_test_img $size
+
+{
+echo "nbd_server_start unix:$TEST_DIR/nbd"
+echo "nbd_server_add -w drive0"
+echo "q"
+} | $QEMU -nodefaults -display none -monitor stdio \
+-drive format=$IMGFMT,file=$TEST_IMG,if=ide,id=drive0 \
+-incoming defer 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/192.out b/tests/qemu-iotests/192.out
new file mode 100644
index 00..1e0be4c4d7
--- /dev/null
+++ b/tests/qemu-iotests/192.out
@@ -0,0 +1,7 @@
+QA output created by 192
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) nbd_server_start unix:TEST_DIR/nbd
+(qemu) nbd_server_add -w drive0
+(qemu) q
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1848077932..afbdc427ea 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -186,3 +186,4 @@
 188 rw auto quick
 189 rw auto
 190 rw auto quick
+192 rw auto quick
-- 
2.13.5

[Qemu-devel] [PULL 4/7] nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

[Eric Blake]

From: Kevin Wolf 

The "inactive" state of BDS affects whether the permissions can be
granted, we must call bdrv_invalidate_cache before bdrv_set_perm to
support "-incoming defer" case.

Reported-by: Christian Ehrhardt 
Signed-off-by: Fam Zheng 
Message-Id: <20170815130740.31229-3-f...@redhat.com>
Signed-off-by: Eric Blake 
---
 nbd/server.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 82a78bf439..993ade30bb 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1045,11 +1045,22 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
dev_offset, off_t size,
   bool writethrough, BlockBackend *on_eject_blk,
   Error **errp)
 {
+AioContext *ctx;
 BlockBackend *blk;
 NBDExport *exp = g_malloc0(sizeof(NBDExport));
 uint64_t perm;
 int ret;

+/*
+ * NBD exports are used for non-shared storage migration.  Make sure
+ * that BDRV_O_INACTIVE is cleared and the image is ready for write
+ * access since the export could be available before migration handover.
+ */
+ctx = bdrv_get_aio_context(bs);
+aio_context_acquire(ctx);
+bdrv_invalidate_cache(bs, NULL);
+aio_context_release(ctx);
+
 /* Don't allow resize while the NBD server is running, otherwise we don't
  * care what happens with the node. */
 perm = BLK_PERM_CONSISTENT_READ;
@@ -1087,15 +1098,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
dev_offset, off_t size,
 exp->eject_notifier.notify = nbd_eject_notifier;
 blk_add_remove_bs_notifier(on_eject_blk, >eject_notifier);
 }
-
-/*
- * NBD exports are used for non-shared storage migration.  Make sure
- * that BDRV_O_INACTIVE is cleared and the image is ready for write
- * access since the export could be available before migration handover.
- */
-aio_context_acquire(exp->ctx);
-blk_invalidate_cache(blk, NULL);
-aio_context_release(exp->ctx);
 return exp;

 fail:
-- 
2.13.5

[Qemu-devel] [PULL 5/7] block-backend: Defer shared_perm tightening migration completion

[Eric Blake]

From: Fam Zheng 

As in the case of nbd_export_new(), bdrv_invalidate_cache() can be
called when migration is still in progress. In this case we are not
ready to tighten the shared permissions fenced by blk->disable_perm.

Defer to a VM state change handler.

Signed-off-by: Fam Zheng 
Message-Id: <20170815130740.31229-4-f...@redhat.com>
Signed-off-by: Eric Blake 
---
 block/block-backend.c | 41 +
 1 file changed, 41 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 968438c149..e9798e897d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -20,6 +20,7 @@
 #include "qapi-event.h"
 #include "qemu/id.h"
 #include "trace.h"
+#include "migration/misc.h"

 /* Number of coroutines to reserve per attached device model */
 #define COROUTINE_POOL_RESERVATION 64
@@ -68,6 +69,7 @@ struct BlockBackend {
 NotifierList remove_bs_notifiers, insert_bs_notifiers;

 int quiesce_counter;
+VMChangeStateEntry *vmsh;
 };

 typedef struct BlockBackendAIOCB {
@@ -129,6 +131,23 @@ static const char *blk_root_get_name(BdrvChild *child)
 return blk_name(child->opaque);
 }

+static void blk_vm_state_changed(void *opaque, int running, RunState state)
+{
+Error *local_err = NULL;
+BlockBackend *blk = opaque;
+
+if (state == RUN_STATE_INMIGRATE) {
+return;
+}
+
+qemu_del_vm_change_state_handler(blk->vmsh);
+blk->vmsh = NULL;
+blk_set_perm(blk, blk->perm, blk->shared_perm, _err);
+if (local_err) {
+error_report_err(local_err);
+}
+}
+
 /*
  * Notifies the user of the BlockBackend that migration has completed. qdev
  * devices can tighten their permissions in response (specifically revoke
@@ -147,6 +166,24 @@ static void blk_root_activate(BdrvChild *child, Error 
**errp)

 blk->disable_perm = false;

+blk_set_perm(blk, blk->perm, BLK_PERM_ALL, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+blk->disable_perm = true;
+return;
+}
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+/* Activation can happen when migration process is still active, for
+ * example when nbd_server_add is called during non-shared storage
+ * migration. Defer the shared_perm update to migration completion. */
+if (!blk->vmsh) {
+blk->vmsh = qemu_add_vm_change_state_handler(blk_vm_state_changed,
+ blk);
+}
+return;
+}
+
 blk_set_perm(blk, blk->perm, blk->shared_perm, _err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -291,6 +328,10 @@ static void blk_delete(BlockBackend *blk)
 if (blk->root) {
 blk_remove_bs(blk);
 }
+if (blk->vmsh) {
+qemu_del_vm_change_state_handler(blk->vmsh);
+blk->vmsh = NULL;
+}
 assert(QLIST_EMPTY(>remove_bs_notifiers.notifiers));
 assert(QLIST_EMPTY(>insert_bs_notifiers.notifiers));
 QTAILQ_REMOVE(_backends, blk, link);
-- 
2.13.5

[Qemu-devel] [PULL 3/7] stubs: Add vm state change handler stubs

[Eric Blake]

From: Fam Zheng 

They will be used by BlockBackend code in block-obj-y, which doesn't
always get linked with common-obj-y. Add stubs to keep ld happy.

Signed-off-by: Fam Zheng 
Message-Id: <20170815130740.31229-2-f...@redhat.com>
Signed-off-by: Eric Blake 
---
 stubs/change-state-handler.c | 14 ++
 stubs/Makefile.objs  |  1 +
 2 files changed, 15 insertions(+)
 create mode 100644 stubs/change-state-handler.c

diff --git a/stubs/change-state-handler.c b/stubs/change-state-handler.c
new file mode 100644
index 00..01b1c6986d
--- /dev/null
+++ b/stubs/change-state-handler.c
@@ -0,0 +1,14 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "sysemu/sysemu.h"
+
+VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
+ void *opaque)
+{
+return NULL;
+}
+
+void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
+{
+/* Nothing to do. */
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f5b47bfd74..e69c217aff 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,6 +19,7 @@ stub-obj-y += is-daemonized.o
 stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 stub-obj-y += machine-init-done.o
 stub-obj-y += migr-blocker.o
+stub-obj-y += change-state-handler.o
 stub-obj-y += monitor.o
 stub-obj-y += notify-event.o
 stub-obj-y += qtest.o
-- 
2.13.5

[Qemu-devel] [PULL 7/7] nbd-client: Fix regression when server sends garbage

[Eric Blake]

When we switched NBD to use coroutines for qemu 2.9 (in particular,
commit a12a712a), we introduced a regression: if a server sends us
garbage (such as a corrupted magic number), we quit the read loop
but do not stop sending further queued commands, resulting in the
client hanging when it never reads the response to those additional
commands.  In qemu 2.8, we properly detected that the server is no
longer reliable, and cancelled all existing pending commands with
EIO, then tore down the socket so that all further command attempts
get EPIPE.

Restore the proper behavior of quitting (almost) all communication
with a broken server: Once we know we are out of sync or otherwise
can't trust the server, we must assume that any further incoming
data is unreliable and therefore end all pending commands with EIO,
and quit trying to send any further commands.  As an exception, we
still (try to) send NBD_CMD_DISC to let the server know we are going
away (in part, because it is easier to do that than to further
refactor nbd_teardown_connection, and in part because it is the
only command where we do not have to wait for a reply).

Based on a patch by Vladimir Sementsov-Ogievskiy.

A malicious server can be created with the following hack,
followed by setting NBD_SERVER_DEBUG to a non-zero value in the
environment when running qemu-nbd:

| --- a/nbd/server.c
| +++ b/nbd/server.c
| @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply 
*reply, Error **errp)
|  stl_be_p(buf + 4, reply->error);
|  stq_be_p(buf + 8, reply->handle);
|
| +static int debug;
| +static int count;
| +if (!count++) {
| +const char *str = getenv("NBD_SERVER_DEBUG");
| +if (str) {
| +debug = atoi(str);
| +}
| +}
| +if (debug && !(count % debug)) {
| +buf[0] = 0;
| +}
|  return nbd_write(ioc, buf, sizeof(buf), errp);
|  }

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
Message-Id: <20170814213426.24681-1-ebl...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
---
 block/nbd-client.h |  1 +
 block/nbd-client.c | 17 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..1935ffbcaa 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {

 Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
 NBDReply reply;
+bool quit;
 } NBDClientSession;

 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..422ecb4307 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 int ret;
 Error *local_err = NULL;

-for (;;) {
+while (!s->quit) {
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, >reply, _err);
 if (ret < 0) {
@@ -107,6 +107,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }

+if (ret < 0) {
+s->quit = true;
+}
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -135,6 +138,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);

+if (s->quit) {
+qemu_co_mutex_unlock(>send_mutex);
+return -EIO;
+}
 if (!s->ioc) {
 qemu_co_mutex_unlock(>send_mutex);
 return -EPIPE;
@@ -143,7 +150,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->quit) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
   NULL);
 if (ret != request->len) {
@@ -154,6 +161,9 @@ static int nbd_co_send_request(BlockDriverState *bs,
 } else {
 rc = nbd_send_request(s->ioc, request);
 }
+if (rc < 0) {
+s->quit = true;
+}
 qemu_co_mutex_unlock(>send_mutex);
 return rc;
 }
@@ -168,8 +178,7 @@ static void nbd_co_receive_reply(NBDClientSession *s,
 /* Wait until we're woken up by nbd_read_reply_entry.  */
 qemu_coroutine_yield();
 *reply = s->reply;
-if (reply->handle != request->handle ||
-!s->ioc) {
+if (reply->handle != request->handle || !s->ioc || s->quit) {
 reply->error = EIO;
 } else {
 if (qiov && reply->error == 0) {
-- 
2.13.5

[Qemu-devel] [PULL 1/7] nbd: Fix trace message for disconnect

[Eric Blake]

NBD_CMD_DISC is a disconnect request, not a data discard request.

Signed-off-by: Eric Blake 
Message-Id: <20170811015749.20365-1-ebl...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
---
 nbd/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/common.c b/nbd/common.c
index a2f28f2eec..e288d1b972 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -182,7 +182,7 @@ const char *nbd_cmd_lookup(uint16_t cmd)
 case NBD_CMD_WRITE:
 return "write";
 case NBD_CMD_DISC:
-return "discard";
+return "disconnect";
 case NBD_CMD_FLUSH:
 return "flush";
 case NBD_CMD_TRIM:
-- 
2.13.5

[Qemu-devel] [PULL for 2.10-rc3 0/7] NBD changes for 2.10-rc3

[Eric Blake]

The following changes since commit 47025a0193f1f910300adfa443305ccf8482ef87:

  qxl: call qemu_spice_display_init_common for secondary devices (2017-08-15 
15:04:51 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2017-08-15

for you to fetch changes up to 72b6ffc76653214b69a94a7b1643ff80df134486:

  nbd-client: Fix regression when server sends garbage (2017-08-15 10:03:28 
-0500)


nbd patches for 2017-08-15

- Eric Blake: nbd: Fix trace message for disconnect
- Stefan Hajnoczi: qemu-iotests: step clock after each test iteration
- Fam Zheng: 0/4 block: Fix non-shared storage migration
- Eric Blake: nbd-client: Fix regression when server sends garbage


Eric Blake (2):
  nbd: Fix trace message for disconnect
  nbd-client: Fix regression when server sends garbage

Fam Zheng (3):
  stubs: Add vm state change handler stubs
  block-backend: Defer shared_perm tightening migration completion
  iotests: Add non-shared storage migration case 192

Kevin Wolf (1):
  nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

Stefan Hajnoczi (1):
  qemu-iotests: step clock after each test iteration

 block/nbd-client.h   |  1 +
 block/block-backend.c| 41 
 block/nbd-client.c   | 17 +---
 nbd/common.c |  2 +-
 nbd/server.c | 20 +++---
 stubs/change-state-handler.c | 14 ++
 stubs/Makefile.objs  |  1 +
 tests/qemu-iotests/093   |  4 +++
 tests/qemu-iotests/192   | 63 
 tests/qemu-iotests/192.out   |  7 +
 tests/qemu-iotests/group |  1 +
 11 files changed, 157 insertions(+), 14 deletions(-)
 create mode 100644 stubs/change-state-handler.c
 create mode 100755 tests/qemu-iotests/192
 create mode 100644 tests/qemu-iotests/192.out

-- 
2.13.5

[Qemu-devel] [PULL 2/7] qemu-iotests: step clock after each test iteration

[Eric Blake]

From: Stefan Hajnoczi 

The 093 throttling test submits twice as many requests as the throttle
limit in order to ensure that we reach the limit.  The remaining
requests are left in-flight at the end of each test iteration.

Commit 452589b6b47e8dc6353df257fc803dfc1383bed8 ("vl.c/exit: pause cpus
before closing block devices") exposed a hang in 093.  This happens
because requests are still in flight when QEMU terminates but
QEMU_CLOCK_VIRTUAL time is frozen.  bdrv_drain_all() hangs forever since
throttled requests cannot complete.

Step the clock at the end of each test iteration so in-flight requests
actually finish.  This solves the hang and is cleaner than leaving tests
in-flight.

Note that this could also be "fixed" by disabling throttling when drives
are closed in QEMU.  That approach has two issues:

1. We must drain requests before disabling throttling, so the hang
   cannot be easily avoided!

2. Any time QEMU disables throttling internally there is a chance that
   malicious users can abuse the code path to bypass throttling limits.

Therefore it makes more sense to fix the test case than to modify QEMU.

Signed-off-by: Stefan Hajnoczi 
Message-Id: <20170815130502.8736-1-stefa...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/093 | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 2ed393a548..ef3997206b 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -133,6 +133,10 @@ class ThrottleTestCase(iotests.QMPTestCase):
 self.assertTrue(check_limit(params['iops_rd'], rd_iops))
 self.assertTrue(check_limit(params['iops_wr'], wr_iops))

+# Allow remaining requests to finish.  We submitted twice as many to
+# ensure the throttle limit is reached.
+self.vm.qtest("clock_step %d" % ns)
+
 # Connect N drives to a VM and test I/O in all of them
 def test_all(self):
 params = {"bps": 4096,
-- 
2.13.5

[Qemu-devel] [PATCH v2 for-2.10 3/3] target/arm: Require alignment for load exclusive

[Richard Henderson]

From: Alistair Francis 

Acording to the ARM ARM exclusive loads require the same allignment as
exclusive stores. Let's update the memops used for the load to match
that of the store. This adds the alignment requirement to the memops.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Alistair Francis 
[rth: Require 16-byte alignment for 64-bit LDXP.]
Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index eac545e4f2..2200e25be0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1861,7 +1861,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
 g_assert(size >= 2);
 if (size == 2) {
 /* The pair must be single-copy atomic for the doubleword.  */
-memop |= MO_64;
+memop |= MO_64 | MO_ALIGN;
 tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
 if (s->be_data == MO_LE) {
 tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 0, 32);
@@ -1871,10 +1871,11 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
 tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 0, 32);
 }
 } else {
-/* The pair must be single-copy atomic for *each* doubleword,
-   but not the entire quadword.  */
+/* The pair must be single-copy atomic for *each* doubleword, not
+   the entire quadword, however it must be quadword aligned.  */
 memop |= MO_64;
-tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
+tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx,
+memop | MO_ALIGN_16);
 
 TCGv_i64 addr2 = tcg_temp_new_i64();
 tcg_gen_addi_i64(addr2, addr, 8);
@@ -1885,7 +1886,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, 
int rt2,
 tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
 }
 } else {
-memop |= size;
+memop |= size | MO_ALIGN;
 tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
 tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
 }
-- 
2.13.4

[Qemu-devel] [PATCH v2 for-2.10 1/3] target/arm: Correct exclusive store cmpxchg memop mask

[Richard Henderson]

From: Alistair Francis 

When we perform the atomic_cmpxchg operation we want to perform the
operation on a pair of 32-bit registers. Previously we were just passing
the register size in which was set to MO_32. This would result in the
high register to be ignored. To fix this issue we hardcode the size to
be 64-bits long when operating on 32-bit pairs.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Alistair Francis 
Message-Id: 

Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 58ed4c6d05..113e2e172b 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, 
int rt, int rt2,
 tcg_gen_concat32_i64(val, cpu_exclusive_val, cpu_exclusive_high);
 tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
get_mem_index(s),
-   size | MO_ALIGN | s->be_data);
+   MO_64 | MO_ALIGN | s->be_data);
 tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
 tcg_temp_free_i64(val);
 } else if (s->be_data == MO_LE) {
-- 
2.13.4

[Qemu-devel] [PATCH v2 for-2.10 2/3] target/arm: Correct load exclusive pair atomicity

[Richard Henderson]

We are not providing the required single-copy atomic semantics for
the 64-bit operation that is the 32-bit paired load.

At the same time, leave the entire 64-bit value in cpu_exclusive_val
and stop writing to cpu_exclusive_high.  This means that we do not
have to re-assemble the 64-bit quantity when it comes time to store.

At the same time, drop a redundant temporary and perform all loads
directly into the cpu_exclusive_* globals.

Tested-by: Alistair Francis 
Reviewed-by: Alistair Francis 
Signed-off-by: Richard Henderson 
---
 target/arm/translate-a64.c | 60 --
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 113e2e172b..eac545e4f2 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1853,29 +1853,42 @@ static void disas_b_exc_sys(DisasContext *s, uint32_t 
insn)
 static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
TCGv_i64 addr, int size, bool is_pair)
 {
-TCGv_i64 tmp = tcg_temp_new_i64();
-TCGMemOp memop = s->be_data + size;
+int idx = get_mem_index(s);
+TCGMemOp memop = s->be_data;
 
 g_assert(size <= 3);
-tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), memop);
-
 if (is_pair) {
-TCGv_i64 addr2 = tcg_temp_new_i64();
-TCGv_i64 hitmp = tcg_temp_new_i64();
-
 g_assert(size >= 2);
-tcg_gen_addi_i64(addr2, addr, 1 << size);
-tcg_gen_qemu_ld_i64(hitmp, addr2, get_mem_index(s), memop);
-tcg_temp_free_i64(addr2);
-tcg_gen_mov_i64(cpu_exclusive_high, hitmp);
-tcg_gen_mov_i64(cpu_reg(s, rt2), hitmp);
-tcg_temp_free_i64(hitmp);
-}
+if (size == 2) {
+/* The pair must be single-copy atomic for the doubleword.  */
+memop |= MO_64;
+tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
+if (s->be_data == MO_LE) {
+tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 0, 32);
+tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 32, 
32);
+} else {
+tcg_gen_extract_i64(cpu_reg(s, rt), cpu_exclusive_val, 32, 32);
+tcg_gen_extract_i64(cpu_reg(s, rt2), cpu_exclusive_val, 0, 32);
+}
+} else {
+/* The pair must be single-copy atomic for *each* doubleword,
+   but not the entire quadword.  */
+memop |= MO_64;
+tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
 
-tcg_gen_mov_i64(cpu_exclusive_val, tmp);
-tcg_gen_mov_i64(cpu_reg(s, rt), tmp);
+TCGv_i64 addr2 = tcg_temp_new_i64();
+tcg_gen_addi_i64(addr2, addr, 8);
+tcg_gen_qemu_ld_i64(cpu_exclusive_high, addr2, idx, memop);
+tcg_temp_free_i64(addr2);
 
-tcg_temp_free_i64(tmp);
+tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
+tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high);
+}
+} else {
+memop |= size;
+tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, memop);
+tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val);
+}
 tcg_gen_mov_i64(cpu_exclusive_addr, addr);
 }
 
@@ -1908,14 +1921,15 @@ static void gen_store_exclusive(DisasContext *s, int 
rd, int rt, int rt2,
 tmp = tcg_temp_new_i64();
 if (is_pair) {
 if (size == 2) {
-TCGv_i64 val = tcg_temp_new_i64();
-tcg_gen_concat32_i64(tmp, cpu_reg(s, rt), cpu_reg(s, rt2));
-tcg_gen_concat32_i64(val, cpu_exclusive_val, cpu_exclusive_high);
-tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
+if (s->be_data == MO_LE) {
+tcg_gen_concat32_i64(tmp, cpu_reg(s, rt), cpu_reg(s, rt2));
+} else {
+tcg_gen_concat32_i64(tmp, cpu_reg(s, rt2), cpu_reg(s, rt));
+}
+tcg_gen_atomic_cmpxchg_i64(tmp, addr, cpu_exclusive_val, tmp,
get_mem_index(s),
MO_64 | MO_ALIGN | s->be_data);
-tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
-tcg_temp_free_i64(val);
+tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, cpu_exclusive_val);
 } else if (s->be_data == MO_LE) {
 gen_helper_paired_cmpxchg64_le(tmp, cpu_env, addr, cpu_reg(s, rt),
cpu_reg(s, rt2));
-- 
2.13.4

[Qemu-devel] [PATCH v2 for-2.10 0/3] Fixup logic for exclusive pair

[Richard Henderson]

In reviewing my previous patch, Peter pointed out that it is
CONSTRAINED UNPREDICTABLE what happens when you mix the number
of registers in a LDX[PR] + STX[RP] pair.  So most of the bug
that I thought that I was fixing isn't a bug at all.

That said, the patch does still fix a real bug wrt single-copy
semantics, so patch 2 is largely unchanged; the commit message
is re-worded.

I also un-squashed Alistair's original patches and dropped the
tcg/tcg-op.c change, to be revisited for 2.11.


r~


Alistair Francis (2):
  target/arm: Correct exclusive store cmpxchg memop mask
  target/arm: Require alignment for load exclusive

Richard Henderson (1):
  target/arm: Correct load exclusive pair atomicity

 target/arm/translate-a64.c | 63 --
 1 file changed, 39 insertions(+), 24 deletions(-)

-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 v2] nbd-client: Fix regression when server sends garbage

[Eric Blake]

On 08/15/2017 09:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.08.2017 00:34, Eric Blake wrote:
>> When we switched NBD to use coroutines for qemu 2.9 (in particular,
>> commit a12a712a), we introduced a regression: if a server sends us
>> garbage (such as a corrupted magic number), we quit the read loop
>> but do not stop sending further queued commands, resulting in the
>> client hanging when it never reads the response to those additional
>> commands.  In qemu 2.8, we properly detected that the server is no
>> longer reliable, and cancelled all existing pending commands with
>> EIO, then tore down the socket so that all further command attempts
>> get EPIPE.
>>
>> Restore the proper behavior of quitting (almost) all communication
>> with a broken server: Once we know we are out of sync or otherwise
>> can't trust the server, we must assume that any further incoming
>> data is unreliable and therefore end all pending commands with EIO,
>> and quit trying to send any further commands.  As an exception, we
>> still (try to) send NBD_CMD_DISC to let the server know we are going
>> away (in part, because it is easier to do that than to further
>> refactor nbd_teardown_connection, and in part because it is the
>> only command where we do not have to wait for a reply).
>>
>> Based on a patch by Vladimir Sementsov-Ogievskiy.
>>

>> +++ b/block/nbd-client.c
>> @@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void
>> *opaque)
>>   int ret;
>>   Error *local_err = NULL;
>>
>> -for (;;) {
>> +while (!s->quit) {
> 
> looks like quit will never be true here
> 
>>   assert(s->reply.handle == 0);
>>   ret = nbd_receive_reply(s->ioc, >reply, _err);
>>   if (ret < 0) {
>> @@ -107,6 +107,9 @@ static coroutine_fn void nbd_read_reply_entry(void
>> *opaque)
>>   qemu_coroutine_yield();
>>   }
>>
>> +if (ret < 0) {
>> +s->quit = true;
> 
> so, you set quit only here.. if we fail on some write, reading coroutine
> will not
> know about it and will continue reading..

Looks like you are correct - your version set the flag in more places
than me, but it looks like you're right that we DO want to set the flag
when writing hits a known failure.

Here's what I plan to squash in:

diff --git i/block/nbd-client.c w/block/nbd-client.c
index bb17e3da45..422ecb4307 100644
--- i/block/nbd-client.c
+++ w/block/nbd-client.c
@@ -161,6 +161,9 @@ static int nbd_co_send_request(BlockDriverState *bs,
 } else {
 rc = nbd_send_request(s->ioc, request);
 }
+if (rc < 0) {
+s->quit = true;
+}
 qemu_co_mutex_unlock(>send_mutex);
 return rc;
 }

-- 
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 v2 for-2.11 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Cornelia Huck]

On Fri, 11 Aug 2017 07:57:55 +0200
Thomas Huth  wrote:

> Re-using the boot_sector code buffer from x86 for other architectures
> is not very nice, especially if we add more architectures later. It's
> also ugly that the test uses a huge pre-initialized array at all - the
> size of the executable is very huge due to this array. So let's use a
> separate buffer for each architecture instead, allocated from the heap,
> so that we really just use the memory that we need.
> 
> Suggested-by: Michael Tsirkin 
> Signed-off-by: Thomas Huth 
> ---
>  tests/boot-sector.c | 41 ++---
>  1 file changed, 26 insertions(+), 15 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 v2] nbd-client: Fix regression when server sends garbage

[Stefan Hajnoczi]

On Mon, Aug 14, 2017 at 04:34:26PM -0500, Eric Blake wrote:
> When we switched NBD to use coroutines for qemu 2.9 (in particular,
> commit a12a712a), we introduced a regression: if a server sends us
> garbage (such as a corrupted magic number), we quit the read loop
> but do not stop sending further queued commands, resulting in the
> client hanging when it never reads the response to those additional
> commands.  In qemu 2.8, we properly detected that the server is no
> longer reliable, and cancelled all existing pending commands with
> EIO, then tore down the socket so that all further command attempts
> get EPIPE.
> 
> Restore the proper behavior of quitting (almost) all communication
> with a broken server: Once we know we are out of sync or otherwise
> can't trust the server, we must assume that any further incoming
> data is unreliable and therefore end all pending commands with EIO,
> and quit trying to send any further commands.  As an exception, we
> still (try to) send NBD_CMD_DISC to let the server know we are going
> away (in part, because it is easier to do that than to further
> refactor nbd_teardown_connection, and in part because it is the
> only command where we do not have to wait for a reply).
> 
> Based on a patch by Vladimir Sementsov-Ogievskiy.
> 
> A malicious server can be created with the following hack,
> followed by setting NBD_SERVER_DEBUG to a non-zero value in the
> environment when running qemu-nbd:
> 
> | --- a/nbd/server.c
> | +++ b/nbd/server.c
> | @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply 
> *reply, Error **errp)
> |  stl_be_p(buf + 4, reply->error);
> |  stq_be_p(buf + 8, reply->handle);
> |
> | +static int debug;
> | +static int count;
> | +if (!count++) {
> | +const char *str = getenv("NBD_SERVER_DEBUG");
> | +if (str) {
> | +debug = atoi(str);
> | +}
> | +}
> | +if (debug && !(count % debug)) {
> | +buf[0] = 0;
> | +}
> |  return nbd_write(ioc, buf, sizeof(buf), errp);
> |  }
> 
> Reported-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Eric Blake 
> ---
> 
> Supercedes both Vladimir and my earlier attempts:
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02131.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html
> 
>  block/nbd-client.h |  1 +
>  block/nbd-client.c | 14 ++
>  2 files changed, 11 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.10 v2] nbd-client: Fix regression when server sends garbage

[Vladimir Sementsov-Ogievskiy]

15.08.2017 00:34, Eric Blake wrote:

When we switched NBD to use coroutines for qemu 2.9 (in particular,
commit a12a712a), we introduced a regression: if a server sends us
garbage (such as a corrupted magic number), we quit the read loop
but do not stop sending further queued commands, resulting in the
client hanging when it never reads the response to those additional
commands.  In qemu 2.8, we properly detected that the server is no
longer reliable, and cancelled all existing pending commands with
EIO, then tore down the socket so that all further command attempts
get EPIPE.

Restore the proper behavior of quitting (almost) all communication
with a broken server: Once we know we are out of sync or otherwise
can't trust the server, we must assume that any further incoming
data is unreliable and therefore end all pending commands with EIO,
and quit trying to send any further commands.  As an exception, we
still (try to) send NBD_CMD_DISC to let the server know we are going
away (in part, because it is easier to do that than to further
refactor nbd_teardown_connection, and in part because it is the
only command where we do not have to wait for a reply).

Based on a patch by Vladimir Sementsov-Ogievskiy.

A malicious server can be created with the following hack,
followed by setting NBD_SERVER_DEBUG to a non-zero value in the
environment when running qemu-nbd:

| --- a/nbd/server.c
| +++ b/nbd/server.c
| @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply 
*reply, Error **errp)
|  stl_be_p(buf + 4, reply->error);
|  stq_be_p(buf + 8, reply->handle);
|
| +static int debug;
| +static int count;
| +if (!count++) {
| +const char *str = getenv("NBD_SERVER_DEBUG");
| +if (str) {
| +debug = atoi(str);
| +}
| +}
| +if (debug && !(count % debug)) {
| +buf[0] = 0;
| +}
|  return nbd_write(ioc, buf, sizeof(buf), errp);
|  }

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
---

Supercedes both Vladimir and my earlier attempts:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02131.html
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html

  block/nbd-client.h |  1 +
  block/nbd-client.c | 14 ++
  2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..1935ffbcaa 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {

  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
  NBDReply reply;
+bool quit;
  } NBDClientSession;

  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..bb17e3da45 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  int ret;
  Error *local_err = NULL;

-for (;;) {
+while (!s->quit) {


looks like quit will never be true here


  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, >reply, _err);
  if (ret < 0) {
@@ -107,6 +107,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  qemu_coroutine_yield();
  }

+if (ret < 0) {
+s->quit = true;


so, you set quit only here.. if we fail on some write, reading coroutine 
will not

know about it and will continue reading..


+}
  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }
@@ -135,6 +138,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
  assert(i < MAX_NBD_REQUESTS);
  request->handle = INDEX_TO_HANDLE(s, i);

+if (s->quit) {
+qemu_co_mutex_unlock(>send_mutex);
+return -EIO;
+}
  if (!s->ioc) {
  qemu_co_mutex_unlock(>send_mutex);
  return -EPIPE;
@@ -143,7 +150,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
  if (qiov) {
  qio_channel_set_cork(s->ioc, true);
  rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->quit) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
NULL);
  if (ret != request->len) {
@@ -168,8 +175,7 @@ static void nbd_co_receive_reply(NBDClientSession *s,
  /* Wait until we're woken up by nbd_read_reply_entry.  */
  qemu_coroutine_yield();
  *reply = s->reply;
-if (reply->handle != request->handle ||
-!s->ioc) {
+if (reply->handle != request->handle || !s->ioc || s->quit) {
  reply->error = EIO;
  } else {
  if (qiov && reply->error == 0) {



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 00/12] Convert over to use keycodemapdb

[Programmingkid]

> On Aug 15, 2017, at 9:49 AM, Daniel P. Berrange  wrote:
> 
> On Tue, Aug 15, 2017 at 09:49:00AM -0400, Programmingkid wrote:
>> 
>>> On Aug 15, 2017, at 4:38 AM, Daniel P. Berrange  wrote:
>>> 
>>> On Mon, Aug 14, 2017 at 02:46:22PM -0400, Programmingkid wrote:
 Sorry but there appears to be an issue with your patchset. I ran this 
 command:
 
 ./configure --target-list=ppc-softmmu,i386-softmmu
 
 Then saw this error message: 
 
 error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.
 
 Running 'make' I saw this error message:
 
 make: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', 
 needed by `Makefile'.  Stop.
 make: *** Waiting for unfinished jobs
 
 Running the 'make clean' command produces this error message:
 
 make[1]: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', 
 needed by `Makefile'.  Stop.
 make: *** [clean] Error 1
 
 Perhaps a missing patch?
>>> 
>>> No, its a missing "git submodule update --init ui/keycodemapdb" call
>> 
>> Thank you for replying. I tried this command and saw this error message:
>> 
>> error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.
>> 
>> Note: I'm using version 3. Will test version 4 soon. 
>> 
>> I suggest adding the git submodule update info to your cover letter
>> saying it is required.
> 
> It isn't supposed to be required - its a bug, hence why the tests are
> failing.
> 
> 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 :|

Version four appears to have the same problem as version three. I still see 
this error message:

make: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', needed by 
`Makefile'.  Stop.
make: *** Waiting for unfinished jobs

Re: [Qemu-devel] [PATCH 01/26] qapi: fix type_seen key error

[Markus Armbruster]

Marc-André Lureau  writes:

> The type_seen member can be of a different type than the 'qtype' being
> checked, since a string create several conflicts. Lookup the real
> conflicting type in the conflict set, that one must be present in
> type_seen.
>
> This fixes the following error, reproducible with the modified test:
>
> Traceback (most recent call last):
>   File "/home/elmarco/src/qq/tests/qapi-schema/test-qapi.py", line 56, in 
> 
> schema = QAPISchema(sys.argv[1])
>   File "/home/elmarco/src/qq/scripts/qapi.py", line 1470, in __init__
> self.exprs = check_exprs(parser.exprs)
>   File "/home/elmarco/src/qq/scripts/qapi.py", line 959, in check_exprs
> check_alternate(expr, info)
>   File "/home/elmarco/src/qq/scripts/qapi.py", line 831, in check_alternate
> % (name, key, types_seen[qtype]))
> KeyError: 'QTYPE_QSTRING'
>
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/qapi.py  | 4 +++-
>  tests/qapi-schema/alternate-conflict-string.json | 4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8aa2775f12..4ecc19e944 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -825,7 +825,9 @@ def check_alternate(expr, info):
>  else:
>  conflicting.add('QTYPE_QNUM')
>  conflicting.add('QTYPE_QBOOL')
> -if conflicting & set(types_seen):
> +conflict = conflicting & set(types_seen)
> +if conflict:
> +qtype = list(conflict)[0]
>  raise QAPISemError(info, "Alternate '%s' member '%s' can't "
> "be distinguished from member '%s'"
> % (name, key, types_seen[qtype]))

I see.

The fix is uninvasive, but I'm not thrilled by repurposing @qtype, and
the meaning of list(dict) is less obvious than dict.keys().  What about

   conflict = conflicting & set(types_seen)
   if conflict:
   conflicting_member_name = conflict.keys()[0]
   raise QAPISemError(info, "Alternate '%s' member '%s' can't "
  "be distinguished from member '%s'"
  % (name, key, conflicting_member_name))

Alternatively, list all conflicting names (I wouldn't bother).

> diff --git a/tests/qapi-schema/alternate-conflict-string.json 
> b/tests/qapi-schema/alternate-conflict-string.json
> index 85adbd4adc..bb2702978e 100644
> --- a/tests/qapi-schema/alternate-conflict-string.json
> +++ b/tests/qapi-schema/alternate-conflict-string.json
> @@ -1,4 +1,4 @@
>  # alternate branches of 'str' type conflict with all scalar types
>  { 'alternate': 'Alt',
> -  'data': { 'one': 'str',
> -'two': 'int' } }
> +  'data': { 'one': 'int',
> +'two': 'str' } }

I had to think for several minutes to convince myself that this is a
better test, not just a test that happens to demonstrate a particular
bug.  It's hot, I'm slow :)

Re: [Qemu-devel] [PATCH for 2.10?] qxl: call qemu_spice_display_init_common for secondary devices

[Peter Maydell]

On 15 August 2017 at 14:39, Stefan Hajnoczi  wrote:
> On Tue, Aug 15, 2017 at 01:15:52AM +0200, Paolo Bonzini wrote:
>> Fixes this 2.10 regression:
>>
>>   $ qemu-system-x86_64  -cpu host -m 6144 -vga qxl -device qxl
>>   qemu-system-x86_64: util/qemu-thread-posix.c:64: qemu_mutex_lock: 
>> Assertion `mutex->initialized' failed.
>>
>> Reported-by: adema...@redhat.com
>> Cc: kra...@redhat.com
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  hw/display/qxl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I have audited the code and cannot see any bad side-effects.  Looks good
> to me!
>
> Reviewed-by: Stefan Hajnoczi 

Thanks; applied to master.

-- PMM

[Qemu-devel] [PATCH for-2.10] mmio-interface: Mark as not user creatable

[Peter Maydell]

The mmio-interface device is not something we want to allow
users to create on the command line:
 * it is intended as an implementation detail of the memory
   subsystem, which gets created and deleted by that
   subsystem on demand; it makes no sense to create it
   by hand on the command line
 * it uses a pointer property 'host_ptr' which can't be
   set on the command line

Mark the device as not user_creatable to avoid confusion.

Signed-off-by: Peter Maydell 
---
 hw/misc/mmio_interface.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
index da154e5..894e980 100644
--- a/hw/misc/mmio_interface.c
+++ b/hw/misc/mmio_interface.c
@@ -111,6 +111,11 @@ static void mmio_interface_class_init(ObjectClass *oc, 
void *data)
 dc->realize = mmio_interface_realize;
 dc->unrealize = mmio_interface_unrealize;
 dc->props = mmio_interface_properties;
+/* Reason: pointer property "host_ptr", and this device
+ * is an implementation detail of the memory subsystem,
+ * not intended to be created directly by the user.
+ */
+dc->user_creatable = false;
 }
 
 static const TypeInfo mmio_interface_info = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.10 v2 4/4] iotests: Add non-shared storage migration case 192

[Eric Blake]

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  tests/qemu-iotests/192 | 63 
> ++
>  tests/qemu-iotests/192.out |  7 ++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 71 insertions(+)
>  create mode 100755 tests/qemu-iotests/192
>  create mode 100644 tests/qemu-iotests/192.out

I tested that this catches the changes made in both 2/4 (+Block node is
read-only) and 3/4 (+Conflicts with use by drive0 as 'root', which does
not allow 'write' on #block121) - so it looks like we've solved the issue.

Tested-by: Eric Blake 
Reviewed-by: Eric Blake 

I'll take this series through my NBD tree, and send a pull request
shortly in time for -rc3

-- 
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 for-2.10 v2 3/4] block-backend: Defer shared_perm tightening migration completion

[Eric Blake]

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> As in the case of nbd_export_new(), bdrv_invalidate_cache() can be
> called when migration is still in progress. In this case we are not
> ready to tighten the shared permissions fenced by blk->disable_perm.
> 
> Defer to a VM state change handler.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/block-backend.c | 41 +
>  1 file changed, 41 insertions(+)

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 v2 2/6] block: add options parameter to bdrv_new_open_driver()

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:52 PM CEST, Manos Pitsidianakis wrote:
> Allow passing a QDict *options parameter to bdrv_new_open_driver() so
> that it can be used if a driver needs it upon creation. The previous
> behaviour (empty bs->options and bs->explicit_options) remains when
> options is NULL.
>
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-devel] [PATCH for-2.10 v2 2/4] nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

[Eric Blake]

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> From: Kevin Wolf 
> 
> The "inactive" state of BDS affects whether the permissions can be
> granted, we must call bdrv_invalidate_cache before bdrv_set_perm to
> support "-incoming defer" case.
> 
> Reported-by: Christian Ehrhardt 
> Signed-off-by: Fam Zheng 
> ---
>  nbd/server.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 82a78bf439..b68b6fb535 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1045,11 +1045,22 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
> dev_offset, off_t size,
>bool writethrough, BlockBackend *on_eject_blk,
>Error **errp)
>  {
> +AioContext  *ctx;

Odd spacing; can fix up as maintainer.

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] [PULL 0/2] Tracing patches

[Peter Maydell]

On 15 August 2017 at 14:27, Stefan Hajnoczi <stefa...@redhat.com> wrote:
> The following changes since commit 5681da292242550f37ba4c03f46a8a6f8ee9278a:
>
>   Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170815' into 
> staging (2017-08-15 09:39:14 +0100)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/tracing-pull-request
>
> for you to fetch changes up to 840d8351774664d01c328d31ed33b0e2d85c866e:
>
>   simpletrace: fix flight recorder --no-header option (2017-08-15 12:50:29 
> +0100)
>
> 
>
> 
>
> Stefan Hajnoczi (2):
>   trace: use static event ID mapping in simpletrace.stp
>   simpletrace: fix flight recorder --no-header option
>
>  scripts/simpletrace.py   | 24 +++--
>  scripts/tracetool/format/simpletrace_stap.py | 31 
> ++--
>  2 files changed, 20 insertions(+), 35 deletions(-)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2 for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Eric Blake]

On 08/15/2017 02:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The only doubt: is it possible to hang on nbd_rwv because some fail in
>>> connection or server?
>> We _already_ have the bug that we are hanging in trying to talk to a
>> broken server, which is a regression introduced in 2.9 and not present
>> in 2.8.  And that's what I'm most worried about getting fixed before
>> 2.10 is released.
>>
>> I don't think that sending any more data to the server is necessarily
>> going to cause a hang, so much as trying to read data that is going to
>> be sent in reply or failing to manipulate the coroutine handlers
>> correctly (that is, our current hang is because even after we detect
>> failure, we are still sending NBD_CMD_FLUSH but no longer have a
>> coroutine in place to read the reply, so we no longer make progress to
>> the point of sending NBD_CMD_DISC and closing the connection).
> 
> but we will not try to read.
> However, I'm convinced, ok, let's send nothing to the broken server.

Can you offer a review on my proposed patch? I'm hoping to send an NBD
pull request in the next couple of hours, to make -rc3.
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02593.html

-- 
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 for-2.10 v2 1/4] stubs: Add vm state change handler stubs

[Eric Blake]

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> They will be used by BlockBackend code in block-obj-y, which doesn't
> always get linked with common-obj-y. Add stubs to keep ld happy.
> 
> Signed-off-by: Fam Zheng 
> ---
>  stubs/Makefile.objs  |  1 +
>  stubs/change-state-handler.c | 14 ++
>  2 files changed, 15 insertions(+)
>  create mode 100644 stubs/change-state-handler.c

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 v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Manos Pitsidianakis]

On Tue, Aug 15, 2017 at 03:24:38PM +0200, Alberto Garcia wrote:

On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:

@@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 return;
 }

+/* Skip implicit filter nodes */
+bs = bdrv_get_first_explicit(bs);
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);


This change works here because right now the only implicit node that we
could have in practice is the throttle node, but I wonder if this is
good enough for any kind of implicit node in general.


It does feel a bit sloppy but block jobs should work the same with
implicit nodes and without, so all we can do is ignore them.




+static inline BlockDriverState *child_bs(BlockDriverState *bs)
+{
+BdrvChild *child = QLIST_FIRST(>children);
+assert(child && !QLIST_NEXT(child, next));
+return child->bs;
+}


This aborts if the bs has a number of children != 1. That's not
something that I would expect from a function named like that.

Considering that you're only using it in bdrv_get_first_explicit(), why
don't you simply move the code there?


It felt useful to have a function that also returns file->bs (we have 
backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)


The other question is of course whether we can rely for the future on
the assumption that implicit nodes only have one children.


This is only to get either bs->backing or bs->file (we can't have both
anyway). I wanted to document it with something like "Used for filter 
drivers with only one child" which fits with what implicit nodes we have 
so far (mirror, commit, throttle and in the future backup)


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3 00/12] Convert over to use keycodemapdb

[Daniel P. Berrange]

On Tue, Aug 15, 2017 at 09:49:00AM -0400, Programmingkid wrote:
> 
> > On Aug 15, 2017, at 4:38 AM, Daniel P. Berrange  wrote:
> > 
> > On Mon, Aug 14, 2017 at 02:46:22PM -0400, Programmingkid wrote:
> >> Sorry but there appears to be an issue with your patchset. I ran this 
> >> command:
> >> 
> >> ./configure --target-list=ppc-softmmu,i386-softmmu
> >> 
> >> Then saw this error message: 
> >> 
> >> error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.
> >> 
> >> Running 'make' I saw this error message:
> >> 
> >> make: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', 
> >> needed by `Makefile'.  Stop.
> >> make: *** Waiting for unfinished jobs
> >> 
> >> Running the 'make clean' command produces this error message:
> >> 
> >> make[1]: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', 
> >> needed by `Makefile'.  Stop.
> >> make: *** [clean] Error 1
> >> 
> >> Perhaps a missing patch?
> > 
> > No, its a missing "git submodule update --init ui/keycodemapdb" call
> 
> Thank you for replying. I tried this command and saw this error message:
> 
> error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.
> 
> Note: I'm using version 3. Will test version 4 soon. 
> 
> I suggest adding the git submodule update info to your cover letter
> saying it is required.

It isn't supposed to be required - its a bug, hence why the tests are
failing.

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

Re: [Qemu-devel] [PATCH v3 00/12] Convert over to use keycodemapdb

[Programmingkid]

> On Aug 15, 2017, at 4:38 AM, Daniel P. Berrange  wrote:
> 
> On Mon, Aug 14, 2017 at 02:46:22PM -0400, Programmingkid wrote:
>> Sorry but there appears to be an issue with your patchset. I ran this 
>> command:
>> 
>> ./configure --target-list=ppc-softmmu,i386-softmmu
>> 
>> Then saw this error message: 
>> 
>> error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.
>> 
>> Running 'make' I saw this error message:
>> 
>> make: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', needed 
>> by `Makefile'.  Stop.
>> make: *** Waiting for unfinished jobs
>> 
>> Running the 'make clean' command produces this error message:
>> 
>> make[1]: *** No rule to make target `ui/input-keymap-atset1-to-qcode.c', 
>> needed by `Makefile'.  Stop.
>> make: *** [clean] Error 1
>> 
>> Perhaps a missing patch?
> 
> No, its a missing "git submodule update --init ui/keycodemapdb" call

Thank you for replying. I tried this command and saw this error message:

error: pathspec 'ui/keycodemapdb' did not match any file(s) known to git.

Note: I'm using version 3. Will test version 4 soon. 

I suggest adding the git submodule update info to your cover letter saying it 
is required. 

Re: [Qemu-devel] [PULL 0/2] Tracing patches

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170815132746.12540-1-stefa...@redhat.com
Subject: [Qemu-devel] [PULL 0/2] Tracing patches
Type: series

=== 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
From https://github.com/patchew-project/qemu
 t [tag update]patchew/20170814231552.24593-1-pbonz...@redhat.com 
-> patchew/20170814231552.24593-1-pbonz...@redhat.com
 t [tag update]patchew/20170815130502.8736-1-stefa...@redhat.com -> 
patchew/20170815130502.8736-1-stefa...@redhat.com
Switched to a new branch 'test'
c8a963888c simpletrace: fix flight recorder --no-header option
272f609607 trace: use static event ID mapping in simpletrace.stp

=== OUTPUT BEGIN ===
Checking PATCH 1/2: trace: use static event ID mapping in simpletrace.stp...
Checking PATCH 2/2: simpletrace: fix flight recorder --no-header option...
ERROR: line over 90 characters
#42: FILE: scripts/simpletrace.py:101:
+"""Deserialize trace records from a file, yielding record tuples 
(event_num, timestamp, pid, arg1, ..., arg6).

total: 1 errors, 0 warnings, 46 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.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH for-2.10] qemu-iotests: step clock after each test iteration

[Eric Blake]

On 08/15/2017 08:05 AM, Stefan Hajnoczi wrote:
> The 093 throttling test submits twice as many requests as the throttle
> limit in order to ensure that we reach the limit.  The remaining
> requests are left in-flight at the end of each test iteration.
> 
> Commit 452589b6b47e8dc6353df257fc803dfc1383bed8 ("vl.c/exit: pause cpus
> before closing block devices") exposed a hang in 093.  This happens
> because requests are still in flight when QEMU terminates but
> QEMU_CLOCK_VIRTUAL time is frozen.  bdrv_drain_all() hangs forever since
> throttled requests cannot complete.
> 
> Step the clock at the end of each test iteration so in-flight requests
> actually finish.  This solves the hang and is cleaner than leaving tests
> in-flight.
> 
> Note that this could also be "fixed" by disabling throttling when drives
> are closed in QEMU.  That approach has two issues:
> 
> 1. We must drain requests before disabling throttling, so the hang
>cannot be easily avoided!
> 
> 2. Any time QEMU disables throttling internally there is a chance that
>malicious users can abuse the code path to bypass throttling limits.
> 
> Therefore it makes more sense to fix the test case than to modify QEMU.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/093 | 4 
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Eric Blake 

I can take this through the NBD tree (since that's one environment that
trips up on the test), if Peter doesn't apply it directly.

-- 
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 for 2.10?] qxl: call qemu_spice_display_init_common for secondary devices

[Stefan Hajnoczi]

On Tue, Aug 15, 2017 at 01:15:52AM +0200, Paolo Bonzini wrote:
> Fixes this 2.10 regression:
> 
>   $ qemu-system-x86_64  -cpu host -m 6144 -vga qxl -device qxl
>   qemu-system-x86_64: util/qemu-thread-posix.c:64: qemu_mutex_lock: Assertion 
> `mutex->initialized' failed.
> 
> Reported-by: adema...@redhat.com
> Cc: kra...@redhat.com
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I have audited the code and cannot see any bad side-effects.  Looks good
to me!

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for 2.10?] qxl: call qemu_spice_display_init_common for secondary devices

[Daniel P. Berrange]

On Tue, Aug 15, 2017 at 01:15:52AM +0200, Paolo Bonzini wrote:
> Fixes this 2.10 regression:
> 
>   $ qemu-system-x86_64  -cpu host -m 6144 -vga qxl -device qxl
>   qemu-system-x86_64: util/qemu-thread-posix.c:64: qemu_mutex_lock: Assertion 
> `mutex->initialized' failed.
> 
> Reported-by: adema...@redhat.com
> Cc: kra...@redhat.com
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 7f8c73b56d..ae3677fd1e 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2054,6 +2054,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error 
> **errp)
>  uint32_t pci_device_rev;
>  uint32_t io_size;
>  
> +qemu_spice_display_init_common(>ssd);
>  qxl->mode = QXL_MODE_UNDEFINED;
>  qxl->generation = 1;
>  qxl->num_memslots = NUM_MEMSLOTS;
> @@ -2176,7 +2177,6 @@ static void qxl_realize_primary(PCIDevice *dev, Error 
> **errp)
>  portio_list_add(>vga_port_list, pci_address_space_io(dev), 0x3b0);
>  
>  vga->con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> -qemu_spice_display_init_common(>ssd);
>  
>  qxl_realize_common(qxl, _err);
>  if (local_err) {

IIUC, we hit the abort because we have 2 QXL devices, and only
qxl_realize_primary() is calling  qemu_spice_display_init_common().
By moving the qemu_spice_display_init_common() call upto into
the qxl_realize_common() method, it also gets triggered by
qxl_realize_secondary(). It makes that we need to initialize the
'ssd' field for both primary and secondary displays

Reviewed-by: Daniel P. Berrange 

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] [PULL 2/2] simpletrace: fix flight recorder --no-header option

[Stefan Hajnoczi]

The simpletrace.py script can pretty-print flight recorder ring buffers.
These are not full simpletrace binary trace files but just the end of a
trace file.  There is no header and the event ID mapping information is
often unavailable since the ring buffer may have filled up and discarded
event ID mapping records.

The simpletrace.stp script that generates ring buffer traces uses the
same trace-events-all input file as simpletrace.py.  Therefore both
scripts have the same global ordering of trace events.  A dynamic event
ID mapping isn't necessary: just use the trace-events-all file as the
reference for how event IDs are numbered.

It is now possible to analyze simpletrace.stp ring buffers again using:

  $ ./simpletrace.py trace-events-all path/to/ring-buffer

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrange 
Message-id: 20170815084430.7128-3-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 scripts/simpletrace.py | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index 2a977e2ab9..a3a6315055 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -97,11 +97,17 @@ def read_trace_header(fobj):
 raise ValueError('Log format %d not supported with this QEMU release!'
  % log_version)
 
-def read_trace_records(edict, fobj):
-"""Deserialize trace records from a file, yielding record tuples 
(event_num, timestamp, pid, arg1, ..., arg6)."""
-idtoname = {
-dropped_event_id: "dropped"
-}
+def read_trace_records(edict, idtoname, fobj):
+"""Deserialize trace records from a file, yielding record tuples 
(event_num, timestamp, pid, arg1, ..., arg6).
+
+Note that `idtoname` is modified if the file contains mapping records.
+
+Args:
+edict (str -> Event): events dict, indexed by name
+idtoname (int -> str): event names dict, indexed by event ID
+fobj (file): input file
+
+"""
 while True:
 t = fobj.read(8)
 if len(t) == 0:
@@ -171,10 +177,16 @@ def process(events, log, analyzer, read_header=True):
 
 dropped_event = Event.build("Dropped_Event(uint64_t num_events_dropped)")
 edict = {"dropped": dropped_event}
+idtoname = {dropped_event_id: "dropped"}
 
 for event in events:
 edict[event.name] = event
 
+# If there is no header assume event ID mapping matches events list
+if not read_header:
+for event_id, event in enumerate(events):
+idtoname[event_id] = event.name
+
 def build_fn(analyzer, event):
 if isinstance(event, str):
 return analyzer.catchall
@@ -197,7 +209,7 @@ def process(events, log, analyzer, read_header=True):
 
 analyzer.begin()
 fn_cache = {}
-for rec in read_trace_records(edict, log):
+for rec in read_trace_records(edict, idtoname, log):
 event_num = rec[0]
 event = edict[event_num]
 if event_num not in fn_cache:
-- 
2.13.4

[Qemu-devel] [PULL 1/2] trace: use static event ID mapping in simpletrace.stp

[Stefan Hajnoczi]

This is a partial revert of commit
7f1b588f20d027730676e627713ae3bbf6baab04 ("trace: emit name <-> ID
mapping in simpletrace header"), which broke the SystemTap flight
recorder because event mapping records may not be present in the ring
buffer when the trace is analyzed.  This means simpletrace.py
--no-header does not know the event ID mapping needed to pretty-print
the trace.

Instead of numbering events dynamically, use a static event ID mapping
as dictated by the event order in the trace-events-all file.

The simpletrace.py script also uses trace-events-all so the next patch
will fix the simpletrace.py --no-header option to take advantage of this
knowledge.

Cc: Daniel P. Berrange 
Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrange 
Message-id: 20170815084430.7128-2-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 scripts/tracetool/format/simpletrace_stap.py | 31 ++--
 1 file changed, 2 insertions(+), 29 deletions(-)

diff --git a/scripts/tracetool/format/simpletrace_stap.py 
b/scripts/tracetool/format/simpletrace_stap.py
index 144b704bcd..e7e44842ca 100644
--- a/scripts/tracetool/format/simpletrace_stap.py
+++ b/scripts/tracetool/format/simpletrace_stap.py
@@ -22,33 +22,8 @@ def global_var_name(name):
 return probeprefix().replace(".", "_") + "_" + name
 
 def generate(events, backend, group):
-id_map = global_var_name("event_name_to_id_map")
-next_id = global_var_name("event_next_id")
-map_func = global_var_name("simple_trace_map_event")
 out('/* This file is autogenerated by tracetool, do not edit. */',
-'',
-'global %(id_map)s',
-'global %(next_id)s',
-'function %(map_func)s(name)',
-'',
-'{',
-'if (!([name] in %(id_map)s)) {',
-'%(id_map)s[name] = %(next_id)s',
-'name_len = strlen(name)',
-'printf("%%8b%%8b%%4b%%.*s", 0, ',
-'   %(next_id)s, name_len, name_len, name)',
-'%(next_id)s = %(next_id)s + 1',
-'}',
-'return %(id_map)s[name]',
-'}',
-'probe begin',
-'{',
-'printf("%%8b%%8b%%8b", 0x, 0xf2b177cb0aa429b4, 
4)',
-'}',
-'',
-id_map=id_map,
-next_id=next_id,
-map_func=map_func)
+'')
 
 for event_id, e in enumerate(events):
 if 'disable' in e.properties:
@@ -56,9 +31,7 @@ def generate(events, backend, group):
 
 out('probe %(probeprefix)s.simpletrace.%(name)s = 
%(probeprefix)s.%(name)s ?',
 '{',
-'id = %(map_func)s("%(name)s")',
 probeprefix=probeprefix(),
-map_func=map_func,
 name=e.name)
 
 # Calculate record size
@@ -77,7 +50,7 @@ def generate(events, backend, group):
 sizestr = ' + '.join(sizes)
 
 # Generate format string and value pairs for record header and 
arguments
-fields = [('8b', 'id'),
+fields = [('8b', str(event_id)),
   ('8b', 'gettimeofday_ns()'),
   ('4b', sizestr),
   ('4b', 'pid()')]
-- 
2.13.4

[Qemu-devel] [PULL 0/2] Tracing patches

[Stefan Hajnoczi]

The following changes since commit 5681da292242550f37ba4c03f46a8a6f8ee9278a:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170815' into 
staging (2017-08-15 09:39:14 +0100)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/tracing-pull-request

for you to fetch changes up to 840d8351774664d01c328d31ed33b0e2d85c866e:

  simpletrace: fix flight recorder --no-header option (2017-08-15 12:50:29 
+0100)





Stefan Hajnoczi (2):
  trace: use static event ID mapping in simpletrace.stp
  simpletrace: fix flight recorder --no-header option

 scripts/simpletrace.py   | 24 +++--
 scripts/tracetool/format/simpletrace_stap.py | 31 ++--
 2 files changed, 20 insertions(+), 35 deletions(-)

-- 
2.13.4