[Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration
This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.
This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of VMS_ALLOC
and uses uint32_t type for a size.
Signed-off-by: Alexey Kardashevskiy
---
include/migration/vmstate.h | 11 +++
vmstate.c | 13 ++---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9a001bd..e45fc49 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -484,6 +484,17 @@ extern const VMStateInfo vmstate_info_bitmap;
.start= (_start),\
}
+#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _start,
_field_size) { \
+.name = (stringify(_field)), \
+.version_id = (_version), \
+.field_exists = (_test), \
+.size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
+.info = &vmstate_info_buffer,\
+.flags= VMS_VBUFFER|VMS_POINTER|VMS_ALLOC, \
+.offset = offsetof(_state, _field),\
+.start= (_start),\
+}
+
#define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
.name = (stringify(_field)), \
.version_id = (_version),\
diff --git a/vmstate.c b/vmstate.c
index ef2f87b..3dde574 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -49,9 +49,16 @@ static void *vmstate_base_addr(void *opaque, VMStateField
*field, bool alloc)
if (field->flags & VMS_POINTER) {
if (alloc && (field->flags & VMS_ALLOC)) {
-int n_elems = vmstate_n_elems(opaque, field);
-if (n_elems) {
-gsize size = n_elems * field->size;
+gsize size = 0;
+if (field->flags & VMS_VBUFFER) {
+size = vmstate_size(opaque, field);
+} else {
+int n_elems = vmstate_n_elems(opaque, field);
+if (n_elems) {
+size = n_elems * field->size;
+}
+}
+if (size) {
*((void **)base_addr + field->start) = g_malloc(size);
}
}
--
2.0.0
[Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration
The only case when sPAPR NVRAM migrates now is if is backed by a file and
copy-storage migration is performed.
This enables RAM copy of NVRAM even if NVRAM is backed by a file.
This defines a VMSTATE descriptor for NVRAM device so the memory copy
of NVRAM can migrate and be written to a backing file on the destination
if one is provided.
Signed-off-by: Alexey Kardashevskiy
---
hw/nvram/spapr_nvram.c | 68 +++---
1 file changed, 59 insertions(+), 9 deletions(-)
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 6a72ef4..254009e 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
return;
}
+assert(nvram->buf);
+
membuf = cpu_physical_memory_map(buffer, &len, 1);
+
+alen = len;
if (nvram->drive) {
alen = bdrv_pread(nvram->drive, offset, membuf, len);
+if (alen > 0) {
+memcpy(nvram->buf + offset, membuf, alen);
+}
} else {
-assert(nvram->buf);
-
memcpy(membuf, nvram->buf + offset, len);
-alen = len;
}
+
cpu_physical_memory_unmap(membuf, len, 1, len);
rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
@@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
}
membuf = cpu_physical_memory_map(buffer, &len, 0);
+
+alen = len;
if (nvram->drive) {
alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
-} else {
-assert(nvram->buf);
-
-memcpy(nvram->buf + offset, membuf, len);
-alen = len;
}
+
+assert(nvram->buf);
+memcpy(nvram->buf + offset, membuf, len);
+
cpu_physical_memory_unmap(membuf, len, 0, len);
rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
@@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
nvram->size = bdrv_getlength(nvram->drive);
} else {
nvram->size = DEFAULT_NVRAM_SIZE;
-nvram->buf = g_malloc0(nvram->size);
}
+nvram->buf = g_malloc0(nvram->size);
+
if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
fprintf(stderr, "spapr-nvram must be between %d and %d bytes in
size\n",
MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
@@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void
*fdt, int node_off)
return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
}
+static int spapr_nvram_pre_load(void *opaque)
+{
+sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+g_free(nvram->buf);
+nvram->buf = NULL;
+nvram->size = 0;
+
+return 0;
+}
+
+static int spapr_nvram_post_load(void *opaque, int version_id)
+{
+sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+if (nvram->drive) {
+int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
+
+if (alen < 0) {
+return alen;
+}
+if (alen != nvram->size) {
+return -1;
+}
+}
+
+return 0;
+}
+
+static const VMStateDescription vmstate_spapr_nvram = {
+.name = "spapr_nvram",
+.version_id = 1,
+.minimum_version_id = 1,
+.pre_load = spapr_nvram_pre_load,
+.post_load = spapr_nvram_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(size, sPAPRNVRAM),
+VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
+VMSTATE_END_OF_LIST()
+},
+};
+
static Property spapr_nvram_properties[] = {
DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, drive),
@@ -184,6 +233,7 @@ static void spapr_nvram_class_init(ObjectClass *klass, void
*data)
k->dt_compatible = "qemu,spapr-nvram";
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
dc->props = spapr_nvram_properties;
+dc->vmsd = &vmstate_spapr_nvram;
}
static const TypeInfo spapr_nvram_type_info = {
--
2.0.0
[Qemu-devel] [PATCH 0/2] spapr_nvram: Support migration
Here are 2 patches to enable sPAPR NVRAM migration. Please comment. Thanks! Alexey Kardashevskiy (2): vmstate: Allow dynamic allocation for VBUFFER during migration spapr_nvram: Enable migration hw/nvram/spapr_nvram.c | 68 +++-- include/migration/vmstate.h | 11 vmstate.c | 13 +++-- 3 files changed, 80 insertions(+), 12 deletions(-) -- 2.0.0
[Qemu-devel] how can i load qemu-system-arm symbol using gdb?
hello all,
I have one question.
i built qemu-system-arm and i want to debug using gdb inside qemu.
but i don't know how can i load qemu symbol which address in 64bit.
for example,
$>gdb --args qemu-system-arm xxx
in gdb,
(gdb) add-symbol-file qemu-system-arm (?)
the additional information is as below.
$> file qemu-system-arm
qemu-system-arm: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV),
dynamically linked (uses shared libs), for GNU/Linux 2.6.24,
BuildID[sha1]=0x542de177694d78c4f062456d3e9f28cc5da1f03a, not stripped
$> readelf -a qemu-system-arm
ELF Header:
Magic: 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
Class: ELF64
Data: 2's complement, little endian
Version: 1 (current)
OS/ABI:UNIX - System V
ABI Version: 0
Type: DYN (Shared object file)
Machine: Advanced Micro Devices X86-64
Version: 0x1
Entry point address: 0x7ad90
Start of program headers: 64 (bytes into file)
Start of section headers: 12742552 (bytes into file)
Flags: 0x0
Size of this header: 64 (bytes)
Size of program headers: 56 (bytes)
Number of program headers: 10
Size of section headers: 64 (bytes)
Number of section headers: 41
Section header string table index: 38
Section Headers:
[Nr] Name Type Address Offset
Size EntSize Flags Link Info Align
[ 0] NULL
0 0 0
[ 1] .interp PROGBITS 0270 0270
001c A 0 0 1
[ 2] .note.ABI-tag NOTE 028c 028c
0020 A 0 0 4
[ 3] .note.gnu.build-i NOTE 02ac 02ac
0024 A 0 0 4
[ 4] .gnu.hash GNU_HASH 02d0 02d0
1420 A 5 0 8
[ 5] .dynsym DYNSYM 16f0 16f0
6180 0018 A 6 3 8
[ 6] .dynstr STRTAB 7870 7870
3ff6 A 0 0 1
[ 7] .gnu.version VERSYM b866 b866
0820 0002 A 5 0 2
[ 8] .gnu.version_rVERNEED c088 c088
0150 A 6 6 8
[ 9] .rela.dyn RELA c1d8 c1d8
0006ae78 0018 A 5 0 8
[10] .rela.plt RELA 00077050 00077050
24a8 0018 A 512 8
[11] .init PROGBITS 000794f8 000794f8
0018 AX 0 0 4
[12] .plt PROGBITS 00079510 00079510
1880 0010 AX 0 0 16
[13] .text PROGBITS 0007ad90 0007ad90
0036ae18 AX 0 0 16
[14] .fini PROGBITS 003e5ba8 003e5ba8
000e AX 0 0 4
[15] .rodata PROGBITS 003e5bc0 003e5bc0
00077ff7 A 0 0 32
[16] .eh_frame_hdr PROGBITS 0045dbb8 0045dbb8
0001a51c A 0 0 4
[17] .eh_frame PROGBITS 004780d8 004780d8
000695e4 A 0 0 8
[18] .tbss NOBITS 006e1e88 004e1e88
0008 WAT 0 0 8
[19] .init_array INIT_ARRAY 006e1e88 004e1e88
05f0 WA 0 0 8
[20] .fini_array FINI_ARRAY 006e2478 004e2478
0008 WA 0 0 8
[21] .ctorsPROGBITS 006e2480 004e2480
0010 WA 0 0 8
[22] .dtorsPROGBITS 006e2490 004e2490
0010 WA 0 0 8
If you know answer, please let me know.
BR,
MK
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
Am 25.09.2014 um 08:21 hat Markus Armbruster geschrieben: > Please copy Kevin & Stefan for block patches. Doing that for you. I > also copy Max, who left his fingerprints on commit 4f11aa8. > > Tony Breeds writes: > > > The command > > qemu-img convert -O raw inputimage.qcow2 outputimage.raw > > > > intermittently creates corrupted output images, when the input image is not > > yet fully synchronized to disk. This patch preferese the use of seek_hole > > checks to determine if the sector is present in the disk image. Does this fix the problem or does it just make it less likely that it becomes apparent? If there is a data corruptor, we need to fix it, not just ensure that only the less common environments are affected. > > While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC. That looks like a logically separate change, so it should probably be a separate patch. Is this fix for the corruptor? The commit message doesn't make it clear. If so and fiemap is safe now, why would we still prefer seek_hole? > > Reported-By: Michael Steffens > > CC: Pádraig Brady > > Signed-off-by: Tony Breeds Kevin
Re: [Qemu-devel] [PATCH v2 0/2] block: Catch simultaneous usage of options and their aliases
Am 24.09.2014 um 16:46 hat Kevin Wolf geschrieben: > v2: > - Remove QAPI-style use of errp, convert to table to keep things > readable anyway [Markus] > > Kevin Wolf (2): > block: Specify -drive legacy option aliases in array > block: Catch simultaneous usage of options and their aliases Applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
Eric Blake writes:
> Demonstrate that the qapi generator silently parses confusing
> types, which may cause other errors later on. Later patches
> will update the expected results as the generator is made stricter.
>
> Signed-off-by: Eric Blake
> ---
> tests/Makefile | 8 ++--
> tests/qapi-schema/data-array-empty.err | 0
> tests/qapi-schema/data-array-empty.exit | 1 +
> tests/qapi-schema/data-array-empty.json | 2 ++
[Twelve new tests...]
> create mode 100644 tests/qapi-schema/returns-unknown.err
> create mode 100644 tests/qapi-schema/returns-unknown.exit
> create mode 100644 tests/qapi-schema/returns-unknown.json
> create mode 100644 tests/qapi-schema/returns-unknown.out
Holy moly!
> diff --git a/tests/Makefile b/tests/Makefile
> index 5e01952..6fe34f7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -203,8 +203,12 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
> double-data.json unknown-expr-key.json redefined-type.json \
> redefined-command.json redefined-builtin.json redefined-event.json \
> type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
> - missing-colon.json missing-comma-list.json \
> - missing-comma-object.json non-objects.json \
> + data-array-empty.json data-array-unknown.json data-int.json \
> + data-unknown.json data-member-unknown.json data-member-array.json \
> + data-member-array-bad.json returns-array-bad.json returns-int.json \
> + returns-unknown.json missing-colon.json missing-comma-list.json \
> + missing-comma-object.json nested-struct-data.json \
> + nested-struct-returns.json non-objects.json \
> qapi-schema-test.json quoted-structural-chars.json \
> trailing-comma-list.json trailing-comma-object.json \
> unclosed-list.json unclosed-object.json unclosed-string.json \
> diff --git a/tests/qapi-schema/data-array-empty.err
> b/tests/qapi-schema/data-array-empty.err
> new file mode 100644
> index 000..e69de29
> diff --git a/tests/qapi-schema/data-array-empty.exit
> b/tests/qapi-schema/data-array-empty.exit
> new file mode 100644
> index 000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-array-empty.json
> b/tests/qapi-schema/data-array-empty.json
> new file mode 100644
> index 000..41b6c1e
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject an array for data if it does not contain a known
> type
> +{ 'command': 'oops', 'data': [ ] }
Do we want to permit anything but a complex type for 'data'?
For what it's worth, docs/qmp/qmp-spec.txt specifies:
2.3 Issuing Commands
The format for command execution is:
{ "execute": json-string, "arguments": json-object, "id": json-value }
Where,
- The "execute" member identifies the command to be executed by the Server
- The "arguments" member is used to pass any arguments required for the
execution of the command, it is optional when no arguments are required
- The "id" member is a transaction identification associated with the
command execution, it is optional and will be part of the response if
provided
The QAPI schema's 'data' is "arguments" on the wire.
Semantically, 'data' of a complex type / json-object "arguments" is an
ordered list of named parameters. Makes obvious sense.
'data' of list type / json-array "arguments" is an ordered list of
unnamed parameters. Makes sense, but it isn't how QMP works. Or C for
that matter. Do we really want to support this in QAPI?
If yes, then 'data': [] means the same thing as 'data': {} or no 'data':
no arguments.
Aside: discussion of list types in qapi-code-gen.txt is spread over the
places that use them. I feel giving them their own section on the same
level as complex types etc. would make sense.
'data' of built-in or enumeration type / json-number or json-string
"arguments" could be regarded as a single unnamed parameter. Even if we
want unnamed parameters, the question remains whether we want two
syntactic forms for a single unnamed parameter, boxed in a [ ] and
unboxed. I doubt it.
One kind of types left to discuss: unions. I figure the semantics of a
simple or flat union type are obvious enough, so we can discuss whether
we want them. Anonymous unions are a different matter, because they
boil down to a single parameter that need not be json-object.
> diff --git a/tests/qapi-schema/data-array-empty.out
> b/tests/qapi-schema/data-array-empty.out
> new file mode 100644
> index 000..67802be
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', [])])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-array-unknown.err
> b/tests/qapi-schema/data-array-unknown.err
> new file mode 100644
> index 000..e69de29
>
Re: [Qemu-devel] [PATCH v3] docs: add blkdebug block driver documentation
Am 24.09.2014 um 11:44 hat Stefan Hajnoczi geschrieben: > The blkdebug block driver is undocumented. Documenting it is worthwhile > since it offers powerful error injection features that are used by > qemu-iotests test cases. > > This document will make it easier for people to learn about and use > blkdebug. > > Signed-off-by: Stefan Hajnoczi Thanks, applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH] vpc: fix beX_to_cpu() and cpu_to_beX() confusion
Am 23.09.2014 um 11:40 hat Stefan Hajnoczi geschrieben: > The beX_to_cpu() and cpu_to_beX() functions perform the same operation - > they do a byteswap if the host CPU endianness is little-endian or a > nothing otherwise. > > The point of two names for the same operation is that it documents which > direction the data is being converted. This makes it clear whether the > data is suitable for CPU processing or in its external representation. > > This patch fixes incorrect beX_to_cpu()/cpu_to_beX() usage. > > Signed-off-by: Stefan Hajnoczi Thanks, applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API
On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote: > On Wed, 24 Sep 2014 15:23:41 +0200 > Gerd Hoffmann wrote: > > > Hi, > > > > > > Can't we do this in usb_bus_new instead of duplicating in every host > > > > adapter? > > > > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > > > the controller? > > > > I was more thinking of just setting the callback in common code, but if > > we can attach the hotplug interface to the usb bus itself not the usb > > host adapters that would be even better. And it'll probably kill some > > headache for the companion controller case. > How making bus a HotplugHandler itself will help with companion controller? When uhci acts as ehci companion it registers the ports with ehci and doesn't manage its own usb bus. ehci will call uhci port ops as needed (depending on ehci configuration). When attaching the hotplug interface to the host controller you'll have to explicitly handle the companion case somehow. When attaching the hotplug interface to the usb bus everything should just work. Additional bonus is that you also don't have to touch the host controller code at all then, it should be doable by changing hw/usb/bus.c only. cheers, Gerd
Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly
On Mi, 2014-09-24 at 17:22 +0200, Igor Mammedov wrote: > On Wed, 24 Sep 2014 14:50:41 +0200 > Gerd Hoffmann wrote: > > > On Mi, 2014-09-24 at 11:48 +, Igor Mammedov wrote: > > > usb-storage uses SCSI bus to provide underling storage > > > (i.e. scsi-disk) and it's hotpluggable. > > > > No. usb-storage itself (the scsi hba) is hotpluggable, but the scsi > > devices connected are not. > Agree, > I'm sorry for my bad English, under "it's hotpluggable" I've meant > usb-storage. /me is confused. That contradicts $subject. cheers, Gerd
[Qemu-devel] [PATCH v2] block: Validate node-name
The device_name of a BlockDriverState is currently checked because it is
always used as a QemuOpts ID and qemu_opts_create() checks whether such
IDs are wellformed.
node-name is supposed to share the same namespace, but it isn't checked
currently. This patch adds explicit checks both for device_name and
node-name so that the same rules will still apply even if QemuOpts won't
be used any more at some point.
qemu-img used to use names with spaces in them, which isn't allowed any
more. Replace them with underscores.
Signed-off-by: Kevin Wolf
---
v2:
- Fix qemu-img to use valid names internally [Stefan]
block.c | 16 +---
include/qemu/option.h | 1 +
qemu-img.c| 6 +++---
util/qemu-option.c| 4 ++--
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index 6934018..ec92c12 100644
--- a/block.c
+++ b/block.c
@@ -335,12 +335,22 @@ void bdrv_register(BlockDriver *bdrv)
QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
}
+static bool bdrv_is_valid_name(const char *name)
+{
+return qemu_opts_id_wellformed(name);
+}
+
/* create a new block device (by default it is empty) */
BlockDriverState *bdrv_new(const char *device_name, Error **errp)
{
BlockDriverState *bs;
int i;
+if (*device_name && !bdrv_is_valid_name(device_name)) {
+error_setg(errp, "Invalid device name");
+return NULL;
+}
+
if (bdrv_find(device_name)) {
error_setg(errp, "Device with id '%s' already exists",
device_name);
@@ -863,9 +873,9 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
return;
}
-/* empty string node name is invalid */
-if (node_name[0] == '\0') {
-error_setg(errp, "Empty node name");
+/* Check for empty string or invalid characters */
+if (!bdrv_is_valid_name(node_name)) {
+error_setg(errp, "Invalid node name");
return;
}
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 59bea75..945347c 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -103,6 +103,7 @@ typedef int (*qemu_opt_loopfunc)(const char *name, const
char *value, void *opaq
int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
int abort_on_failure);
+int qemu_opts_id_wellformed(const char *id);
QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
int fail_if_exists, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index dbf0904..ea4bbae 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1011,14 +1011,14 @@ static int img_compare(int argc, char **argv)
goto out3;
}
-bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
+bs1 = bdrv_new_open("image_1", filename1, fmt1, flags, true, quiet);
if (!bs1) {
error_report("Can't open file %s", filename1);
ret = 2;
goto out3;
}
-bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
+bs2 = bdrv_new_open("image_2", filename2, fmt2, flags, true, quiet);
if (!bs2) {
error_report("Can't open file %s", filename2);
ret = 2;
@@ -1359,7 +1359,7 @@ static int img_convert(int argc, char **argv)
total_sectors = 0;
for (bs_i = 0; bs_i < bs_n; bs_i++) {
-char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
+char *id = bs_n > 1 ? g_strdup_printf("source_%d", bs_i)
: g_strdup("source");
bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
true, quiet);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6dc27ce..0cf9960 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -641,7 +641,7 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
return NULL;
}
-static int id_wellformed(const char *id)
+int qemu_opts_id_wellformed(const char *id)
{
int i;
@@ -662,7 +662,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char
*id,
QemuOpts *opts = NULL;
if (id) {
-if (!id_wellformed(id)) {
+if (!qemu_opts_id_wellformed(id)) {
error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an
identifier");
#if 0 /* conversion from qerror_report() to error_set() broke this: */
error_printf_unless_qmp("Identifiers consist of letters, digits,
'-', '.', '_', starting with a letter.\n");
--
1.8.3.1
Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"
Hi, > But there is room for improvement here, > it it's possible to error out even earlier if usb-bot would be marked > as not hotpluggable device and add to qdev_device_add() check if > device is hotpluggable even before it's created. usb-bot not being hot-pluggable is a consequence of the fact that we can only hotplug single devices in qemu today. usb-bot without scsi devices hooked up is useless. Plugging in usb-bot first and plugging in disks later doesn't work (well, could sort-of work when you plug them fast, but you are racing with the guest initializing the device then). With some way to first compose the whole thing (usb-bot + disks), then plug-in as whole usb-bot hotplugging would work though. cheers, Gerd
Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"
On Mi, 2014-09-24 at 17:21 +0200, Paolo Bonzini wrote: > Il 24/09/2014 17:15, Igor Mammedov ha scritto: > > But there is room for improvement here, > > it it's possible to error out even earlier if usb-bot would be marked > > as not hotpluggable device and add to qdev_device_add() check if > > device is hotpluggable even before it's created. > > That would work too. Gerd, what about usb-uas? Basically the same camp as usb-bot. UAS doesn't suffer the LUN numeration issue which BOT has, but there likewise is no signaling about scsi devices coming and going. cheers, Gerd
Re: [Qemu-devel] [PATCH v4] Add machine parameter qemu-kvm-migration for live migrate compatibility with qemu-kvm
* Michael Tokarev (m...@tls.msk.ru) wrote: > 22.09.2014 23:34, Alex Bligh wrote: > > This patch series adds inbound migrate capability from qemu-kvm version > > 1.0. [...] > > Isn't it quite a bit too late already? That's an old version by > now, and supporting migration from it is interesting for long-term > support distributions - like redhat for example, with several > years of release cycle. Is it really necessary at this time to > add this ability to upstream qemu? > > It'd be very nice to have this capability right when qemu-kvm > tree has been merged (or even before that), but it's been some > years ago already. It's amazing what different combinations of QEMU people have out there; and it seems reasonable to let Alex fix this problem if it helps him; there's no reason to deny others the same fix. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32
On 24.09.2014 17:19, Richard Henderson wrote: > On 09/24/2014 01:20 AM, Claudio Fontana wrote: >>> @@ -1118,7 +1119,8 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, >>> TCGMemOp memop, >>> tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, off_r); >>> break; >>> case MO_SB: >>> -tcg_out_ldst_r(s, I3312_LDRSBX, data_r, addr_r, off_r); >>> +tcg_out_ldst_r(s, type ? I3312_LDRSBX : I3312_LDRSBW, >>> + data_r, addr_r, off_r); >> >> >> since we are using the enum type TCGType, why do we check type as "type ?" >> >> I would have expected the conditional to be something like >> >> type == TCG_TYPE_I32 ? I3312_LDRSBW : I3312_LDRSBX >> >> It's pretty obvious what is happening but it might spare someone a lookup >> into the header file >> to test that type 0 is indeed TCG_TYPE_I32. > > We assert the boolean-ish nature of TCGType at the start of the file, and use > it for the "ext" variable throughout. Would it help if the variable weren't > named "type"? > > > r~ I could not find a comment describing that in tcg.h, did I miss it in the patch? I find it difficult to come up with a better name for TCGType, I wonder what that could be.. but what about not caring about the boolish nature of TCGType and instead check it explicitly? I understand that there are multiple enumeration constants with the same value, but if the names of the first two enumeration constants are general enough, that should be understandable, no? A comment before TCGType could explain this, and then we could have values like TCG_TYPE_32 = 0, TCG_TYPE_64, /* .. all the other aliases */ Which is basically what we have now isn't it? TCG_TYPE_I32 and TCG_TYPE_I64 are general enough I think, so why aren't we checking those constants explicitly? Ciao, Claudio
Re: [Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()
On Thu, 25 Sep 2014 10:00:05 +0800
Tang Chen wrote:
>
> On 09/24/2014 07:47 PM, Igor Mammedov wrote:
> > it would allow transparently switch detection if Bus
> > is hotpluggable from allow_hotplug field to hotplug_handler
> > link and drop allow_hotplug field once all users are
> > converted to hotplug handler API.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > hw/core/qdev.c | 6 +++---
> > hw/i386/acpi-build.c | 2 +-
> > hw/pci/pci-hotplug-old.c | 4 ++--
> > include/hw/qdev-core.h | 5 +
> > qdev-monitor.c | 2 +-
> > 5 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index fcb1638..5e5b963 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState
> > *child)
> > BusChild *kid = g_malloc0(sizeof(*kid));
> >
> > if (qdev_hotplug) {
> > -assert(bus->allow_hotplug);
> > +assert(qbus_is_hotpluggable(bus));
> > }
> >
> > kid->index = bus->max_index++;
> > @@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> > {
> > DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >
> > -if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> > +if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
> > error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> > return;
> > }
> > @@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error
> > **errp)
> > DeviceState *dev = DEVICE(obj);
> >
> > return dc->hotpluggable && (dev->parent_bus == NULL ||
> > -dev->parent_bus->allow_hotplug);
> > +qbus_is_hotpluggable(dev->parent_bus));
> > }
> >
> > static bool device_get_hotplugged(Object *obj, Error **err)
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index a313321..00be4bb 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > unsigned *bsel_alloc = opaque;
> > unsigned *bus_bsel;
> >
> > -if (bus->qbus.allow_hotplug) {
> > +if (qbus_is_hotpluggable(BUS(bus))) {
> > bus_bsel = g_malloc(sizeof *bus_bsel);
> >
> > *bus_bsel = (*bsel_alloc)++;
> > diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> > index cf2caeb..f9b7398 100644
> > --- a/hw/pci/pci-hotplug-old.c
> > +++ b/hw/pci/pci-hotplug-old.c
> > @@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
> > monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
> > return NULL;
> > }
> > -if (!((BusState*)bus)->allow_hotplug) {
> > +if (!qbus_is_hotpluggable(BUS(bus))) {
> > monitor_printf(mon, "PCI bus doesn't support hotplug\n");
> > return NULL;
> > }
> > @@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
> > monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
> > return NULL;
> > }
> > -if (!((BusState*)bus)->allow_hotplug) {
> > +if (!qbus_is_hotpluggable(BUS(bus))) {
> > monitor_printf(mon, "PCI bus doesn't support hotplug\n");
> > return NULL;
> > }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 178fee2..48a96d2 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState
> > *bus, DeviceState *handler,
> >QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> > bus->allow_hotplug = 1;
> > }
> > +
> > +static inline bool qbus_is_hotpluggable(BusState *bus)
> > +{
> > + return bus->allow_hotplug || bus->hotplug_handler;
>
> Why not synchronize these two members ? What is the case that bus has
> a hotplug_handler but allow_hotplug is false ?
>
> BTW, I think there are other places that using bus->allow_hotplug
> directly, right ?
There is no point in synchronizing them since by the end of this
series 'allow_hotplug' field will be removed and only 'hotplug_handler'
will be left. allow_hotplug is kept till the 29/30 patch for maintaining
series bisect-ability and for the sake of splitting patches to small clean
ones.
>
> eg:
> s390_virtio_bus_init()
> {
> ..
> _bus->allow_hotplug = 1;
> ..
> }
>
> Thanks.
>
> > +}
> > #endif
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 5ec6606..f6db461 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> > return NULL;
> > }
> > }
> > -if (qdev_hotplug && bus && !bus->allow_hotplug) {
> > +if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
> > qerror_report(QERR_BUS_NO_HOTPLUG, bus->
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
Markus Armbruster writes:
> Eric Blake writes:
>
>> Demonstrate that the qapi generator silently parses confusing
>> types, which may cause other errors later on. Later patches
>> will update the expected results as the generator is made stricter.
[...]
>> diff --git a/tests/qapi-schema/returns-array-bad.json
>> b/tests/qapi-schema/returns-array-bad.json
>> new file mode 100644
>> index 000..14882c1
>> --- /dev/null
>> +++ b/tests/qapi-schema/returns-array-bad.json
>> @@ -0,0 +1,2 @@
>> +# FIXME: we should reject an array return that is not a single type
>> +{ 'command': 'oops', 'returns': [ 'str', 'str' ] }
Yes, we want this test, and your remaining tests of 'returns' are fine,
too.
> Do we want to permit anything but a complex type for 'returns'?
>
> For what it's worth, docs/qmp/qmp-spec.txt specifies:
>
> 2.4.1 success
> -
>
> The format of a success response is:
>
> { "return": json-object, "id": json-value }
>
> Where,
>
> - The "return" member contains the command returned data, which is defined
> in a per-command basis or an empty json-object if the command does not
> return data
> - The "id" member contains the transaction identification associated
> with the command execution if issued by the Client
>
> The QAPI schema's 'returns' becomes "return" on the wire. We suck.
>
> qmp-spec.txt is *wrong*! We actually use json-array in addition to
> json-object.
Actually, we use json-int and json-str as well:
query-migrate-cache-size, ringbuf-read, human-monitor-command.
> Similar argument on types wanted as for 'data' / "arguments" above. I
> think we should permit exactly the same QAPI types, plus lists.
The similarity to 'data' just isn't there. Separate analysis needed.
Any QAPI types that don't make sense, other than list with length != 1?
[...]
Re: [Qemu-devel] [PATCH 11/30] qdev: HotplugHandler: provide unplug callback
On Thu, 25 Sep 2014 09:53:20 +0800
Tang Chen wrote:
>
> On 09/24/2014 07:48 PM, Igor Mammedov wrote:
> > it to be called for actual device removal and
> > will allow to separate request and removal handling
> > phases of x86-CPU devices and also it's a handler
> > to be called for synchronously removable devices.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > unplug handling for bus-less devices will be added
> > later in this series.
> > ---
> > hw/core/hotplug.c| 11 +++
> > hw/core/qdev.c | 13 +++--
> > include/hw/hotplug.h | 12
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > index 2ec4736..4e01074 100644
> > --- a/hw/core/hotplug.c
> > +++ b/hw/core/hotplug.c
> > @@ -34,6 +34,17 @@ void hotplug_handler_unplug_request(HotplugHandler
> > *plug_handler,
> > }
> > }
> >
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +DeviceState *plugged_dev,
> > +Error **errp)
> > +{
> > +HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> > +
> > +if (hdc->unplug) {
> > +hdc->unplug(plug_handler, plugged_dev, errp);
> > +}
> > +}
> > +
> > static const TypeInfo hotplug_handler_info = {
> > .name = TYPE_HOTPLUG_HANDLER,
> > .parent= TYPE_INTERFACE,
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index c98e5db..c89d781 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -227,8 +227,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> > qdev_hot_removed = true;
> >
> > if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> > -hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> > - dev, errp);
> > +HotplugHandlerClass *hdc;
> > +
> > +/* If device supports async unplug just request it to be done,
> > + * otherwise just remove it synchronously */
> > +hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
> > +if (hdc->unplug_request) {
> > +
> > hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> > + dev, errp);
> > +} else {
> > +hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> > errp);
> > +}
> > } else {
> > assert(dc->unplug != NULL);
> > if (dc->unplug(dev) < 0) { /* legacy handler */
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > index e397d08..451d522 100644
> > --- a/include/hw/hotplug.h
> > +++ b/include/hw/hotplug.h
> > @@ -50,6 +50,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
> >* @unplug_request: unplug request callback.
> >* Used as a means to initiate device unplug for devices
> > that
> >* require asynchronous unplug handling.
> > + * @unplug_request: unplug callback.
>
> Typo, s/unplug_request/unplug ?
Yep, I'll fix it.
>
> Thanks.
>
> > + * Used for device removal with devices that implement
> > + * asynchronous and synchronous (suprise) removal.
> >*/
> > typedef struct HotplugHandlerClass {
> > /* */
> > @@ -58,6 +61,7 @@ typedef struct HotplugHandlerClass {
> > /* */
> > hotplug_fn plug;
> > hotplug_fn unplug_request;
> > +hotplug_fn unplug;
> > } HotplugHandlerClass;
> >
> > /**
> > @@ -77,4 +81,12 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
> > void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> > DeviceState *plugged_dev,
> > Error **errp);
> > +/**
> > + * hotplug_handler_unplug:
> > + *
> > + * Calls #HotplugHandlerClass.unplug callback of @plug_handler.
> > + */
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +DeviceState *plugged_dev,
> > +Error **errp);
> > #endif
>
>
Re: [Qemu-devel] [PATCH] ohci: Split long traces to smaller ones
On Do, 2014-09-25 at 10:16 +1000, Alexey Kardashevskiy wrote: > Recent traces rework introduced 2 tracepoints with 13 and 20 > arguments. When dtrace backend is selected > (--enable-trace-backend=dtrace), compile fails as > sys/sdt.h defines DTRACE_PROBE up to DTRACE_PROBE12 only. > > This splits long tracepoints. Can you also change the tracing-enabled check to use '#ifndef CONFIG_TRACE_NOP' as suggested by Stefan please? thanks, Gerd
Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"
On Thu, 25 Sep 2014 10:01:53 +0200 Gerd Hoffmann wrote: > On Mi, 2014-09-24 at 17:21 +0200, Paolo Bonzini wrote: > > Il 24/09/2014 17:15, Igor Mammedov ha scritto: > > > But there is room for improvement here, > > > it it's possible to error out even earlier if usb-bot would be marked > > > as not hotpluggable device and add to qdev_device_add() check if > > > device is hotpluggable even before it's created. > > > > That would work too. Gerd, what about usb-uas? > > Basically the same camp as usb-bot. UAS doesn't suffer the LUN > numeration issue which BOT has, but there likewise is no signaling about > scsi devices coming and going. Thet's why a excluded UAS from this series, Do I need to make it hotpluggable (like SCSI HBAs without hotplug signalling)? /i.e./ it would be possible to hotplug it but guest won't notice the new drive on UAS until reboot/ > > cheers, > Gerd > >
Re: [Qemu-devel] [PATCH v2 7/7] virtio-scsi: Handle TMF request cancellation asynchronously
Il 25/09/2014 04:01, Fam Zheng ha scritto:
>>> > > -static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>>> > > +typedef struct {
>>> > > +VirtIOSCSIReq *tmf_req;
>>> > > +intremaining;
>>> > > +} VirtIOSCSICancelTracker;
>> >
>> > What about putting "remaining" directly in VirtIOSCSIReq?
> It's rarely used, so I preferred managing it here.
>
It complicates the code though. If you really feel like economizing
space, put it in a union with "QTAILQ_ENTRY(VirtIOSCSIReq) next;".
Paolo
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
On 09/24/2014 07:48 PM, Kevin Wolf wrote:
> Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
>> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo
>> Bonzini geschrieben:
Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> Yes, that's true. We can't fix this problem in qcow2, though, because
> it's a more general one. I think we must make sure that
> bdrv_invalidate_cache() doesn't yield.
>
> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> moving the problem to the caller (where and why is it even called from a
> coroutine?), or possibly by creating a new coroutine for the driver
> callback and running that in a nested event loop that only handles
> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> chance to process new requests in this thread.
Incoming migration runs in a coroutine (the coroutine entry point is
process_incoming_migration_co). But everything after qemu_fclose() can
probably be moved into a separate bottom half, so that it gets out of
coroutine context.
>>>
>>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
>>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
>>> isn't a problem that can be completely fixed in qcow2.
>>
>>
>> Ok. Tried :) Not very successful though. The patch is below.
>>
>> Is that the correct bottom half? When I did it, I started getting crashes
>> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
>> Normally the code would check s->l1_size and then use but they are out of
>> sync.
>
> No, that's not the place we were talking about.
>
> What Paolo meant is that in process_incoming_migration_co(), you can
> split out the final part that calls bdrv_invalidate_cache_all() into a
> BH (you need to move everything until the end of the function into the
> BH then). His suggestion was to move everything below the qemu_fclose().
Ufff. I took it very literally. Ok. Let it be
process_incoming_migration_co(). But there is something I am missing about
BHs. Here is a patch:
diff --git a/migration.c b/migration.c
index 6db04a6..101043e 100644
--- a/migration.c
+++ b/migration.c
@@ -88,6 +88,9 @@ void qemu_start_incoming_migration(const char *uri, Error
**errp)
}
}
+static QEMUBH *migration_complete_bh;
+static void process_incoming_migration_complete(void *opaque);
+
static void process_incoming_migration_co(void *opaque)
{
QEMUFile *f = opaque;
@@ -117,6 +120,16 @@ static void process_incoming_migration_co(void *opaque)
} else {
runstate_set(RUN_STATE_PAUSED);
}
+
+migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
+ process_incoming_migration_complete,
+ NULL);
+}
+
+static void process_incoming_migration_complete(void *opaque)
+{
+qemu_bh_delete(migration_complete_bh);
+migration_complete_bh = NULL;
}
void process_incoming_migration(QEMUFile *f)
Then I run it under gdb and set breakpoint in
process_incoming_migration_complete - and it never hits. Why is that? Thanks.
--
Alexey
Re: [Qemu-devel] [PATCH v6 3/3] ivshmem: add check on protocol version in QEMU
On 09/23/2014 05:58 PM, Stefan Hajnoczi wrote: I'm not sure a full-fledged feature negotiation system is needed. The ivshmem protocol is local to the host and all participants are under control of the administrator. I suggested a protocol version to protect against misconfiguration. For example, building QEMU from source but talking to an outdated ivhsmem server that is still running from before. Remember that ivshmem-server and QEMU are shipped together by the distro. So in 99% of the cases they will have the same version anyway. But we want to protect against rare misconfiguration that break things (user mixing and matching incompatible software). Ok, so how about keeping with this simple mechanism at the moment ? If this versionning system is too limited in the future, I think we will need a global rework on the protocol, anyway. -- David Marchand
Re: [Qemu-devel] [PATCH v3 5/7] scsi: Introduce scsi_req_cancel_complete
Il 25/09/2014 04:20, Fam Zheng ha scritto:
> Let the aio cb do the clean up and notification job after scsi_req_cancel, in
> preparation for asynchronous cancellation.
>
> Signed-off-by: Fam Zheng
> ---
> hw/scsi/scsi-bus.c | 14 ++
> hw/scsi/scsi-disk.c| 8
> hw/scsi/scsi-generic.c | 1 +
> include/hw/scsi/scsi.h | 1 +
> 4 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 764f6cf..c91db63 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1718,6 +1718,16 @@ void scsi_req_complete(SCSIRequest *req, int status)
> scsi_req_unref(req);
> }
>
> +/* Called by the devices when the request is canceled. */
> +void scsi_req_cancel_complete(SCSIRequest *req)
> +{
> +assert(req->io_canceled);
> +if (req->bus->info->cancel) {
> +req->bus->info->cancel(req);
> +}
> +scsi_req_unref(req);
> +}
> +
> void scsi_req_cancel(SCSIRequest *req)
> {
> trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> @@ -1730,10 +1740,6 @@ void scsi_req_cancel(SCSIRequest *req)
> if (req->aiocb) {
> bdrv_aio_cancel(req->aiocb);
> }
> -if (req->bus->info->cancel) {
> -req->bus->info->cancel(req);
> -}
> -scsi_req_unref(req);
> }
>
> static int scsi_ua_precedence(SCSISense sense)
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ef13e66..7a7938a 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -168,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
> r->req.aiocb = NULL;
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -214,6 +215,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -240,6 +242,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> }
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -280,6 +283,7 @@ static void scsi_read_complete(void * opaque, int ret)
> r->req.aiocb = NULL;
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -312,6 +316,7 @@ static void scsi_do_read(void *opaque, int ret)
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> }
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -432,6 +437,7 @@ static void scsi_write_complete(void * opaque, int ret)
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> }
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -1524,6 +1530,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>
> r->req.aiocb = NULL;
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> @@ -1623,6 +1630,7 @@ static void scsi_write_same_complete(void *opaque, int
> ret)
> r->req.aiocb = NULL;
> block_acct_done(bdrv_get_stats(s->qdev.conf.bs), &r->acct);
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7e85047..01bca08 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -94,6 +94,7 @@ static void scsi_command_complete(void *opaque, int ret)
>
> r->req.aiocb = NULL;
> if (r->req.io_canceled) {
> +scsi_req_cancel_complete(&r->req);
> goto done;
> }
> if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 1118107..a75a7c8 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -265,6 +265,7 @@ void scsi_req_complete(SCSIRequest *req, int status);
> uint8_t *scsi_req_get_buf(SCSIRequest *req);
> int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
> void scsi_req_abort(SCSIRequest *req, int status);
> +void scsi_req_cancel_complete(SCSIRequest *req);
> void scsi_req_cancel(SCSIRequest *req);
> void scsi_req_retry(SCSIRequest *req);
> void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
>
Reviewed-by: Paolo Bonzini
[Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
During code review for xen I noticed that --enable-debug-info would still strip the binaries because strip_opt= defaults to yes. If --enable-debug-info is passed to configure it has to be assumed that not only the compiled binaries have debugsymbols, also the installed binaries should keep the symbols. The requirement to pass also --disable-strip looks odd. Signed-off-by: Olaf Hering Cc: Paolo Bonzini Cc: Peter Maydell Cc: Michael Tokarev Cc: Stefan Hajnoczi Cc: Stefan Weil --- configure | 1 + 1 file changed, 1 insertion(+) diff --git a/configure b/configure index 862f6d2..1fd5c6b 100755 --- a/configure +++ b/configure @@ -357,6 +357,7 @@ for opt do EXTRA_LDFLAGS="$optarg" ;; --enable-debug-info) debug_info="yes" + strip_opt="no" ;; --disable-debug-info) debug_info="no" ;;
Re: [Qemu-devel] [PATCH v3 6/7] scsi: Introduce scsi_req_cancel_async
Il 25/09/2014 04:20, Fam Zheng ha scritto:
> Devices will call this function to start an asynchronous cancellation. The
> bus->info->cancel will be called after the request is canceled.
>
> Devices will probably need to track a separate TMF request that triggers this
> cancellation, and wait until the cancellation is done before completing it. So
> we store a notifier list in SCSIRequest and in scsi_req_cancel_complete we
> notify them.
>
> Signed-off-by: Fam Zheng
> ---
> hw/scsi/scsi-bus.c | 23 +++
> include/hw/scsi/scsi.h | 3 +++
> 2 files changed, 26 insertions(+)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index c91db63..df7585a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -566,6 +566,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops,
> SCSIDevice *d,
> req->ops = reqops;
> object_ref(OBJECT(d));
> object_ref(OBJECT(qbus->parent));
> +notifier_list_init(&req->cancel_notifiers);
> trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
> return req;
> }
> @@ -1725,9 +1726,31 @@ void scsi_req_cancel_complete(SCSIRequest *req)
> if (req->bus->info->cancel) {
> req->bus->info->cancel(req);
> }
> +notifier_list_notify(&req->cancel_notifiers, req);
I think you also have to call notifier_list_notify from
scsi_req_complete, because a cancelled request might end up being
completed instead of cancelled.
In fact, the next obvious step (enabled by your bdrv_aio_cancel cleanup)
would be to _not_ call scsi_req_cancel_complete if we can report
completion or if there was an I/O error. This can happen for
scsi-generic, scsi_dma_complete_noio, etc. Basically, moving the
io_canceled check from the beginning of the completion routine to just
before bdrv_aio_* or dma_aio_* are called.
Paolo
> scsi_req_unref(req);
> }
>
> +/* Cancel @req asynchronously. @notifier is added to @req's cancellation
> + * notifier list, the bus will be notified the requests cancellation is
> + * completed.
> + * */
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
> +{
> +trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> +if (notifier) {
> +notifier_list_add(&req->cancel_notifiers, notifier);
> +}
> +if (req->io_canceled) {
> +return;
> +}
> +scsi_req_ref(req);
> +scsi_req_dequeue(req);
> +req->io_canceled = true;
> +if (req->aiocb) {
> +bdrv_aio_cancel_async(req->aiocb);
> +}
> +}
> +
> void scsi_req_cancel(SCSIRequest *req)
> {
> trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index a75a7c8..c47dc53 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -5,6 +5,7 @@
> #include "block/block.h"
> #include "hw/block/block.h"
> #include "sysemu/sysemu.h"
> +#include "qemu/notify.h"
>
> #define MAX_SCSI_DEVS255
>
> @@ -53,6 +54,7 @@ struct SCSIRequest {
> void *hba_private;
> size_tresid;
> SCSICommand cmd;
> +NotifierList cancel_notifiers;
>
> /* Note:
> * - fields before sense are initialized by scsi_req_alloc;
> @@ -267,6 +269,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf,
> int len);
> void scsi_req_abort(SCSIRequest *req, int status);
> void scsi_req_cancel_complete(SCSIRequest *req);
> void scsi_req_cancel(SCSIRequest *req);
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier);
> void scsi_req_retry(SCSIRequest *req);
> void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
> void scsi_device_set_ua(SCSIDevice *sdev, SCSISense sense);
>
Re: [Qemu-devel] [PATCH] qemu-socket: Eliminate silly QERR_ macros
Il 25/09/2014 08:49, Markus Armbruster ha scritto:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> They're used with error_set() and qerror_report(), and expand into the
> first *two* arguments. This trickiness has become pointless. Clean
> up.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/qmp/qerror.h | 12
> util/qemu-sockets.c | 26 +-
> 2 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 774e75d..0ca6cbd 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -154,16 +154,4 @@ void qerror_report_err(Error *err);
> #define QERR_UNSUPPORTED \
> ERROR_CLASS_GENERIC_ERROR, "this feature or command is not currently
> supported"
>
> -#define QERR_SOCKET_CONNECT_FAILED \
> -ERROR_CLASS_GENERIC_ERROR, "Failed to connect socket"
> -
> -#define QERR_SOCKET_LISTEN_FAILED \
> -ERROR_CLASS_GENERIC_ERROR, "Failed to listen on socket"
> -
> -#define QERR_SOCKET_BIND_FAILED \
> -ERROR_CLASS_GENERIC_ERROR, "Failed to bind socket"
> -
> -#define QERR_SOCKET_CREATE_FAILED \
> -ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
> -
> #endif /* QERROR_H */
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 4a25585..1eef590 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -159,7 +159,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset,
> Error **errp)
> slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> if (slisten < 0) {
> if (!e->ai_next) {
> -error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> +error_setg_errno(errp, errno, "Failed to create socket");
> }
> continue;
> }
> @@ -183,7 +183,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset,
> Error **errp)
> }
> if (p == port_max) {
> if (!e->ai_next) {
> -error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
> +error_setg_errno(errp, errno, "Failed to bind socket");
> }
> }
> }
> @@ -194,7 +194,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset,
> Error **errp)
>
> listen:
> if (listen(slisten,1) != 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
> +error_setg_errno(errp, errno, "Failed to listen on socket");
> closesocket(slisten);
> freeaddrinfo(res);
> return -1;
> @@ -281,7 +281,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool
> *in_progress,
>
> sock = qemu_socket(addr->ai_family, addr->ai_socktype,
> addr->ai_protocol);
> if (sock < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> +error_setg_errno(errp, errno, "Failed to create socket");
> return -1;
> }
> socket_set_fast_reuse(sock);
> @@ -302,7 +302,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool
> *in_progress,
> connect_state);
> *in_progress = true;
> } else if (rc < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> +error_setg_errno(errp, errno, "Failed to connect socket");
> closesocket(sock);
> return -1;
> }
> @@ -466,20 +466,20 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
> /* create socket */
> sock = qemu_socket(peer->ai_family, peer->ai_socktype,
> peer->ai_protocol);
> if (sock < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> +error_setg_errno(errp, errno, "Failed to create socket");
> goto err;
> }
> socket_set_fast_reuse(sock);
>
> /* bind socket */
> if (bind(sock, local->ai_addr, local->ai_addrlen) < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
> +error_setg_errno(errp, errno, "Failed to bind socket");
> goto err;
> }
>
> /* connect to peer */
> if (connect(sock,peer->ai_addr,peer->ai_addrlen) < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> +error_setg_errno(errp, errno, "Failed to connect socket");
> goto err;
> }
>
> @@ -684,7 +684,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>
> sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> if (sock < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> +error_setg_errno(errp, errno, "Failed to create socket");
> return -1;
> }
>
> @@ -709,11 +709,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>
> unlink(un.sun_path);
> if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> -error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
> +error_setg_errno(errp, errno, "Failed to bind so
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
Am 25.09.2014 um 10:41 hat Alexey Kardashevskiy geschrieben:
> On 09/24/2014 07:48 PM, Kevin Wolf wrote:
> > Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
> >> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat
> >> Paolo Bonzini geschrieben:
> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> > Yes, that's true. We can't fix this problem in qcow2, though, because
> > it's a more general one. I think we must make sure that
> > bdrv_invalidate_cache() doesn't yield.
> >
> > Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> > moving the problem to the caller (where and why is it even called from a
> > coroutine?), or possibly by creating a new coroutine for the driver
> > callback and running that in a nested event loop that only handles
> > bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> > chance to process new requests in this thread.
>
> Incoming migration runs in a coroutine (the coroutine entry point is
> process_incoming_migration_co). But everything after qemu_fclose() can
> probably be moved into a separate bottom half, so that it gets out of
> coroutine context.
> >>>
> >>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
> >>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> >>> isn't a problem that can be completely fixed in qcow2.
> >>
> >>
> >> Ok. Tried :) Not very successful though. The patch is below.
> >>
> >> Is that the correct bottom half? When I did it, I started getting crashes
> >> in various sport on accesses to s->l1_cache which is NULL after
> >> qcow2_close.
> >> Normally the code would check s->l1_size and then use but they are out of
> >> sync.
> >
> > No, that's not the place we were talking about.
> >
> > What Paolo meant is that in process_incoming_migration_co(), you can
> > split out the final part that calls bdrv_invalidate_cache_all() into a
> > BH (you need to move everything until the end of the function into the
> > BH then). His suggestion was to move everything below the qemu_fclose().
>
> Ufff. I took it very literally. Ok. Let it be
> process_incoming_migration_co(). But there is something I am missing about
> BHs. Here is a patch:
>
>
> diff --git a/migration.c b/migration.c
> index 6db04a6..101043e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,6 +88,9 @@ void qemu_start_incoming_migration(const char *uri, Error
> **errp)
> }
> }
>
> +static QEMUBH *migration_complete_bh;
> +static void process_incoming_migration_complete(void *opaque);
> +
> static void process_incoming_migration_co(void *opaque)
> {
> QEMUFile *f = opaque;
> @@ -117,6 +120,16 @@ static void process_incoming_migration_co(void *opaque)
> } else {
> runstate_set(RUN_STATE_PAUSED);
> }
> +
> +migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
> + process_incoming_migration_complete,
> + NULL);
> +}
> +
> +static void process_incoming_migration_complete(void *opaque)
> +{
> +qemu_bh_delete(migration_complete_bh);
> +migration_complete_bh = NULL;
> }
>
> void process_incoming_migration(QEMUFile *f)
>
>
>
> Then I run it under gdb and set breakpoint in
> process_incoming_migration_complete - and it never hits. Why is that? Thanks.
You need to call qemu_bh_schedule().
Kevin
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote: > Does this fix the problem or does it just make it less likely that it > becomes apparent? Sorry for not making this clearer in my commit message. I haven't been able to reproduce the corruption with the fiemap flag change. > If there is a data corruptor, we need to fix it, not just ensure that > only the less common environments are affected. I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the corrupter and then, as you say, makes that code less commonly executed. > That looks like a logically separate change, so it should probably be > a separate patch. Sure I can do that, and be more explicit about the reason in the commit message. > Is this fix for the corruptor? The commit message doesn't make it > clear. If so and fiemap is safe now, why would we still prefer > seek_hole? The preference for seek_hole was a suggestion from Pádraig Brady , so I'll let him defend that :) but as I said above I think it was about reducing the situations where fiemap was/is called. Tony. pgp19uz_IjiQT.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH] ohci: Split long traces to smaller ones
Alexey Kardashevskiy writes:
> Recent traces rework introduced 2 tracepoints with 13 and 20
> arguments. When dtrace backend is selected
> (--enable-trace-backend=dtrace), compile fails as
> sys/sdt.h defines DTRACE_PROBE up to DTRACE_PROBE12 only.
FWIW lltng ust limits TP_ARGS to 10 fields.
>
> This splits long tracepoints.
>
> Signed-off-by: Alexey Kardashevskiy
>
> ---
>
> And this one should be squashed into
> [PATCH] ohci: Convert fprint/DPRINTF/print to traces
>
> Sorry about that...
> ---
> hw/usb/hcd-ohci.c | 20
> trace-events | 6 --
> 2 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 7ea871d..8d3c9cc 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -723,11 +723,14 @@ static int ohci_service_iso_td(OHCIState *ohci, struct
> ohci_ed *ed,
> trace_usb_ohci_iso_td_head(
> ed->head & OHCI_DPTR_MASK, ed->tail & OHCI_DPTR_MASK,
> iso_td.flags, iso_td.bp, iso_td.next, iso_td.be,
> - iso_td.offset[0], iso_td.offset[1], iso_td.offset[2],
> iso_td.offset[3],
> - iso_td.offset[4], iso_td.offset[5], iso_td.offset[6],
> iso_td.offset[7],
> - ohci->frame_number, starting_frame,
> - frame_count, relative_frame_number,
> + ohci->frame_number, starting_frame,
> + frame_count, relative_frame_number,
> OHCI_BM(iso_td.flags, TD_DI), OHCI_BM(iso_td.flags, TD_CC));
> +trace_usb_ohci_iso_td_head_offset(
> + iso_td.offset[0], iso_td.offset[1],
> + iso_td.offset[2], iso_td.offset[3],
> + iso_td.offset[4], iso_td.offset[5],
> + iso_td.offset[6], iso_td.offset[7]);
>
> if (relative_frame_number < 0) {
>
> trace_usb_ohci_iso_td_relative_frame_number_neg(relative_frame_number);
> @@ -1199,13 +1202,14 @@ static int ohci_service_ed_list(OHCIState *ohci,
> uint32_t head, int completion)
> }
>
> while ((ed.head & OHCI_DPTR_MASK) != ed.tail) {
> -trace_usb_ohci_ed_pkt(cur,
> +trace_usb_ohci_ed_pkt(cur, (ed.head & OHCI_ED_H) != 0,
> +(ed.head & OHCI_ED_C) != 0, ed.head & OHCI_DPTR_MASK,
> +ed.tail & OHCI_DPTR_MASK, ed.next & OHCI_DPTR_MASK);
> +trace_usb_ohci_ed_pkt_flags(
> OHCI_BM(ed.flags, ED_FA), OHCI_BM(ed.flags, ED_EN),
> OHCI_BM(ed.flags, ED_D), (ed.flags & OHCI_ED_S)!= 0,
> (ed.flags & OHCI_ED_K) != 0, (ed.flags & OHCI_ED_F) != 0,
> -OHCI_BM(ed.flags, ED_MPS), (ed.head & OHCI_ED_H) != 0,
> -(ed.head & OHCI_ED_C) != 0, ed.head & OHCI_DPTR_MASK,
> -ed.tail & OHCI_DPTR_MASK, ed.next & OHCI_DPTR_MASK);
> +OHCI_BM(ed.flags, ED_MPS));
>
> active = 1;
>
> diff --git a/trace-events b/trace-events
> index a747ab1..02aa592 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -297,7 +297,8 @@ usb_port_release(int bus, const char *port) "bus %d, port
> %s"
>
> # hw/usb/hcd-ohci.c
> usb_ohci_iso_td_read_failed(uint32_t addr) "ISO_TD read error at %x"
> -usb_ohci_iso_td_head(uint32_t head, uint32_t tail, uint32_t flags, uint32_t
> bp, uint32_t next, uint32_t be, uint32_t o0, uint32_t o1, uint32_t o2,
> uint32_t o3, uint32_t o4, uint32_t o5, uint32_t o6, uint32_t o7, uint32_t
> framenum, uint32_t startframe, uint32_t framecount, int rel_frame_num,
> uint32_t bm_di, uint32_t td_cc) "ISO_TD ED head 0x%.8x tailp 0x%.8x\n0x%.8x
> 0x%.8x 0x%.8x 0x%.8x\n0x%.8x 0x%.8x 0x%.8x 0x%.8x\n0x%.8x 0x%.8x 0x%.8x
> 0x%.8x\nframe_number 0x%.8x starting_frame 0x%.8x\nframe_count 0x%.8x
> relative %d\ndi 0x%.8x cc 0x%.8x"
> +usb_ohci_iso_td_head(uint32_t head, uint32_t tail, uint32_t flags, uint32_t
> bp, uint32_t next, uint32_t be, uint32_t framenum, uint32_t startframe,
> uint32_t framecount, int rel_frame_num, uint32_t bm_di, uint32_t td_cc)
> "ISO_TD ED head 0x%.8x tailp 0x%.8x\n0x%.8x 0x%.8x 0x%.8x
> 0x%.8x\nframe_number 0x%.8x starting_frame 0x%.8x\nframe_count 0x%.8x
> relative %d\ndi 0x%.8x cc 0x%.8x"
> +usb_ohci_iso_td_head_offset(uint32_t o0, uint32_t o1, uint32_t o2, uint32_t
> o3, uint32_t o4, uint32_t o5, uint32_t o6, uint32_t o7) "0x%.8x 0x%.8x 0x%.8x
> 0x%.8x 0x%.8x 0x%.8x 0x%.8x 0x%.8x"
> usb_ohci_iso_td_relative_frame_number_neg(int rel) "ISO_TD R=%d < 0"
> usb_ohci_iso_td_relative_frame_number_big(int rel, int count) "ISO_TD R=%d >
> FC=%d"
> usb_ohci_iso_td_bad_direction(int dir) "Bad direction %d"
> @@ -336,7 +337,8 @@ usb_ohci_td_pkt_full(const char *dir, const char *buf)
> "%s data: %s"
> usb_ohci_td_too_many_pending(void) ""
> usb_ohci_td_packet_status(int status) "status=%d"
> usb_ohci_ed_read_error(uint32_t addr) "ED read error at %x"
> -usb_ohci_ed_pkt(uint32_t cur, uint32_t fa, uint32_t en, uint32_t d, int s,
> int k, int f, uint32_t mps, int h, int c, uint32_t h
[Qemu-devel] [PATCH] ohci: drop computed flags from trace events
This exceeded the trace argument limit for LTTNG UST and wasn't really needed as the flags value is stored anyway. Dropping this fixes the compile failure for UST. It can probably be merged with the previous trace shortening patch. Signed-off-by: Alex Bennée --- hw/usb/hcd-ohci.c | 3 +-- trace-events | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 8d3c9cc..9a84eb6 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -724,8 +724,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, ed->head & OHCI_DPTR_MASK, ed->tail & OHCI_DPTR_MASK, iso_td.flags, iso_td.bp, iso_td.next, iso_td.be, ohci->frame_number, starting_frame, - frame_count, relative_frame_number, - OHCI_BM(iso_td.flags, TD_DI), OHCI_BM(iso_td.flags, TD_CC)); + frame_count, relative_frame_number); trace_usb_ohci_iso_td_head_offset( iso_td.offset[0], iso_td.offset[1], iso_td.offset[2], iso_td.offset[3], diff --git a/trace-events b/trace-events index a3c1aac..ac962be 100644 --- a/trace-events +++ b/trace-events @@ -297,7 +297,7 @@ usb_port_release(int bus, const char *port) "bus %d, port %s" # hw/usb/hcd-ohci.c usb_ohci_iso_td_read_failed(uint32_t addr) "ISO_TD read error at %x" -usb_ohci_iso_td_head(uint32_t head, uint32_t tail, uint32_t flags, uint32_t bp, uint32_t next, uint32_t be, uint32_t framenum, uint32_t startframe, uint32_t framecount, int rel_frame_num, uint32_t bm_di, uint32_t td_cc) "ISO_TD ED head 0x%.8x tailp 0x%.8x\n0x%.8x 0x%.8x 0x%.8x 0x%.8x\nframe_number 0x%.8x starting_frame 0x%.8x\nframe_count 0x%.8x relative %d\ndi 0x%.8x cc 0x%.8x" +usb_ohci_iso_td_head(uint32_t head, uint32_t tail, uint32_t flags, uint32_t bp, uint32_t next, uint32_t be, uint32_t framenum, uint32_t startframe, uint32_t framecount, int rel_frame_num) "ISO_TD ED head 0x%.8x tailp 0x%.8x\n0x%.8x 0x%.8x 0x%.8x 0x%.8x\nframe_number 0x%.8x starting_frame 0x%.8x\nframe_count 0x%.8x relative %d" usb_ohci_iso_td_head_offset(uint32_t o0, uint32_t o1, uint32_t o2, uint32_t o3, uint32_t o4, uint32_t o5, uint32_t o6, uint32_t o7) "0x%.8x 0x%.8x 0x%.8x 0x%.8x 0x%.8x 0x%.8x 0x%.8x 0x%.8x" usb_ohci_iso_td_relative_frame_number_neg(int rel) "ISO_TD R=%d < 0" usb_ohci_iso_td_relative_frame_number_big(int rel, int count) "ISO_TD R=%d > FC=%d" -- 2.1.0
[Qemu-devel] [PATCH] scripts/tracetool: don't barf on formats with precision
This only affects lttng user space tracing at the moment.
Signed-off-by: Alex Bennée
---
scripts/tracetool/__init__.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 36c789d..5aa2eed 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -146,7 +146,7 @@ class Event(object):
"\s*"
"(?:(?:(?P\".+),)?\s*(?P\".+))?"
"\s*")
-_FMT = re.compile("(%\w+|%.*PRI\S+)")
+_FMT = re.compile("(%[\d\.]*\w+|%.*PRI\S+)")
_VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec"])
--
2.1.0
Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration
On 25.09.14 09:02, Alexey Kardashevskiy wrote:
> The only case when sPAPR NVRAM migrates now is if is backed by a file and
> copy-storage migration is performed.
>
> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
>
> This defines a VMSTATE descriptor for NVRAM device so the memory copy
> of NVRAM can migrate and be written to a backing file on the destination
> if one is provided.
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> hw/nvram/spapr_nvram.c | 68
> +++---
> 1 file changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 6a72ef4..254009e 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu,
> sPAPREnvironment *spapr,
> return;
> }
>
> +assert(nvram->buf);
> +
> membuf = cpu_physical_memory_map(buffer, &len, 1);
> +
> +alen = len;
> if (nvram->drive) {
> alen = bdrv_pread(nvram->drive, offset, membuf, len);
> +if (alen > 0) {
> +memcpy(nvram->buf + offset, membuf, alen);
Why?
> +}
> } else {
> -assert(nvram->buf);
> -
> memcpy(membuf, nvram->buf + offset, len);
> -alen = len;
> }
> +
> cpu_physical_memory_unmap(membuf, len, 1, len);
>
> rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
> @@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu,
> sPAPREnvironment *spapr,
> }
>
> membuf = cpu_physical_memory_map(buffer, &len, 0);
> +
> +alen = len;
> if (nvram->drive) {
> alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
> -} else {
> -assert(nvram->buf);
> -
> -memcpy(nvram->buf + offset, membuf, len);
> -alen = len;
> }
> +
> +assert(nvram->buf);
> +memcpy(nvram->buf + offset, membuf, len);
> +
> cpu_physical_memory_unmap(membuf, len, 0, len);
>
> rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
> @@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
> nvram->size = bdrv_getlength(nvram->drive);
> } else {
> nvram->size = DEFAULT_NVRAM_SIZE;
> -nvram->buf = g_malloc0(nvram->size);
> }
>
> +nvram->buf = g_malloc0(nvram->size);
> +
> if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
> fprintf(stderr, "spapr-nvram must be between %d and %d bytes in
> size\n",
> MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
> @@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void
> *fdt, int node_off)
> return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
> }
>
> +static int spapr_nvram_pre_load(void *opaque)
> +{
> +sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
> +
> +g_free(nvram->buf);
> +nvram->buf = NULL;
> +nvram->size = 0;
> +
> +return 0;
> +}
> +
> +static int spapr_nvram_post_load(void *opaque, int version_id)
> +{
> +sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
> +
> +if (nvram->drive) {
> +int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
In the file backed case you're already overwriting the disk backed
version, no?
Also, couldn't you just do the copy and provisioning of buf in a
pre_save hook?
Alex
> +
> +if (alen < 0) {
> +return alen;
> +}
> +if (alen != nvram->size) {
> +return -1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_nvram = {
> +.name = "spapr_nvram",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.pre_load = spapr_nvram_pre_load,
> +.post_load = spapr_nvram_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(size, sPAPRNVRAM),
> +VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> static Property spapr_nvram_properties[] = {
> DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
> DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, drive),
> @@ -184,6 +233,7 @@ static void spapr_nvram_class_init(ObjectClass *klass,
> void *data)
> k->dt_compatible = "qemu,spapr-nvram";
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> dc->props = spapr_nvram_properties;
> +dc->vmsd = &vmstate_spapr_nvram;
> }
>
> static const TypeInfo spapr_nvram_type_info = {
>
Re: [Qemu-devel] [PATCH] ohci: Split long traces to smaller ones
On 09/25/2014 06:09 PM, Gerd Hoffmann wrote: > On Do, 2014-09-25 at 10:16 +1000, Alexey Kardashevskiy wrote: >> Recent traces rework introduced 2 tracepoints with 13 and 20 >> arguments. When dtrace backend is selected >> (--enable-trace-backend=dtrace), compile fails as >> sys/sdt.h defines DTRACE_PROBE up to DTRACE_PROBE12 only. >> >> This splits long tracepoints. > > Can you also change the tracing-enabled check to use '#ifndef > CONFIG_TRACE_NOP' as suggested by Stefan please? Nope :( As I said in that thread, I am not familiar with "dtrace" - are traces configurable with it? With --enable-trace-backend=dtrace, trace_event_get_state is not defined so CONFIG_TRACE_NOP is not 100% equal replacement. -- Alexey
[Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report
From: Gonglei
Signed-off-by: Gonglei
---
util/oslib-posix.c | 8
util/oslib-win32.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 016a047..00310f6 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -88,7 +88,7 @@ int qemu_daemon(int nochdir, int noclose)
void *qemu_oom_check(void *ptr)
{
if (ptr == NULL) {
-fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
+error_report("Failed to allocate memory: %s", strerror(errno));
abort();
}
return ptr;
@@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
ret = sigaction(SIGBUS, &act, &oldact);
if (ret) {
-perror("os_mem_prealloc: failed to install signal handler");
+error_report("os_mem_prealloc: failed to install signal handler");
exit(1);
}
@@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
if (sigsetjmp(sigjump, 1)) {
-fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
+error_report("os_mem_prealloc: failed to preallocate pages");
exit(1);
} else {
int i;
@@ -404,7 +404,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
ret = sigaction(SIGBUS, &oldact, NULL);
if (ret) {
-perror("os_mem_prealloc: failed to reinstall signal handler");
+error_report("os_mem_prealloc: failed to reinstall signal
handler");
exit(1);
}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index a3eab4a..47a0df7 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -44,7 +44,7 @@
void *qemu_oom_check(void *ptr)
{
if (ptr == NULL) {
-fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
+error_report("Failed to allocate memory: %lu", GetLastError());
abort();
}
return ptr;
--
1.7.12.4
[Qemu-devel] [PATCH 0/4] os: convert fprintf/perror to error_report
From: Gonglei Those are trivial patches. Please consider applying to trivial-next. Thanks! Gonglei (4): os-posix/win32: convert fprintf/perror to error_report os-posix: report error message when lock file failed osdep: convert fprintf to error_report oslib-posix/win32: convert fprintf/perror to error_report os-posix.c | 35 +++ os-win32.c | 3 ++- util/osdep.c | 8 util/oslib-posix.c | 8 util/oslib-win32.c | 2 +- 5 files changed, 30 insertions(+), 26 deletions(-) -- 1.7.12.4
[Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to error_report
From: Gonglei
Signed-off-by: Gonglei
---
os-posix.c | 34 ++
os-win32.c | 3 ++-
2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/os-posix.c b/os-posix.c
index cb2a7f7..9d5ae70 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -39,6 +39,7 @@
#include "sysemu/sysemu.h"
#include "net/slirp.h"
#include "qemu-options.h"
+#include "qemu/error-report.h"
#ifdef CONFIG_LINUX
#include
@@ -120,11 +121,11 @@ void os_set_proc_name(const char *s)
/* Could rewrite argv[0] too, but that's a bit more complicated.
This simple way is enough for `top'. */
if (prctl(PR_SET_NAME, name)) {
-perror("unable to change process name");
+error_report("unable to change process name");
exit(1);
}
#else
-fprintf(stderr, "Change of process name not supported by your OS\n");
+error_report("Change of process name not supported by your OS");
exit(1);
#endif
}
@@ -145,7 +146,7 @@ void os_parse_cmd_args(int index, const char *optarg)
case QEMU_OPTION_runas:
user_pwd = getpwnam(optarg);
if (!user_pwd) {
-fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+error_report("User \"%s\" doesn't exist", optarg);
exit(1);
}
break;
@@ -167,20 +168,20 @@ static void change_process_uid(void)
{
if (user_pwd) {
if (setgid(user_pwd->pw_gid) < 0) {
-fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+error_report("Failed to setgid(%d)\n", user_pwd->pw_gid);
exit(1);
}
if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-user_pwd->pw_name, user_pwd->pw_gid);
+error_report("Failed to initgroups(\"%s\", %d)",
+ user_pwd->pw_name, user_pwd->pw_gid);
exit(1);
}
if (setuid(user_pwd->pw_uid) < 0) {
-fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+error_report("Failed to setuid(%d)", user_pwd->pw_uid);
exit(1);
}
if (setuid(0) != -1) {
-fprintf(stderr, "Dropping privileges failed\n");
+error_report("Dropping privileges failed");
exit(1);
}
}
@@ -190,11 +191,11 @@ static void change_root(void)
{
if (chroot_dir) {
if (chroot(chroot_dir) < 0) {
-fprintf(stderr, "chroot failed\n");
+error_report("chroot failed");
exit(1);
}
if (chdir("/")) {
-perror("not able to chdir to /");
+error_report("not able to chdir to /");
exit(1);
}
}
@@ -224,7 +225,7 @@ void os_daemonize(void)
if (len != 1)
exit(1);
else if (status == 1) {
-fprintf(stderr, "Could not acquire pidfile: %s\n",
strerror(errno));
+error_report("Could not acquire pidfile: %s", strerror(errno));
exit(1);
} else
exit(0);
@@ -267,7 +268,7 @@ void os_setup_post(void)
exit(1);
if (chdir("/")) {
-perror("not able to chdir to /");
+error_report("not able to chdir to /");
exit(1);
}
TFR(fd = qemu_open("/dev/null", O_RDWR));
@@ -292,10 +293,11 @@ void os_pidfile_error(void)
if (daemonize) {
uint8_t status = 1;
if (write(fds[1], &status, 1) != 1) {
-perror("daemonize. Writing to pipe\n");
+error_report("daemonize. Writing to pipe");
}
-} else
-fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
+} else {
+error_report("Could not acquire pid file: %s", strerror(errno));
+}
}
void os_set_line_buffering(void)
@@ -338,7 +340,7 @@ int os_mlock(void)
ret = mlockall(MCL_CURRENT | MCL_FUTURE);
if (ret < 0) {
-perror("mlockall");
+error_report("mlockall");
}
return ret;
diff --git a/os-win32.c b/os-win32.c
index 5f95caa..28877b3 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -33,6 +33,7 @@
#include "config-host.h"
#include "sysemu/sysemu.h"
#include "qemu-options.h"
+#include "qemu/error-report.h"
/***/
/* Functions missing in mingw */
@@ -106,7 +107,7 @@ void os_parse_cmd_args(int index, const char *optarg)
void os_pidfile_error(void)
{
-fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
+error_report("Could not acquire pid file: %s", strerror(errno));
}
int qemu_create_pidfile(const char *filename)
--
1.7.12.4
[Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report
From: Gonglei
Signed-off-by: Gonglei
---
util/osdep.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/osdep.c b/util/osdep.c
index b2bd154..7f6e483 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -401,9 +401,9 @@ void fips_set_state(bool requested)
#endif /* __linux__ */
#ifdef _FIPS_DEBUG
-fprintf(stderr, "FIPS mode %s (requested %s)\n",
- (fips_enabled ? "enabled" : "disabled"),
- (requested ? "enabled" : "disabled"));
+error_report("FIPS mode %s (requested %s)",
+ (fips_enabled ? "enabled" : "disabled"),
+ (requested ? "enabled" : "disabled"));
#endif
}
@@ -428,7 +428,7 @@ int socket_init(void)
ret = WSAStartup(MAKEWORD(2, 2), &Data);
if (ret != 0) {
err = WSAGetLastError();
-fprintf(stderr, "WSAStartup: %d\n", err);
+error_report("WSAStartup: %d", err);
return -1;
}
atexit(socket_cleanup);
--
1.7.12.4
[Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed
From: Gonglei
It will cause that create vm failed When manager
tool is killed forcibly (kill -9 libvirtd_pid),
the file not was unlink, and unlock. It's better
that report the error message for users.
Signed-off-by: Huangweidong
Signed-off-by: Gonglei
---
os-posix.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/os-posix.c b/os-posix.c
index 9d5ae70..89831dc 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
return -1;
}
if (lockf(fd, F_TLOCK, 0) == -1) {
+error_report("lock file '%s' failed: %s", filename, strerror(errno));
close(fd);
return -1;
}
--
1.7.12.4
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
Am 25.09.2014 um 11:00 hat Tony Breeds geschrieben: > On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote: > > > Does this fix the problem or does it just make it less likely that it > > becomes apparent? > > Sorry for not making this clearer in my commit message. > > I haven't been able to reproduce the corruption with the fiemap flag > change. > > > If there is a data corruptor, we need to fix it, not just ensure that > > only the less common environments are affected. > > I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the > corrupter and then, as you say, makes that code less commonly executed. > > > That looks like a logically separate change, so it should probably be > > a separate patch. > > Sure I can do that, and be more explicit about the reason in the commit > message. > > > Is this fix for the corruptor? The commit message doesn't make it > > clear. If so and fiemap is safe now, why would we still prefer > > seek_hole? > > The preference for seek_hole was a suggestion from Pádraig Brady , so > I'll let him defend that :) but as I said above I think it was about > reducing the situations where fiemap was/is called. Okay. I'm not opposed to the change, we just need to split and document it properly. So first we should fix the bug by adding FIEMAP_FLAG_SYNC. Or actually, it might just be a workaround for a kernel bug. Is the expected behaviour documented, and if yes, what is it? We should describe as precisely as possible what the situation is and why we have to add this flag in qemu. You can probably reuse a good part of your current commit message for that and extend it a bit. And then we may or may not make the second change with the preference for seek_hole. If FIEMAP_FLAG_SYNC actually does additional writes or an fsync(), switching is a performance optimisation and might be justified as such. Maybe Pádraig has some more input on this. Kevin pgp1Ryc8CZopE.pgp Description: PGP signature
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
On 09/25/2014 06:57 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 10:41 hat Alexey Kardashevskiy geschrieben:
>> On 09/24/2014 07:48 PM, Kevin Wolf wrote:
>>> Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat
Paolo Bonzini geschrieben:
>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>>> Yes, that's true. We can't fix this problem in qcow2, though, because
>>> it's a more general one. I think we must make sure that
>>> bdrv_invalidate_cache() doesn't yield.
>>>
>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>>> moving the problem to the caller (where and why is it even called from a
>>> coroutine?), or possibly by creating a new coroutine for the driver
>>> callback and running that in a nested event loop that only handles
>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>>> chance to process new requests in this thread.
>>
>> Incoming migration runs in a coroutine (the coroutine entry point is
>> process_incoming_migration_co). But everything after qemu_fclose() can
>> probably be moved into a separate bottom half, so that it gets out of
>> coroutine context.
>
> Alexey, you should probably rather try this (and add a bdrv_drain_all()
> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> isn't a problem that can be completely fixed in qcow2.
Ok. Tried :) Not very successful though. The patch is below.
Is that the correct bottom half? When I did it, I started getting crashes
in various sport on accesses to s->l1_cache which is NULL after
qcow2_close.
Normally the code would check s->l1_size and then use but they are out of
sync.
>>>
>>> No, that's not the place we were talking about.
>>>
>>> What Paolo meant is that in process_incoming_migration_co(), you can
>>> split out the final part that calls bdrv_invalidate_cache_all() into a
>>> BH (you need to move everything until the end of the function into the
>>> BH then). His suggestion was to move everything below the qemu_fclose().
>>
>> Ufff. I took it very literally. Ok. Let it be
>> process_incoming_migration_co(). But there is something I am missing about
>> BHs. Here is a patch:
>>
> You need to call qemu_bh_schedule().
Right. Cool. So is below what was suggested? I am doublechecking as it does
not solve the original issue - the bottomhalf is called first and then
nbd_trip() crashes in qcow2_co_flush_to_os().
diff --git a/block.c b/block.c
index d06dd51..1e6dfd1 100644
--- a/block.c
+++ b/block.c
@@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp)
if (local_err) {
error_propagate(errp, local_err);
return;
}
ret = refresh_total_sectors(bs, bs->total_sectors);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not refresh total sector count");
return;
}
+
+bdrv_drain_all();
}
void bdrv_invalidate_cache_all(Error **errp)
{
BlockDriverState *bs;
Error *local_err = NULL;
QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
AioContext *aio_context = bdrv_get_aio_context(bs);
diff --git a/migration.c b/migration.c
index 6db04a6..dc026a9 100644
--- a/migration.c
+++ b/migration.c
@@ -81,49 +81,64 @@ void qemu_start_incoming_migration(const char *uri,
Error **errp)
else if (strstart(uri, "unix:", &p))
unix_start_incoming_migration(p, errp);
else if (strstart(uri, "fd:", &p))
fd_start_incoming_migration(p, errp);
#endif
else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
}
+static QEMUBH *migration_complete_bh;
+static void process_incoming_migration_complete(void *opaque);
+
static void process_incoming_migration_co(void *opaque)
{
QEMUFile *f = opaque;
-Error *local_err = NULL;
int ret;
ret = qemu_loadvm_state(f);
qemu_fclose(f);
free_xbzrle_decoded_buf();
if (ret < 0) {
error_report("load of migration failed: %s", strerror(-ret));
exit(EXIT_FAILURE);
}
qemu_announce_self();
bdrv_clear_incoming_migration_all();
+
+migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
+ process_incoming_migration_complete,
+ NULL);
+qemu_bh_schedule(migration_complete_bh);
+}
+
+static void process_incoming_migration_complete(void *opaque)
+{
+Error *local_err = NULL;
+
/* Make sure all file formats flush their mutable metadata */
bdrv_invalidate_cache_all(&local_err);
if (local_err) {
qerror_report_err(local_err);
error_free(local_err);
exit(EXIT_FAILURE);
}
if (autostart) {
vm_start();
} else {
runstate_set(RUN_STATE_PAUSED);
}
+qemu_bh_delete(migration_compl
Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration
On 09/25/2014 07:43 PM, Alexander Graf wrote:
>
>
> On 25.09.14 09:02, Alexey Kardashevskiy wrote:
>> The only case when sPAPR NVRAM migrates now is if is backed by a file and
>> copy-storage migration is performed.
>>
>> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
>>
>> This defines a VMSTATE descriptor for NVRAM device so the memory copy
>> of NVRAM can migrate and be written to a backing file on the destination
>> if one is provided.
>>
>> Signed-off-by: Alexey Kardashevskiy
>> ---
>> hw/nvram/spapr_nvram.c | 68
>> +++---
>> 1 file changed, 59 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
>> index 6a72ef4..254009e 100644
>> --- a/hw/nvram/spapr_nvram.c
>> +++ b/hw/nvram/spapr_nvram.c
>> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu,
>> sPAPREnvironment *spapr,
>> return;
>> }
>>
>> +assert(nvram->buf);
>> +
>> membuf = cpu_physical_memory_map(buffer, &len, 1);
>> +
>> +alen = len;
>> if (nvram->drive) {
>> alen = bdrv_pread(nvram->drive, offset, membuf, len);
>> +if (alen > 0) {
>> +memcpy(nvram->buf + offset, membuf, alen);
>
> Why?
This way I do not need pre_save hook and I keep the buf in sync with the
file. If I implement pre_save, then buf will serve 2 purposes - it is
either NVRAM itself (if there is no backing file, exists during guest's
lifetime) or it is a migration copy (exists between pre_save and post_load
and then it is disposed). Two quite different uses of the same thing
confuse me. But - I do not mind doing it your way, no big deal, should I?
>> +}
>> } else {
>> -assert(nvram->buf);
>> -
>> memcpy(membuf, nvram->buf + offset, len);
>> -alen = len;
>> }
>> +
>> cpu_physical_memory_unmap(membuf, len, 1, len);
>>
>> rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
>> @@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu,
>> sPAPREnvironment *spapr,
>> }
>>
>> membuf = cpu_physical_memory_map(buffer, &len, 0);
>> +
>> +alen = len;
>> if (nvram->drive) {
>> alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
>> -} else {
>> -assert(nvram->buf);
>> -
>> -memcpy(nvram->buf + offset, membuf, len);
>> -alen = len;
>> }
>> +
>> +assert(nvram->buf);
>> +memcpy(nvram->buf + offset, membuf, len);
>> +
>> cpu_physical_memory_unmap(membuf, len, 0, len);
>>
>> rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
>> @@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
>> nvram->size = bdrv_getlength(nvram->drive);
>> } else {
>> nvram->size = DEFAULT_NVRAM_SIZE;
>> -nvram->buf = g_malloc0(nvram->size);
>> }
>>
>> +nvram->buf = g_malloc0(nvram->size);
>> +
>> if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
>> fprintf(stderr, "spapr-nvram must be between %d and %d bytes in
>> size\n",
>> MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
>> @@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev,
>> void *fdt, int node_off)
>> return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
>> }
>>
>> +static int spapr_nvram_pre_load(void *opaque)
>> +{
>> +sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
>> +
>> +g_free(nvram->buf);
>> +nvram->buf = NULL;
>> +nvram->size = 0;
>> +
>> +return 0;
>> +}
>> +
>> +static int spapr_nvram_post_load(void *opaque, int version_id)
>> +{
>> +sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
>> +
>> +if (nvram->drive) {
>> +int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
>
> In the file backed case you're already overwriting the disk backed
> version, no?
Yes. Is that bad?
> Also, couldn't you just do the copy and provisioning of buf in a
> pre_save hook?
I can do this too. I just do not see why that would be lot better though :)
--
Alexey
Re: [Qemu-devel] [PATCH] qemu-iotests: Fail test if explict test case number is unknown
On Thu, Sep 25, 2014 at 09:46:24AM +0800, Fam Zheng wrote:
> On Wed, 09/24 10:05, Stefan Hajnoczi wrote:
> > On Tue, Sep 23, 2014 at 10:26:26AM +0800, Fam Zheng wrote:
> > > diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> > > index 70df659..2403a20 100644
> > > --- a/tests/qemu-iotests/common
> > > +++ b/tests/qemu-iotests/common
> > > @@ -382,10 +382,16 @@ BEGIN{ for (t='$start'; t<='$end'; t++)
> > > printf "%03d\n",t }' \
> > > echo $id >>$tmp.list
> > > else
> > > # oops
> > > -echo "$id - unknown test, ignored"
> > > +if [ "$start" == "$end" -a "$id" == "$end" ]
> > > +then
> > > +echo "$id - unknown test"
> > > +exit 1
> > > +else
> > > +echo "$id - unknown test, ignored"
> > > +fi
> > > fi
> > > fi
> > > -done
> > > +done || exit 1
> >
> > What is the purpose of this line?
>
> The exit inside the loop is in a subshell so won't cause the whole script to
> exit.
Thanks for explaining. I see it now, was expecting ( ) but the subshell
comes from awk ... | while ... done.
Reviewed-by: Stefan Hajnoczi
pgpwjFmJRmeRW.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH v2] qemu-iotests: Fail test if explicit test case number is unknown
On Wed, Sep 24, 2014 at 11:05:57AM +0800, Fam Zheng wrote: > When we expand a number range, we just print "$id - unknown test, > ignored", this is convenient if we want to run a range of tests. > > When we designate a test case number explicitly, we shouldn't just > ignore it if the case script doesn't exist. > > Print an error and fail the test. > > Signed-off-by: Fam Zheng > > --- > v2: In subject explict -> explicit. > --- > tests/qemu-iotests/common | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi pgpVmpqan17ol.pgp Description: PGP signature
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
> Right. Cool. So is below what was suggested? I am doublechecking as it does
> not solve the original issue - the bottomhalf is called first and then
> nbd_trip() crashes in qcow2_co_flush_to_os().
>
> diff --git a/block.c b/block.c
> index d06dd51..1e6dfd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
> Error **errp)
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> ret = refresh_total_sectors(bs, bs->total_sectors);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not refresh total sector count");
> return;
> }
> +
> +bdrv_drain_all();
> }
Try moving the bdrv_drain_all() call to the top of the function (at
least it must be called before bs->drv->bdrv_invalidate_cache).
> +static QEMUBH *migration_complete_bh;
> +static void process_incoming_migration_complete(void *opaque);
> +
> static void process_incoming_migration_co(void *opaque)
> {
> QEMUFile *f = opaque;
> -Error *local_err = NULL;
> int ret;
>
> ret = qemu_loadvm_state(f);
> qemu_fclose(f);
Paolo suggested to move eveything starting from here, but as far as I
can tell, leaving the next few lines here shouldn't hurt.
> free_xbzrle_decoded_buf();
> if (ret < 0) {
> error_report("load of migration failed: %s", strerror(-ret));
> exit(EXIT_FAILURE);
> }
> qemu_announce_self();
>
> bdrv_clear_incoming_migration_all();
> +
> +migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
> + process_incoming_migration_complete,
> + NULL);
> +qemu_bh_schedule(migration_complete_bh);
> +}
> +
> +static void process_incoming_migration_complete(void *opaque)
> +{
> +Error *local_err = NULL;
> +
> /* Make sure all file formats flush their mutable metadata */
> bdrv_invalidate_cache_all(&local_err);
> if (local_err) {
> qerror_report_err(local_err);
> error_free(local_err);
> exit(EXIT_FAILURE);
> }
>
> if (autostart) {
> vm_start();
> } else {
> runstate_set(RUN_STATE_PAUSED);
> }
> +qemu_bh_delete(migration_complete_bh);
> +migration_complete_bh = NULL;
> }
That part looks good to me. I hope moving bdrv_drain_all() does the
trick, otherwise there's somthing wrong with our reasoning.
Kevin
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
On 25 September 2014 09:06, Olaf Hering wrote: > During code review for xen I noticed that --enable-debug-info would > still strip the binaries because strip_opt= defaults to yes. > If --enable-debug-info is passed to configure it has to be assumed > that not only the compiled binaries have debugsymbols, also the > installed binaries should keep the symbols. The requirement to pass > also --disable-strip looks odd. I think defaulting to build with debug but strip on install makes sense. It follows for example how Debian recommend building things: https://www.debian.org/doc/debian-policy/ch-files.html and means that your installed binaries don't have the extraneous debug info but you can keep the build tree to make sense of backtraces etc later. I also prefer configure's options to be orthogonal: --enable-debug-info should only change how we deal with debug info, and not also have an effect on whether we strip it on install. (Plus your patch as it stands means that the behaviour you get by default now differs from the behaviour if you explicitly say --enable-debug-info.) thanks -- PMM
Re: [Qemu-devel] [PATCH v2] block: Validate node-name
On Thu, Sep 25, 2014 at 09:54:02AM +0200, Kevin Wolf wrote: > The device_name of a BlockDriverState is currently checked because it is > always used as a QemuOpts ID and qemu_opts_create() checks whether such > IDs are wellformed. > > node-name is supposed to share the same namespace, but it isn't checked > currently. This patch adds explicit checks both for device_name and > node-name so that the same rules will still apply even if QemuOpts won't > be used any more at some point. > > qemu-img used to use names with spaces in them, which isn't allowed any > more. Replace them with underscores. > > Signed-off-by: Kevin Wolf > --- > v2: > - Fix qemu-img to use valid names internally [Stefan] > > block.c | 16 +--- > include/qemu/option.h | 1 + > qemu-img.c| 6 +++--- > util/qemu-option.c| 4 ++-- > 4 files changed, 19 insertions(+), 8 deletions(-) Reviewed-by: Stefan Hajnoczi pgpM3CHpsxt_C.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
On Thu, Sep 25, Peter Maydell wrote: > On 25 September 2014 09:06, Olaf Hering wrote: > > During code review for xen I noticed that --enable-debug-info would > > still strip the binaries because strip_opt= defaults to yes. > > If --enable-debug-info is passed to configure it has to be assumed > > that not only the compiled binaries have debugsymbols, also the > > installed binaries should keep the symbols. The requirement to pass > > also --disable-strip looks odd. > > I think defaulting to build with debug but strip on install > makes sense. It follows for example how Debian recommend > building things: > https://www.debian.org/doc/debian-policy/ch-files.html > and means that your installed binaries don't have the > extraneous debug info but you can keep the build tree > to make sense of backtraces etc later. make install DESTIDIR=$RPM_BUILD_ROOT will also remove debug symbols, even if the rpmbuild helper scripts would be able to extract them into a separate -debuginfo package. So will --disable-strip remain for ever? Can I depend on that? Olaf
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
On Thu, Sep 25, 2014 at 10:06:35AM +0200, Olaf Hering wrote: > During code review for xen I noticed that --enable-debug-info would > still strip the binaries because strip_opt= defaults to yes. > If --enable-debug-info is passed to configure it has to be assumed > that not only the compiled binaries have debugsymbols, also the > installed binaries should keep the symbols. The requirement to pass > also --disable-strip looks odd. Perhaps package maintainers rely on installed binaries not having debug symbols? It's common to split the debug symbols into separate ELF files that are shipped in a different package (qemu-debuginfo or similar). If you make this change and packagers are unaware, they could accidentally ship qemu packages that contain the full debug symbols in the binaries. That said, I don't really know... The package maintainers can give you a definitive answer whether or not this is a good thing to do. Stefan pgpOUHL4Frf_u.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API
On Thu, 25 Sep 2014 09:50:58 +0200 Gerd Hoffmann wrote: > On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote: > > On Wed, 24 Sep 2014 15:23:41 +0200 > > Gerd Hoffmann wrote: > > > > > Hi, > > > > > > > > Can't we do this in usb_bus_new instead of duplicating in every host > > > > > adapter? > > > > > > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > > > > the controller? > > > > > > I was more thinking of just setting the callback in common code, but if > > > we can attach the hotplug interface to the usb bus itself not the usb > > > host adapters that would be even better. And it'll probably kill some > > > headache for the companion controller case. > > How making bus a HotplugHandler itself will help with companion controller? > > When uhci acts as ehci companion it registers the ports with ehci and > doesn't manage its own usb bus. ehci will call uhci port ops as needed > (depending on ehci configuration). > > When attaching the hotplug interface to the host controller you'll have > to explicitly handle the companion case somehow. > > When attaching the hotplug interface to the usb bus everything should > just work. Additional bonus is that you also don't have to touch the > host controller code at all then, it should be doable by changing > hw/usb/bus.c only. hotplug-handler.[plug|unplug] callbacks are class wide, so if USB bus might ever need to have different callbacks depending on host it might not work. But since so far it uses the only qdev_simple_device_unplug_cb(), having BUS as hotplug-handler should work too. > > cheers, > Gerd > >
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
On Thu, Sep 25, Stefan Hajnoczi wrote: > If you make this change and packagers are unaware, they could > accidentally ship qemu packages that contain the full debug symbols in > the binaries. rpm will take care of that all by itself. Olaf
Re: [Qemu-devel] [PATCH] ohci: drop computed flags from trace events
On Thu, Sep 25, 2014 at 10:38:44AM +0100, Alex Bennée wrote: > This exceeded the trace argument limit for LTTNG UST and wasn't really > needed as the flags value is stored anyway. Dropping this fixes the > compile failure for UST. It can probably be merged with the previous > trace shortening patch. > > Signed-off-by: Alex Bennée > --- > hw/usb/hcd-ohci.c | 3 +-- > trace-events | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi pgpLRZKQ5Yttk.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
On 25 September 2014 11:47, Olaf Hering wrote: > On Thu, Sep 25, Peter Maydell wrote: > So will --disable-strip remain for ever? Can I depend on that? We're not planning to remove it, certainly. We don't make strong guarantees about configure commandlines remaining stable across QEMU versions but we aren't going to gratuitously break them either. (If you're a distro packager it's probably good practice to scan the configure help output for new option flags you might want to explicitly enable/disable for new versions anyway, though.) -- PMM
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
25.09.2014 14:49, Stefan Hajnoczi wrote: [] > Perhaps package maintainers rely on installed binaries not having debug > symbols? Package maintainer can and _should_ watch for changes in new releases and update their packages to accomodate changes made upstream. > It's common to split the debug symbols into separate ELF files that are > shipped in a different package (qemu-debuginfo or similar). We was thinking about shipping these in debian (currently we don't build with debug info enabled), but it turned out to be rather problematic due to amount of binaries and size of the symbols. I still consider enabling debug info for at least x86 system targets (as most widely used). Either way, in debian we strip executables outside of upstream build system usually. > If you make this change and packagers are unaware, they could > accidentally ship qemu packages that contain the full debug symbols in > the binaries. And it will be their problem entirely, especially if they wont notice the size difference :) No, really, this is not something an upstream should think too much about. Thanks, /mjt
Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API
On Wed, 24 Sep 2014 11:48:09 +
Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov
> ---
> hw/s390x/virtio-ccw.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
Well, I think I now see what's going on here. More below...
> @@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
> static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> +HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> dc->props = virtio_ccw_properties;
> dc->init = virtio_ccw_busdev_init;
> dc->exit = virtio_ccw_busdev_exit;
> -dc->unplug = virtio_ccw_busdev_unplug;
Before, this callback was invoked when a device of the virtio-ccw class
was unplugged.
> dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> -
> +hc->unplug = virtio_ccw_busdev_unplug;
Now, this callback is supposed to be invoked instead. However, the
unplugging code invokes the callback for the _parent bus_, which
means...
> }
>
> static const TypeInfo virtio_ccw_device_info = {
> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> {
> SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> k->init = virtual_css_bridge_init;
> +hc->unplug = qdev_simple_device_unplug_cb;
...we're invoking this one, as the parent bus for virtio-ccw devices is
the virtual-css bus.
If I change this callback to the virtio-ccw one, everything works as
expected.
> }
So, to summarize, what happened before was
bridge device <--- (simple unplug invoked for dev)
-> virtual bus
-> virtio proxy device <--- virtio unplug invoked for dev
-> virtio bus
-> virtio device
which your patch changed to
bridge device
-> virtual bus<--- simple unplug invoked for virtio proxy dev
-> virtio proxy device
-> virtio bus <--- (virtio unplug invoked for virtio dev)
-> virtio device
Am I understanding this correctly?
Re: [Qemu-devel] [PATCH v3 07/23] block: Eliminate bdrv_iterate(), use bdrv_next()
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > Signed-off-by: Markus Armbruster > Reviewed-by: Benoît Canet Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [PATCH v3 08/23] block: Eliminate BlockDriverState member device_name[]
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > device_name[] can become non-empty only in bdrv_new_root() and > bdrv_move_feature_fields(). The latter is used only to undo damage > done by bdrv_swap(). The former is called only by blk_new_with_bs(). > Therefore, when a BlockDriverState's device_name[] is non-empty, then > it's been created with a BlockBackend, and vice versa. Furthermore, > blk_new_with_bs() keeps the two names equal. > > Therefore, device_name[] is redundant. Eliminate it. > > Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
On 09/25/2014 08:20 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
>> Right. Cool. So is below what was suggested? I am doublechecking as it does
>> not solve the original issue - the bottomhalf is called first and then
>> nbd_trip() crashes in qcow2_co_flush_to_os().
>>
>> diff --git a/block.c b/block.c
>> index d06dd51..1e6dfd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
>> Error **errp)
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>>
>> ret = refresh_total_sectors(bs, bs->total_sectors);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret, "Could not refresh total sector
>> count");
>> return;
>> }
>> +
>> +bdrv_drain_all();
>> }
>
> Try moving the bdrv_drain_all() call to the top of the function (at
> least it must be called before bs->drv->bdrv_invalidate_cache).
Ok, I did. Did not help.
>
>> +static QEMUBH *migration_complete_bh;
>> +static void process_incoming_migration_complete(void *opaque);
>> +
>> static void process_incoming_migration_co(void *opaque)
>> {
>> QEMUFile *f = opaque;
>> -Error *local_err = NULL;
>> int ret;
>>
>> ret = qemu_loadvm_state(f);
>> qemu_fclose(f);
>
> Paolo suggested to move eveything starting from here, but as far as I
> can tell, leaving the next few lines here shouldn't hurt.
Ouch. I was looking at wrong qcow2_fclose() all this time :)
Aaaany what you suggested did not help -
bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
executed and the situation is still the same.
>
>> free_xbzrle_decoded_buf();
>> if (ret < 0) {
>> error_report("load of migration failed: %s", strerror(-ret));
>> exit(EXIT_FAILURE);
>> }
>> qemu_announce_self();
>>
>> bdrv_clear_incoming_migration_all();
>> +
>> +migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
>> + process_incoming_migration_complete,
>> + NULL);
>> +qemu_bh_schedule(migration_complete_bh);
>> +}
>> +
>> +static void process_incoming_migration_complete(void *opaque)
>> +{
>> +Error *local_err = NULL;
>> +
>> /* Make sure all file formats flush their mutable metadata */
>> bdrv_invalidate_cache_all(&local_err);
>> if (local_err) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> exit(EXIT_FAILURE);
>> }
>>
>> if (autostart) {
>> vm_start();
>> } else {
>> runstate_set(RUN_STATE_PAUSED);
>> }
>> +qemu_bh_delete(migration_complete_bh);
>> +migration_complete_bh = NULL;
>> }
>
> That part looks good to me. I hope moving bdrv_drain_all() does the
> trick, otherwise there's somthing wrong with our reasoning.
>
> Kevin
>
--
Alexey
Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to error_report
On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> From: Gonglei
>
> Signed-off-by: Gonglei
> ---
> os-posix.c | 34 ++
> os-win32.c | 3 ++-
> 2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index cb2a7f7..9d5ae70 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -39,6 +39,7 @@
> #include "sysemu/sysemu.h"
> #include "net/slirp.h"
> #include "qemu-options.h"
> +#include "qemu/error-report.h"
>
> #ifdef CONFIG_LINUX
> #include
> @@ -120,11 +121,11 @@ void os_set_proc_name(const char *s)
> /* Could rewrite argv[0] too, but that's a bit more complicated.
> This simple way is enough for `top'. */
> if (prctl(PR_SET_NAME, name)) {
> -perror("unable to change process name");
> +error_report("unable to change process name");
This loses the value of errno that perror would have displayed. Is that
reduction in error message quality intentional? If not, then this is
not a trivial conversion; if it is, then your commit message should call
it out.
> @@ -167,20 +168,20 @@ static void change_process_uid(void)
> {
> if (user_pwd) {
> if (setgid(user_pwd->pw_gid) < 0) {
> -fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> +error_report("Failed to setgid(%d)\n", user_pwd->pw_gid);
No trailing \n for error_report, please. (You got it right in most of
your conversions)
> @@ -190,11 +191,11 @@ static void change_root(void)
> {
> if (chroot_dir) {
> if (chroot(chroot_dir) < 0) {
> -fprintf(stderr, "chroot failed\n");
> +error_report("chroot failed");
> exit(1);
> }
> if (chdir("/")) {
> -perror("not able to chdir to /");
> +error_report("not able to chdir to /");
Another loss of errno value from perror.
> exit(1);
> }
> }
> @@ -224,7 +225,7 @@ void os_daemonize(void)
> if (len != 1)
> exit(1);
> else if (status == 1) {
> -fprintf(stderr, "Could not acquire pidfile: %s\n",
> strerror(errno));
> +error_report("Could not acquire pidfile: %s",
> strerror(errno));
This code is broken. The earlier 'if (len != 1)' fails to print a
message before exiting (not to mention it violates coding style by
omitting {}). Then, if we get inside the 'else if (status == 1)'
conditional, then we KNOW that read() succeeded, and therefore errno is
unspecified. Printing strerror(errno) on a random value is NOT helpful.
> @@ -267,7 +268,7 @@ void os_setup_post(void)
> exit(1);
>
> if (chdir("/")) {
> -perror("not able to chdir to /");
> +error_report("not able to chdir to /");
Another loss of errno reporting.
> exit(1);
> }
> TFR(fd = qemu_open("/dev/null", O_RDWR));
> @@ -292,10 +293,11 @@ void os_pidfile_error(void)
> if (daemonize) {
> uint8_t status = 1;
> if (write(fds[1], &status, 1) != 1) {
> -perror("daemonize. Writing to pipe\n");
> +error_report("daemonize. Writing to pipe");
and another.
> @@ -338,7 +340,7 @@ int os_mlock(void)
>
> ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> if (ret < 0) {
> -perror("mlockall");
> +error_report("mlockall");
and another.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report
On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> From: Gonglei
>
> Signed-off-by: Gonglei
> ---
> util/osdep.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index b2bd154..7f6e483 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -401,9 +401,9 @@ void fips_set_state(bool requested)
> #endif /* __linux__ */
>
> #ifdef _FIPS_DEBUG
> -fprintf(stderr, "FIPS mode %s (requested %s)\n",
> - (fips_enabled ? "enabled" : "disabled"),
> - (requested ? "enabled" : "disabled"));
> +error_report("FIPS mode %s (requested %s)",
> + (fips_enabled ? "enabled" : "disabled"),
> + (requested ? "enabled" : "disabled"));
Do we really want debugging messages going through error_report()? This
may be one hunk we don't want.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report
On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> From: Gonglei
>
> Signed-off-by: Gonglei
> ---
> util/oslib-posix.c | 8
> util/oslib-win32.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> @@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>
> ret = sigaction(SIGBUS, &act, &oldact);
> if (ret) {
> -perror("os_mem_prealloc: failed to install signal handler");
> +error_report("os_mem_prealloc: failed to install signal handler");
> exit(1);
> }
>
> @@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
> pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
>
> if (sigsetjmp(sigjump, 1)) {
> -fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
> +error_report("os_mem_prealloc: failed to preallocate pages");
Another reduction in error message quality. You'll definitely need to
respin this series, and at this point, I don't know if you can call it
trivial.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] scripts/tracetool: don't barf on formats with precision
On Thu, Sep 25, 2014 at 10:40:14AM +0100, Alex Bennée wrote: > This only affects lttng user space tracing at the moment. > > Signed-off-by: Alex Bennée > --- > scripts/tracetool/__init__.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my tracing tree: https://github.com/stefanha/qemu/commits/tracing Stefan pgpk1qVP0Nvit.pgp Description: PGP signature
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben:
> On 09/25/2014 08:20 PM, Kevin Wolf wrote:
> > Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
> >> Right. Cool. So is below what was suggested? I am doublechecking as it does
> >> not solve the original issue - the bottomhalf is called first and then
> >> nbd_trip() crashes in qcow2_co_flush_to_os().
> >>
> >> diff --git a/block.c b/block.c
> >> index d06dd51..1e6dfd1 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
> >> Error **errp)
> >> if (local_err) {
> >> error_propagate(errp, local_err);
> >> return;
> >> }
> >>
> >> ret = refresh_total_sectors(bs, bs->total_sectors);
> >> if (ret < 0) {
> >> error_setg_errno(errp, -ret, "Could not refresh total sector
> >> count");
> >> return;
> >> }
> >> +
> >> +bdrv_drain_all();
> >> }
> >
> > Try moving the bdrv_drain_all() call to the top of the function (at
> > least it must be called before bs->drv->bdrv_invalidate_cache).
>
>
> Ok, I did. Did not help.
>
>
> >
> >> +static QEMUBH *migration_complete_bh;
> >> +static void process_incoming_migration_complete(void *opaque);
> >> +
> >> static void process_incoming_migration_co(void *opaque)
> >> {
> >> QEMUFile *f = opaque;
> >> -Error *local_err = NULL;
> >> int ret;
> >>
> >> ret = qemu_loadvm_state(f);
> >> qemu_fclose(f);
> >
> > Paolo suggested to move eveything starting from here, but as far as I
> > can tell, leaving the next few lines here shouldn't hurt.
>
>
> Ouch. I was looking at wrong qcow2_fclose() all this time :)
> Aaaany what you suggested did not help -
> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
> executed and the situation is still the same.
Hm, do you have a backtrace? The idea with the BH was that it would be
executed _outside_ coroutine context and therefore wouldn't be able to
yield. If it's still executed in coroutine context, it would be
interesting to see who that caller is.
Kevin
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
On 09/24/2014 11:23 PM, Tony Breeds wrote: > The command > qemu-img convert -O raw inputimage.qcow2 outputimage.raw > > intermittently creates corrupted output images, when the input image is not yet fully synchronized to disk. This patch preferese the use of seek_hole checks to determine if the sector is present in the disk image. s/preferese/prefers/ Please wrap your commit messages to fit in 70 or so columns (so that git log, which indents messages, is still legible in an 80-column window). > > While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC. > > Reported-By: Michael Steffens > CC: Pádraig Brady > Signed-off-by: Tony Breeds > --- > block/raw-posix.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > Adding Kevin and Stefan in cc (per scripts/get_maintainer.pl). Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Patch v2 1/8] stm32f205_timer: Add the stm32f205 Timer
On Mon, Sep 22, 2014 at 9:40 PM, Peter Crosthwaite
wrote:
> On Fri, Sep 19, 2014 at 2:54 PM, Alistair Francis
> wrote:
>> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
>> to QEMU.
>>
>> Signed-off-by: Alistair Francis
>> ---
>> PUBLIC
>> V2:
>> - Reorder the Makefile config
>> - Fix up the debug printing
>> - Correct the timer event trigger
>> Changes from RFC:
>> - Small changes to functionality and style. Thanks to Peter C
>> - Rename to make the timer more generic
>> - Split the config settings to device level
>>
>> default-configs/arm-softmmu.mak| 1 +
>> hw/timer/Makefile.objs | 2 +
>> hw/timer/stm32f205_timer.c | 279
>> +
>> include/hw/timer/stm32f205_timer.h | 101 ++
>> 4 files changed, 383 insertions(+)
>> create mode 100644 hw/timer/stm32f205_timer.c
>> create mode 100644 include/hw/timer/stm32f205_timer.h
>>
>> diff --git a/default-configs/arm-softmmu.mak
>> b/default-configs/arm-softmmu.mak
>> index f3513fa..cf23b24 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>> CONFIG_REALVIEW=y
>> CONFIG_ZAURUS=y
>> CONFIG_ZYNQ=y
>> +CONFIG_STM32F205_TIMER=y
>>
>> CONFIG_VERSATILE_PCI=y
>> CONFIG_VERSATILE_I2C=y
>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>> index 2c86c3d..4bd9617 100644
>> --- a/hw/timer/Makefile.objs
>> +++ b/hw/timer/Makefile.objs
>> @@ -31,3 +31,5 @@ obj-$(CONFIG_DIGIC) += digic-timer.o
>> obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>>
>> obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
>> +
>> +common-obj-$(CONFIG_STM32F205_TIMER) += stm32f205_timer.o
>> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
>> new file mode 100644
>> index 000..37f2318
>> --- /dev/null
>> +++ b/hw/timer/stm32f205_timer.c
>> @@ -0,0 +1,279 @@
>> +/*
>> + * STM32F205 Timer
>> + *
>> + * Copyright (c) 2014 Alistair Francis
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/timer/stm32f205_timer.h"
>> +
>> +#ifndef STM_TIMER_ERR_DEBUG
>> +#define STM_TIMER_ERR_DEBUG 0
>> +#endif
>> +
>> +#define DB_PRINT_L(lvl, fmt, args...) do { \
>> +if (STM_TIMER_ERR_DEBUG >= lvl) { \
>> +qemu_log("stm32f205_timer: %s:" fmt, __func__, ## args); \
>> +} \
>> +} while (0);
>> +
>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>> +
>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s);
>> +
>> +static void stm32f205_timer_interrupt(void *opaque)
>> +{
>> +STM32f205TimerState *s = opaque;
>> +
>> +DB_PRINT("Interrupt\n");
>> +
>> +if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
>> +s->tim_sr |= 1;
>> +qemu_irq_pulse(s->irq);
>> +stm32f205_timer_set_alarm(s);
>> +}
>> +}
>> +
>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s)
>> +{
>> +uint32_t ticks;
>> +int64_t now;
>> +
>> +DB_PRINT("Alarm raised at: 0x%x\n", s->tim_cr1);
>
> "alarm set at". The callback would be the alarm raising.
>
>> +
>> +now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +ticks = s->tim_arr - (s->tick_offset + (now * get_ticks_per_sec())) *
>> +(s->tim_psc + 1);
>
> Still doesn't add up just yet. You have:
>
> ticks = s->tim_arr -
>
> Implying ticks and s->tim_arr must be the same physical quantity which
> must be timer ticks. However ...
>
>> +
>> +DB_PRINT("Alarm set in %d ticks\n", ticks);
>> +
>> +if (ticks == 0) {
>> +timer_del(s->timer);
>> +stm32f205_timer_interrupt(s);
>> +} else {
>> +timer_mod(s->timer, (now + (int64_t) ticks));
>
> ... you add ticks to now with is terms of ns, effectively adding
> cycles and ns together. A frequency scaling factor is missing
> somewhere.
>
Yep, will fix
>>
Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to error_report
> Subject: Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror
> to
> error_report
>
> On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> > From: Gonglei
> >
> > Signed-off-by: Gonglei
> > ---
> > os-posix.c | 34 ++
> > os-win32.c | 3 ++-
> > 2 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/os-posix.c b/os-posix.c
> > index cb2a7f7..9d5ae70 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -39,6 +39,7 @@
> > #include "sysemu/sysemu.h"
> > #include "net/slirp.h"
> > #include "qemu-options.h"
> > +#include "qemu/error-report.h"
> >
> > #ifdef CONFIG_LINUX
> > #include
> > @@ -120,11 +121,11 @@ void os_set_proc_name(const char *s)
> > /* Could rewrite argv[0] too, but that's a bit more complicated.
> > This simple way is enough for `top'. */
> > if (prctl(PR_SET_NAME, name)) {
> > -perror("unable to change process name");
> > +error_report("unable to change process name");
>
> This loses the value of errno that perror would have displayed. Is that
> reduction in error message quality intentional?
Of course not. :)
> If not, then this is
> not a trivial conversion; if it is, then your commit message should call
> it out.
>
I agree with you. Actually I missed the usage of perror(), thanks for
your point. Maybe I should remove the changes about perror() in
next version.
> > @@ -167,20 +168,20 @@ static void change_process_uid(void)
> > {
> > if (user_pwd) {
> > if (setgid(user_pwd->pw_gid) < 0) {
> > -fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> > +error_report("Failed to setgid(%d)\n", user_pwd->pw_gid);
>
> No trailing \n for error_report, please. (You got it right in most of
> your conversions)
>
Hmm, yes.
>
> > @@ -190,11 +191,11 @@ static void change_root(void)
> > {
> > if (chroot_dir) {
> > if (chroot(chroot_dir) < 0) {
> > -fprintf(stderr, "chroot failed\n");
> > +error_report("chroot failed");
> > exit(1);
> > }
> > if (chdir("/")) {
> > -perror("not able to chdir to /");
> > +error_report("not able to chdir to /");
>
> Another loss of errno value from perror.
>
> > exit(1);
> > }
> > }
> > @@ -224,7 +225,7 @@ void os_daemonize(void)
> > if (len != 1)
> > exit(1);
> > else if (status == 1) {
> > -fprintf(stderr, "Could not acquire pidfile: %s\n",
> strerror(errno));
> > +error_report("Could not acquire pidfile: %s",
> strerror(errno));
>
> This code is broken. The earlier 'if (len != 1)' fails to print a
> message before exiting (not to mention it violates coding style by
> omitting {}). Then, if we get inside the 'else if (status == 1)'
> conditional, then we KNOW that read() succeeded, and therefore errno is
> unspecified. Printing strerror(errno) on a random value is NOT helpful.
>
Good catch! Will fix those. :)Thanks for your reviewing! Eric.
Best regards,
-Gonglei
>
> > @@ -267,7 +268,7 @@ void os_setup_post(void)
> > exit(1);
> >
> > if (chdir("/")) {
> > -perror("not able to chdir to /");
> > +error_report("not able to chdir to /");
>
> Another loss of errno reporting.
>
> > exit(1);
> > }
> > TFR(fd = qemu_open("/dev/null", O_RDWR));
> > @@ -292,10 +293,11 @@ void os_pidfile_error(void)
> > if (daemonize) {
> > uint8_t status = 1;
> > if (write(fds[1], &status, 1) != 1) {
> > -perror("daemonize. Writing to pipe\n");
> > +error_report("daemonize. Writing to pipe");
>
> and another.
>
> > @@ -338,7 +340,7 @@ int os_mlock(void)
> >
> > ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> > if (ret < 0) {
> > -perror("mlockall");
> > +error_report("mlockall");
>
> and another.
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
[Qemu-devel] [PULL] Update OpenBIOS images
Hi Peter, The following commit updates the OpenBIOS images to SVN r1320 (with corresponding changes to the TCX FCode ROM in order to enable guests to use the hardware TCX acceleration if required). Please pull. ATB, Mark. The following changes since commit 4f2280b2190e39aa6761cc8188626ed9aad350c1: Merge remote-tracking branch 'remotes/mcayland/tags/qemu-sparc-signed' into staging (2014-09-24 13:45:13 +0100) are available in the git repository at: https://github.com/mcayland/qemu.git tags/qemu-openbios-signed for you to fetch changes up to 1862ed02dfe69d8972641b35495669cc5d4d97c2: Update OpenBIOS images (2014-09-25 13:34:03 +0100) Update OpenBIOS images Mark Cave-Ayland (1): Update OpenBIOS images pc-bios/QEMU,tcx.bin | Bin 1410 -> 1411 bytes pc-bios/openbios-ppc | Bin 746588 -> 746588 bytes pc-bios/openbios-sparc32 | Bin 381512 -> 381512 bytes pc-bios/openbios-sparc64 | Bin 1616768 -> 1616768 bytes roms/openbios|2 +- 5 files changed, 1 insertion(+), 1 deletion(-)
Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report
> Subject: Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report
>
> On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> > From: Gonglei
> >
> > Signed-off-by: Gonglei
> > ---
> > util/osdep.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index b2bd154..7f6e483 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -401,9 +401,9 @@ void fips_set_state(bool requested)
> > #endif /* __linux__ */
> >
> > #ifdef _FIPS_DEBUG
> > -fprintf(stderr, "FIPS mode %s (requested %s)\n",
> > - (fips_enabled ? "enabled" : "disabled"),
> > - (requested ? "enabled" : "disabled"));
> > +error_report("FIPS mode %s (requested %s)",
> > + (fips_enabled ? "enabled" : "disabled"),
> > + (requested ? "enabled" : "disabled"));
>
> Do we really want debugging messages going through error_report()? This
> may be one hunk we don't want.
>
Yep, agree.
Best regards,
-Gonglei
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report
> Subject: Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert
> fprintf/perror
> to error_report
>
> On 09/25/2014 03:46 AM, arei.gong...@huawei.com wrote:
> > From: Gonglei
> >
> > Signed-off-by: Gonglei
> > ---
> > util/oslib-posix.c | 8
> > util/oslib-win32.c | 2 +-
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
>
> > @@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t
> memory)
> >
> > ret = sigaction(SIGBUS, &act, &oldact);
> > if (ret) {
> > -perror("os_mem_prealloc: failed to install signal handler");
> > +error_report("os_mem_prealloc: failed to install signal handler");
> > exit(1);
> > }
> >
> > @@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t
> memory)
> > pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
> >
> > if (sigsetjmp(sigjump, 1)) {
> > -fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
> > +error_report("os_mem_prealloc: failed to preallocate pages");
>
> Another reduction in error message quality. You'll definitely need to
> respin this series, and at this point, I don't know if you can call it
> trivial.
>
OK.
Best regards,
-Gonglei
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v3 09/23] block: Merge BlockBackend and BlockDriverState name spaces
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > BlockBackend's name space is separate only to keep the initial patches > simple. Time to merge the two. > > Signed-off-by: Markus Armbruster > Reviewed-by: Benoît Canet Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API
Il 25/09/2014 12:55, Igor Mammedov ha scritto: > hotplug-handler.[plug|unplug] callbacks are class wide, so if > USB bus might ever need to have different callbacks depending on > host it might not work. > > But since so far it uses the only qdev_simple_device_unplug_cb(), > having BUS as hotplug-handler should work too. Yeah, in effect the USB bus is using attach/detach as the controller-specific part of the hotplug handler. Perhaps the same can work for SCSI as well? I'm waiting for v2. :) Paolo
Re: [Qemu-devel] [PATCH v3 10/23] block: Eliminate DriveInfo member bdrv, use blk_by_legacy_dinfo()
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > The patch is big, but all it really does is replacing > > dinfo->bdrv > > by > > blk_bs(blk_by_legacy_dinfo(dinfo)) > > The replacement is repetitive, but the conversion of device models to > BlockBackend is imminent, and will shorten it to just > blk_legacy_dinfo(dinfo). > > Line wrapping muddies the waters a bit. I also omit tests whether > dinfo->bdrv is null, because it never is. > > Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API
On Thu, 25 Sep 2014 13:08:38 +0200
Cornelia Huck wrote:
> On Wed, 24 Sep 2014 11:48:09 +
> Igor Mammedov wrote:
>
> > Signed-off-by: Igor Mammedov
> > ---
> > hw/s390x/virtio-ccw.c | 24
> > 1 file changed, 16 insertions(+), 8 deletions(-)
>
> Well, I think I now see what's going on here. More below...
>
>
> > @@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
> > static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > +HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >
> > dc->props = virtio_ccw_properties;
> > dc->init = virtio_ccw_busdev_init;
> > dc->exit = virtio_ccw_busdev_exit;
> > -dc->unplug = virtio_ccw_busdev_unplug;
>
> Before, this callback was invoked when a device of the virtio-ccw class
> was unplugged.
>
> > dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> > -
> > +hc->unplug = virtio_ccw_busdev_unplug;
>
> Now, this callback is supposed to be invoked instead. However, the
> unplugging code invokes the callback for the _parent bus_, which
> means...
>
> > }
> >
> > static const TypeInfo virtio_ccw_device_info = {
>
> > static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> > {
> > SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> > +HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >
> > k->init = virtual_css_bridge_init;
> > +hc->unplug = qdev_simple_device_unplug_cb;
>
> ...we're invoking this one, as the parent bus for virtio-ccw devices is
> the virtual-css bus.
>
> If I change this callback to the virtio-ccw one, everything works as
> expected.
>
> > }
>
> So, to summarize, what happened before was
>
> bridge device <--- (simple unplug invoked for dev)
simple unplug should not exits for above device
> -> virtual bus
> -> virtio proxy device <--- virtio unplug invoked for dev
>-> virtio bus
> -> virtio device
>
> which your patch changed to
>
> bridge device
> -> virtual bus<--- simple unplug invoked for virtio proxy dev
> -> virtio proxy device
>-> virtio bus <--- (virtio unplug invoked for virtio dev)
> -> virtio device
>
> Am I understanding this correctly?
Let's try other way around:
bridge device (virtual_css_bridge) - non hotpluggable sysbus device
-> virtual bus (VIRTUAL_CSS_BUS)
-> virtio proxy device |
-> virtio bus |- virtio_ccw_device_foo composite device managed with
-> virtio device | device_add/del command
internal "virtio device" is the only child of "virtio bus" and it's not supposed
to be managed via device_add/del.
So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug
stored in "bridge device" with "virtual bus" using bridge as hotplug-handler.
does following patch work for you?
---
From 8f249aa4686f0a7dfa5d9636d1eee68f1d264316 Mon Sep 17 00:00:00 2001
From: Igor Mammedov
Date: Fri, 19 Sep 2014 09:07:10 +
Subject: [PATCH] s390x: convert virtio-ccw to hotplug handler API
Signed-off-by: Igor Mammedov
---
hw/s390x/virtio-ccw.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 33a1d86..036bd20 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -230,7 +230,7 @@ VirtualCssBus *virtual_css_bus_init(void)
cbus = VIRTUAL_CSS_BUS(bus);
/* Enable hotplugging */
-bus->allow_hotplug = 1;
+qbus_set_hotplug_handler(bus, dev, &error_abort);
return cbus;
}
@@ -1590,7 +1590,8 @@ static int virtio_ccw_busdev_exit(DeviceState *dev)
return _info->exit(_dev);
}
-static int virtio_ccw_busdev_unplug(DeviceState *dev)
+static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
{
VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
SubchDev *sch = _dev->sch;
@@ -1609,7 +1610,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
object_unparent(OBJECT(dev));
-return 0;
}
static Property virtio_ccw_properties[] = {
@@ -1624,9 +1624,7 @@ static void virtio_ccw_device_class_init(ObjectClass
*klass, void *data)
dc->props = virtio_ccw_properties;
dc->init = virtio_ccw_busdev_init;
dc->exit = virtio_ccw_busdev_exit;
-dc->unplug = virtio_ccw_busdev_unplug;
dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
-
}
static const TypeInfo virtio_ccw_device_info = {
@@ -1636,6 +1634,10 @@ static const TypeInfo virtio_ccw_device_info = {
.class_init = virtio_ccw_device_class_init,
.class_size = sizeof(VirtIOCCWDeviceClass),
.abstract = true,
+.interfaces = (InterfaceInfo[]) {
+{ TYPE_HOTPLUG_HANDLER },
+{ }
+}
};
/* Virtual-css Bus Bridge Device /
@@ -1650,
Re: [Qemu-devel] [PATCH v3 11/23] block: Rename BlockDriverAIOCB* to BlockAIOCB*
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > I'll use BlockDriverAIOCB with block backends shortly, and the name is > going to fit badly there. It's a block layer thing anyway, not just a > block driver thing. > > Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf
[Qemu-devel] [PATCH v4 2/7] sysbus: Make devices spawnable via -device
Now that we can properly map sysbus devices that haven't been connected to
something forcefully by C code, we can allow the -device command line option
to spawn them.
For machines that don't implement dynamic sysbus assignment in their board
files we add a new bool "has_dynamic_sysbus" to the machine class.
When that property is false (default), we bail out when we see dynamically
spawned sysbus devices, like we did before.
Signed-off-by: Alexander Graf
---
v1 -> v2:
- use bool in MachineClass rather than property
v2 -> v3:
- use search helper
v3 -> v4:
- reword error message
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 7f3418c..19d3e3a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -12,6 +12,9 @@
#include "hw/boards.h"
#include "qapi/visitor.h"
+#include "hw/sysbus.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
static char *machine_get_accel(Object *obj, Error **errp)
{
@@ -257,8 +260,35 @@ static void machine_set_iommu(Object *obj, bool value,
Error **errp)
ms->iommu = value;
}
+static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque)
+{
+error_report("Option '-device %s' cannot be handled by this machine",
+ object_class_get_name(object_get_class(OBJECT(sbdev;
+exit(1);
+}
+
+static void machine_init_notify(Notifier *notifier, void *data)
+{
+Object *machine = qdev_get_machine();
+ObjectClass *oc = object_get_class(machine);
+MachineClass *mc = MACHINE_CLASS(oc);
+
+if (mc->has_dynamic_sysbus) {
+/* Our machine can handle dynamic sysbus devices, we're all good */
+return;
+}
+
+/*
+ * Loop through all dynamically created devices and check whether there
+ * are sysbus devices among them. If there are, error out.
+ */
+foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
+}
+
static void machine_initfn(Object *obj)
{
+MachineState *ms = MACHINE(obj);
+
object_property_add_str(obj, "accel",
machine_get_accel, machine_set_accel, NULL);
object_property_add_bool(obj, "kernel-irqchip",
@@ -303,6 +333,10 @@ static void machine_initfn(Object *obj)
object_property_add_bool(obj, "iommu",
machine_get_iommu,
machine_set_iommu, NULL);
+
+/* Register notifier when init is done for sysbus sanity checks */
+ms->sysbus_notifier.notify = machine_init_notify;
+qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
}
static void machine_finalize(Object *obj)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 19437e6..7bfe381 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -283,13 +283,6 @@ static void sysbus_device_class_init(ObjectClass *klass,
void *data)
DeviceClass *k = DEVICE_CLASS(klass);
k->init = sysbus_device_init;
k->bus_type = TYPE_SYSTEM_BUS;
-/*
- * device_add plugs devices into suitable bus. For "real" buses,
- * that actually connects the device. For sysbus, the connections
- * need to be made separately, and device_add can't do that. The
- * device would be left unconnected, and could not possibly work.
- */
-k->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo sysbus_device_type_info = {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index dfb6718..12e77ea 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -35,7 +35,8 @@ struct QEMUMachine {
use_sclp:1,
no_floppy:1,
no_cdrom:1,
-no_sdcard:1;
+no_sdcard:1,
+has_dynamic_sysbus:1;
int is_default;
const char *default_machine_opts;
const char *default_boot_order;
@@ -93,7 +94,8 @@ struct MachineClass {
use_sclp:1,
no_floppy:1,
no_cdrom:1,
-no_sdcard:1;
+no_sdcard:1,
+has_dynamic_sysbus:1;
int is_default;
const char *default_machine_opts;
const char *default_boot_order;
@@ -110,6 +112,8 @@ struct MachineClass {
struct MachineState {
/*< private >*/
Object parent_obj;
+Notifier sysbus_notifier;
+
/*< public >*/
char *accel;
diff --git a/vl.c b/vl.c
index dbdca59..0540db4 100644
--- a/vl.c
+++ b/vl.c
@@ -1591,6 +1591,7 @@ static void machine_class_init(ObjectClass *oc, void
*data)
mc->no_floppy = qm->no_floppy;
mc->no_cdrom = qm->no_cdrom;
mc->no_sdcard = qm->no_sdcard;
+mc->has_dynamic_sysbus = qm->has_dynamic_sysbus;
mc->is_default = qm->is_default;
mc->default_machine_opts = qm->default_machine_opts;
mc->default_boot_order = qm->default_boot_order;
Re: [Qemu-devel] [PATCH v3 12/23] virtio-blk: Drop redundant VirtIOBlock member conf
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > Commit 12c5674 turned it into a pointer to member blk.conf. > > Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API
On Thu, 25 Sep 2014 14:47:31 +0200 Paolo Bonzini wrote: > Il 25/09/2014 12:55, Igor Mammedov ha scritto: > > hotplug-handler.[plug|unplug] callbacks are class wide, so if > > USB bus might ever need to have different callbacks depending on > > host it might not work. > > > > But since so far it uses the only qdev_simple_device_unplug_cb(), > > having BUS as hotplug-handler should work too. > > Yeah, in effect the USB bus is using attach/detach as the > controller-specific part of the hotplug handler. > > Perhaps the same can work for SCSI as well? I'm waiting for v2. :) Hopefully it should be ready by the end of this day. > > Paolo
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
On 09/25/2014 01:30 AM, Kevin Wolf wrote: > Am 25.09.2014 um 08:21 hat Markus Armbruster geschrieben: >> Please copy Kevin & Stefan for block patches. Doing that for you. I >> also copy Max, who left his fingerprints on commit 4f11aa8. >> >> Tony Breeds writes: >> >>> The command >>> qemu-img convert -O raw inputimage.qcow2 outputimage.raw >>> >>> intermittently creates corrupted output images, when the input image is not >>> yet fully synchronized to disk. This patch preferese the use of seek_hole >>> checks to determine if the sector is present in the disk image. > > Does this fix the problem or does it just make it less likely that it > becomes apparent? > > If there is a data corruptor, we need to fix it, not just ensure that > only the less common environments are affected. > >>> While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC. > > That looks like a logically separate change, so it should probably be > a separate patch. > > Is this fix for the corruptor? The commit message doesn't make it > clear. If so and fiemap is safe now, why would we still prefer > seek_hole? My understanding, based on what coreutils learned: fiemap without FIEMAP_FLAG_SYNC is a data corrupter. fiemap _with_ FIEMAP_FLAG_SYNC is a performance killer (noticeably slower, because it forces a sync). SEEK_HOLE is a much simpler interface, and therefore the kernel can optimize it to return correct results faster than it can for fiemap, and it does not suffer from the fiemap on unsynced file data corruption. So coreutils _prefers_ seek_hole first, and when it has to use fiemap, prefers using fiemap with FIEMAP_FLAG_SYNC. I agree with splitting this into two patches; one to add the flag as a data corruption fix but acknowledging that it slows fiemap down, the other to relegate fiemap to the fallback case since seek_hole can be faster when it doesn't have to force a sync. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] qemu-iotests: Fail test if explicit test case number is unknown
Am 24.09.2014 um 05:05 hat Fam Zheng geschrieben: > When we expand a number range, we just print "$id - unknown test, > ignored", this is convenient if we want to run a range of tests. > > When we designate a test case number explicitly, we shouldn't just > ignore it if the case script doesn't exist. > > Print an error and fail the test. > > Signed-off-by: Fam Zheng Thanks, applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH v2] block: Validate node-name
Am 25.09.2014 um 09:54 hat Kevin Wolf geschrieben: > The device_name of a BlockDriverState is currently checked because it is > always used as a QemuOpts ID and qemu_opts_create() checks whether such > IDs are wellformed. > > node-name is supposed to share the same namespace, but it isn't checked > currently. This patch adds explicit checks both for device_name and > node-name so that the same rules will still apply even if QemuOpts won't > be used any more at some point. > > qemu-img used to use names with spaces in them, which isn't allowed any > more. Replace them with underscores. > > Signed-off-by: Kevin Wolf Applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH] -machine vmport=off: Allow disabling of VMWare ioport emulation
What is happening with this patch? I would like to use this code. -Don Slutz From: qemu-devel-bounces+don=cloudswitch@nongnu.org [qemu-devel-bounces+don=cloudswitch@nongnu.org] on behalf of Gerd Hoffmann [kra...@redhat.com] Sent: Tuesday, May 20, 2014 6:10 AM To: Richard W.M. Jones Cc: m...@redhat.com; qemu-devel@nongnu.org; arm...@redhat.com; Dr. David Alan Gilbert; aligu...@amazon.com; Anthony PERARD Subject: Re: [Qemu-devel] [PATCH] -machine vmport=off: Allow disabling of VMWare ioport emulation Hi, > It was disabled in this patch. The commit message is saying that > vmport cannot work in Xen, but I'm not exactly clear why. > > commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9 > Author: Anthony PERARD > Date: Tue May 3 17:06:54 2011 +0100 > > pc, Disable vmport initialisation with Xen. > > This is because there is not synchronisation of the vcpu register > between Xen and QEMU, so vmport can't work properly. Ah, ok. The backdoor has side effects (writing the port does modify vcpu registers). That is the bit which is problematic for xen. Scratch the idea then. Original patch is fine. Reviewed-by: Gerd Hoffmann cheers, Gerd
Re: [Qemu-devel] [PATCH] build: preserve debug symbols with --enable-debug-info
Il 25/09/2014 12:49, Stefan Hajnoczi ha scritto: > On Thu, Sep 25, 2014 at 10:06:35AM +0200, Olaf Hering wrote: >> During code review for xen I noticed that --enable-debug-info >> would still strip the binaries because strip_opt= defaults to >> yes. If --enable-debug-info is passed to configure it has to be >> assumed that not only the compiled binaries have debugsymbols, >> also the installed binaries should keep the symbols. The >> requirement to pass also --disable-strip looks odd. > > Perhaps package maintainers rely on installed binaries not having > debug symbols? If so, that should be taken care of by the distribution. Of course, a distribution is free to separate the debug info and ship it as a separate package; in that case, it makes sense to distribute stripped binaries. But I think discarding symbols on "make install" is in general a bad idea, especially for long-lived processes such as QEMU where you often have non-reproducible bugs. If symbols are gone, even the simplest bug becomes basically impossible to diagnose from a core dump. The GNU Makefile standards have "make install" and "make install-strip" targets. It would be nice to add "make install-strip" and at the same time flip the default from --enable-strip to --disable-strip. Paolo
Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap
On 09/25/2014 10:00 AM, Tony Breeds wrote: > On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote: > >> Does this fix the problem or does it just make it less likely that it >> becomes apparent? > > Sorry for not making this clearer in my commit message. > > I haven't been able to reproduce the corruption with the fiemap flag > change. > >> If there is a data corruptor, we need to fix it, not just ensure that >> only the less common environments are affected. > > I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the > corrupter and then, as you say, makes that code less commonly executed. > >> That looks like a logically separate change, so it should probably be >> a separate patch. > > Sure I can do that, and be more explicit about the reason in the commit > message. > >> Is this fix for the corruptor? The commit message doesn't make it >> clear. If so and fiemap is safe now, why would we still prefer >> seek_hole? syncing a file has performance side effects, so best go try the (more portable) seek hole interface. > The preference for seek_hole was a suggestion from Pádraig Brady , so > I'll let him defend that :) but as I said above I think it was about > reducing the situations where fiemap was/is called. IMHO it's a kernel bug that fiemap() needs a sync to be useful. This was being fixed up in various file systems but Dave Chiner pushed back on XFS adjustments, and thus changes were stalled. The current situation is it's best to avoid fiemap(). thanks, Pádraig.
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
On 09/25/2014 02:06 AM, Markus Armbruster wrote: >> >> The QAPI schema's 'returns' becomes "return" on the wire. We suck. >> >> qmp-spec.txt is *wrong*! We actually use json-array in addition to >> json-object. > > Actually, we use json-int and json-str as well: > query-migrate-cache-size, ringbuf-read, human-monitor-command. > >> Similar argument on types wanted as for 'data' / "arguments" above. I >> think we should permit exactly the same QAPI types, plus lists. > > The similarity to 'data' just isn't there. Separate analysis needed. Correct. 'data' and 'returns' are different beasts when it comes to acceptable types. And different still from the acceptable type of each member of a dictionary. But my check_type function in 13/19 is flexible enough to cover all the cases. > > Any QAPI types that don't make sense, other than list with length != 1? Return of an anon union isn't used yet, but _might_ make sense (as the only feasible way of changing existing commands that return an array or primitive extensible to instead return a dict) - except that back-compat demands that we can't return a dict in place of a primitive unless the arguments of the command are also enhanced (that is, older callers are not expecting a dict, so we can't return a dict unless the caller witnesses they are new enough by explicitly asking for a dict return). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation
On 09/25/2014 10:39 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben:
>> On 09/25/2014 08:20 PM, Kevin Wolf wrote:
>>> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
Right. Cool. So is below what was suggested? I am doublechecking as it does
not solve the original issue - the bottomhalf is called first and then
nbd_trip() crashes in qcow2_co_flush_to_os().
diff --git a/block.c b/block.c
index d06dd51..1e6dfd1 100644
--- a/block.c
+++ b/block.c
@@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp)
if (local_err) {
error_propagate(errp, local_err);
return;
}
ret = refresh_total_sectors(bs, bs->total_sectors);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not refresh total sector
count");
return;
}
+
+bdrv_drain_all();
}
>>>
>>> Try moving the bdrv_drain_all() call to the top of the function (at
>>> least it must be called before bs->drv->bdrv_invalidate_cache).
>>
>>
>> Ok, I did. Did not help.
>>
>>
>>>
+static QEMUBH *migration_complete_bh;
+static void process_incoming_migration_complete(void *opaque);
+
static void process_incoming_migration_co(void *opaque)
{
QEMUFile *f = opaque;
-Error *local_err = NULL;
int ret;
ret = qemu_loadvm_state(f);
qemu_fclose(f);
>>>
>>> Paolo suggested to move eveything starting from here, but as far as I
>>> can tell, leaving the next few lines here shouldn't hurt.
>>
>>
>> Ouch. I was looking at wrong qcow2_fclose() all this time :)
>> Aaaany what you suggested did not help -
>> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
>> executed and the situation is still the same.
>
> Hm, do you have a backtrace? The idea with the BH was that it would be
> executed _outside_ coroutine context and therefore wouldn't be able to
> yield. If it's still executed in coroutine context, it would be
> interesting to see who that caller is.
Like this?
process_incoming_migration_complete
bdrv_invalidate_cache_all
bdrv_drain_all
aio_dispatch
node->io_read (which is nbd_read)
nbd_trip
bdrv_co_flush
[...]
--
Alexey
Re: [Qemu-devel] [PATCH] ohci: Split long traces to smaller ones
On Do, 2014-09-25 at 19:38 +1000, Alexey Kardashevskiy wrote: > On 09/25/2014 06:09 PM, Gerd Hoffmann wrote: > > On Do, 2014-09-25 at 10:16 +1000, Alexey Kardashevskiy wrote: > >> Recent traces rework introduced 2 tracepoints with 13 and 20 > >> arguments. When dtrace backend is selected > >> (--enable-trace-backend=dtrace), compile fails as > >> sys/sdt.h defines DTRACE_PROBE up to DTRACE_PROBE12 only. > >> > >> This splits long tracepoints. > > > > Can you also change the tracing-enabled check to use '#ifndef > > CONFIG_TRACE_NOP' as suggested by Stefan please? > > Nope :( As I said in that thread, I am not familiar with "dtrace" - are > traces configurable with it? With --enable-trace-backend=dtrace, > trace_event_get_state is not defined so CONFIG_TRACE_NOP is not 100% equal > replacement. Ah, ok. IIRC with dtrace / systemtap you enable the trace point by other means than qemu monitor commands, which is probably the reason trace_event_get_state isn't there. So trying to skip the calls with tracing turned off isn't going to fly as qemu doesn't know in the first place whenever a tracepoint is enabled or not. cheers, Gerd
Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"
On Do, 2014-09-25 at 10:12 +0200, Igor Mammedov wrote: > > Basically the same camp as usb-bot. UAS doesn't suffer the LUN > > numeration issue which BOT has, but there likewise is no signaling about > > scsi devices coming and going. > Thet's why a excluded UAS from this series, > Do I need to make it hotpluggable (like SCSI HBAs without hotplug > signalling)? /i.e./ it would be possible to hotplug it but guest > won't notice the new drive on UAS until reboot/ Yes, in that regard uas is more simliar to lsi than to bot. Hotplug works, but needs manual rescan / driver reload / reboot to make the guest see the device. cheers, Gerd
Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API
On Thu, 25 Sep 2014 15:11:10 +0200 Igor Mammedov wrote: > On Thu, 25 Sep 2014 13:08:38 +0200 > Cornelia Huck wrote: > > So, to summarize, what happened before was > > > > bridge device <--- (simple unplug invoked for dev) > simple unplug should not exits for above device Yes. I'm not sure why it does. > > > -> virtual bus > > -> virtio proxy device <--- virtio unplug invoked for dev > > >-> virtio bus > > -> virtio device > > > > which your patch changed to > > > > bridge device > > -> virtual bus<--- simple unplug invoked for virtio proxy dev > > -> virtio proxy device > >-> virtio bus <--- (virtio unplug invoked for virtio dev) > > -> virtio device > > > > Am I understanding this correctly? > Let's try other way around: > > bridge device (virtual_css_bridge) - non hotpluggable sysbus device > -> virtual bus (VIRTUAL_CSS_BUS) >-> virtio proxy device | > -> virtio bus |- virtio_ccw_device_foo composite device managed > with > -> virtio device | device_add/del command > > internal "virtio device" is the only child of "virtio bus" and it's not > supposed > to be managed via device_add/del. This makes sense; I'm wondering why virtio-bus had allow_hotplug set before, though. > > So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug > stored in "bridge device" with "virtual bus" using bridge as hotplug-handler. > > does following patch work for you? It does react as expected with a simple device_add/device_del (your patches applied up to patch 19 with this one on top).
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
On 09/25/2014 01:34 AM, Markus Armbruster wrote:
> Eric Blake writes:
>
>> Demonstrate that the qapi generator silently parses confusing
>> types, which may cause other errors later on. Later patches
>> will update the expected results as the generator is made stricter.
>>
>> Signed-off-by: Eric Blake
>> ---
>> tests/Makefile | 8 ++--
>> tests/qapi-schema/data-array-empty.err | 0
>> tests/qapi-schema/data-array-empty.exit | 1 +
>> tests/qapi-schema/data-array-empty.json | 2 ++
> [Twelve new tests...]
>> create mode 100644 tests/qapi-schema/returns-unknown.err
>> create mode 100644 tests/qapi-schema/returns-unknown.exit
>> create mode 100644 tests/qapi-schema/returns-unknown.json
>> create mode 100644 tests/qapi-schema/returns-unknown.out
>
> Holy moly!
Yeah, this series cleans up a lot of cruft, which means a lot of testing.
>> +++ b/tests/qapi-schema/data-array-empty.json
>> @@ -0,0 +1,2 @@
>> +# FIXME: we should reject an array for data if it does not contain a known
>> type
>> +{ 'command': 'oops', 'data': [ ] }
>
> Do we want to permit anything but a complex type for 'data'?
Oh, good question. Probably not (I just tested, and nothing already
does that). I'll tighten it in v5 (mostly doc changes, plus a one-liner
in 13/19 to remove allow_array=True when calling check_type for a
command data member).
>
> For what it's worth, docs/qmp/qmp-spec.txt specifies:
Ooh, I probably ought to skim that file when making my doc improvements
on the front end of the series.
>
> 'data' of list type / json-array "arguments" is an ordered list of
> unnamed parameters. Makes sense, but it isn't how QMP works. Or C for
> that matter. Do we really want to support this in QAPI?
No existing command takes a non-dict for "arguments", and the generator
probably chokes on it.
>
> If yes, then 'data': [] means the same thing as 'data': {} or no 'data':
> no arguments.
>
> Aside: discussion of list types in qapi-code-gen.txt is spread over the
> places that use them. I feel giving them their own section on the same
> level as complex types etc. would make sense.
Good idea, will do in v5.
>
> 'data' of built-in or enumeration type / json-number or json-string
> "arguments" could be regarded as a single unnamed parameter. Even if we
> want unnamed parameters, the question remains whether we want two
> syntactic forms for a single unnamed parameter, boxed in a [ ] and
> unboxed. I doubt it.
No. We don't (patch 13/19 already forbids them, with no violators
found). It's not extensible (well, maybe it is by having some way to
mark a dict so that at most one of its keys is the default key to be
implied when used in a non-dict form, and all other keys being optional,
but that's ugly).
>
> One kind of types left to discuss: unions. I figure the semantics of a
> simple or flat union type are obvious enough, so we can discuss whether
> we want them. Anonymous unions are a different matter, because they
> boil down to a single parameter that need not be json-object.
Oooh, I didn't even consider anon unions. We absolutely need to support
{ 'command': 'foo', 'data': 'FlatUnion' } (blockdev-add, anyone), but
you are probably right that we don't want to support { 'command': 'foo',
'data': 'AnonUnion'}, because it would allow "arguments" to be a
non-dictionary (unless the AnonUnion has only a dict branch, but then
why make it a union?). I'll have to make qapi.py be smarter about
regular vs. anon unions - it might be easier by using an actual
different keyword for anon unions, because they are so different in
nature. (Generated code will be the same, just a tweak to the qapi
representation and to qapi.py). I'll play with that for v5.
>> +++ b/tests/qapi-schema/data-member-array-bad.json
>> @@ -0,0 +1,2 @@
>> +# FIXME: we should reject data if it does not contain a valid array type
>> +{ 'command': 'oops', 'data': { 'member': [ { 'nested': 'str' } ] } }
>
> I'm probably just suffering from temporary denseness here... why is this
> example problematic? To me, it looks like a single argument 'member' of
> type "array of complex type with a single member 'nested' of
> builtin-type 'str'".
The generator does not have a way to produce a List of an unnamed type.
All lists are of named types (or rather, every creation of a named type
generates code for both that type and its list counterpart). Maybe we
eventually want to support an array of an anonymous type, but the
generator doesn't handle it now. So it was easier to forbid it when
writing 13/19.
>> +# FIXME: we should reject an array return that is not a single type
>> +{ 'command': 'oops', 'returns': [ 'str', 'str' ] }
>
> Do we want to permit anything but a complex type for 'returns'?
Sadly, yes. We have existing commands that do just that. I already
documented that new commands should avoid it (it's not extensible).
>
> The QAPI schema's 'returns' becomes "return" on the wire. We suck.
W
Re: [Qemu-devel] [PATCH 1/8] virtio-gpu/2d: add hardware spec include file
Hi Dave, Triggered by the framebuffer endian issues we have with stdvga I've started to check where we stand with virtio-gpu and whenever we have to fix something in the virtio protocol before setting in stone with the upstream merge. Fixed the protocol. Basically s/uint32_t/__le32/g. No changes on x86_64 obviously. Fixed the kernel driver to byteswap values when needed. See https://www.kraxel.org/cgit/linux/log/?h=virtio-gpu-rebase Result is bigendian guest on little endian host boots fine (without virgl). Colors are wrong though. Other way around still needs a bunch of fixes in qemu so virtio-gpu works correctly on bigendian hosts. To be done. So, on the color issue: How are these defined exactly? > +VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM = 1, > +VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM = 2, > +VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM = 3, > +VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM = 4, Looking at the code it seems the name defines the byte ordering in memory, i.e. "B8G8R8A8" means blue first, then red, then green, then alpha. Or DRM_FORMAT_ARGB on little endian / DRM_FORMAT_BGRX on big endian. Correct? Easy fix in the guest driver then ... > +VIRTIO_GPU_FORMAT_B5G5R5A1_UNORM = 5, > +VIRTIO_GPU_FORMAT_B5G6R5_UNORM= 7, Do we really wanna go down this route? I'm inclined to just zap 16bpp support. > +VIRTIO_GPU_FORMAT_R8_UNORM= 64, What is this btw? Looks like gray or alpha, but why "R8" not "G8" or "A8" then? Why the gap in the enumeration? With the formats being clarified fixed we are settled for 2d mode I think. What about 3d mode? We are passing blobs between virglrenderer and guest driver: capabilities and gallium 3d command stream. What happens to them in case host and guest endianness don't match? I think at least the capabilities have 32bit values which need to be swapped. Dunno about the gallium commands ... cheers, Gerd
Re: [Qemu-devel] [PATCH] -machine vmport=off: Allow disabling of VMWare ioport emulation
* Slutz, Donald Christopher (dsl...@verizon.com) wrote: > What is happening with this patch? I would like to use this code. I need to rework it for the new machine types code; but it was pretty low down my list of priorities; but I can try and get a minute for it again. Dave > >-Don Slutz > > > From: qemu-devel-bounces+don=cloudswitch@nongnu.org > [qemu-devel-bounces+don=cloudswitch@nongnu.org] on behalf of Gerd > Hoffmann [kra...@redhat.com] > Sent: Tuesday, May 20, 2014 6:10 AM > To: Richard W.M. Jones > Cc: m...@redhat.com; qemu-devel@nongnu.org; arm...@redhat.com; Dr. David Alan > Gilbert; aligu...@amazon.com; Anthony PERARD > Subject: Re: [Qemu-devel] [PATCH] -machine vmport=off: Allow disabling of > VMWare ioport emulation > > Hi, > > > It was disabled in this patch. The commit message is saying that > > vmport cannot work in Xen, but I'm not exactly clear why. > > > > commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9 > > Author: Anthony PERARD > > Date: Tue May 3 17:06:54 2011 +0100 > > > > pc, Disable vmport initialisation with Xen. > > > > This is because there is not synchronisation of the vcpu register > > between Xen and QEMU, so vmport can't work properly. > > Ah, ok. The backdoor has side effects (writing the port does modify > vcpu registers). That is the bit which is problematic for xen. Scratch > the idea then. > > Original patch is fine. > > Reviewed-by: Gerd Hoffmann > > cheers, > Gerd > > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API
On Thu, 25 Sep 2014 16:32:37 +0200 Cornelia Huck wrote: > On Thu, 25 Sep 2014 15:11:10 +0200 > Igor Mammedov wrote: > > > On Thu, 25 Sep 2014 13:08:38 +0200 > > Cornelia Huck wrote: > > > > So, to summarize, what happened before was > > > > > > bridge device <--- (simple unplug invoked for dev) > > simple unplug should not exits for above device > > Yes. I'm not sure why it does. > > > > > > -> virtual bus > > > -> virtio proxy device <--- virtio unplug invoked for dev > > > > >-> virtio bus > > > -> virtio device > > > > > > which your patch changed to > > > > > > bridge device > > > -> virtual bus<--- simple unplug invoked for virtio proxy dev > > > -> virtio proxy device > > >-> virtio bus <--- (virtio unplug invoked for virtio dev) > > > -> virtio device > > > > > > Am I understanding this correctly? > > Let's try other way around: > > > > bridge device (virtual_css_bridge) - non hotpluggable sysbus device > > -> virtual bus (VIRTUAL_CSS_BUS) > >-> virtio proxy device | > > -> virtio bus |- virtio_ccw_device_foo composite device managed > > with > > -> virtio device | device_add/del command > > > > internal "virtio device" is the only child of "virtio bus" and it's not > > supposed > > to be managed via device_add/del. > > This makes sense; I'm wondering why virtio-bus had allow_hotplug set > before, though. it was due to behavior of bus_add_child() which required bus to be hotplugggable. > > > > > So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug > > stored in "bridge device" with "virtual bus" using bridge as > > hotplug-handler. > > > > does following patch work for you? > > It does react as expected with a simple device_add/device_del (your > patches applied up to patch 19 with this one on top). Thanks for confirmation, I'll amend s390-virtio in similar manner for V2
[Qemu-devel] [Bug 1373362] Re: qemu-2.1.1 i386-softmmu compile error: q35_dsdt_applesmc_sta undeclared
Works fine here. Even made sure the resulting binary runs okay (which it does). Maybe try in a clean chroot or something? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1373362 Title: qemu-2.1.1 i386-softmmu compile error: q35_dsdt_applesmc_sta undeclared Status in QEMU: New Status in “qemu” package in Gentoo Linux: New Bug description: I try to compile qemu-2.1.1 (Gentoo/x86), but the i386-softmmu fails to compile: CPP i386-softmmu/q35-acpi-dsdt.dsl.i.orig ACPI_PREPROCESS i386-softmmu/q35-acpi-dsdt.dsl.i IASL i386-softmmu/q35-acpi-dsdt.dsl.i ACPI_EXTRACT i386-softmmu/q35-acpi-dsdt.off CAT i386-softmmu/hw/i386/q35-acpi-dsdt.hex CCi386-softmmu/hw/i386/acpi-build.o /tmp/portage/app-emulation/qemu-2.1.1/work/qemu-2.1.1/hw/i386/acpi-build.c: In function 'acpi_get_dsdt': /tmp/portage/app-emulation/qemu-2.1.1/work/qemu-2.1.1/hw/i386/acpi-build.c:119:24: error: 'q35_dsdt_applesmc_sta' undeclared (first use in this function) applesmc_sta = q35_dsdt_applesmc_sta; ^ /tmp/portage/app-emulation/qemu-2.1.1/work/qemu-2.1.1/hw/i386/acpi-build.c:119:24: note: each undeclared identifier is reported only once for each function it appears in make[1]: *** [hw/i386/acpi-build.o] Error 1 make: *** [subdir-i386-softmmu] Error 2 Something seems to go wrong when generating the file i386-softmmu/hw/i386/q35-acpi-dsdt.hex: # grep -r q35_dsdt_applesmc_sta ../ ../softmmu-build/x86_64-softmmu/q35-acpi-dsdt.dsl.i:/* ACPI_EXTRACT_NAME_BYTE_CONST q35_dsdt_applesmc_sta */ ../softmmu-build/x86_64-softmmu/q35-acpi-dsdt.dsl.i.orig: ACPI_EXTRACT_NAME_BYTE_CONST q35_dsdt_applesmc_sta ../softmmu-build/i386-softmmu/q35-acpi-dsdt.dsl.i:/* ACPI_EXTRACT_NAME_BYTE_CONST q35_dsdt_applesmc_sta */ ../softmmu-build/i386-softmmu/q35-acpi-dsdt.dsl.i.orig: ACPI_EXTRACT_NAME_BYTE_CONST q35_dsdt_applesmc_sta ../hw/i386/acpi-build.c:applesmc_sta = q35_dsdt_applesmc_sta; ../hw/i386/q35-acpi-dsdt.dsl:#define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1373362/+subscriptions
[Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously
Add the termination signals SIGINT, SIGHUP and SIGTERM to the list of signals which we handle synchronously via a signalfd. This avoids a race condition where if we took the SIGTERM in the middle of qemu_shutdown_requested: int r = shutdown_requested; [SIGTERM here...] shutdown_requested = 0; then the setting of the shutdown_requested flag by termsig_handler() would be lost and QEMU would fail to shut down. This was causing 'make check' to hang occasionally. Signed-off-by: Peter Maydell Cc: qemu-sta...@nongnu.org --- main-loop.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/main-loop.c b/main-loop.c index 3cc79f8..8746abc 100644 --- a/main-loop.c +++ b/main-loop.c @@ -84,6 +84,9 @@ static int qemu_signal_init(void) sigaddset(&set, SIGIO); sigaddset(&set, SIGALRM); sigaddset(&set, SIGBUS); +sigaddset(&set, SIGINT); +sigaddset(&set, SIGHUP); +sigaddset(&set, SIGTERM); pthread_sigmask(SIG_BLOCK, &set, NULL); sigdelset(&set, SIG_IPI); -- 1.9.1
Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously
Il 25/09/2014 17:51, Peter Maydell ha scritto: > Add the termination signals SIGINT, SIGHUP and SIGTERM to the > list of signals which we handle synchronously via a signalfd. > This avoids a race condition where if we took the SIGTERM > in the middle of qemu_shutdown_requested: > int r = shutdown_requested; > [SIGTERM here...] > shutdown_requested = 0; > > then the setting of the shutdown_requested flag by > termsig_handler() would be lost and QEMU would fail to > shut down. This was causing 'make check' to hang occasionally. > > Signed-off-by: Peter Maydell > Cc: qemu-sta...@nongnu.org > --- > main-loop.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/main-loop.c b/main-loop.c > index 3cc79f8..8746abc 100644 > --- a/main-loop.c > +++ b/main-loop.c > @@ -84,6 +84,9 @@ static int qemu_signal_init(void) > sigaddset(&set, SIGIO); > sigaddset(&set, SIGALRM); > sigaddset(&set, SIGBUS); > +sigaddset(&set, SIGINT); > +sigaddset(&set, SIGHUP); > +sigaddset(&set, SIGTERM); > pthread_sigmask(SIG_BLOCK, &set, NULL); > > sigdelset(&set, SIG_IPI); > Reviewed-by: Paolo Bonzini Paolo
Re: [Qemu-devel] Microcheckpointing: Memory-VCPU / Disk State consistency
Am 24.09.2014 10:47, schrieb Stefan Hajnoczi: I think the assumption with drive-mirror is that you throw away the destination image if something fails. That's the exact opposite of MC where we want to fail over to the destination :). This was not obivous for me... Here is one example of a mechanism like this: QEMU has a block job called drive-backup which copies sectors that are about to be overwritten to an external file. Once the data has been copied into the external file, the sectors in the original image file can be overwritten safely. The Secondary runs drive-backup so that writes coming from the Primary stash rollback data into an external qcow2 file. When the Primary wishes to commit we drop the qcow2 rollback file since we no longer need the ability to roll back - this is cheap and not a lot of I/O needs to be performed for the commit operation. If the Secondary needs to take over it can use the rollback qcow2 file as its disk image and the guest will see the state of the disk at the last commit point. The sectors that were modified since commit in the original image file are covered by the data in the rollback qcow2 file. There are a bunch of details on making this efficient but in principle this approach makes both commit and rollback fairly lightweight. Until yesterday I’ve seen backup as mechanism that makes a point in time snapshot of a block device and saves the contents of that snapshot to an other block device. Your proposal is a new interpretation of backup :-) I must admit that I had to think twice to get an idea what your point is. I don’t know if I have understood all aspects of your proposal as my mental model of a possible architecture is not quite clear yet. I will try to summarize in “MC-words” what I have understood: The general Idea is to use drive-backup to get a consistent snapshot of a mirrored block device on the secondary for a given period of time I will call it epoch(n) and snapshot(n). As a starting point we need to block-devices with exact the same state on primary and secondary. In other word there must be an exact copy of the primary image on the secondary. In epoche(n) the primary mirror its writes to the image file of secondary. This leads to a continuous stream of updated blocks to the image of the secondary. In parallel the secondary use drive-backup to get a rollback-snapshot(n) its own image file for each running epoche. At the beginning of epoche(n+1) we start a (new) rollback-snapshot(n+1) and keep rollback-snapshot(n). When in normal operation we drop rollback-snapshot(n) when epoche(n+1) is successfully completed. In case of a failure in epoche(n+1) we make a fail over and use rollback-snapshot(n) to get back the consistent block device state of epoche(n) Is this your idea? Does this procedure guaranty the block-device semantics of the primary? Walid
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
Eric Blake writes:
> On 09/25/2014 01:34 AM, Markus Armbruster wrote:
>> Eric Blake writes:
>>
>>> Demonstrate that the qapi generator silently parses confusing
>>> types, which may cause other errors later on. Later patches
>>> will update the expected results as the generator is made stricter.
>>>
>>> Signed-off-by: Eric Blake
>>> ---
>>> tests/Makefile | 8 ++--
>>> tests/qapi-schema/data-array-empty.err | 0
>>> tests/qapi-schema/data-array-empty.exit | 1 +
>>> tests/qapi-schema/data-array-empty.json | 2 ++
>> [Twelve new tests...]
>>> create mode 100644 tests/qapi-schema/returns-unknown.err
>>> create mode 100644 tests/qapi-schema/returns-unknown.exit
>>> create mode 100644 tests/qapi-schema/returns-unknown.json
>>> create mode 100644 tests/qapi-schema/returns-unknown.out
>>
>> Holy moly!
>
> Yeah, this series cleans up a lot of cruft, which means a lot of testing.
Very much appreciated.
>>> +++ b/tests/qapi-schema/data-array-empty.json
>>> @@ -0,0 +1,2 @@
>>> +# FIXME: we should reject an array for data if it does not contain
>>> a known type
>>> +{ 'command': 'oops', 'data': [ ] }
>>
>> Do we want to permit anything but a complex type for 'data'?
>
> Oh, good question. Probably not (I just tested, and nothing already
> does that). I'll tighten it in v5 (mostly doc changes, plus a one-liner
> in 13/19 to remove allow_array=True when calling check_type for a
> command data member).
Yes, please.
>> For what it's worth, docs/qmp/qmp-spec.txt specifies:
>
> Ooh, I probably ought to skim that file when making my doc improvements
> on the front end of the series.
>
>> 'data' of list type / json-array "arguments" is an ordered list of
>> unnamed parameters. Makes sense, but it isn't how QMP works. Or C for
>> that matter. Do we really want to support this in QAPI?
>
> No existing command takes a non-dict for "arguments", and the generator
> probably chokes on it.
Let's stick to dict arguments.
>> If yes, then 'data': [] means the same thing as 'data': {} or no 'data':
>> no arguments.
>>
>> Aside: discussion of list types in qapi-code-gen.txt is spread over the
>> places that use them. I feel giving them their own section on the same
>> level as complex types etc. would make sense.
>
> Good idea, will do in v5.
>
>>
>> 'data' of built-in or enumeration type / json-number or json-string
>> "arguments" could be regarded as a single unnamed parameter. Even if we
>> want unnamed parameters, the question remains whether we want two
>> syntactic forms for a single unnamed parameter, boxed in a [ ] and
>> unboxed. I doubt it.
>
> No. We don't (patch 13/19 already forbids them, with no violators
> found). It's not extensible (well, maybe it is by having some way to
> mark a dict so that at most one of its keys is the default key to be
> implied when used in a non-dict form, and all other keys being optional,
> but that's ugly).
Agreed.
>> One kind of types left to discuss: unions. I figure the semantics of a
>> simple or flat union type are obvious enough, so we can discuss whether
>> we want them. Anonymous unions are a different matter, because they
>> boil down to a single parameter that need not be json-object.
>
> Oooh, I didn't even consider anon unions. We absolutely need to support
> { 'command': 'foo', 'data': 'FlatUnion' } (blockdev-add, anyone), but
> you are probably right that we don't want to support { 'command': 'foo',
> 'data': 'AnonUnion'}, because it would allow "arguments" to be a
> non-dictionary (unless the AnonUnion has only a dict branch, but then
> why make it a union?). I'll have to make qapi.py be smarter about
> regular vs. anon unions - it might be easier by using an actual
> different keyword for anon unions, because they are so different in
> nature. (Generated code will be the same, just a tweak to the qapi
> representation and to qapi.py). I'll play with that for v5.
Okay :)
>>> +++ b/tests/qapi-schema/data-member-array-bad.json
>>> @@ -0,0 +1,2 @@
>>> +# FIXME: we should reject data if it does not contain a valid array type
>>> +{ 'command': 'oops', 'data': { 'member': [ { 'nested': 'str' } ] } }
>>
>> I'm probably just suffering from temporary denseness here... why is this
>> example problematic? To me, it looks like a single argument 'member' of
>> type "array of complex type with a single member 'nested' of
>> builtin-type 'str'".
>
> The generator does not have a way to produce a List of an unnamed type.
> All lists are of named types (or rather, every creation of a named type
> generates code for both that type and its list counterpart). Maybe we
> eventually want to support an array of an anonymous type, but the
> generator doesn't handle it now. So it was easier to forbid it when
> writing 13/19.
I see. We already accepted restricting nested structs (see series
subject), and this is just one instance.
>>> +# FIXME: we should reject an array return that is not a single type
>>> +{
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
Eric Blake writes: > On 09/25/2014 02:06 AM, Markus Armbruster wrote: > >>> >>> The QAPI schema's 'returns' becomes "return" on the wire. We suck. >>> >>> qmp-spec.txt is *wrong*! We actually use json-array in addition to >>> json-object. >> >> Actually, we use json-int and json-str as well: >> query-migrate-cache-size, ringbuf-read, human-monitor-command. >> >>> Similar argument on types wanted as for 'data' / "arguments" above. I >>> think we should permit exactly the same QAPI types, plus lists. >> >> The similarity to 'data' just isn't there. Separate analysis needed. > > Correct. 'data' and 'returns' are different beasts when it comes to > acceptable types. And different still from the acceptable type of each > member of a dictionary. But my check_type function in 13/19 is flexible > enough to cover all the cases. > >> >> Any QAPI types that don't make sense, other than list with length != 1? > > Return of an anon union isn't used yet, but _might_ make sense (as the > only feasible way of changing existing commands that return an array or > primitive extensible to instead return a dict) - Good point. > except that back-compat > demands that we can't return a dict in place of a primitive unless the > arguments of the command are also enhanced (that is, older callers are > not expecting a dict, so we can't return a dict unless the caller > witnesses they are new enough by explicitly asking for a dict return). I think we can keep things simple for now and reject anonymous unions. We can always relax the check when we run into a use. You're giving the generator a good shove from "god knows what it accepts, but as long as you stick to stuff that is being used already, probably generates something that works" towards "if it accepts it, it works". I like it.
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
On 09/25/2014 10:12 AM, Markus Armbruster wrote: >>> Do we want to permit anything but a complex type for 'returns'? >> >> Sadly, yes. We have existing commands that do just that. I already >> documented that new commands should avoid it (it's not extensible). > > If we care, we can whitelist the existing offenders, and reject new > offenders. Also a good idea. The whitelist may grow over time, but forcing a developer to modify the whitelist calls attention to their action :) I'll add that to my v5 queue. Thanks again for a thought-provoking review. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
