Re: [Qemu-devel] qemu drive-mirror to rbd storage : no sparse rbd image
On Wed, 10/08 13:15, Alexandre DERUMIER wrote: > Hi, > > I'm currently planning to migrate our storage to ceph/rbd through qemu > drive-mirror > > and It seem that drive-mirror with rbd block driver, don't create a sparse > image. (all zeros are copied to the target rbd). > > Also note, that it's working fine with "qemu-img convert" , the rbd volume is > sparse after conversion. What is the source format? If the zero clusters are actually unallocated in the source image, drive-mirror will not write those clusters either. I.e. with "drive-mirror sync=top", both source and target should have the same "qemu-img map" output. Fam > > > Could it be related to the "bdrv_co_write_zeroes" missing features in > block/rbd.c ? > > (It's available in other block drivers (scsi,gluster,raw-aio) , and I don't > have this problem with theses block drivers). > > > > Regards, > > Alexandre Derumier > > >
[Qemu-devel] [PATCH] qcow2: fix double-free of Qcow2DiscardRegion in qcow2_process_discards
In qcow2_update_snapshot_refcount -> qcow2_process_discards() -> bdrv_discard()
may free the Qcow2DiscardRegion which is referenced by "next" pointer in
qcow2_process_discards() now, in next iteration, d = next, so g_free(d)
will double-free this Qcow2DiscardRegion.
qcow2_snapshot_delete
|- qcow2_update_snapshot_refcount
|-- qcow2_process_discards
|--- bdrv_discard
| aio_poll
|- aio_dispatch
|-- bdrv_co_io_em_complete
|--- qemu_coroutine_enter(co->coroutine, NULL); <=== coroutine entry is
bdrv_co_do_rw
|--- g_free(d) <== free first Qcow2DiscardRegion is okay
|--- d = next; <== this set is done in QTAILQ_FOREACH_SAFE() macro.
|--- g_free(d); <== double-free will happen if during previous iteration,
bdrv_discard had free this object.
bdrv_co_do_rw
|- bdrv_co_do_writev
|-- bdrv_co_do_pwritev
|--- bdrv_aligned_pwritev
| qcow2_co_writev
|- qcow2_alloc_cluster_link_l2
|-- qcow2_free_any_clusters
|--- qcow2_free_clusters
| update_refcount
|- qcow2_process_discards
|-- g_free(d) <== In next iteration, this Qcow2DiscardRegion will be
double-free.
Signed-off-by: Zhang Haoyu
Signed-off-by: Fu Xuewei
---
block/qcow2-refcount.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2bcaaf9..3b759a3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -462,9 +462,9 @@ fail_block:
void qcow2_process_discards(BlockDriverState *bs, int ret)
{
BDRVQcowState *s = bs->opaque;
-Qcow2DiscardRegion *d, *next;
+Qcow2DiscardRegion *d;
-QTAILQ_FOREACH_SAFE(d, &s->discards, next, next) {
+while ((d = QTAILQ_FIRST(&s->discards)) != NULL) {
QTAILQ_REMOVE(&s->discards, d, next);
/* Discard is optional, ignore the return value */
--
1.7.12.4
Re: [Qemu-devel] [Bug?] qemu abort when trying to passthrough BCM5719 Gigabit Ethernet
On Sat, 2014-10-11 at 13:58 +0800, zhanghailiang wrote: > Hi all, > > When i try to passthrough BCM5719 Gigabit Ethernet to guest using the qemu > master branch, it aborted, > and show kvm_set_phys_mem:error registering slot:Bad Address. > > qemu command: > #./qemu/qemu/x86_64-softmmu/qemu-system-x86_64 --enable-kvm -smp 4 -m 4096 > -vnc :99 -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -drive > file=/home/suse11_sp3_64,if=none,id=drive-scsi0-0-0-0,format=raw,cache=none,aio=native > -device scsi-hd,bus=scsi0.0,channel=0,scsi- > id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 -device > pci-assign,host=01:00.1,id=mydevice -net none > > info about guest and host: > host OS: 3.16.5 > *guest OS: Novell SuSE Linux Enterprise Server 11 SP3* > #cat /proc/cpuinfo > processor : 31 > vendor_id : GenuineIntel > cpu family : 6 > model : 62 > model name : Intel(R) Xeon(R) CPU E5-2640 v2 @ 2.00GHz > stepping: 4 > microcode : 0x416 > cpu MHz : 1926.875 > cache size : 20480 KB > physical id : 1 > siblings: 16 > core id : 7 > cpu cores : 8 > apicid : 47 > initial apicid : 47 > fpu : yes > fpu_exception : yes > cpuid level : 13 > wp : yes > flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca > cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx > pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology > nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx > smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt > tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm ida arat epb xsaveopt pln > pts dtherm tpr_shadow vnmi flexpriority ept vpid fsgsbase smep erms > bogomips: 4005.35 > clflush size: 64 > cache_alignment : 64 > address sizes : 46 bits physical, 48 bits virtual > power management: > > gdb info: > (gdb) bt > #0 0x71ce9989 in raise () from /usr/lib64/libc.so.6 > #1 0x71ceb098 in abort () from /usr/lib64/libc.so.6 > #2 0x556275cf in kvm_set_phys_mem (section=0x7fffedcea790, add=true) > at /home/qemu/qemu/kvm-all.c:711 > #3 0x5562980f in address_space_update_topology_pass > (as=as@entry=0x55d01ca0 , adding=adding@entry=true, > new_view=, new_view=, > old_view=0x7fffe8022a90, old_view=0x7fffe8022a90) at > /home/qemu/qemu/memory.c:752 > #4 0x5562b910 in address_space_update_topology (as=0x55d01ca0 > ) at /home/qemu/qemu/memory.c:767 > #5 memory_region_transaction_commit () at /home/qemu/qemu/memory.c:808 > #6 0x557a75b4 in pci_update_mappings (d=0x562ba9f0) at > hw/pci/pci.c:1113 > #7 0x557a7932 in pci_default_write_config (d=d@entry=0x562ba9f0, > addr=addr@entry=20, val_in=val_in@entry=4294967295, l=l@entry=4) > at hw/pci/pci.c:1165 > #8 0x5566c17e in assigned_dev_pci_write_config > (pci_dev=0x562ba9f0, address=20, val=4294967295, len=4) > at /home/qemu/qemu/hw/i386/kvm/pci-assign.c:1196 > #9 0x55628fea in access_with_adjusted_size (addr=addr@entry=0, > value=value@entry=0x7fffedceaae0, size=size@entry=4, > access_size_min=, access_size_max=, > access=0x55629160 , mr=0x56231f00) > at /home/qemu/qemu/memory.c:480 > #10 0x5562dbf7 in memory_region_dispatch_write (size=4, > data=18446744073709551615, addr=0, mr=0x56231f00) at > /home/qemu/qemu/memory.c:1122 > #11 io_mem_write (mr=mr@entry=0x56231f00, addr=0, val=, > size=4) at /home/qemu/qemu/memory.c:1958 > #12 0x555f8963 in address_space_rw (as=0x55d01d80 > , addr=addr@entry=3324, buf=0x77fec000 > "\377\377\377\377", > len=len@entry=4, is_write=is_write@entry=true) at > /home/qemu/qemu/exec.c:2145 > #13 0x55628491 in kvm_handle_io (count=1, size=4, > direction=, data=, port=3324) at > /home/qemu/qemu/kvm-all.c:1614 > #14 kvm_cpu_exec (cpu=cpu@entry=0x5620e610) at > /home/qemu/qemu/kvm-all.c:1771 > #15 0x55617182 in qemu_kvm_cpu_thread_fn (arg=0x5620e610) at > /home/qemu/qemu/cpus.c:953 > #16 0x76ba2df3 in start_thread () from /usr/lib64/libpthread.so.0 > #17 0x71daa3dd in clone () from /usr/lib64/libc.so.6 > > messages log: > Oct 10 07:43:18 localhost kernel: kvm: zapping shadow pages for mmio > generation wraparound > Oct 10 07:43:27 localhost kernel: kvm [13251]: vcpu0 disabled perfctr wrmsr: > 0xc1 data 0xabcd > Oct 10 07:43:28 localhost kernel: intel_iommu_map: iommu width (48) is not > sufficient for the mapped address (fe001000) > Oct 10 07:43:28 localhost kernel: kvm_iommu_map_address:iommu failed to map > pfn=94880 > > After bisected the commits, i found the commit 453c43a4a241a7 leads to this > problem. > > commit 57271d63c4d93352406704d540453c43a4a241a7 > Author: Paolo Bonzini > Date: Thu Nov 7 17:14:37 2013 +0100 > > exec: make address spaces 64-bit wi
Re: [Qemu-devel] qemu drive-mirror to rbd storage : no sparse rbd image
>>What is the source format? If the zero clusters are actually unallocated in >>the >>source image, drive-mirror will not write those clusters either. I.e. with >>"drive-mirror sync=top", both source and target should have the same "qemu-img >>map" output. Thanks for your reply, I had tried drive mirror (sync=full) with raw file (sparse) -> rbd (no sparse) rbd (sparse) -> rbd (no sparse) raw file (sparse) -> qcow2 on ext4 (sparse) rbd (sparse) -> raw on ext4 (sparse) Also I see that I have the same problem with target file format on xfs. raw file (sparse) -> qcow2 on xfs (no sparse) rbd (sparse) -> raw on xfs (no sparse) I only have this problem with drive-mirror, qemu-img convert seem to simply skip zero blocks. Or maybe this is because I'm using sync=full ? What is the difference between full and top ? ""sync": what parts of the disk image should be copied to the destination; possibilities include "full" for all the disk, "top" for only the sectors allocated in the topmost image". (what is topmost image ?) - Mail original - De: "Fam Zheng" À: "Alexandre DERUMIER" Cc: "qemu-devel" , "Ceph Devel" Envoyé: Samedi 11 Octobre 2014 09:01:18 Objet: Re: [Qemu-devel] qemu drive-mirror to rbd storage : no sparse rbd image On Wed, 10/08 13:15, Alexandre DERUMIER wrote: > Hi, > > I'm currently planning to migrate our storage to ceph/rbd through qemu > drive-mirror > > and It seem that drive-mirror with rbd block driver, don't create a sparse > image. (all zeros are copied to the target rbd). > > Also note, that it's working fine with "qemu-img convert" , the rbd volume is > sparse after conversion. What is the source format? If the zero clusters are actually unallocated in the source image, drive-mirror will not write those clusters either. I.e. with "drive-mirror sync=top", both source and target should have the same "qemu-img map" output. Fam > > > Could it be related to the "bdrv_co_write_zeroes" missing features in > block/rbd.c ? > > (It's available in other block drivers (scsi,gluster,raw-aio) , and I don't > have this problem with theses block drivers). > > > > Regards, > > Alexandre Derumier > > >
Re: [Qemu-devel] qemu drive-mirror to rbd storage : no sparse rbd image
On Sat, 10/11 10:00, Alexandre DERUMIER wrote: > >>What is the source format? If the zero clusters are actually unallocated in > >>the > >>source image, drive-mirror will not write those clusters either. I.e. with > >>"drive-mirror sync=top", both source and target should have the same > >>"qemu-img > >>map" output. > > Thanks for your reply, > > I had tried drive mirror (sync=full) with > > raw file (sparse) -> rbd (no sparse) > rbd (sparse) -> rbd (no sparse) > raw file (sparse) -> qcow2 on ext4 (sparse) > rbd (sparse) -> raw on ext4 (sparse) > > Also I see that I have the same problem with target file format on xfs. > > raw file (sparse) -> qcow2 on xfs (no sparse) > rbd (sparse) -> raw on xfs (no sparse) > These don't tell me much. Maybe it's better to show the actual commands and how you tell sparse from no sparse? Does "qcow2 -> qcow2" work for you on xfs? > > I only have this problem with drive-mirror, qemu-img convert seem to simply > skip zero blocks. > > > Or maybe this is because I'm using sync=full ? > > What is the difference between full and top ? > > ""sync": what parts of the disk image should be copied to the destination; > possibilities include "full" for all the disk, "top" for only the sectors > allocated in the topmost image". > > (what is topmost image ?) For "sync=top", only the clusters allocated in the image itself is copied; for "full", all those clusters allocated in the image itself, and its backing image, and it's backing's backing image, ..., are copied. The image itself, having a backing image or not, is called the topmost image. Fam
Re: [Qemu-devel] qemu drive-mirror to rbd storage : no sparse rbd image
On Sat, Oct 11, 2014 at 12:25 PM, Fam Zheng wrote: > On Sat, 10/11 10:00, Alexandre DERUMIER wrote: >> >>What is the source format? If the zero clusters are actually unallocated >> >>in the >> >>source image, drive-mirror will not write those clusters either. I.e. with >> >>"drive-mirror sync=top", both source and target should have the same >> >>"qemu-img >> >>map" output. >> >> Thanks for your reply, >> >> I had tried drive mirror (sync=full) with >> >> raw file (sparse) -> rbd (no sparse) >> rbd (sparse) -> rbd (no sparse) >> raw file (sparse) -> qcow2 on ext4 (sparse) >> rbd (sparse) -> raw on ext4 (sparse) >> >> Also I see that I have the same problem with target file format on xfs. >> >> raw file (sparse) -> qcow2 on xfs (no sparse) >> rbd (sparse) -> raw on xfs (no sparse) >> > > These don't tell me much. Maybe it's better to show the actual commands and > how > you tell sparse from no sparse? > > Does "qcow2 -> qcow2" work for you on xfs? > >> >> I only have this problem with drive-mirror, qemu-img convert seem to simply >> skip zero blocks. >> >> >> Or maybe this is because I'm using sync=full ? >> >> What is the difference between full and top ? >> >> ""sync": what parts of the disk image should be copied to the destination; >> possibilities include "full" for all the disk, "top" for only the sectors >> allocated in the topmost image". >> >> (what is topmost image ?) > > For "sync=top", only the clusters allocated in the image itself is copied; for > "full", all those clusters allocated in the image itself, and its backing > image, and it's backing's backing image, ..., are copied. > > The image itself, having a backing image or not, is called the topmost image. > > Fam > -- Just a wild guess - Alexandre, did you tried detect-zeroes blk option for mirroring targets?
[Qemu-devel] [PATCH] qcow2: fix leak of Qcow2DiscardRegion in update_refcount_discard
When the Qcow2DiscardRegion is adjacent to another one referenced by "d", free this Qcow2DiscardRegion metadata referenced by "p" after it was removed from s->discards queue. Signed-off-by: Zhang Haoyu --- block/qcow2-refcount.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 2bcaaf9..c31d85a 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -524,6 +524,7 @@ found: QTAILQ_REMOVE(&s->discards, p, next); d->offset = MIN(d->offset, p->offset); d->bytes += p->bytes; +g_free(p); } } -- 1.7.12.4
Re: [Qemu-devel] [PATCH] target-arm: correctly UNDEF writes to FPINST/FPINST2 from EL0
On Fri, Oct 10, 2014 at 8:57 PM, Peter Maydell wrote:
> The ARM ARM requires that the FPINST and FPINST2 VFP control
> registers are not accessible to code at EL0. We were already
> correctly implementing this for reads of these registers; add
> the missing check for the write code path.
>
> Signed-off-by: Peter Maydell
Reviewed-by: Laurent Desnogues
Thanks,
Laurent
> ---
> target-arm/translate.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 8a2994f..d8ee2e4 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -3199,6 +3199,9 @@ static int disas_vfp_insn(CPUARMState * env,
> DisasContext *s, uint32_t insn)
> break;
> case ARM_VFP_FPINST:
> case ARM_VFP_FPINST2:
> +if (IS_USER(s)) {
> +return 1;
> +}
> tmp = load_reg(s, rd);
> store_cpu_field(tmp, vfp.xregs[rn]);
> break;
> --
> 1.9.1
>
Re: [Qemu-devel] [PATCH] target-arm: Report a valid L1Ip field in CTR_EL0 for CPU type "any"
On Fri, Oct 10, 2014 at 8:46 PM, Peter Maydell wrote: > For the CPU type "any" (only used with linux-user) we were reporting > the L1Ip field as 0b00, which is reserved. Change this field to 0b10 > instead, indicating a VIPT icache as the comment describes. > > Signed-off-by: Peter Maydell Reviewed-by: Laurent Desnogues Thanks, Laurent > --- > target-arm/cpu64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index c30f47e..4807ce1 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -151,7 +151,7 @@ static void aarch64_any_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_V8_SHA256); > set_feature(&cpu->env, ARM_FEATURE_V8_PMULL); > set_feature(&cpu->env, ARM_FEATURE_CRC); > -cpu->ctr = 0x80030003; /* 32 byte I and D cacheline size, VIPT icache */ > +cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */ > cpu->dcz_blocksize = 7; /* 512 bytes */ > } > #endif > -- > 1.9.1 >
Re: [Qemu-devel] [PATCH v2 0/2] raw-posix: Fix raw_co_get_block_status()
Am 09.10.2014 um 20:58 schrieb Benoît Canet: On Wed, Oct 08, 2014 at 09:43:19PM +0200, Max Reitz wrote: On 22.09.2014 17:36, Max Reitz wrote: raw_co_get_block_status() should return 0 and set *pnum to 0 after the EOF; currently it does this merely by accident, so implement it directly. Also, nb_sectors should be clamped against the image end. While doing that, centralize the generation of raw_co_get_block_status()'s return value along the way. v2: - Patch 1: Clamp nb_sectors against image end - Patch 2: Fix alignment issue Max Reitz (2): raw-posix: Fix raw_co_get_block_status() after EOF raw-posix: raw_co_get_block_status() return value block/raw-posix.c | 36 ++-- 1 file changed, 22 insertions(+), 14 deletions(-) Ping. (This should be rather simple to review) Hi Max, I will review these tomorow. Thanks a lot for all of your reviews! Max
Re: [Qemu-devel] how to dynamically add a block device using qmp?
Ken Chiang writes:
> Hello,
>
> I am trying to add a block device dynamically using qmp and are having
> some issues.
>
> After successfully adding the block device using "blockdev-add" and
> verifying that it has been added using "query-block", I am unable to
> see the block device in the VM under /dev/sdXX
Block devices consist of a frontend (a.k.a. device model) and a backend.
You added only a backend. To add the frontend, use device_add.
> I am using ubuntu14.04LTS: qmp version:
> {"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 2}, "package":
> " (Debian 2.0.0+dfsg-2ubuntu1.2)"}, "capabilities": []}}
>
> Here's what I am doing:
> 1. /usr/bin/kvm-spice -hda kvmimages/ubuntu.image -m 768 -usbdevice tablet
> -vnc :5 -qmp tcp:localhost:,server,nowait
>
> 2. telnet localhost
>
> 3. In the telnet session run:
> {"execute":"qmp_capabilities"}
> {"return": {}}
> { "execute": "blockdev-add",
>"arguments": { "options" : {
> "id": "disk1",
> "driver": "qcow2",
> "file": { "driver": "file",
> "filename": "testvm.qc2" } } } }
> {"return": {}}
>
> { "execute": "query-block" }
>
> 4. query-block returns the device "disk1", but when I check the vm:
># vncviewer :5
>
> there are no new devices.
For a virtio-blk disk, try:
{ "execute": "device_add", "arguments": {
"driver": "virtio-blk-pci", "id": "vda", "drive": "disk1" } }
For a SCSI disk, try:
{ "execute": "device_add", "arguments": {
"driver": "virtio-scsi-pci", "id": "vs-hba" } }
{ "execute": "device_add", "arguments": {
"driver": "scsi-disk", "id": "sda", "drive": "disk1",
"bus": "vs-hba.0" } }
[...]
Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file
Am 10.10.2014 um 13:50 schrieb Benoît Canet:
The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
When falling through to the underlying file in
bdrv_co_get_block_status(), do not let the number of sectors for which
information could be obtained be overwritten.
Signed-off-by: Max Reitz
---
block.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 3e252a2..c922664 100644
--- a/block.c
+++ b/block.c
@@ -3991,9 +3991,11 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
if (bs->file &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
(ret & BDRV_BLOCK_OFFSET_VALID)) {
+int backing_pnum;
+
ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
-*pnum, pnum);
-if (ret2 >= 0) {
+*pnum, &backing_pnum);
+if (ret2 >= 0 && backing_pnum >= *pnum) {
About backing_pnum >= *pnum.
The documentation of bdrv_co_get_block_status says:
* 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes
* beyond the end of the disk image it will be clamped.
*/
static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors, int *pnum)
So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
This means that backing_pnum > *pnum will never happen.
I think either this test is wrong or the doc is wrong.
Thank you for confusing me, I had to think quite a while about this. *g*
The condition is not for error checking. If it was, it would be the
wrong order (the condition should be true on success, that's why it's
"ret2 >= 0" and not "ret2 < 0", so it should then be "backing_pnum <=
*pnum"). So what this is testing is whether all sectors in the
underlying file in the queried range are read as zero. But if
"backing_pnum < *pnum" that is not the case, some clusters are not zero.
So we may not set the zero flag if backing_pnum < *pnum; or as it reads
in the code, we may only set it if backing_pnum >= *pnum. This is not
about whether *pnum > backing_pnum, but more about whether backing_pnum
== *pnum (but >= would be fine, too, if bdrv_co_get_block_status()
supported it, so that's why I wrote it that way).
However, I'm starting to think about whether it would be better, for the
backing_pnum < *pnum case, not to not set the zero flag, but rather
simply set *pnum = backing_pnum. And this in turn would be pretty
equivalent to just omitting this patch, because:
If we get to this point where we query the underlying file and it
returns a certain number of sectors is zero; then we therefore want to
set *pnum = backing_pnum (both if backing_pnum < *pnum and if
backing_pnum == *pnum; backing_pnum > *pnum cannot happen, as you
pointed out). On the other hand, if the sectors are not reported to be
zero, but backing_pnum < *pnum, we want to shorten *pnum accordingly as
well because this may indicate that after another backing_pnum sectors,
we arrive at a hole in the file.
There is only one point I can imagine where it makes sense not to let
backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status()
reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond
the EOF. I think this might actually happen with qcow2, if one cluster
simply lies beyond the EOF (which is perfectly valid). So I conclude
that this patch has its use after all but needs to be modified so that
backing_pnum always overwrites *pnum; except for when backing_pnum is
zero (which should only happen at or after the EOF) in which case the
zero flag should be set and *pnum should be left as it was.
And now in all honesty: Thanks for confusing me, I guess I can think
better when I'm confused. :-)
Max
Best regards
Benoît
/* Ignore errors. This is just providing extra information, it
* is useful but not necessary.
*/
--
2.0.4
Re: [Qemu-devel] [PATCH 2/3] qemu-io: Respect early image end for map
Am 10.10.2014 um 14:03 schrieb Benoît Canet:
+} else if (!num) {
+error_report("Unexpected end of image");
+return 0;
I think this test can miss some case of Unexpected end of image.
For example supose that in map_is_allocated the first bdrv_is_allocated
actually succeed then *pnum = num. Then the bottom loop has exit on failure
and the function return.
in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test
fails to see the unexpected end of image error.
num != 0 because some sectors where successfully queried. In my opinion,
we should print the information about them we have. Then, the do-while
loop is repeated; and this time, map_is_allocated() either again returns
num > 0 (for whatever reason, but I'd be fine with it) or, more
probably, it'll be num == 0 this time. So the error is not missed, it's
just printed one iteration later.
Max
Best regards
Benoît
}
retstr = ret ? "allocated" : "not allocated";
--
2.0.4
Re: [Qemu-devel] [PATCH v5 08/11] qcow2: Rebuild refcount structure during check
Am 09.10.2014 um 01:09 schrieb Eric Blake:
On 08/29/2014 03:41 PM, Max Reitz wrote:
The previous commit introduced the "rebuild" variable to qcow2's
implementation of the image consistency check. Now make use of this by
adding a function which creates a completely new refcount structure
based solely on the in-memory information gathered before.
The old refcount structure will be leaked, however.
Might be worth mentioning in the commit message that a later commit will
deal with the leak.
Signed-off-by: Max Reitz
---
block/qcow2-refcount.c | 286 -
1 file changed, 283 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6300cec..318c152 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1603,6 +1603,266 @@ static void compare_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
}
/*
+ * Allocates a cluster using an in-memory refcount table (IMRT) in contrast to
+ * the on-disk refcount structures.
+ *
+ * *first_free_cluster does not necessarily point to the first free cluster,
but
+ * may point to one cluster as close as possible before it. The offset returned
+ * will never be before that cluster.
Took me a couple reads of the comment and code to understand that. If
I'm correct, this alternative wording may be better:
On input, *first_free_cluster tells where to start looking, and need not
actually be a free cluster; the returned offset will not be before that
cluster. On output, *first_free_cluster points to the actual first free
cluster found.
Or, depending on the semantics you intended [1]:
On input, *first_free_cluster tells where to start looking, and need not
actually be a free cluster; the returned offset will not be before that
cluster. On output, *first_free_cluster points to the first gap found,
even if that gap was too small to be used as the returned offset.
Yes, *first_free_cluster has nothing to do with the allocated cluster
range. The offset of that allocated range will be returned by the
function. *first_free_cluster should merely always point somewhere
before or at the first gap (of any size) just so alloc_clusters_imrt()
does not have to start searching at the beginning of the IMRT next time
it is called. However, this also makes it useful to limit the search of
the cluster range to the end of the IMRT (or even after the end) by the
caller setting it to some arbitrary value, so it has a dual-use.
+ *
+ * Note that *first_free_cluster is a cluster index whereas the return value is
+ * an offset.
+ */
+static int64_t alloc_clusters_imrt(BlockDriverState *bs,
+ int cluster_count,
+ uint16_t **refcount_table,
+ int64_t *nb_clusters,
+ int64_t *first_free_cluster)
+{
+BDRVQcowState *s = bs->opaque;
+int64_t cluster = *first_free_cluster, i;
+bool first_gap = true;
+int contiguous_free_clusters;
+
+/* Starting at *first_free_cluster, find a range of at least cluster_count
+ * continuously free clusters */
+for (contiguous_free_clusters = 0;
+ cluster < *nb_clusters && contiguous_free_clusters < cluster_count;
+ cluster++)
+{
+if (!(*refcount_table)[cluster]) {
+contiguous_free_clusters++;
+if (first_gap) {
+/* If this is the first free cluster found, update
+ * *first_free_cluster accordingly */
+*first_free_cluster = cluster;
+first_gap = false;
+}
+} else if (contiguous_free_clusters) {
+contiguous_free_clusters = 0;
+}
[1] Should you be resetting first_gap in the 'else'? If you don't, then
*first_free_cluster is NOT the start of the cluster just allocated, but
the first free cluster encountered on the way to the eventual
allocation. I guess it depends on how the callers are using the
information; since the function is static, I guess I'll find out later
in my review.
Yes, *first_free_cluster is not that allocated cluster. I don't keep the
offset in a dedicated variable, because it'll always be cluster -
contiguous_free_clusters (see the next comment).
+}
+
+/* If contiguous_free_clusters is greater than zero, it contains the number
+ * of continuously free clusters until the current cluster; the first free
+ * cluster in the current "gap" is therefore
+ * cluster - contiguous_free_clusters */
+
+/* If no such range could be found, grow the in-memory refcount table
+ * accordingly to append free clusters at the end of the image */
+if (contiguous_free_clusters < cluster_count) {
+int64_t old_nb_clusters = *nb_clusters;
+
+/* There already is a gap of contiguous_free_clusters; we need
s/gap/tail/, since we are at the end of the table?
Well, tail doesn't imply that it's e
Re: [Qemu-devel] [PATCH v5 01/11] qcow2: Calculate refcount block entry count
Am 10.10.2014 um 14:29 schrieb Benoît Canet:
On Fri, Aug 29, 2014 at 11:40:53PM +0200, Max Reitz wrote:
The size of a refblock entry is (in theory) variable; calculate
therefore the number of entries per refblock and the according bit shift
(1 << x == entry count) when opening an image.
Signed-off-by: Max Reitz
---
block/qcow2.c | 2 ++
block/qcow2.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index f9e045f..172ad00 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -689,6 +689,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options,
int flags,
s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
s->l2_size = 1 << s->l2_bits;
+s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3);
After carefull examination (s->refcount_order - 3) == REFCOUNT_SHIFT.
Why not also creating s->refcount_shift and make use of it in order to avoid
torturing the mind of the next reader.
I'm sorry. *g*
Well, I'm against creating s->refcount_shift, because that name implies
you could use it for shifts. But you shouldn't do that, because it may
be both negative and positive.
Or simply recall in a comment that there is 2^3 bits in a byte.
I like this more. :-)
Max
Best regards
Benoît
+s->refcount_block_size = 1 << s->refcount_block_bits;
bs->total_sectors = header.size / 512;
s->csize_shift = (62 - (s->cluster_bits - 8));
s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
diff --git a/block/qcow2.h b/block/qcow2.h
index 6aeb7ea..7c01fb7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -222,6 +222,8 @@ typedef struct BDRVQcowState {
int l2_size;
int l1_size;
int l1_vm_state_index;
+int refcount_block_bits;
+int refcount_block_size;
int csize_shift;
int csize_mask;
uint64_t cluster_offset_mask;
--
2.1.0
Re: [Qemu-devel] [PATCH v5 08/11] qcow2: Rebuild refcount structure during check
Am 10.10.2014 um 14:44 schrieb Benoît Canet:
+*nb_clusters = cluster + cluster_count - contiguous_free_clusters;
+*refcount_table = g_try_realloc(*refcount_table,
+*nb_clusters * sizeof(uint16_t));
Something tells me that these sizeof(uint16_t) are connected to
s->refcount_order
and indirectely to REFCOUNT_SHIFT and that this code could benefit from this
relationship thus probably saving you work in the future.
Yes, see the answer to Eric. I'm totally in favor of variable refcounts,
see my "qcow2: Drop REFCOUNT_SHIFT series". ;-)
Once again, thank you for your reviews!
Max
+if (!*refcount_table) {
+return -ENOMEM;
+}
+
+memset(*refcount_table + old_nb_clusters, 0,
+ (*nb_clusters - old_nb_clusters) * sizeof(uint16_t));
+}
+
+/* Go back to the first free cluster */
+cluster -= contiguous_free_clusters;
+for (i = 0; i < cluster_count; i++) {
+(*refcount_table)[cluster + i] = 1;
+}
+
+return cluster << s->cluster_bits;
+}
+
+/*
+ * Creates a new refcount structure based solely on the in-memory information
+ * given through *refcount_table. All necessary allocations will be reflected
+ * in that array.
+ *
+ * On success, the old refcount structure is leaked (it will be covered by the
+ * new refcount structure).
+ */
+static int rebuild_refcount_structure(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ uint16_t **refcount_table,
+ int64_t *nb_clusters)
+{
+BDRVQcowState *s = bs->opaque;
+int64_t first_free_cluster = 0, rt_ofs = -1, cluster = 0;
+int64_t rb_ofs, rb_start, rb_index;
+uint32_t reftable_size = 0;
+uint64_t *reftable = NULL;
+uint16_t *on_disk_rb;
+int i, ret = 0;
+struct {
+uint64_t rt_offset;
+uint32_t rt_clusters;
+} QEMU_PACKED rt_offset_and_clusters;
+
+qcow2_cache_empty(bs, s->refcount_block_cache);
+
+write_refblocks:
+for (; cluster < *nb_clusters; cluster++) {
+if (!(*refcount_table)[cluster]) {
+continue;
+}
+
+rb_index = cluster >> s->refcount_block_bits;
+rb_start = rb_index << s->refcount_block_bits;
+
+/* Don't allocate a cluster in a refblock already written to disk */
+if (first_free_cluster < rb_start) {
+first_free_cluster = rb_start;
+}
+rb_ofs = alloc_clusters_imrt(bs, 1, refcount_table, nb_clusters,
+ &first_free_cluster);
+if (rb_ofs < 0) {
+fprintf(stderr, "ERROR allocating refblock: %s\n", strerror(-ret));
+res->check_errors++;
+ret = rb_ofs;
+goto fail;
+}
+
+if (reftable_size <= rb_index) {
+uint32_t old_rt_size = reftable_size;
+reftable_size = ROUND_UP((rb_index + 1) * sizeof(uint64_t),
+ s->cluster_size) / sizeof(uint64_t);
+reftable = g_try_realloc(reftable,
+ reftable_size * sizeof(uint64_t));
+if (!reftable) {
+res->check_errors++;
+ret = -ENOMEM;
+goto fail;
+}
+
+memset(reftable + old_rt_size, 0,
+ (reftable_size - old_rt_size) * sizeof(uint64_t));
+
+/* The offset we have for the reftable is now no longer valid;
+ * this will leak that range, but we can easily fix that by running
+ * a leak-fixing check after this rebuild operation */
+rt_ofs = -1;
+}
+reftable[rb_index] = rb_ofs;
+
+/* If this is apparently the last refblock (for now), try to squeeze
the
+ * reftable in */
+if (rb_index == (*nb_clusters - 1) >> s->refcount_block_bits &&
+rt_ofs < 0)
+{
+rt_ofs = alloc_clusters_imrt(bs, size_to_clusters(s, reftable_size
*
+
sizeof(uint64_t)),
+ refcount_table, nb_clusters,
+ &first_free_cluster);
+if (rt_ofs < 0) {
+fprintf(stderr, "ERROR allocating reftable: %s\n",
+strerror(-ret));
+res->check_errors++;
+ret = rt_ofs;
+goto fail;
+}
+}
+
+ret = qcow2_pre_write_overlap_check(bs, 0, rb_ofs, s->cluster_size);
+if (ret < 0) {
+fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
+goto fail;
+}
+
+on_disk_rb = g_malloc0(s->cluster_size);
+for (i = 0; i < s->cluster_size / sizeof(uint16_t) &&
+rb_start + i < *nb_clusters; i++)
+{
+on_disk_rb[i] = cpu_to_be16((*ref
Re: [Qemu-devel] [PATCH v12 03/14] qcow2: Optimize bdrv_make_empty()
Am 10.10.2014 um 14:32 schrieb Eric Blake:
On 08/26/2014 03:36 PM, Max Reitz wrote:
bdrv_make_empty() is currently only called if the current image
represents an external snapshot that has been committed to its base
image; it is therefore unlikely to have internal snapshots. In this
case, bdrv_make_empty() can be greatly sped up by emptying the L1 and
refcount table (while having the dirty flag set) and creating a trivial
refcount structure.
If there are snapshots, fall back to the simple implementation (discard
all clusters).
Signed-off-by: Max Reitz
---
block/blkdebug.c | 2 +
block/qcow2.c | 137 --
include/block/block.h | 2 +
3 files changed, 126 insertions(+), 15 deletions(-)
+} else {
+int l1_clusters;
+int64_t offset;
+uint64_t *new_reftable;
+uint8_t l1_ofs_rt_ofs_cls[20]; /* L1 offset; RT offset and clusters */
+uint64_t rt_entry;
+
+ret = qcow2_cache_empty(bs, s->l2_table_cache);
if (ret < 0) {
-break;
+return ret;
+}
+
+ret = qcow2_cache_empty(bs, s->refcount_block_cache);
+if (ret < 0) {
+return ret;
+}
+
+/* Refcounts will be broken utterly */
+ret = qcow2_mark_dirty(bs);
qcow2_mark_dirty does assert(s->qcow_version >= 3). But this code can
be reached when committing a qcow2 0.10 compat-level file, right?
You're right, this code doesn't work for compat=0.10. As you proposed in
your own answer to this, I'll just use the fallback code in that case
(in v13)
+/* "Create" an empty reftable (one cluster) directly after the image
+ * header and an empty L1 table three clusters after the image header;
+ * the cluster between those two will be used as the first refblock */
+cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 0], 3 * s->cluster_size);
+cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 8], s->cluster_size);
+cpu_to_be32w((uint32_t *)&l1_ofs_rt_ofs_cls[16], 1);
The offsets here feel a bit magic.
Well, such byte offsets are everywhere in the qcow2 code which does such
small writes to the image header. Making them non-magic would mean
offsetof(...) - offsetof(...). Can and will do, but I probably won't
find it much more readable. ;-)
Max
Re: [Qemu-devel] [PATCH v12 14/14] iotests: Add test for qcow2's bdrv_make_empty
Am 10.10.2014 um 18:47 schrieb Eric Blake: On 08/26/2014 03:36 PM, Max Reitz wrote: Add a test for qcow2's fast bdrv_make_empty implementation on images without internal snapshots. This test may need to be limited to compat=1.1 files. Will do. Signed-off-by: Max Reitz --- tests/qemu-iotests/098 | 78 ++ tests/qemu-iotests/098.out | 45 ++ tests/qemu-iotests/group | 1 + 3 files changed, 124 insertions(+) create mode 100755 tests/qemu-iotests/098 create mode 100644 tests/qemu-iotests/098.out Reviewed-by: Eric Blake Thanks! Max
Re: [Qemu-devel] [PATCH v2 1/2] libqos: improve event_index test with timeout
On 29 September 2014 16:40, Stefan Hajnoczi wrote: > The virtio event_index feature lets the device driver tell the device > how many requests to process before raising the next interrupt. > virtio-blk-test.c tries to verify that the device does not raise an > interrupt unnecessarily. > > Unfortunately the test has a race condition. It spins checking for an > interrupt up to 100 times and then assumes the request has finished. On > a slow host the I/O request could still be in flight and the test would > fail. > > This patch waits for the request to complete, or until a 30-second > timeout is reached. If an interrupt is raised while waiting the test > fails since the device was not supposed to raise interrupts. > > Signed-off-by: Stefan Hajnoczi Hi; just noticed this breaks 'make check' build on MacOSX: /Users/pm215/src/qemu/tests/libqos/virtio.c:84:25: warning: implicit declaration of function 'g_get_monotonic_time' is invalid in C99 [-Wimplicit-function-declaration] gint64 start_time = g_get_monotonic_time(); ^ (and subsequent linker error). g_get_monotonic_time() only appeared in glib 2.28, and our minimum is 2.12. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 1/2] libqos: improve event_index test with timeout
On 11 October 2014 12:43, Peter Maydell wrote: > Hi; just noticed this breaks 'make check' build on MacOSX: > > /Users/pm215/src/qemu/tests/libqos/virtio.c:84:25: warning: implicit > declaration of function > 'g_get_monotonic_time' is invalid in C99 > [-Wimplicit-function-declaration] > gint64 start_time = g_get_monotonic_time(); > ^ > (and subsequent linker error). > > g_get_monotonic_time() only appeared in glib 2.28, and our > minimum is 2.12. vhost-user-test.c has a helper function that has a fallback for not having g_get_monotonic_time(); we probably want to put that somewhere more generally accessible. -- PMM
Re: [Qemu-devel] latest rc: virtio-blk hangs forever after migration
On Fri, Oct 10, 2014 at 09:33:04AM +0200, Marcin Gibuła wrote: > >Does anybody know why the APIC state loaded by the first call to > >kvm_arch_get_registers() is wrong, in the first place? What exactly is > >different in the APIC state in the second kvm_arch_get_registers() call, > >and when/why does it change? > > > >If cpu_synchronize_state() does the wrong thing if it is called at the > >wrong moment, then we may have other hidden bugs, because the user can > >trigger cpu_synchronize_all_states() calls arbitrarily using monitor > >commands. > > My guess is, it's not wrong, it's just outdated when second call occures. > Maybe it's an ordering issue - could kvmclock state change handler be called > before other activity is suspended (?) Changing the ordering could just hide the problem. If the APIC state in the kernel can change outside ioctl(KVM_RUN) calls, then it can't be cached the same way the other registers loaded by kvm_arch_get_registers() are. The current code invalidates the QEMU register data (i.e. sets kvm_vcpu_dirty=false) only on 3 cases: 1) VCPU init; 2) VCPU reset; 3) on kvm_cpu_exec(), immediately before calling ioctl(KVM_RUN). We may need a separate API to load APIC state? I don't know what are the actual users X86CPU.apic_state, today. -- Eduardo
[Qemu-devel] AArch64: ld/st exclusive pair bug
Hello, there's a bug in target-arm/translate-a64.c:disas_ldst_excl. The line: TCGv_i64 tcg_rt2 = cpu_reg(s, rt); is accessing the wrong register. Thanks, Laurent
[Qemu-devel] [PATCH v2] libvixl: a64: Skip "-Wunused-variable" for gcc 5.0.0 or higher
'instructions-a64.h' has unused variables for qemu which can be found by
gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break.
But they may be used by another projects (not qemu).
So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic
(still print warning, but not break building). The related warnings:
CXX disas/arm-a64.o
In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0,
from disas/arm-a64.cc:20:
disas/libvixl/a64/instructions-a64.h:98:13: error:
'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable]
const float kFP32PositiveInfinity = rawbits_to_float(0x7f80);
^
disas/libvixl/a64/instructions-a64.h:99:13: error:
'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable]
const float kFP32NegativeInfinity = rawbits_to_float(0xff80);
^
disas/libvixl/a64/instructions-a64.h:100:14: error:
'vixl::kFP64PositiveInfinity' defined but not used [-Werror=unused-variable]
const double kFP64PositiveInfinity =
^
disas/libvixl/a64/instructions-a64.h:102:14: error:
'vixl::kFP64NegativeInfinity' defined but not used [-Werror=unused-variable]
const double kFP64NegativeInfinity =
^
disas/libvixl/a64/instructions-a64.h:107:21: error:
'vixl::kFP64SignallingNaN' defined but not used [-Werror=unused-variable]
static const double kFP64SignallingNaN =
^
disas/libvixl/a64/instructions-a64.h:109:20: error:
'vixl::kFP32SignallingNaN' defined but not used [-Werror=unused-variable]
static const float kFP32SignallingNaN = rawbits_to_float(0x7f81);
^
disas/libvixl/a64/instructions-a64.h:112:21: error: 'vixl::kFP64QuietNaN'
defined but not used [-Werror=unused-variable]
static const double kFP64QuietNaN =
^
disas/libvixl/a64/instructions-a64.h:114:20: error: 'vixl::kFP32QuietNaN'
defined but not used [-Werror=unused-variable]
static const float kFP32QuietNaN = rawbits_to_float(0x7fc1);
^
disas/libvixl/a64/instructions-a64.h:117:21: error: 'vixl::kFP64DefaultNaN'
defined but not used [-Werror=unused-variable]
static const double kFP64DefaultNaN =
^
disas/libvixl/a64/instructions-a64.h:119:20: error: 'vixl::kFP32DefaultNaN'
defined but not used [-Werror=unused-variable]
static const float kFP32DefaultNaN = rawbits_to_float(0x7fc0);
^
cc1plus: all warnings being treated as errors
make: *** [disas/arm-a64.o] Error 1
After this patch, can pass upstream gcc 5.0.0 building (print warning,
but not break building), and fedora 20 gcc 4.8.1 building (not find
warnings).
Signed-off-by: Chen Gang
---
disas/libvixl/a64/instructions-a64.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/disas/libvixl/a64/instructions-a64.h
b/disas/libvixl/a64/instructions-a64.h
index d5b90c5..5f707f3 100644
--- a/disas/libvixl/a64/instructions-a64.h
+++ b/disas/libvixl/a64/instructions-a64.h
@@ -95,6 +95,12 @@ const unsigned kDoubleExponentBits = 11;
const unsigned kFloatMantissaBits = 23;
const unsigned kFloatExponentBits = 8;
+/* For QEMU, gcc 5.0.0 or higher finds unused variables, so ignore diagnostic
*/
+#if __GNUC__ >= 5
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"
+#endif
+
const float kFP32PositiveInfinity = rawbits_to_float(0x7f80);
const float kFP32NegativeInfinity = rawbits_to_float(0xff80);
const double kFP64PositiveInfinity =
@@ -118,6 +124,9 @@ static const double kFP64DefaultNaN =
rawbits_to_double(UINT64_C(0x7ff8));
static const float kFP32DefaultNaN = rawbits_to_float(0x7fc0);
+#if __GNUC__ >= 5
+#pragma GCC diagnostic pop
+#endif
enum LSDataSize {
LSByte= 0,
--
1.9.3
Re: [Qemu-devel] [PATCH v2] libvixl: a64: Skip "-Wunused-variable" for gcc 5.0.0 or higher
On 10/11/14 22:07, Chen Gang wrote:
> 'instructions-a64.h' has unused variables for qemu which can be found by
> gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break.
> But they may be used by another projects (not qemu).
>
> So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic
> (still print warning, but not break building). The related warnings:
>
> CXX disas/arm-a64.o
> In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0,
>from disas/arm-a64.cc:20:
> disas/libvixl/a64/instructions-a64.h:98:13: error:
> 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable]
>const float kFP32PositiveInfinity = rawbits_to_float(0x7f80);
>^
> disas/libvixl/a64/instructions-a64.h:99:13: error:
> 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable]
>const float kFP32NegativeInfinity = rawbits_to_float(0xff80);
>^
> disas/libvixl/a64/instructions-a64.h:100:14: error:
> 'vixl::kFP64PositiveInfinity' defined but not used [-Werror=unused-variable]
>const double kFP64PositiveInfinity =
> ^
> disas/libvixl/a64/instructions-a64.h:102:14: error:
> 'vixl::kFP64NegativeInfinity' defined but not used [-Werror=unused-variable]
>const double kFP64NegativeInfinity =
> ^
> disas/libvixl/a64/instructions-a64.h:107:21: error:
> 'vixl::kFP64SignallingNaN' defined but not used [-Werror=unused-variable]
>static const double kFP64SignallingNaN =
>^
> disas/libvixl/a64/instructions-a64.h:109:20: error:
> 'vixl::kFP32SignallingNaN' defined but not used [-Werror=unused-variable]
>static const float kFP32SignallingNaN = rawbits_to_float(0x7f81);
> ^
> disas/libvixl/a64/instructions-a64.h:112:21: error: 'vixl::kFP64QuietNaN'
> defined but not used [-Werror=unused-variable]
>static const double kFP64QuietNaN =
>^
> disas/libvixl/a64/instructions-a64.h:114:20: error: 'vixl::kFP32QuietNaN'
> defined but not used [-Werror=unused-variable]
>static const float kFP32QuietNaN = rawbits_to_float(0x7fc1);
> ^
> disas/libvixl/a64/instructions-a64.h:117:21: error: 'vixl::kFP64DefaultNaN'
> defined but not used [-Werror=unused-variable]
>static const double kFP64DefaultNaN =
>^
> disas/libvixl/a64/instructions-a64.h:119:20: error: 'vixl::kFP32DefaultNaN'
> defined but not used [-Werror=unused-variable]
>static const float kFP32DefaultNaN = rawbits_to_float(0x7fc0);
> ^
> cc1plus: all warnings being treated as errors
> make: *** [disas/arm-a64.o] Error 1
>
> After this patch, can pass upstream gcc 5.0.0 building (print warning,
> but not break building), and fedora 20 gcc 4.8.1 building (not find
Oh, sorry, under fedora 20, it is "gcc version 4.8.3 20140624 (Red Hat
4.8.3-1) (GCC)".
And the detail gcc 5.0.0 is "gcc version 5.0.0 20141003 (experimental)
(GCC)"
> warnings).
>
> Signed-off-by: Chen Gang
> ---
> disas/libvixl/a64/instructions-a64.h | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/disas/libvixl/a64/instructions-a64.h
> b/disas/libvixl/a64/instructions-a64.h
> index d5b90c5..5f707f3 100644
> --- a/disas/libvixl/a64/instructions-a64.h
> +++ b/disas/libvixl/a64/instructions-a64.h
> @@ -95,6 +95,12 @@ const unsigned kDoubleExponentBits = 11;
> const unsigned kFloatMantissaBits = 23;
> const unsigned kFloatExponentBits = 8;
>
> +/* For QEMU, gcc 5.0.0 or higher finds unused variables, so ignore
> diagnostic */
> +#if __GNUC__ >= 5
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-variable"
> +#endif
> +
> const float kFP32PositiveInfinity = rawbits_to_float(0x7f80);
> const float kFP32NegativeInfinity = rawbits_to_float(0xff80);
> const double kFP64PositiveInfinity =
> @@ -118,6 +124,9 @@ static const double kFP64DefaultNaN =
> rawbits_to_double(UINT64_C(0x7ff8));
> static const float kFP32DefaultNaN = rawbits_to_float(0x7fc0);
>
> +#if __GNUC__ >= 5
> +#pragma GCC diagnostic pop
> +#endif
>
> enum LSDataSize {
>LSByte= 0,
>
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] AArch64: ld/st exclusive pair bug
On 11 October 2014 14:04, Laurent Desnogues wrote: > there's a bug in target-arm/translate-a64.c:disas_ldst_excl. The line: > > TCGv_i64 tcg_rt2 = cpu_reg(s, rt); > > is accessing the wrong register. Yeah, obvious cut-n-paste error, but this doesn't actually affect the exclusive code path, does it? In fact, looking at the decode tables I can't find any instructions with is_excl false and is_pair true (these would be load acquire/ store release pair insns, which would be LDARP/STLRP if they existed.) I think the actual bug here is that we've missed an unallocated-encoding case, isn't it? The pseudocode check is if o2:o1:o0 == '100' || o2:o1:o0 == '11x' which in our terms would be if ((!is_excl && !is_pair && !is_lasr) || (!is_excl && is_pair)) [plus the pair with wrong size check which we get right.] Then the code that purports to handle non-exclusive pair accesses is dead and can be deleted... -- PMM
[Qemu-devel] AArch64: duplicate store in do_fp_st
Hello, in target-arm/translate-a64.c:do_fp_st, it looks like there's an extraneous store: TCGv_i64 tcg_hiaddr = tcg_temp_new_i64(); tcg_gen_qemu_st_i64(tmp, tcg_addr, get_mem_index(s), MO_TEQ); tcg_gen_qemu_st64(tmp, tcg_addr, get_mem_index(s)); The second one can safely be removed. Thanks, Laurent
Re: [Qemu-devel] [PATCH 2/3] qemu-io: Respect early image end for map
The Saturday 11 Oct 2014 à 11:53:40 (+0200), Max Reitz wrote :
> Am 10.10.2014 um 14:03 schrieb Benoît Canet:
> >>+} else if (!num) {
> >>+error_report("Unexpected end of image");
> >>+return 0;
> >I think this test can miss some case of Unexpected end of image.
> >
> >For example supose that in map_is_allocated the first bdrv_is_allocated
> >actually succeed then *pnum = num. Then the bottom loop has exit on failure
> >and the function return.
> >
> >in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very
> >test
> >fails to see the unexpected end of image error.
>
> num != 0 because some sectors where successfully queried. In my opinion, we
> should print the information about them we have. Then, the do-while loop is
> repeated; and this time, map_is_allocated() either again returns num > 0
> (for whatever reason, but I'd be fine with it) or, more probably, it'll be
> num == 0 this time. So the error is not missed, it's just printed one
> iteration later.
Ok that make sense.
Best regards
Benoît
>
> Max
>
> >Best regards
> >
> >Benoît
> >
> >> }
> >> retstr = ret ? "allocated" : "not allocated";
> >>--
> >>2.0.4
> >>
> >>
>
>
Re: [Qemu-devel] [PATCH 2/3] qemu-io: Respect early image end for map
The Saturday 16 Aug 2014 à 20:54:17 (+0200), Max Reitz wrote :
> bdrv_is_allocated() may report zero clusters which most probably means
> the image (file) is shorter than expected. Respect this case in order to
> avoid an infinite loop.
>
> Signed-off-by: Max Reitz
> ---
> qemu-io-cmds.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index c503fc6..860b834 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1909,7 +1909,7 @@ static int map_is_allocated(BlockDriverState *bs,
> int64_t sector_num,
>
> num_checked = MIN(nb_sectors, INT_MAX);
> ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
> -if (ret == firstret) {
> +if (ret == firstret && num) {
> *pnum += num;
> } else {
> break;
> @@ -1936,6 +1936,9 @@ static int map_f(BlockDriverState *bs, int argc, char
> **argv)
> if (ret < 0) {
> error_report("Failed to get allocation status: %s",
> strerror(-ret));
> return 0;
> +} else if (!num) {
> +error_report("Unexpected end of image");
> +return 0;
> }
>
> retstr = ret ? "allocated" : "not allocated";
> --
> 2.0.4
>
>
Reviewed-by: Benoît Canet
Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file
The Saturday 11 Oct 2014 à 11:44:20 (+0200), Max Reitz wrote :
> Am 10.10.2014 um 13:50 schrieb Benoît Canet:
> >The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
> >>When falling through to the underlying file in
> >>bdrv_co_get_block_status(), do not let the number of sectors for which
> >>information could be obtained be overwritten.
> >>
> >>Signed-off-by: Max Reitz
> >>---
> >> block.c | 6 --
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index 3e252a2..c922664 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -3991,9 +3991,11 @@ static int64_t coroutine_fn
> >>bdrv_co_get_block_status(BlockDriverState *bs,
> >> if (bs->file &&
> >> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> >> (ret & BDRV_BLOCK_OFFSET_VALID)) {
> >>+int backing_pnum;
> >>+
> >> ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> >>-*pnum, pnum);
> >>-if (ret2 >= 0) {
> >>+*pnum, &backing_pnum);
> >>+if (ret2 >= 0 && backing_pnum >= *pnum) {
> >About backing_pnum >= *pnum.
> >
> >The documentation of bdrv_co_get_block_status says:
> >
> > * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors
> > goes
> > * beyond the end of the disk image it will be clamped.
> > */
> >static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> > int64_t sector_num,
> > int nb_sectors, int
> > *pnum)
> >
> >So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
> >
> >This means that backing_pnum > *pnum will never happen.
> >
> >I think either this test is wrong or the doc is wrong.
>
> Thank you for confusing me, I had to think quite a while about this. *g*
>
> The condition is not for error checking. If it was, it would be the wrong
> order (the condition should be true on success, that's why it's "ret2 >= 0"
> and not "ret2 < 0", so it should then be "backing_pnum <= *pnum"). So what
> this is testing is whether all sectors in the underlying file in the queried
> range are read as zero. But if "backing_pnum < *pnum" that is not the case,
> some clusters are not zero. So we may not set the zero flag if backing_pnum
> < *pnum; or as it reads in the code, we may only set it if backing_pnum >=
> *pnum. This is not about whether *pnum > backing_pnum, but more about
> whether backing_pnum == *pnum (but >= would be fine, too, if
> bdrv_co_get_block_status() supported it, so that's why I wrote it that way).
>
> However, I'm starting to think about whether it would be better, for the
> backing_pnum < *pnum case, not to not set the zero flag, but rather simply
> set *pnum = backing_pnum. And this in turn would be pretty equivalent to
> just omitting this patch, because:
>
> If we get to this point where we query the underlying file and it returns a
> certain number of sectors is zero; then we therefore want to set *pnum =
> backing_pnum (both if backing_pnum < *pnum and if backing_pnum == *pnum;
> backing_pnum > *pnum cannot happen, as you pointed out). On the other hand,
> if the sectors are not reported to be zero, but backing_pnum < *pnum, we
> want to shorten *pnum accordingly as well because this may indicate that
> after another backing_pnum sectors, we arrive at a hole in the file.
>
> There is only one point I can imagine where it makes sense not to let
> backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status()
> reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond the
> EOF. I think this might actually happen with qcow2, if one cluster simply
> lies beyond the EOF (which is perfectly valid). So I conclude that this
> patch has its use after all but needs to be modified so that backing_pnum
> always overwrites *pnum; except for when backing_pnum is zero (which should
> only happen at or after the EOF) in which case the zero flag should be set
> and *pnum should be left as it was.
>
> And now in all honesty: Thanks for confusing me, I guess I can think better
> when I'm confused. :-)
>
You better have killer english skills to sumarize this in a nice commit message
:)
I'll read the next version.
Best regards
Benoît
> Max
>
> >Best regards
> >
> >Benoît
> >
> >
> >> /* Ignore errors. This is just providing extra information,
> >> it
> >> * is useful but not necessary.
> >> */
> >>--
> >>2.0.4
> >>
> >>
>
>
Re: [Qemu-devel] [PATCH v5 08/11] qcow2: Rebuild refcount structure during check
> +int64_t first_free_cluster = 0, rt_ofs = -1, cluster = 0; > +int64_t rb_ofs, rb_start, rb_index; Everytime a few day pass and I read rb_ofs and rt_ofs again I found these names obfuscated. I know Linus says that C is a spartan language but these look odd. Maybe refcount_block_offset, refcount_block_* and refcount_table_offset would be better. I'll try to read this patch for real before you repost it. Best regards Benoît
Re: [Qemu-devel] [PATCH v2] libvixl: a64: Skip "-Wunused-variable" for gcc 5.0.0 or higher
On 11 October 2014 15:13, Chen Gang wrote: MJT: please don't put this in -trivial, it will clash with the update to libvixl 1.6 currently on the list. > On 10/11/14 22:07, Chen Gang wrote: >> 'instructions-a64.h' has unused variables for qemu which can be found by >> gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. >> But they may be used by another projects (not qemu). >> >> So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic >> (still print warning, but not break building). The related warnings: >> >> CXX disas/arm-a64.o >> In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, >>from disas/arm-a64.cc:20: >> disas/libvixl/a64/instructions-a64.h:98:13: error: >> 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] >>const float kFP32PositiveInfinity = rawbits_to_float(0x7f80); >>^ >> disas/libvixl/a64/instructions-a64.h:99:13: error: >> 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] So, several questions here: (1) Does anybody know how the header in question should be providing constant floats and doubles to its users in a way that doesn't cause gcc to complain? If so, I can pass the answer along so at least the next version of the library is OK... (2) Bonus question, anybody know why gcc is perfectly happy with the unused integer constants in the .h file but doesn't like the float and doubles? (3) Does gcc 5 emit these warnings for all the .cc files that pull in this header, or only for the arm-a64.cc one? Some other approaches to this that would confine the fix to the makefiles rather than requiring us to modify the vixl source itself: a) add a -Wno- option for the affected .o files b) use -isystem rather than -I to include the libvixl directory on the include path (a)'s probably better I guess. thanks -- PMM
Re: [Qemu-devel] [PATCH v2] libvixl: a64: Skip "-Wunused-variable" for gcc 5.0.0 or higher
On 10/12/14 5:25, Peter Maydell wrote: > On 11 October 2014 15:13, Chen Gang wrote: > > MJT: please don't put this in -trivial, it will clash with > the update to libvixl 1.6 currently on the list. > OK thanks (also remove -trivial from replying mailing list). > >> On 10/11/14 22:07, Chen Gang wrote: >>> 'instructions-a64.h' has unused variables for qemu which can be found by >>> gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. >>> But they may be used by another projects (not qemu). >>> >>> So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic >>> (still print warning, but not break building). The related warnings: >>> >>> CXX disas/arm-a64.o >>> In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, >>>from disas/arm-a64.cc:20: >>> disas/libvixl/a64/instructions-a64.h:98:13: error: >>> 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] >>>const float kFP32PositiveInfinity = rawbits_to_float(0x7f80); >>>^ >>> disas/libvixl/a64/instructions-a64.h:99:13: error: >>> 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] > > So, several questions here: > > (1) Does anybody know how the header in question should be > providing constant floats and doubles to its users in a way > that doesn't cause gcc to complain? If so, I can pass the > answer along so at least the next version of the library > is OK... > Hmm... may try to simplify rawbits_to_float() and rawbits_to_double() to let gcc know about it is only for constant? (it seems original authors intended to do it in function, I shall try to know why) I shall try to get a conclusion within this month. > (2) Bonus question, anybody know why gcc is perfectly happy > with the unused integer constants in the .h file but doesn't > like the float and doubles? > For (2), they use a real function rawbits_to_float(), is it gcc own issue? I shall try to analyze it (at present, I am also upstream gcc members, and make a patch for it per month). I shall try to finish analyzing within this month. > (3) Does gcc 5 emit these warnings for all the .cc files > that pull in this header, or only for the arm-a64.cc one? > For (3), yes, it is, so I have to let it in header again. > Some other approaches to this that would confine the > fix to the makefiles rather than requiring us to modify > the vixl source itself: > a) add a -Wno- option for the affected .o files It is one way, but may have effect with gcc 4 version, and also it is effect with the whole file which is wider than current way. > b) use -isystem rather than -I to include the libvixl > directory on the include path > It sounds good to me, although for me, it is not related with current issue. Thanks -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH] hw/display/vga: Remove unused arrays dmask4 and dmask16
On Fri, Oct 10, 2014 at 08:44:29PM +0100, Peter Maydell wrote: > Following cleanup of the vga device code in commit d2e043a8041, > the arrays dmask4 and dmask16 are now unused. gcc doesn't warn > about this, but clang does; remove them. > > Signed-off-by: Peter Maydell Reviewed-by: David Gibson -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgpKgYnQa7gUJ.pgp Description: PGP signature
