Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
On 2019/4/15 17:25, Daniel P. Berrangé wrote:
On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
...
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf..551bec9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -128,6 +128,8 @@ GlobalProperty pc_compat_3_1[] = {
{ "EPYC" "-" TYPE_X86_CPU, "nrip-save", "off" },
{ "EPYC-IBPB" "-" TYPE_X86_CPU, "npt", "off" },
{ "EPYC-IBPB" "-" TYPE_X86_CPU, "nrip-save", "off" },
+{ "Dhyana" "-" TYPE_X86_CPU, "npt", "off" },
+{ "Dhyana" "-" TYPE_X86_CPU, "nrip-save", "off" },
{ "Skylake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Skylake-Client-IBRS" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Skylake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
@@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
{ TYPE_X86_CPU, "topoext", "off" },
{ "EPYC-" TYPE_X86_CPU, "xlevel", "0x800a" },
{ "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x800a" },
+{ "Dhyana-" TYPE_X86_CPU, "xlevel", "0x800a" },
};
const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);
You can drop the changes in this file. This CPU model didn't exist
in any older QEMU releases, so there's no machine type backcompat
required, at least from upstream QEMU POV.
Then how about running QEMU with the parameter like "-M pc-i440fx-2.12"?
Although the default machine is newer than that.
--
Regards,
Pu Wen
Re: [Qemu-devel] [PATCH v3 2/7] qemu-img: Add salvaging mode to convert
13.04.2019 19:53, Max Reitz wrote: > This adds a salvaging mode (--salvage) to qemu-img convert which ignores > read errors and treats the respective areas as containing only zeroes. > This can be used for instance to at least partially recover the data > from terminally corrupted qcow2 images. > > Signed-off-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH] util/error: do not free error on error_abort
16.04.2019 9:34, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> 15.04.2019 16:08, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy writes:
>>>
15.04.2019 8:51, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> It would be nice to have Error object not freed away when debugging a
>> coredump.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> util/error.c | 8 +---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/error.c b/util/error.c
>> index 934a78e1b1..f9180c0f30 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -32,9 +32,11 @@ Error *error_fatal;
>> static void error_handle_fatal(Error **errp, Error *err)
>> {
>> if (errp == &error_abort) {
>> -fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>> -err->func, err->src, err->line);
>> -error_report_err(err);
>> +error_report("Unexpected error in %s() at %s:%d: %s",
>> + err->func, err->src, err->line,
>> error_get_pretty(err));
>> +if (err->hint) {
>> +error_printf_unless_qmp("%s", err->hint->str);
>> +}
>> abort();
>> }
>> if (errp == &error_fatal) {
>
> No objections to not freeing the error object on the path to abort().
>
> You also format the error message differently. Commit 1e9b65bb1ba's
> example
>
>Unexpected error in parse_block_error_action() at
> .../qemu/blockdev.c:322:
>qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid
> write error action
>Aborted (core dumped)
>
> changes to (guesswork, not tested):
>
>qemu-system-x86_64: -drive if=none,werror=foo: Unexpected
> error in parse_block_error_action() at .../qemu/blockdev.c:322: 'foo'
> invalid write error action
>Aborted (core dumped)
hm. checkpatch don't allow to put \n into error_report. So, should I call
error_report twice?
Transition from fprintf to error_report is OK for you?
>>>
>>> Let's simply not mess with the formatting at all. Something like:
>>>
>>> (1) Inline error_report_err(), delete the error_free()
>>>
>>> (2) Optional: replace fprintf() by error_printf()
>>>
>>> (3) Optional: clean up the additional copy of
>>>
>>> if (err->hint) {
>>> error_printf_unless_qmp("%s", err->hint->str);
>>> }
>>>
>>> (3a) Either create a helper function, replacing all three copies,
>>>
>>> (3b) Or simply delete the new copy from the path leading to abort(),
>>>since the hint is unlikely to be useful there.
>>>
>>
>> Why we print error always but hint only "unless_qmp"?
>
> "Hints" are intended for giving hints to a human user (although they are
> misused for other purposes in places):
>
> /*
> * Append a printf-style human-readable explanation to an existing error.
> * If the error is later reported to a human user with
> * error_report_err() or warn_report_err(), the hints will be shown,
> * too. If it's reported via QMP, the hints will be ignored.
> * Intended use is adding helpful hints on the human user interface,
> * e.g. a list of valid values. It's not for clarifying a confusing
> * error message.
> * @errp may be NULL, but not &error_fatal or &error_abort.
> * Trivially the case if you call it only after error_setg() or
> * error_propagate().
> * May be called multiple times. The resulting hint should end with a
> * newline.
> */
> void error_append_hint(Error **errp, const char *fmt, ...)
> GCC_FMT_ATTR(2, 3);
>
Hmm, this means, that in error_report_err checking for qmp monitor is wrong
thing,
as error_report_err is exactly for human user who will read qemu log and will
need
maximum information.
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH] util/error: do not free error on error_abort
Vladimir Sementsov-Ogievskiy writes:
> 15.04.2019 16:08, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> 15.04.2019 8:51, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy writes:
> It would be nice to have Error object not freed away when debugging a
> coredump.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>util/error.c | 8 +---
>1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/util/error.c b/util/error.c
> index 934a78e1b1..f9180c0f30 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -32,9 +32,11 @@ Error *error_fatal;
>static void error_handle_fatal(Error **errp, Error *err)
>{
>if (errp == &error_abort) {
> -fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> -err->func, err->src, err->line);
> -error_report_err(err);
> +error_report("Unexpected error in %s() at %s:%d: %s",
> + err->func, err->src, err->line,
> error_get_pretty(err));
> +if (err->hint) {
> +error_printf_unless_qmp("%s", err->hint->str);
> +}
>abort();
>}
>if (errp == &error_fatal) {
No objections to not freeing the error object on the path to abort().
You also format the error message differently. Commit 1e9b65bb1ba's
example
Unexpected error in parse_block_error_action() at
.../qemu/blockdev.c:322:
qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid
write error action
Aborted (core dumped)
changes to (guesswork, not tested):
qemu-system-x86_64: -drive if=none,werror=foo: Unexpected error
in parse_block_error_action() at .../qemu/blockdev.c:322: 'foo' invalid
write error action
Aborted (core dumped)
>>>
>>> hm. checkpatch don't allow to put \n into error_report. So, should I call
>>> error_report twice?
>>>
>>> Transition from fprintf to error_report is OK for you?
>>
>> Let's simply not mess with the formatting at all. Something like:
>>
>> (1) Inline error_report_err(), delete the error_free()
>>
>> (2) Optional: replace fprintf() by error_printf()
>>
>> (3) Optional: clean up the additional copy of
>>
>> if (err->hint) {
>> error_printf_unless_qmp("%s", err->hint->str);
>> }
>>
>> (3a) Either create a helper function, replacing all three copies,
>>
>> (3b) Or simply delete the new copy from the path leading to abort(),
>> since the hint is unlikely to be useful there.
>>
>
> Why we print error always but hint only "unless_qmp"?
"Hints" are intended for giving hints to a human user (although they are
misused for other purposes in places):
/*
* Append a printf-style human-readable explanation to an existing error.
* If the error is later reported to a human user with
* error_report_err() or warn_report_err(), the hints will be shown,
* too. If it's reported via QMP, the hints will be ignored.
* Intended use is adding helpful hints on the human user interface,
* e.g. a list of valid values. It's not for clarifying a confusing
* error message.
* @errp may be NULL, but not &error_fatal or &error_abort.
* Trivially the case if you call it only after error_setg() or
* error_propagate().
* May be called multiple times. The resulting hint should end with a
* newline.
*/
void error_append_hint(Error **errp, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
Re: [Qemu-devel] [PATCH 14/17] qom/cpu: Simplify how CPUClass:cpu_dump_state() prints
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> CPUClass method dump_statistics() takes an fprintf()-like callback and >> a FILE * to pass to it. Most callers pass fprintf() and stderr. >> log_cpu_state() passes fprintf() and qemu_log_file. >> hmp_info_registers() passes monitor_fprintf() and the current monitor >> cast to FILE *. monitor_fprintf() casts it right back, and is >> otherwise identical to monitor_printf(). >> >> The callback gets passed around a lot, which is tiresome. The >> type-punning around monitor_fprintf() is ugly. >> >> Drop the callback, and call qemu_fprintf() instead. Also gets rid of >> the type-punning, since qemu_fprintf() takes NULL instead of the >> current monitor cast to FILE *. >> >> Signed-off-by: Markus Armbruster > > > Yes, I think so. Thanks for persevering! > There seems to be a place which changes hmp_info_local_apic in a plce > that was changed in an earlier patch which seems a shame, but OK. Hmm. I think I can avoid that. > Reviewed-by: Dr. David Alan Gilbert Thanks!
Re: [Qemu-devel] [PATCH 10/17] target: Clean up how the dump_mmu() print
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> The various dump_mmu() take an fprintf()-like callback and a FILE * to >> pass to it, and so do their helper functions. Passing around callback >> and argument is rather tiresome. >> >> Most dump_mmu() are called only by the target's hmp_info_tlb(). These >> all pass monitor_printf() cast to fprintf_function and the current >> monitor cast to FILE *. >> >> SPARC's dump_mmu() gets also called from target/sparc/ldst_helper.c a >> few times #ifdef DEBUG_MMU. These calls pass fprintf() and stdout. > > You might want to fixup the DPRINTF_MMU in ldst_helper.c to also > use qemu_printf. Hmm. Makes DPRINTF_MMU() consistent with how dump_mmu() prints, but inconsistent with how the other DPRINTF_*() print. Dunno... All that debug code should really be tracepoints of course. I'll leave DPRINTF_MMU() alone unless a maintainer (cc'ed) wants me to update it. >> The type-punning is technically undefined behaviour, but works in >> practice. Clean up: drop the callback, and call qemu_printf() >> instead. >> >> Signed-off-by: Markus Armbruster > > Reviewed-by: Dr. David Alan Gilbert Thanks!
Re: [Qemu-devel] [PATCH 09/17] target: Simplify how the TARGET_cpu_list() print
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> The various TARGET_cpu_list() take an fprintf()-like callback and a >> FILE * to pass to it. Their callers (vl.c's main() via list_cpus(), >> bsd-user/main.c's main(), linux-user/main.c's main()) all pass >> fprintf() and stdout. Thus, the flexibility provided by the (rather >> tiresome) indirection isn't actually used. >> >> Drop the callback, and call qemu_fprintf() instead. > > Actually calling qemu_printf Typo, will fix. Thanks! >> Calling printf() would also work, but would make the code unsuitable >> for monitor context without making it simpler. > > Gernally OK; but just checking - are there any flag combos that will > mean this ends up with the result going down a monitor rather than > stdout, and will that upset something like libvirt that might be using > this to enumerate a cpu list? No. qemu_printf() prints to current monitor if we have one, else to stdout. Thus, it prints to stdout as long as !cur_mon. cur_mon is thread-local, and always set like this: Monitor *old_mon = cur_mon; cur_mon = ... non-null value ... ... do something ... cur-mon = old_mon; It's set and restored * in monitor_qmp_dispatch() around executing a QMP command * in monitor_read() around handling HMP input (this includes executing a command) * in qmp_human_monitor_command() around executing the HMP command (this is where monitors become nested) Therefore, cur_mon is null unless we're executing a QMP command, an HMP command, or are processing HMP input. Clearer now?
[Qemu-devel] [Bug 1574572] Re: config 20 sriov direct bond ports，vm create failed.
Can you please provide the complete QEMU command line when reporting QEMU bugs? Just dumping a log of Nova is not very useful for debugging QEMU problems. Thanks. ** Changed in: qemu Status: New => Incomplete -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1574572 Title: config 20 sriov direct bond ports，vm create failed. Status in QEMU: Incomplete Bug description: nova log： 2016-04-08 09:57:48.640 5057 INFO nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] report alarm_instance_shutoff success 2016-04-08 09:57:48.712 5057 INFO nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] [instance: d860169c- 0dac-448f-a644-01a9b200cebe] During _sync_instance_power_state the DB power_state (1) does not match the vm_power_state from the hypervisor (4). Updating power_state in the DB to match the hypervisor. 2016-04-08 09:57:48.791 5057 WARNING nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] [instance: d860169c- 0dac-448f-a644-01a9b200cebe] Instance shutdown by itself. Calling the heal_instance_state. Current vm_state: active, current task_state: None, original DB power_state: 1, current VM power_state: 4 2016-04-08 09:57:48.892 5057 INFO nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] alarm_notice_heal_event result:1,host_name:tfg120,instance_id :d860169c-0dac- 448f-a644-01a9b200cebe,instance_name:vfnicdirect,vm_state:active,power_state:shutdown,action:start 2016-04-08 09:57:48.997 5057 INFO nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] Refresh_instance_block_device_info:False 2016-04-08 09:57:48.998 5057 INFO nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] [instance: d860169c- 0dac-448f-a644-01a9b200cebe] Rebooting instance 2016-04-08 09:57:49.373 5057 WARNING nova.compute.manager [req- 4e1b4d70-62b6-4158-8413-3c9f226fd13b - - - - -] [instance: d860169c- 0dac-448f-a644-01a9b200cebe] trying to reboot a non-running instance: (state: 4 expected: 1) 2016-04-08 09:57:49.479 5057 INFO nova.virt.libvirt.driver [-] [instance: d860169c-0dac-448f-a644-01a9b200cebe] Instance destroyed successfully. libvirtd log： 2016-04-08 02:05:05.785+: 4778: info : qemuDomainDestroyFlags:2227 : Log: VM: name= instance-00b8 2016-04-08 02:05:16.156+: 4771: info : qemuDomainDefineXMLFlags:7576 : Creating domain 'instance-00b8' 2016-04-08 02:05:16.158+: 4773: info : qemuDomainCreateWithFlags:7448 : Log: VM: name= instance-00b8 2016-04-08 02:05:16.158+: 4773: info : qemuProcessStart:4412 : vm=0x7f19482fdb30 name=instance-00b8 id=-1 asyncJob=0 migrateFrom= stdin_fd=-1 stdin_path= snapshot=(nil) vmop=0 flags=0x1 2016-04-08 02:05:16.169+: 4773: info : virNetDevReplaceNetConfig:2541 : Replace Net Config of linkdev enp132s0f0, vf 28, macaddress 00:d1:d4:00:05:03, vlanid 1250, stateDir /var/run/libvirt/hostdevmgr 2016-04-08 02:05:16.169+: 4773: info : virNetDevReplaceNetConfig:2566 : Replace Vf Config of enp132s0f0, vf 28, vlanid 1250, stateDir /var/run/libvirt/hostdevmgr 2016-04-08 02:05:16.169+: 4773: info : virNetDevReplaceVfConfig:2390 : pflinkdev enp132s0f0, vf 28,vlanid 1250 2016-04-08 02:05:16.178+: 4773: info : virNetDevReplaceVfConfig:2428 : save oldmac 00:d1:d4:00:05:03, oldvlanid 1250 2016-04-08 02:05:16.178+: 4773: info : virNetDevSetVfConfig:2196 : ifname enp132s0f0,ifindex -1,vf 28,macaddress 00:d1:d4:00:05:03, vlanid 1250 qemu log： kvm_alloc_slot: no free slot available 2016-04-08 06:21:04.793+: shutting down To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1574572/+subscriptions
[Qemu-devel] [Bug 1543163] Re: VMs unable to boot from network with e1000 since 2.2
Looking through old bug tickets... can you still reproduce this issue with the latest version of QEMU? Or could we close this ticket nowadays? ** Changed in: qemu Status: New => Incomplete -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1543163 Title: VMs unable to boot from network with e1000 since 2.2 Status in QEMU: Incomplete Bug description: With KVM/QEMU version 1.4, using e1000 NIC, virtual machines boot from network alright; they get an IP from DHCP server (machine A), then connect to Microsoft System Center (machine B) and proceed to boot into Windows PE, install images etc. However, with versions at least 2.2 and 2.5, using same NIC, virtual machines fail to boot from network; they still get an IP from A, but then fail "contacting B using gateway (0.0.0.0). Different NICs do not help except for i82551, which however causes problems further down the road (presumably because there is no driver for that card in PE (based on Windows 7/8/10)). Note that actual physical machines work. Wireshark reveals this: QEMU 1.4 + Intel e1000 ** 1) client sends DHCP discover 2) machine A answers, offers an IP, gateway and itself as next server 3) machine B answers, offers no IP, but gateway and itself as next server 4) client requests the IP 5) machine A ACKs 6) client sends unicast request for ??? to B 7) B offers (unicast) a boot file (some .com) 8) client sends another unicast request for ??? to B 9) B again offers the boot file (then they chat a bit, B offers another file, client downloads some stuff via TFTP etc.) QEMU 2.2/2.5 + Intel e1000 ** 1) client sends DHCP discover 2) machine A answers, offers an IP, gateway and itself as next server 3) machine B answers, offers no IP, but gateway and itself as next server 4) client requests the IP 5) machine A ACKs 6) client sends _broadcast_ request for ??? to B 7) B offers (likewise broadcast) a boot file 8) client sends another request, this time unicast and claims "Client IP address = 0.0.0.0" (and it basically hangs) So, since some time after 1.4, QEMUlated machines with otherwise trusty e1000 can no longer boot from network and it might be due to different behaviour. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1543163/+subscriptions
[Qemu-devel] [Bug 1544524] Re: "info chardev" not showing the real port in use
Seems like this has been fixed in recent versions of QEMU again: $ qemu-system-x86_64 -serial telnet::0,server,nowait -nographic QEMU 3.1.93 monitor - type 'help' for more information (qemu) info chardev parallel0: filename=null serial0: filename=disconnected:telnet:0.0.0.0:41346,server compat_monitor0: filename=stdio ** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1544524 Title: "info chardev" not showing the real port in use Status in QEMU: Fix Released Bug description: With Qemu 2.1.2 === pharidos@uks2:~$ qemu-system-x86_64 -hda Disk.qcow2 -serial telnet::0,server,nowait -nographic QEMU 2.1.2 monitor - type 'help' for more information (qemu) info chardev parallel0: filename=null serial0: filename=telnet:0.0.0.0:44189,server ^^^ serial console is using port 44189 compat_monitor0: filename=stdio (qemu) With Qemu 2.5.0 === pharidos@uks2:~$ qemu-system-x86_64 -hda Disk.qcow2 -serial telnet::0,server,nowait -nographic QEMU 2.5.0 monitor - type 'help' for more information (qemu) info chardev parallel0: filename=null serial0: filename=disconnected:telnet::0,server ^ serial console port not shown compat_monitor0: filename=stdio (qemu) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1544524/+subscriptions
[Qemu-devel] [Bug 1811758] Re: virtio-rng backend should use getentropy() syscall when available
Feel free to send some patches to implement this! Alternatively, you could also try to write a mail to the virtio-rng maintainer (see the MAINTAINER file in the top directory of the sources), maybe he can help. ** Changed in: qemu Importance: Undecided => Wishlist -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1811758 Title: virtio-rng backend should use getentropy() syscall when available Status in QEMU: New Bug description: According to https://wiki.qemu.org/Features/VirtIORNG the default backend for `virtio-rng-pci` is `/dev/random`. Alternately, the user can point it to a different backend file, like `/dev/urandom`. However, both of these files have suboptimal behavior in one way or another, as documented in `random(7)`. Instead, the default behavior should be to pull the requested octets from the `getrandom()` system call, if available, called with no flags set. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1811758/+subscriptions
[Qemu-devel] [Bug 1824053] Re: Qemu-img convert appears to be stuck on aarch64 host with low probability
Marking this bug as fixed according to comment 2. ** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1824053 Title: Qemu-img convert appears to be stuck on aarch64 host with low probability Status in QEMU: Fix Released Bug description: Hi, I found a problem that qemu-img convert appears to be stuck on aarch64 host with low probability. The convert command line is "qemu-img convert -f qcow2 -O raw disk.qcow2 disk.raw ". The bt is below: Thread 2 (Thread 0x4b776e50 (LWP 27215)): #0 0x4a3f2994 in sigtimedwait () from /lib64/libc.so.6 #1 0x4a39c60c in sigwait () from /lib64/libpthread.so.0 #2 0xaae82610 in sigwait_compat (opaque=0xc5163b00) at util/compatfd.c:37 #3 0xaae85038 in qemu_thread_start (args=args@entry=0xc5163b90) at util/qemu_thread_posix.c:496 #4 0x4a3918bc in start_thread () from /lib64/libpthread.so.0 #5 0x4a492b2c in thread_start () from /lib64/libc.so.6 Thread 1 (Thread 0x4b573370 (LWP 27214)): #0 0x4a489020 in ppoll () from /lib64/libc.so.6 #1 0xaadaefc0 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=) at qemu_timer.c:391 #3 0xaadae014 in os_host_main_loop_wait (timeout=) at main_loop.c:272 #4 0xaadae190 in main_loop_wait (nonblocking=) at main_loop.c:534 #5 0xaad97be0 in convert_do_copy (s=0xdc32eb48) at qemu-img.c:1923 #6 0xaada2d70 in img_convert (argc=, argv=) at qemu-img.c:2414 #7 0xaad99ac4 in main (argc=7, argv=) at qemu-img.c:5305 The problem seems to be very similar to the phenomenon described by this patch (https://resources.ovirt.org/pub/ovirt-4.1/src/qemu-kvm-ev/0025-aio_notify-force-main-loop-wakeup-with-SIGIO-aarch64.patch), which force main loop wakeup with SIGIO. But this patch was reverted by the patch (http://ovirt.repo.nfrance.com/src/qemu-kvm-ev/kvm- Revert-aio_notify-force-main-loop-wakeup-with-SIGIO-.patch). The problem still seems to exist in aarch64 host. The qemu version I used is 2.8.1. The host version is 4.19.28-1.2.108.aarch64. Do you have any solutions to fix it? Thanks for your reply ! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1824053/+subscriptions
[Qemu-devel] [Bug 1788582] Re: Race condition during shutdown
Ok, so marking this bug as "fixed" according to Marc's comment.
** Changed in: qemu
Status: New => Fix Released
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1788582
Title:
Race condition during shutdown
Status in QEMU:
Fix Released
Bug description:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1788582/+subscriptions
[Qemu-devel] [PATCH v3] spapr: add splpar hcalls H_PROD, H_CONFER
These implementations have a few deficiencies that are noted, but are
good enough for Linux to use.
Signed-off-by: Nicholas Piggin
---
v3: Removed wrong comment about GPR3, drop H_JOIN for now (at least until
it is tested some more in Linux/KVM), and expand the comment about not
prod bit.
hw/ppc/spapr_hcall.c | 71
1 file changed, 71 insertions(+)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8a736797b9..8892ad008b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1065,6 +1065,74 @@ static target_ulong h_cede(PowerPCCPU *cpu,
SpaprMachineState *spapr,
return H_SUCCESS;
}
+static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+target_long target = args[0];
+CPUState *cs = CPU(cpu);
+
+/*
+ * This does not do a targeted yield or confer, but check the parameter
+ * anyway. -1 means confer to all/any other CPUs.
+ */
+if (target != -1 && !CPU(spapr_find_cpu(target))) {
+return H_PARAMETER;
+}
+
+/*
+ * PAPR calls for waiting until proded in this case (or presumably
+ * an external interrupt if MSR[EE]=1, without dispatch sequence count
+ * check.
+ */
+if (cpu == spapr_find_cpu(target)) {
+cs->halted = 1;
+cs->exception_index = EXCP_HALTED;
+cs->exit_request = 1;
+
+return H_SUCCESS;
+}
+
+/*
+ * This does not implement the dispatch sequence check that PAPR calls for,
+ * but PAPR also specifies a stronger implementation where the target must
+ * be run (or EE, or H_PROD) before H_CONFER returns. Without such a hard
+ * scheduling requirement implemented, there is no correctness reason to
+ * implement the dispatch sequence check.
+ */
+cs->exception_index = EXCP_YIELD;
+cpu_loop_exit(cs);
+
+return H_SUCCESS;
+}
+
+static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+target_long target = args[0];
+CPUState *cs;
+
+/*
+ * PAPR specifies there should be a prod flag should be associated with
+ * a vCPU, which gets set here, tested by H_CEDE, and cleared any time
+ * the vCPU is dispatched, including via preemption.
+ *
+ * We don't implement this because it is not used by Linux. The bit would
+ * be difficult or impossible to use properly because preemption can not
+ * be prevented so dispatch sequence count would have to somehow be used
+ * to detect it.
+ */
+
+cs = CPU(spapr_find_cpu(target));
+if (!cs) {
+return H_PARAMETER;
+}
+
+cs->halted = 0;
+qemu_cpu_kick(cs);
+
+return H_SUCCESS;
+}
+
static target_ulong h_rtas(PowerPCCPU *cpu, SpaprMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -1860,6 +1928,9 @@ static void hypercall_register_types(void)
/* hcall-splpar */
spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
spapr_register_hypercall(H_CEDE, h_cede);
+spapr_register_hypercall(H_CONFER, h_confer);
+spapr_register_hypercall(H_PROD, h_prod);
+
spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
/* processor register resource access h-calls */
--
2.20.1
[Qemu-devel] [Bug 1824853] Re: 4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] == off' failed
Returning -1 does not help because all that signals that the buffer is full.
We then flush the buffer and try again, assuming the at the buffer will not
fill.
Given that the buffer is usually many megabytes, this is reasonable.
We need something different to signal that the buffer is not full, but that
another offset has overflowed.
** Changed in: qemu
Status: New => Confirmed
** Changed in: qemu
Status: Confirmed => In Progress
** Changed in: qemu
Assignee: (unassigned) => Richard Henderson (rth)
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1824853
Title:
4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion
`s->gen_insn_end_off[num_insns] == off' failed
Status in QEMU:
In Progress
Bug description:
I tried to bootstrap and regtested gcc trunk (gcc svn rev 270278,
datestamp 20190411) inside my arm64-gentoo installation under qemu-
system-aarch64.
Qemu version was 4.0.0-rc3 and -cpu cortex-a57. Qemu configured with
only --target-list=aarch64-softmmu,aarch64-linux-user and compiled
using gcc "version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1~16.04)".
Executable created from gcc/testsuite/gcc.target/aarch64/advsimd-
intrinsics/vldX.c compiled with -O2 crashed the whole qemu-system.
To investigate a bit I also manually run
~/gcc/inst/trunk/bin/gcc
~/gcc/src/trunk/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vldX.c
with different options like:
-O0 -lm -o d0.exe
-O1 -lm -o d1.exe
-O2 -lm -o d2.exe
-O0 -static -lm -o s0.exe
-O1 -static -lm -o s1.exe
-O2 -static -lm -o s2.exe
So, now I have 6 different arm64 executables created with different
optimization levels. O0 and O1 versions run ok.
Three sN.exe static executables I've also tried in qemu user mode (with same
-cpu), no issue in user mode.
And inside qemu-system I can see that
running "d2.exe" (attached) gives:
tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] ==
off' failed.
And running "s2.exe" gives:
tcg/tcg.c:320: set_jmp_reset_offset: Assertion `s->tb_jmp_reset_offset[which]
== off' failed.
It seems like this test is an counter-example for logic that
"tcg_ctx->nb_ops < 4000" implies tcg will fit into 16-bit signed size
(see tcg_op_buf_full comments).
Richard's changes in abebf92597186 and 9f754620651d were not enough,
translation block must be smaller, or we have to find some proper way to bail
out when buffer overflows.
I don't know why this situation is not caught by code_gen_highwater logic in
tcg.c
I've also tried this "bail out" patch
diff --git a/tcg/tcg.c b/tcg/tcg.c
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -3949,7 +3949,8 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
size_t off = tcg_current_code_size(s);
s->gen_insn_end_off[num_insns] = off;
/* Assert that we do not overflow our stored offset. */
-assert(s->gen_insn_end_off[num_insns] == off);
+if (s->gen_insn_end_off[num_insns] != off)
+ return -1;
}
num_insns++;
for (i = 0; i < TARGET_INSN_START_WORDS; ++i) {
But then running "d2.exe" just hangs the whole qemu-system. It seems
that when tcg_gen_code return -1 (like in highwater logic mentioned
before), we just re-call it again and again.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1824853/+subscriptions
Re: [Qemu-devel] [PATCH v4] migration: do not rom_reset() during incoming migration
On Mon, Apr 08, 2019 at 04:42:13AM -0400, Catherine Ho wrote:
> Commit 18269069c310 ("migration: Introduce ignore-shared capability")
> addes ignore-shared capability to bypass the shared ramblock (e,g,
> membackend + numa node). It does good to live migration.
>
> As told by Yury,this commit expectes that QEMU doesn't write to guest RAM
> until VM starts, but it does on aarch64 qemu:
> Backtrace:
> 1 0x55f4a296dd84 in address_space_write_rom_internal () at
> exec.c:3458
> 2 0x55f4a296de3a in address_space_write_rom () at exec.c:3479
> 3 0x55f4a2d519ff in rom_reset () at hw/core/loader.c:1101
> 4 0x55f4a2d475ec in qemu_devices_reset () at hw/core/reset.c:69
> 5 0x55f4a2c90a28 in qemu_system_reset () at vl.c:1675
> 6 0x55f4a2c9851d in main () at vl.c:4552
>
> Actually, on arm64 virt marchine, ramblock "dtb" will be filled into ram
> druing rom_reset. In ignore-shared incoming case, this rom filling
> is not required since all the data has been stored in memory backend
> file.
>
> Further more, as suggested by Peter Xu, if we do rom_reset() now with
> these ROMs then the RAM data should be re-filled again too with the
> migration stream coming in.
>
> Fixes: commit 18269069c310 ("migration: Introduce ignore-shared
> capability")
> Suggested-by: Yury Kotov
> Suggested-by: Peter Xu
> Signed-off-by: Catherine Ho
Acked-by: Peter Xu
--
Peter Xu
[Qemu-devel] [Bug 1811758] Re: virtio-rng backend should use getentropy() syscall when available
any word on this? If this is not considered for adoption, i would like to know the reason, so that we can have better predictions about what to do for entropy in a QEMU system. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1811758 Title: virtio-rng backend should use getentropy() syscall when available Status in QEMU: New Bug description: According to https://wiki.qemu.org/Features/VirtIORNG the default backend for `virtio-rng-pci` is `/dev/random`. Alternately, the user can point it to a different backend file, like `/dev/urandom`. However, both of these files have suboptimal behavior in one way or another, as documented in `random(7)`. Instead, the default behavior should be to pull the requested octets from the `getrandom()` system call, if available, called with no flags set. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1811758/+subscriptions
Re: [Qemu-devel] Maintainers, please tell us how to boot your machines!
On Tue, Mar 12, 2019 at 10:36 AM Markus Armbruster wrote: > > Dear board code maintainers, > > This is a (rather late) follow-up to the last QEMU summit. Minutes[*]: > > * Deprecating unmaintained features (devices, targets, backends) in QEMU > >QEMU has a mechanism to deprecate features but there remains a lot of >old unmaintained code. Refactoring is hindered by untested legacy >code, so there is a desire to deprecate unmaintained features more >often. > >[...] > >We should require at least a minimal test for each board; if nobody >cares enough to come up with one, that board should be deprecated. > >[...] > >Also see the qemu-devel discussion about deprecating code: >https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html. > > That's a link to "Minutes of KVM Forum BoF on deprecating stuff". > Quote: > > * One obvious class of candidates for removal is machines we don't know >how to boot, or can't boot, say because we lack required firmware >and/or OS. > >Of course, "can boot" should be an automated test. As a first step >towards that, we should at least document how to boot each machine. >We're going to ask machine maintainers to do that. > > Let's get going on this. > > I gathered the machine types, mapped them to source files, which I fed > to get_maintainer.pl. Results are appended. If you're cc'ed, > MAINTAINERS fingers you for at least one machine type's source file. > Please tell us for all of them how to to a "meaningful" boot test. > > For now, what's "meaningful" is entirely up to you. Booting Linux > certainly is. > > Make sure to include a complete QEMU command line. If your QEMU command > line requires resources beyond the QEMU source tree and what we build > from it, please detail them, and provide download URLs as far as > possible. > > Goals for this exercise: > > * Gather information we need to cover more machines in our automated > testing. > > Related work: > [PATCH v4 00/19] Acceptance Tests: target architecture support > Message-Id: <20190312121150.8638-1-cr...@redhat.com> > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03881.html > > * Maybe identify a few machines we don't know how to boot anymore. > > Thanks in advance for your help! > > > > Machines with at least one maintainer: > > > = hw/arm/mcimx7d-sabre.c = > Peter Maydell (odd fixer:MCIMX7D SABRE / i...) > Andrey Smirnov (reviewer:MCIMX7D SABRE / i...) > qemu-...@nongnu.org (open list:MCIMX7D SABRE / i...) > Sorry I am late to this party. I haven't used i.MX7 emulation in a while and things didn't go smoothly out of the box, so I had to create a number of fixes (submitted to the ML). I use Linux kernel built using Buildroot for validation. Buildroot should have a default i.MX7 Sabred SD configuration and that should probably work. I usually change mine slightly to use compiled-in rootfs to simplify storage setup. In case this is helpful here's a number of commands I use to start my test cases: * PCIe (e1000 ethernet attached), USB (usb stick attached), SD: arm-softmmu/qemu-system-arm -smp 2 -m 1024 -machine mcimx7d-sabre -nographic -serial mon:stdio -kernel -dtb -device e1000e,bus="dw-pcie",netdev=lan0 -netdev tap,id=lan0,ifname=tap0,script=no,downscript=no -append "console=ttymxc0,115200 noinitrd" -usb -drive if=none,id=stick,file=,format=raw -device usb-storage,bus=usb-bus.0,drive=stick -drive id=sd1,if=sd,format=file,file= -drive id=sd2,if=sd,format=file,file= -driv eid=sd3,if=sd,format=file,file= * EHCI USB attached via PCIe with legacy interrupts, Ethernet connected to built-in controller: arm-softmmu/qemu-system-arm -smp 2 -m 1024 -machine mcimx7d-sabre -nographic -serial mon:stdio -kernel -dtb -device usb-ehci,id=ehci,bus="dw-pcie" -net nic,model=imx.fec,netdev=lan0 -netdev tap,id=lan0,ifname=tap0,script=no,downscript=no -append "console=ttymxc0,115200 noinitrd pci=nomsi" -usb -drive if=none,id=stick,file=,format=raw Also, I don't think anyone would try to do this, but just as a warning, building QEMU with --enable-tcg-interpreter doesn't really work that well (/init gets SIGKILLed every time), so I'd recommend avoiding using that option. Hopefully this is enough info, but if not, feel free to ask me more questions. Thanks, Andrey Smirnov
Re: [Qemu-devel] [PATCH v4] migration: do not rom_reset() during incoming migration
Ping, thanks
B.R.
Catherine
On Mon, 8 Apr 2019 at 16:43, Catherine Ho wrote:
> Commit 18269069c310 ("migration: Introduce ignore-shared capability")
> addes ignore-shared capability to bypass the shared ramblock (e,g,
> membackend + numa node). It does good to live migration.
>
> As told by Yury,this commit expectes that QEMU doesn't write to guest RAM
> until VM starts, but it does on aarch64 qemu:
> Backtrace:
> 1 0x55f4a296dd84 in address_space_write_rom_internal () at
> exec.c:3458
> 2 0x55f4a296de3a in address_space_write_rom () at exec.c:3479
> 3 0x55f4a2d519ff in rom_reset () at hw/core/loader.c:1101
> 4 0x55f4a2d475ec in qemu_devices_reset () at hw/core/reset.c:69
> 5 0x55f4a2c90a28 in qemu_system_reset () at vl.c:1675
> 6 0x55f4a2c9851d in main () at vl.c:4552
>
> Actually, on arm64 virt marchine, ramblock "dtb" will be filled into ram
> druing rom_reset. In ignore-shared incoming case, this rom filling
> is not required since all the data has been stored in memory backend
> file.
>
> Further more, as suggested by Peter Xu, if we do rom_reset() now with
> these ROMs then the RAM data should be re-filled again too with the
> migration stream coming in.
>
> Fixes: commit 18269069c310 ("migration: Introduce ignore-shared
> capability")
> Suggested-by: Yury Kotov
> Suggested-by: Peter Xu
> Signed-off-by: Catherine Ho
> ---
> hw/core/loader.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index fe5cb24122..040109464b 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1087,6 +1087,15 @@ static void rom_reset(void *unused)
> {
> Rom *rom;
>
> +/*
> + * We don't need to fill in the RAM with ROM data because we'll fill
> + * the data in during the next incoming migration in all cases. Note
> + * that some of those RAMs can actually be modified by the guest on
> ARM
> + * so this is probably the only right thing to do here.
> + */
> +if (runstate_check(RUN_STATE_INMIGRATE))
> +return;
> +
> QTAILQ_FOREACH(rom, &roms, next) {
> if (rom->fw_file) {
> continue;
> --
> 2.17.1
>
>
[Qemu-devel] [PATCH 4/5] pci: designware: Update MSI mapping when MSI address changes
MSI mapping needs to be update when MSI address changes, so add the code to do so. Signed-off-by: Andrey Smirnov Cc: Peter Maydell Cc: Michael S. Tsirkin Cc: qemu-devel@nongnu.org Cc: qemu-...@nongnu.org --- hw/pci-host/designware.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c index 6affe823c0..e80facc4a0 100644 --- a/hw/pci-host/designware.c +++ b/hw/pci-host/designware.c @@ -289,11 +289,13 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address, case DESIGNWARE_PCIE_MSI_ADDR_LO: root->msi.base &= 0xULL; root->msi.base |= val; +designware_pcie_root_update_msi_mapping(root); break; case DESIGNWARE_PCIE_MSI_ADDR_HI: root->msi.base &= 0xULL; root->msi.base |= (uint64_t)val << 32; +designware_pcie_root_update_msi_mapping(root); break; case DESIGNWARE_PCIE_MSI_INTR0_ENABLE: -- 2.20.1
[Qemu-devel] [PATCH 3/5] pci: designware: Update MSI mapping unconditionally
Expression to calculate update_msi_mapping in code handling writes to
DESIGNWARE_PCIE_MSI_INTR0_ENABLE is missing an ! operator and should
be:
!!root->msi.intr[0].enable ^ !!val;
so that MSI mapping is updated when enabled transitions from either
"none" -> "any" or "any" -> "none". Since that register shouldn't be
written to very often, change the code to update MSI mapping
unconditionally instead of trying to fix the update_msi_mapping logic.
Signed-off-by: Andrey Smirnov
Cc: Peter Maydell
Cc: Michael S. Tsirkin
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
hw/pci-host/designware.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 29ea313798..6affe823c0 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -296,16 +296,10 @@ static void designware_pcie_root_config_write(PCIDevice
*d, uint32_t address,
root->msi.base |= (uint64_t)val << 32;
break;
-case DESIGNWARE_PCIE_MSI_INTR0_ENABLE: {
-const bool update_msi_mapping = !root->msi.intr[0].enable ^ !!val;
-
+case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:
root->msi.intr[0].enable = val;
-
-if (update_msi_mapping) {
-designware_pcie_root_update_msi_mapping(root);
-}
+designware_pcie_root_update_msi_mapping(root);
break;
-}
case DESIGNWARE_PCIE_MSI_INTR0_MASK:
root->msi.intr[0].mask = val;
--
2.20.1
[Qemu-devel] [PATCH 2/5] i.mx7d: Add no-op/unimplemented PCIE PHY IP block
Add no-op/unimplemented PCIE PHY IP block. Needed by new kernels to
use PCIE.
Signed-off-by: Andrey Smirnov
Cc: Peter Maydell
Cc: Michael S. Tsirkin
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
include/hw/arm/fsl-imx7.h | 3 +++
hw/arm/fsl-imx7.c | 5 +
2 files changed, 8 insertions(+)
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index aae4f860fc..3efa697adc 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -125,6 +125,9 @@ enum FslIMX7MemoryMap {
FSL_IMX7_ADC2_ADDR= 0x3062,
FSL_IMX7_ADCn_SIZE= 0x1000,
+FSL_IMX7_PCIE_PHY_ADDR= 0x306D,
+FSL_IMX7_PCIE_PHY_SIZE= 0x1,
+
FSL_IMX7_GPC_ADDR = 0x303A,
FSL_IMX7_I2C1_ADDR= 0x30A2,
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 1abfa5910c..813fb55ca9 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -532,6 +532,11 @@ static void fsl_imx7_realize(DeviceState *dev, Error
**errp)
*/
create_unimplemented_device("dma-apbh", FSL_IMX7_DMA_APBH_ADDR,
FSL_IMX7_DMA_APBH_SIZE);
+/*
+ * PCIe PHY
+ */
+create_unimplemented_device("pcie-phy", FSL_IMX7_PCIE_PHY_ADDR,
+FSL_IMX7_PCIE_PHY_SIZE);
}
static void fsl_imx7_class_init(ObjectClass *oc, void *data)
--
2.20.1
[Qemu-devel] [PATCH 1/5] i.mx7d: Add no-op/unimplemented APBH DMA module
Instantiate no-op APBH DMA module. Needed to boot latest Linux kernel.
Signed-off-by: Andrey Smirnov
Cc: Peter Maydell
Cc: Michael S. Tsirkin
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
include/hw/arm/fsl-imx7.h | 3 +++
hw/arm/fsl-imx7.c | 6 ++
2 files changed, 9 insertions(+)
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index d848262bfd..aae4f860fc 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -179,6 +179,9 @@ enum FslIMX7MemoryMap {
FSL_IMX7_PCIE_REG_SIZE= 16 * 1024,
FSL_IMX7_GPR_ADDR = 0x3034,
+
+FSL_IMX7_DMA_APBH_ADDR= 0x3300,
+FSL_IMX7_DMA_APBH_SIZE= 0x2000,
};
enum FslIMX7IRQs {
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 7663ad6861..1abfa5910c 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -526,6 +526,12 @@ static void fsl_imx7_realize(DeviceState *dev, Error
**errp)
*/
create_unimplemented_device("lcdif", FSL_IMX7_LCDIF_ADDR,
FSL_IMX7_LCDIF_SIZE);
+
+/*
+ * DMA APBH
+ */
+create_unimplemented_device("dma-apbh", FSL_IMX7_DMA_APBH_ADDR,
+FSL_IMX7_DMA_APBH_SIZE);
}
static void fsl_imx7_class_init(ObjectClass *oc, void *data)
--
2.20.1
[Qemu-devel] [PATCH 5/5] i.mx7d: pci: Update PCI IRQ mapping to match HW
Datasheet for i.MX7 is incorrect and i.MX7's PCI IRQ mapping matches
that of i.MX6:
* INTD/MSI122
* INTC123
* INTB124
* INTA125
Fix all of the relevant code to reflect that fact. Needed by latest
Linux kernels.
Signed-off-by: Andrey Smirnov
Cc: Peter Maydell
Cc: Michael S. Tsirkin
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
include/hw/arm/fsl-imx7.h | 8
hw/pci-host/designware.c | 6 --
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 3efa697adc..9750003a4f 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -213,10 +213,10 @@ enum FslIMX7IRQs {
FSL_IMX7_USB2_IRQ = 42,
FSL_IMX7_USB3_IRQ = 40,
-FSL_IMX7_PCI_INTA_IRQ = 122,
-FSL_IMX7_PCI_INTB_IRQ = 123,
-FSL_IMX7_PCI_INTC_IRQ = 124,
-FSL_IMX7_PCI_INTD_IRQ = 125,
+FSL_IMX7_PCI_INTA_IRQ = 125,
+FSL_IMX7_PCI_INTB_IRQ = 124,
+FSL_IMX7_PCI_INTC_IRQ = 123,
+FSL_IMX7_PCI_INTD_IRQ = 122,
FSL_IMX7_UART7_IRQ= 126,
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index e80facc4a0..f4c58b25c1 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -50,6 +50,8 @@
#define DESIGNWARE_PCIE_ATU_DEVFN(x) (((x) >> 16) & 0xff)
#define DESIGNWARE_PCIE_ATU_UPPER_TARGET 0x91C
+#define DESIGNWARE_PCIE_IRQ_MSI3
+
static DesignwarePCIEHost *
designware_pcie_root_to_host(DesignwarePCIERoot *root)
{
@@ -66,7 +68,7 @@ static void designware_pcie_root_msi_write(void *opaque,
hwaddr addr,
root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
if (root->msi.intr[0].status & ~root->msi.intr[0].mask) {
-qemu_set_irq(host->pci.irqs[0], 1);
+qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 1);
}
}
@@ -310,7 +312,7 @@ static void designware_pcie_root_config_write(PCIDevice *d,
uint32_t address,
case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
root->msi.intr[0].status ^= val;
if (!root->msi.intr[0].status) {
-qemu_set_irq(host->pci.irqs[0], 0);
+qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 0);
}
break;
--
2.20.1
[Qemu-devel] [PATCH 0/5] Various i.MX7 fixes
Everyone:
I recently revisited my i.MX7 work and this series contains all of the
fixes I had to make to get it to work with latest (5.1-rc1) Linux
kernel again as well as fixes for genuine bugs that I somehow missed
during original submission ("pci: designware" patches). Hopefully each
patch is self-explanatory.
Feedback is welcome!
Thanks,
Andrey Smirnov
Andrey Smirnov (5):
i.mx7d: Add no-op/unimplemented APBH DMA module
i.mx7d: Add no-op/unimplemented PCIE PHY IP block
pci: designware: Update MSI mapping unconditionally
pci: designware: Update MSI mapping when MSI address changes
i.mx7d: pci: Update PCI IRQ mapping to match HW
include/hw/arm/fsl-imx7.h | 14 ++
hw/arm/fsl-imx7.c | 11 +++
hw/pci-host/designware.c | 18 --
3 files changed, 29 insertions(+), 14 deletions(-)
--
2.20.1
Re: [Qemu-devel] [RFC 3/3] RDMA/virtio-rdma: VirtIO rdma driver
On 4/11/19 4:01 AM, Yuval Shaia wrote:
> +++ b/drivers/infiniband/hw/virtio/Kconfig
> @@ -0,0 +1,6 @@
> +config INFINIBAND_VIRTIO_RDMA
> + tristate "VirtIO Paravirtualized RDMA Driver"
> + depends on NETDEVICES && ETHERNET && PCI && INET
> + ---help---
> + This driver provides low-level support for VirtIO Paravirtual
> + RDMA adapter.
Does this driver really depend on Ethernet, or does it also work with
Ethernet support disabled?
> +static inline struct virtio_rdma_info *to_vdev(struct ib_device *ibdev)
> +{
> + return container_of(ibdev, struct virtio_rdma_info, ib_dev);
> +}
Is it really worth to introduce this function? Have you considered to
use container_of(ibdev, struct virtio_rdma_info, ib_dev) directly instead
of to_vdev()?
> +static void rdma_ctrl_ack(struct virtqueue *vq)
> +{
> + struct virtio_rdma_info *dev = vq->vdev->priv;
> +
> + wake_up(&dev->acked);
> +
> + printk("%s\n", __func__);
> +}
Should that printk() be changed into pr_debug()? The same comment holds for
all other printk() calls.
> +#define VIRTIO_RDMA_BOARD_ID 1
> +#define VIRTIO_RDMA_HW_NAME "virtio-rdma"
> +#define VIRTIO_RDMA_HW_REV 1
> +#define VIRTIO_RDMA_DRIVER_VER "1.0"
Is a driver version number useful in an upstream driver?
> +struct ib_cq *virtio_rdma_create_cq(struct ib_device *ibdev,
> + const struct ib_cq_init_attr *attr,
> + struct ib_ucontext *context,
> + struct ib_udata *udata)
> +{
> + struct scatterlist in, out;
> + struct virtio_rdma_ib_cq *vcq;
> + struct cmd_create_cq *cmd;
> + struct rsp_create_cq *rsp;
> + struct ib_cq *cq = NULL;
> + int rc;
> +
> + /* TODO: Check MAX_CQ */
> +
> + cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> + if (!cmd)
> + return ERR_PTR(-ENOMEM);
> +
> + rsp = kmalloc(sizeof(*rsp), GFP_ATOMIC);
> + if (!rsp) {
> + kfree(cmd);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + vcq = kzalloc(sizeof(*vcq), GFP_KERNEL);
> + if (!vcq)
> + goto out;
Are you sure that you want to mix GFP_ATOMIC and GFP_KERNEL in a single
function?
Thanks,
Bart.
Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren
For any chip that has a CLINT, we want the frequency of the time register
and the frequency of the CLINT to match. That frequency,
SIFIVE_CLINT_TIMEBASE_FREQ
(=10MHz) is currently defined in hw/riscv/sifive_clint.h and so isn't
visible to target/riscv/cpu.c where the CPURISCVState is first created.
Instead, I first initialize the frequency to a reasonable default (1GHz)
and then let the CLINT override the value if one is attached. Phrased
differently, the values produced by the `sifive_clint.c:
cpu_riscv_read_rtc()` and `csr.c: read_time()` must match, and this is one
way of doing that.
I'd be open to other suggestions.
Jonathan
On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis
wrote:
> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens
> wrote:
> >
> > Currently mcounteren.TM acts as though it is hardwired to zero, even
> though
> > QEMU
> > allows it to be set. This change resolves the issue by allowing reads to
> the
> > time and timeh control registers when running in a privileged mode where
> > such
> > accesses are allowed.
> >
> > Signed-off-by: Jonathan Behrens
> > ---
> > hw/riscv/sifive_clint.c | 1 +
> > target/riscv/cpu.c | 14 ++
> > target/riscv/cpu.h | 2 ++
> > target/riscv/csr.c | 17 +++--
> > 4 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> > index d4c159e937..3ad4fe6139 100644
> > --- a/hw/riscv/sifive_clint.c
> > +++ b/hw/riscv/sifive_clint.c
> > @@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr
> > size, uint32_t num_harts,
> > env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >&sifive_clint_timer_cb, cpu);
> > env->timecmp = 0;
> > +env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ;
>
> Why do you need to set this here?
>
> Alistair
>
> > }
> >
> > DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT);
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index d61bce6d55..ff17d54691 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env, int
> > resetvec)
> > #endif
> > }
> >
> > +static void set_time_freq(CPURISCVState *env, uint64_t freq)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +env->time_freq = freq;
> > +#endif
> > +}
> > +
> > static void riscv_any_cpu_init(Object *obj)
> > {
> > CPURISCVState *env = &RISCV_CPU(obj)->env;
> > set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > set_resetvec(env, DEFAULT_RSTVEC);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > #if defined(TARGET_RISCV32)
> > @@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > @@ -131,6 +140,7 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv32imacu_nommu_cpu_init(Object *obj)
> > @@ -140,6 +150,7 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > #elif defined(TARGET_RISCV64)
> > @@ -152,6 +163,7 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > @@ -162,6 +174,7 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv64imacu_nommu_cpu_init(Object *obj)
> > @@ -171,6 +184,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > #endif
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 20bce8742e..67b1769ad3 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -173,7 +173,9 @@ struct CPURISCVState {
> > /* temporary htif re
Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren
On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens wrote:
>
> Currently mcounteren.TM acts as though it is hardwired to zero, even though
> QEMU
> allows it to be set. This change resolves the issue by allowing reads to the
> time and timeh control registers when running in a privileged mode where
> such
> accesses are allowed.
>
> Signed-off-by: Jonathan Behrens
> ---
> hw/riscv/sifive_clint.c | 1 +
> target/riscv/cpu.c | 14 ++
> target/riscv/cpu.h | 2 ++
> target/riscv/csr.c | 17 +++--
> 4 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> index d4c159e937..3ad4fe6139 100644
> --- a/hw/riscv/sifive_clint.c
> +++ b/hw/riscv/sifive_clint.c
> @@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr
> size, uint32_t num_harts,
> env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>&sifive_clint_timer_cb, cpu);
> env->timecmp = 0;
> +env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ;
Why do you need to set this here?
Alistair
> }
>
> DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT);
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d61bce6d55..ff17d54691 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env, int
> resetvec)
> #endif
> }
>
> +static void set_time_freq(CPURISCVState *env, uint64_t freq)
> +{
> +#ifndef CONFIG_USER_ONLY
> +env->time_freq = freq;
> +#endif
> +}
> +
> static void riscv_any_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> set_resetvec(env, DEFAULT_RSTVEC);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> #if defined(TARGET_RISCV32)
> @@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_MMU);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> @@ -131,6 +140,7 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_MMU);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> static void rv32imacu_nommu_cpu_init(Object *obj)
> @@ -140,6 +150,7 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> #elif defined(TARGET_RISCV64)
> @@ -152,6 +163,7 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_MMU);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> @@ -162,6 +174,7 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_MMU);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> static void rv64imacu_nommu_cpu_init(Object *obj)
> @@ -171,6 +184,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> set_resetvec(env, DEFAULT_RSTVEC);
> set_feature(env, RISCV_FEATURE_PMP);
> +set_time_freq(env, NANOSECONDS_PER_SECOND);
> }
>
> #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 20bce8742e..67b1769ad3 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -173,7 +173,9 @@ struct CPURISCVState {
> /* temporary htif regs */
> uint64_t mfromhost;
> uint64_t mtohost;
> +
> uint64_t timecmp;
> +uint64_t time_freq;
>
> /* physical memory protection */
> pmp_table_t pmp_state;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e1d91b6c60..6083c782a1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -191,22 +191,31 @@ static int read_instreth(CPURISCVState *env, int
> csrno, target_ulong *val)
> }
> #endif /* TARGET_RISCV32 */
>
> -#if defined(CONFIG_USER_ONLY)
> static int read_time(CPURISCVState *env, int csrno, target_ulong *val)
> {
> +#if !defined(CONFIG_USER_ONLY)
> +*val = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +env->time_freq, NANOSECONDS_PER_SECOND);
> +#else
> *val = cpu_get_host_ticks();
> +#endif
> return 0;
> }
>
> #if defined(TARGET_RISCV32)
> static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
Re: [Qemu-devel] [PATCH for 4.1 v3 2/6] target/riscv: Fall back to generating a RISC-V CPU
On Mon, Apr 15, 2019 at 1:38 AM Igor Mammedov wrote:
>
> On Fri, 12 Apr 2019 14:19:16 -0700
> Alistair Francis wrote:
>
...
> > >
> > > if it's programming error it should be assert
> > > but in general instance_init() should never fail.
> >
> > We are checking for a user specified CPU string, not a programming
> > error so an assert() isn't correct here.
> >
> > If *init() can't fail how should this be handled?
> typical flow for device object is
>
> object_new(foo) -> foo_init_fn()
> set properties on foo
> set 'realize' property to 'true' -> foo_realize_fn()
>
> all checks should be performed in realize() or during property setting.
I thought there was a reason realise didn't work, but now I can't see
what it is. I can move it to realise.
>
> > > the same applies to errors or warnings within this function.
> > >
> > > > > > +}
> > > > > > +
> > > > > > +if (riscv_cpu[0] == 'r' && riscv_cpu[1] == 'v') {
> > > > > > +/* Starts with "rv" */
> > > > > > +if (riscv_cpu[2] == '3' && riscv_cpu[3] == '2') {
> > > > > > +valid = true;
> > > > > > +rvxlen = RV32;
> > > > > > +}
> > > > > > +if (riscv_cpu[2] == '6' && riscv_cpu[3] == '4') {
> > > > > > +valid = true;
> > > > > > +rvxlen = RV64;
> > > > > > +}
> > > > > > +}
> > > > > > +
> > > > > > +if (!valid) {
> > > > > > +error_report("'%s' does not appear to be a valid RISC-V
> > > > > > CPU",
> > > > > > + riscv_cpu);
> > > > > > +exit(1);
> > > > > > +}
> > > > > > +
> > > > > > +for (i = 4; i < strlen(riscv_cpu); i++) {
> > > > > > +switch (riscv_cpu[i]) {
> > > > > > +case 'i':
> > > > > > +if (target_misa & RVE) {
> > > > > > +error_report("I and E extensions are
> > > > > > incompatible");
> > > > > > +exit(1);
> > > > > > +}
> > > > > > +target_misa |= RVI;
> > > > > > +continue;
> > > > > > +case 'e':
> > > > > > +if (target_misa & RVI) {
> > > > > > +error_report("I and E extensions are
> > > > > > incompatible");
> > > > > > +exit(1);
> > > > > > +}
> > > > > > +target_misa |= RVE;
> > > > > > +continue;
> > > > > > +case 'g':
> > > > > > +target_misa |= RVI | RVM | RVA | RVF | RVD;
> > > > > > +continue;
> > > > > > +case 'm':
> > > > > > +target_misa |= RVM;
> > > > > > +continue;
> > > > > > +case 'a':
> > > > > > +target_misa |= RVA;
> > > > > > +continue;
> > > > > > +case 'f':
> > > > > > +target_misa |= RVF;
> > > > > > +continue;
> > > > > > +case 'd':
> > > > > > +target_misa |= RVD;
> > > > > > +continue;
> > > > > > +case 'c':
> > > > > > +target_misa |= RVC;
> > > > > > +continue;
> > > > > > +case 's':
> > > > > > +target_misa |= RVS;
> > > > > > +continue;
> > > > > > +case 'u':
> > > > > > +target_misa |= RVU;
> > > > > > +continue;
> > > > > > +default:
> > > > > > +warn_report("QEMU does not support the %c extension",
> > > > > > +riscv_cpu[i]);
> > > that's what looks to me like fallback, and what makes
> > > CPU features for this CPU type non deterministic.
> >
> > What do you mean? QEMU will always parse the specified CPU string and
> > create a CPU based on the features specified by the user. For each
> > version of QEMU it will always result in the same outcome for the same
> > user supplied command line argument.
>
> I've meant that here you would print warning and happily continue parsing
> user input ignoring input error. One should error out instead and make
> user fix error.
Ah, ok. I can change this one to an error.
>
> >
> > Newer QEMU versions will support more features though, so they will
> > accept different options.
> >
> > > I'm not sure what you are trying to achieve here, but it look like
> > > you are trying to verify MachineClass field in object constructor
> > > along with other things.
> >
> > I'm just trying to parse the -cpu string option.
> >
> > >
> > > I'd put all MachineClass checks in class_init and abort on error
> > > since invalid mcc->isa_str is codding error.
> >
> > No, it's a user error.
>
> Parsing -cpu by assigning to class members user input looks unusual.
>
> We had a custom parsing for x86 & sparc cpus, but that was factored out
> into compat utilities and we shouldn't do it anywhere else without very
> compelling reason.
I did see that, but it didn't seem to work the same way as this.
>
> Currently it's preferred to use canonical form for cpu properties, like:
>
> -cpu foo,prop1=x,prop2=y,...
Yeah, but I don't see how that would work nicely.
Instea
[Qemu-devel] [Bug 1805256] Re: qemu-img hangs on high core count ARM system
No, sorry - this bugs still persists w/ latest upstream (@ afccfc0). I found a report of similar symptoms: https://patchwork.kernel.org/patch/10047341/ https://bugzilla.redhat.com/show_bug.cgi?id=1524770#c13 To be clear, ^ is already fixed upstream, so it is not the *same* issue - but perhaps related. ** Bug watch added: Red Hat Bugzilla #1524770 https://bugzilla.redhat.com/show_bug.cgi?id=1524770 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: Confirmed Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
Re: [Qemu-devel] Following up questions related to QEMU and I/O Thread
On 4/16/19 1:34 AM, Wei Li wrote: > Hi @Paolo Bonzini & @Stefan Hajnoczi, > > Would you please help confirm whether @Paolo Bonzini's multiqueue feature > change will benefit virtio-scsi or not? Thanks! > > @Stefan Hajnoczi, > I also spent some time on exploring the virtio-scsi multi-queue features via > num_queues parameter as below, here are what we found: > > 1. Increase number of Queues from one to the same number as CPU will get > better IOPS increase. > 2. Increase number of Queues to the number (e.g. 8) larger than the number of > vCPU (e.g. 2) can get even better IOPS increase. As mentioned in below link, when the number of hw queues is larger than nr_cpu_ids, the blk-mq layer would limit and only use at most nr_cpu_ids queues (e.g., /sys/block/sda/mq/). That is, when the num_queus=4 while vcpus is 2, there should be only 2 queues available /sys/block/sda/mq/ https://lore.kernel.org/lkml/1553682995-5682-1-git-send-email-dongli.zh...@oracle.com/ I am just curious how increasing the num_queues from 2 to 4 would double the iops, while there are only 2 vcpus available... Dongli Zhang > > In addition, It seems Qemu can get better IOPS while the attachment uses more > queues than the number of vCPU, how could it possible? Could you please help > us better understand the behavior? Thanks a lot! > > > Host CPU Configuration: > CPU(s):2 > Thread(s) per core:2 > Core(s) per socket:1 > Socket(s): 1 > > Commands for multi queue Setup: > (QEMU) device_add driver=virtio-scsi-pci num_queues=1 id=test1 > (QEMU) device_add driver=virtio-scsi-pci num_queues=2 id=test2 > (QEMU) device_add driver=virtio-scsi-pci num_queues=4 id=test4 > (QEMU) device_add driver=virtio-scsi-pci num_queues=8 id=test8 > > > Result: > | 8 Queues | 4 Queues | 2 Queues| Single Queue > IOPS | +29% | 27% |11% | > Baseline > > Thanks, > Wei > > On 4/5/19, 2:09 PM, "Wei Li" wrote: > > Thanks Stefan for your quick response! > > Hi Paolo, > Could you please send us a link related to the multiqueue feature which > you are working on so that we could start getting some details about the > feature. > > Thanks again, > Wei > > On 4/1/19, 3:54 AM, "Stefan Hajnoczi" wrote: > > On Fri, Mar 29, 2019 at 08:16:36AM -0700, Wei Li wrote: > > Thanks Stefan for your reply and guidance! > > > > We spent some time on exploring the multiple I/O Threads approach > per your feedback. Based on the perf measurement data, we did see some IOPS > improvement for multiple volumes, which is great. :) > > > > In addition, IOPS for single Volume will still be a bottleneck, it > seems like multiqueue block layer feature which Paolo is working on may be > able to help improving the IOPS for single volume. > > > > @Paolo, @Stefan, > > Would you mind sharing the multiqueue feature code branch with us? > So that we could get some rough idea about this feature and maybe start doing > some exploration? > > Paolo last worked on this code, so he may be able to send you a link. > > Stefan > > > > >
[Qemu-devel] [Bug 1805256] Re: qemu-img hangs on high core count ARM system
** Changed in: qemu Status: New => Confirmed -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: Confirmed Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
Re: [Qemu-devel] recent QEMU-w64 does not load multiboot kernels by default
On Mon, Apr 15, 2019 at 3:20 PM Max Filippov wrote: > recent qemu-system-i386 for w64 (e.g. one installed from > https://qemu.weilnetz.de/w64/qemu-w64-setup-20190218.exe ) > refuses to load multiboot kernel by default: > > "C:\Program Files\qemu\qemu-system-i386.exe" -kernel p:\tmp\kernel > C:\Program Files\qemu\qemu-system-i386.exe: Error loading > uncompressed kernel without PVH ELF Note Example kernel is available here: http://jcmvbkbc.spb.ru/~dumb/tmp/201904151526/kernel -- Thanks. -- Max
[Qemu-devel] recent QEMU-w64 does not load multiboot kernels by default
Hello, recent qemu-system-i386 for w64 (e.g. one installed from https://qemu.weilnetz.de/w64/qemu-w64-setup-20190218.exe ) refuses to load multiboot kernel by default: "C:\Program Files\qemu\qemu-system-i386.exe" -kernel p:\tmp\kernel C:\Program Files\qemu\qemu-system-i386.exe: Error loading uncompressed kernel without PVH ELF Note Adding -M pc-i440fx-3.1 (as well as most other machines, but not pc-i440fx-4.0) fixes that. On linux host the same kernel gets loaded either way (with or without -M). -- Thanks. -- Max
Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote: > Add a new base CPU model called 'Dhyana' to model processors from Hygon > Dhyana(family 18h), which derived from AMD EPYC(family 17h). > > The following features bits have been removed compare to AMD EPYC: > aes, pclmulqdq, sha_ni > > The Hygon Dhyana support to KVM in Linux is already accepted upstream[1]. > So add Hygon Dhyana support to Qemu is necessary to create Hygon's own > CPU model. > > Reference: > [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87 > > Signed-off-by: Pu Wen Thanks for the patch. I'm wondering if we should let the CPU model be used only on Hygon hosts, to avoid confusion. -- Eduardo
[Qemu-devel] [Bug 1823458] Re: race condition between vhost_net_stop and CHR_EVENT_CLOSED on shutdown crashes qemu
** Description changed: [impact] on shutdown of a guest, there is a race condition that results in qemu crashing instead of normally shutting down. The bt looks similar to this (depending on the specific version of qemu, of course; this is taken from 2.5 version of qemu): (gdb) bt #0 __GI___pthread_mutex_lock (mutex=0x0) at ../nptl/pthread_mutex_lock.c:66 #1 0x5636c0bc4389 in qemu_mutex_lock (mutex=mutex@entry=0x0) at /build/qemu-7I4i1R/qemu-2.5+dfsg/util/qemu-thread-posix.c:73 #2 0x5636c0988130 in qemu_chr_fe_write_all (s=s@entry=0x0, buf=buf@entry=0x7ffe65c086a0 "\v", len=len@entry=20) at /build/qemu-7I4i1R/qemu-2.5+dfsg/qemu-char.c:205 #3 0x5636c08f3483 in vhost_user_write (msg=msg@entry=0x7ffe65c086a0, fds=fds@entry=0x0, fd_num=fd_num@entry=0, dev=0x5636c1bf6b70, dev=0x5636c1bf6b70) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/virtio/vhost-user.c:195 #4 0x5636c08f411c in vhost_user_get_vring_base (dev=0x5636c1bf6b70, ring=0x7ffe65c087e0) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/virtio/vhost-user.c:364 #5 0x5636c08efff0 in vhost_virtqueue_stop (dev=dev@entry=0x5636c1bf6b70, vdev=vdev@entry=0x5636c2853338, vq=0x5636c1bf6d00, idx=1) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/virtio/vhost.c:895 #6 0x5636c08f2944 in vhost_dev_stop (hdev=hdev@entry=0x5636c1bf6b70, vdev=vdev@entry=0x5636c2853338) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/virtio/vhost.c:1262 #7 0x5636c08db2a8 in vhost_net_stop_one (net=0x5636c1bf6b70, dev=dev@entry=0x5636c2853338) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/net/vhost_net.c:293 #8 0x5636c08dbe5b in vhost_net_stop (dev=dev@entry=0x5636c2853338, ncs=0x5636c209d110, total_queues=total_queues@entry=1) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/net/vhost_net.c:371 #9 0x5636c08d7745 in virtio_net_vhost_status (status=7 '\a', n=0x5636c2853338) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/net/virtio-net.c:150 #10 virtio_net_set_status (vdev=, status=) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/net/virtio-net.c:162 #11 0x5636c08ec42c in virtio_set_status (vdev=0x5636c2853338, val=) at /build/qemu-7I4i1R/qemu-2.5+dfsg/hw/virtio/virtio.c:624 #12 0x5636c098fed2 in vm_state_notify (running=running@entry=0, state=state@entry=RUN_STATE_SHUTDOWN) at /build/qemu-7I4i1R/qemu-2.5+dfsg/vl.c:1605 #13 0x5636c089172a in do_vm_stop (state=RUN_STATE_SHUTDOWN) at /build/qemu-7I4i1R/qemu-2.5+dfsg/cpus.c:724 #14 vm_stop (state=RUN_STATE_SHUTDOWN) at /build/qemu-7I4i1R/qemu-2.5+dfsg/cpus.c:1407 #15 0x5636c085d240 in main_loop_should_exit () at /build/qemu-7I4i1R/qemu-2.5+dfsg/vl.c:1883 #16 main_loop () at /build/qemu-7I4i1R/qemu-2.5+dfsg/vl.c:1931 #17 main (argc=, argv=, envp=) at /build/qemu-7I4i1R/qemu-2.5+dfsg/vl.c:4683 [test case] unfortunately since this is a race condition, it's very hard to arbitrarily reproduce; it depends very much on the overall configuration of the guest as well as how exactly it's shut down - specifically, its vhost user net must be closed from the host side at a specific time during qemu shutdown. I have someone with such a setup who has reported to me their setup is able to reproduce this reliably, but the config is too complex for me to reproduce so I have relied on their reproduction and testing to debug and craft the patch for this. [regression potential] - the change adds flags to prevent repeated calls to both vhost_net_stop() - and vhost_net_cleanup() (really, prevents repeated calls to - vhost_dev_cleanup(), but vhost_net_cleanup() does nothing else). Any - regression would be seen when stopping and/or cleaning up a vhost net. - Regressions might include failure to hot-remove a vhost net from a - guest, or failure to cleanup (i.e. mem leak), or crashes during cleanup - or stopping a vhost net. - - However, the flags are very unintrusive, and only in the shutdown path - (of a vhost_dev or vhost_net), and are unlikely to cause any - regressions. + the change adds a flag to prevent repeated calls to vhost_net_stop(). + This also prevents any calls to vhost_net_cleanup() from + net_vhost_user_event(). Any regression would be seen when stopping + and/or cleaning up a vhost net. Regressions might include failure to + hot-remove a vhost net from a guest, or failure to cleanup (i.e. mem + leak), or crashes during cleanup or stopping a vhost net. [other info] this was originally seen in the 2.5 version of qemu - specifically, the UCA version in trusty-mitaka (which uses the xenial qemu codebase). However, this appears to still apply upstream, and I am sending a patch to the qemu list to patch upstream as well. The specific race condition for this (in the qemu 2.5 code version) is: as shown in above bt, thread A starts shutting down qemu, e.g.: vm_stop->do_vm_stop->vm_state_notify virtio_set_status virtio_net_set_status virtio_net_vhost_status in this function, code gets to
[Qemu-devel] [Bug 1824053] Re: Qemu-img convert appears to be stuck on aarch64 host with low probability
Hi, unfortunately a lot has changed from 2.8 and it might be hard to identify a single individual fix that may be responsible for this; there are aio_context fixes that go in nearly every version. It may be quickest (unfortunately) to start git-bisecting the problem to see if you can identify which build alleviates the behavior to see if it isn't something you can backport directly -- but you might find that this particular fix has a lot of requisites and you might find it difficult to backport to 2.8.1. Best of luck, --js -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1824053 Title: Qemu-img convert appears to be stuck on aarch64 host with low probability Status in QEMU: New Bug description: Hi, I found a problem that qemu-img convert appears to be stuck on aarch64 host with low probability. The convert command line is "qemu-img convert -f qcow2 -O raw disk.qcow2 disk.raw ". The bt is below: Thread 2 (Thread 0x4b776e50 (LWP 27215)): #0 0x4a3f2994 in sigtimedwait () from /lib64/libc.so.6 #1 0x4a39c60c in sigwait () from /lib64/libpthread.so.0 #2 0xaae82610 in sigwait_compat (opaque=0xc5163b00) at util/compatfd.c:37 #3 0xaae85038 in qemu_thread_start (args=args@entry=0xc5163b90) at util/qemu_thread_posix.c:496 #4 0x4a3918bc in start_thread () from /lib64/libpthread.so.0 #5 0x4a492b2c in thread_start () from /lib64/libc.so.6 Thread 1 (Thread 0x4b573370 (LWP 27214)): #0 0x4a489020 in ppoll () from /lib64/libc.so.6 #1 0xaadaefc0 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=) at qemu_timer.c:391 #3 0xaadae014 in os_host_main_loop_wait (timeout=) at main_loop.c:272 #4 0xaadae190 in main_loop_wait (nonblocking=) at main_loop.c:534 #5 0xaad97be0 in convert_do_copy (s=0xdc32eb48) at qemu-img.c:1923 #6 0xaada2d70 in img_convert (argc=, argv=) at qemu-img.c:2414 #7 0xaad99ac4 in main (argc=7, argv=) at qemu-img.c:5305 The problem seems to be very similar to the phenomenon described by this patch (https://resources.ovirt.org/pub/ovirt-4.1/src/qemu-kvm-ev/0025-aio_notify-force-main-loop-wakeup-with-SIGIO-aarch64.patch), which force main loop wakeup with SIGIO. But this patch was reverted by the patch (http://ovirt.repo.nfrance.com/src/qemu-kvm-ev/kvm- Revert-aio_notify-force-main-loop-wakeup-with-SIGIO-.patch). The problem still seems to exist in aarch64 host. The qemu version I used is 2.8.1. The host version is 4.19.28-1.2.108.aarch64. Do you have any solutions to fix it? Thanks for your reply ! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1824053/+subscriptions
Re: [Qemu-devel] curses.c: "We need a terminal output" ?
On 4/13/19 5:02 AM, Joachim Durchholz wrote: > Hi all, > > what's the reasoning behind "We need a terminal output" in curses.c? > > I don't really see a scenario where it's problematic if qemu emits > escape sequences to a pipe. > I have experienced one scenario where it is problematic: It doesn't work > properly with pexpect and expectit (a Python and a Java library that do > expect). I suspect they fail to use a pty, but then I see no harm in not > doing so. > I can see another scenario when this is problematic: Unit testing code > that directly writes to the console. E.g. testing BIOS code, or testing > operating systems when using the serial port is not an option. > > Regards, > Jo >
Re: [Qemu-devel] Booting from a Bootcamp partition
On 4/15/19 6:23 AM, Programmingkid wrote: > >> On Apr 15, 2019, at 5:54 AM, Stefan Hajnoczi wrote: >> >> On Sun, Apr 14, 2019 at 07:33:17PM -0400, Programmingkid wrote: >>> Hi I was wondering if anyone has been able to boot from a bootcamp >>> partition inside of QEMU. I know this partition can be used in QEMU but my >>> own attempts at booting Windows 7 on my bootcamp partition did not work. I >>> always see "A disk read error occurred". Has anyone else been successful at >>> this? >> >> Hi, >> It's difficult to help without more information: >> 1. What is your QEMU command-line? > > sudo qemu-system-x86_64 -name "Windows 7" -hda "/dev/disk0s4" -boot "c" -m > 3000 > >> 2. What is the partition table on the disk? > > The bootcamp partition is this: /dev/disk0s4 > (Are you trying to boot a partition as an entire block device ...?) >> General problems with booting Windows are usually caused by a guest >> configuration that doesn't match the hardware configuration that Windows >> was installed under. Can you check that the AHCI bus address of the >> disk and partition numbering matches what Windows expected? > > How would someone do this? > >> It may also be useful to enable tracing (see docs/devel/tracing.txt) to >> see what the ahci_* trace event log says. I have CCed John Snow, one of >> the few people who can read this log :). > > I'm guessing you believe the problem is the partition can't be read in QEMU. > I know for sure it can. I attached this partition to another QEMU VM and was > able to see it on the Desktop. > Well, we don't know what the problem is. From the command line above it looks like it'd be using the legacy IDE emulation instead of the newer SATA emulation, though. I'd wager that the Windows boot loader here is not expecting to use IDE. "disk read error" could mean a lot of things from the POV of a guest, but having disk emulation tracing would show us what the guest is trying to do, at least. > My guess is the computer is using an EFI firmware and QEMU uses the > traditional BIOS firmware (SeaBIOS). So I think trying UEFI in QEMU might > work. > Try using -M q35 which will engage SATA and AHCI emulation, and try using UEFI, yes.
Re: [Qemu-devel] [PULL 0/1] slirp gcc9 build fix
Patchew URL: https://patchew.org/QEMU/20190415180618.1450-1-samuel.thiba...@ens-lyon.org/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190415180618.1450-1-samuel.thiba...@ens-lyon.org Subject: [Qemu-devel] [PULL 0/1] slirp gcc9 build fix Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update]patchew/20190411152520.10061-1-arm...@redhat.com -> patchew/20190411152520.10061-1-arm...@redhat.com * [new tag] patchew/20190415180618.1450-1-samuel.thiba...@ens-lyon.org -> patchew/20190415180618.1450-1-samuel.thiba...@ens-lyon.org Switched to a new branch 'test' 7e45fd0bf6 slirp: Gcc 9 -O3 fix === OUTPUT BEGIN === ERROR: code indent should never use tabs #24: FILE: slirp/src/socket.c:174: +^Isize_t buf_len;$ ERROR: code indent should never use tabs #33: FILE: slirp/src/socket.c:185: +^Ibuf_len = sopreprbuf(so, iov, &n);$ ERROR: code indent should never use tabs #34: FILE: slirp/src/socket.c:186: +^Iassert(buf_len != 0);$ ERROR: code indent should never use tabs #42: FILE: slirp/src/socket.c:262: +^Iassert(size > 0);$ total: 4 errors, 0 warnings, 23 lines checked Commit 7e45fd0bf690 (slirp: Gcc 9 -O3 fix) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190415180618.1450-1-samuel.thiba...@ens-lyon.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[Qemu-devel] [PULL 1/1] slirp: Gcc 9 -O3 fix
From: "Dr. David Alan Gilbert"
Gcc 9 needs some convincing that sopreprbuf really is going to fill
in iov in the call from soreadbuf, even though the failure case
shouldn't happen.
Signed-off-by: Dr. David Alan Gilbert
Message-Id: <20190415121740.9881-1-dgilb...@redhat.com>
Signed-off-by: Samuel Thibault
---
slirp/src/socket.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/slirp/src/socket.c b/slirp/src/socket.c
index 4a3c935e25..bb752fdcae 100644
--- a/slirp/src/socket.c
+++ b/slirp/src/socket.c
@@ -171,6 +171,7 @@ int
soread(struct socket *so)
{
int n, nn;
+ size_t buf_len;
struct sbuf *sb = &so->so_snd;
struct iovec iov[2];
@@ -181,7 +182,8 @@ soread(struct socket *so)
* No need to check if there's enough room to read.
* soread wouldn't have been called if there weren't
*/
- sopreprbuf(so, iov, &n);
+ buf_len = sopreprbuf(so, iov, &n);
+ assert(buf_len != 0);
nn = recv(so->s, iov[0].iov_base, iov[0].iov_len,0);
if (nn <= 0) {
@@ -257,6 +259,7 @@ int soreadbuf(struct socket *so, const char *buf, int size)
* No need to check if there's enough room to read.
* soread wouldn't have been called if there weren't
*/
+ assert(size > 0);
if (sopreprbuf(so, iov, &n) < size)
goto err;
--
2.20.1
Re: [Qemu-devel] [PATCH v3] slirp: Gcc 9 -O3 fix
Dr. David Alan Gilbert (git), le lun. 15 avril 2019 13:17:40 +0100, a ecrit: > From: "Dr. David Alan Gilbert" > > Gcc 9 needs some convincing that sopreprbuf really is going to fill > in iov in the call from soreadbuf, even though the failure case > shouldn't happen. > > Signed-off-by: Dr. David Alan Gilbert Applied to my tree, thanks! Samuel
[Qemu-devel] [PULL 0/1] slirp gcc9 build fix
The following changes since commit afccfc0c4c6134a7bc9da6375996b3b91d291de4: Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2019-04-12 17:06:49 +0100) are available in the Git repository at: https://people.debian.org/~sthibault/qemu.git tags/samuel-thibault for you to fetch changes up to 6fabae61a9393fd2bc703837e464b9c34ec5ef25: slirp: Gcc 9 -O3 fix (2019-04-15 20:01:18 +0200) Slirp updates Dr. David Alan Gilbert (1): slirp: Gcc 9 -O3 fix Dr. David Alan Gilbert (1): slirp: Gcc 9 -O3 fix slirp/src/socket.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-)
Re: [Qemu-devel] Following up questions related to QEMU and I/O Thread
Hi @Paolo Bonzini & @Stefan Hajnoczi, Would you please help confirm whether @Paolo Bonzini's multiqueue feature change will benefit virtio-scsi or not? Thanks! @Stefan Hajnoczi, I also spent some time on exploring the virtio-scsi multi-queue features via num_queues parameter as below, here are what we found: 1. Increase number of Queues from one to the same number as CPU will get better IOPS increase. 2. Increase number of Queues to the number (e.g. 8) larger than the number of vCPU (e.g. 2) can get even better IOPS increase. In addition, It seems Qemu can get better IOPS while the attachment uses more queues than the number of vCPU, how could it possible? Could you please help us better understand the behavior? Thanks a lot! Host CPU Configuration: CPU(s):2 Thread(s) per core:2 Core(s) per socket:1 Socket(s): 1 Commands for multi queue Setup: (QEMU) device_add driver=virtio-scsi-pci num_queues=1 id=test1 (QEMU) device_add driver=virtio-scsi-pci num_queues=2 id=test2 (QEMU) device_add driver=virtio-scsi-pci num_queues=4 id=test4 (QEMU) device_add driver=virtio-scsi-pci num_queues=8 id=test8 Result: | 8 Queues | 4 Queues | 2 Queues| Single Queue IOPS| +29% | 27% |11% | Baseline Thanks, Wei On 4/5/19, 2:09 PM, "Wei Li" wrote: Thanks Stefan for your quick response! Hi Paolo, Could you please send us a link related to the multiqueue feature which you are working on so that we could start getting some details about the feature. Thanks again, Wei On 4/1/19, 3:54 AM, "Stefan Hajnoczi" wrote: On Fri, Mar 29, 2019 at 08:16:36AM -0700, Wei Li wrote: > Thanks Stefan for your reply and guidance! > > We spent some time on exploring the multiple I/O Threads approach per your feedback. Based on the perf measurement data, we did see some IOPS improvement for multiple volumes, which is great. :) > > In addition, IOPS for single Volume will still be a bottleneck, it seems like multiqueue block layer feature which Paolo is working on may be able to help improving the IOPS for single volume. > > @Paolo, @Stefan, > Would you mind sharing the multiqueue feature code branch with us? So that we could get some rough idea about this feature and maybe start doing some exploration? Paolo last worked on this code, so he may be able to send you a link. Stefan
[Qemu-devel] [Bug 1824853] Re: 4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] == off' failed
Also attaching static-compiled executable "s2.exe".
** Attachment added: "s2.exe"
https://bugs.launchpad.net/qemu/+bug/1824853/+attachment/5256000/+files/s2.exe
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1824853
Title:
4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion
`s->gen_insn_end_off[num_insns] == off' failed
Status in QEMU:
New
Bug description:
I tried to bootstrap and regtested gcc trunk (gcc svn rev 270278,
datestamp 20190411) inside my arm64-gentoo installation under qemu-
system-aarch64.
Qemu version was 4.0.0-rc3 and -cpu cortex-a57. Qemu configured with
only --target-list=aarch64-softmmu,aarch64-linux-user and compiled
using gcc "version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1~16.04)".
Executable created from gcc/testsuite/gcc.target/aarch64/advsimd-
intrinsics/vldX.c compiled with -O2 crashed the whole qemu-system.
To investigate a bit I also manually run
~/gcc/inst/trunk/bin/gcc
~/gcc/src/trunk/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vldX.c
with different options like:
-O0 -lm -o d0.exe
-O1 -lm -o d1.exe
-O2 -lm -o d2.exe
-O0 -static -lm -o s0.exe
-O1 -static -lm -o s1.exe
-O2 -static -lm -o s2.exe
So, now I have 6 different arm64 executables created with different
optimization levels. O0 and O1 versions run ok.
Three sN.exe static executables I've also tried in qemu user mode (with same
-cpu), no issue in user mode.
And inside qemu-system I can see that
running "d2.exe" (attached) gives:
tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] ==
off' failed.
And running "s2.exe" gives:
tcg/tcg.c:320: set_jmp_reset_offset: Assertion `s->tb_jmp_reset_offset[which]
== off' failed.
It seems like this test is an counter-example for logic that
"tcg_ctx->nb_ops < 4000" implies tcg will fit into 16-bit signed size
(see tcg_op_buf_full comments).
Richard's changes in abebf92597186 and 9f754620651d were not enough,
translation block must be smaller, or we have to find some proper way to bail
out when buffer overflows.
I don't know why this situation is not caught by code_gen_highwater logic in
tcg.c
I've also tried this "bail out" patch
diff --git a/tcg/tcg.c b/tcg/tcg.c
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -3949,7 +3949,8 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
size_t off = tcg_current_code_size(s);
s->gen_insn_end_off[num_insns] = off;
/* Assert that we do not overflow our stored offset. */
-assert(s->gen_insn_end_off[num_insns] == off);
+if (s->gen_insn_end_off[num_insns] != off)
+ return -1;
}
num_insns++;
for (i = 0; i < TARGET_INSN_START_WORDS; ++i) {
But then running "d2.exe" just hangs the whole qemu-system. It seems
that when tcg_gen_code return -1 (like in highwater logic mentioned
before), we just re-call it again and again.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1824853/+subscriptions
[Qemu-devel] [Bug 1824853] [NEW] 4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] == off' failed
Public bug reported:
I tried to bootstrap and regtested gcc trunk (gcc svn rev 270278,
datestamp 20190411) inside my arm64-gentoo installation under qemu-
system-aarch64.
Qemu version was 4.0.0-rc3 and -cpu cortex-a57. Qemu configured with
only --target-list=aarch64-softmmu,aarch64-linux-user and compiled using
gcc "version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1~16.04)".
Executable created from gcc/testsuite/gcc.target/aarch64/advsimd-
intrinsics/vldX.c compiled with -O2 crashed the whole qemu-system.
To investigate a bit I also manually run
~/gcc/inst/trunk/bin/gcc
~/gcc/src/trunk/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vldX.c
with different options like:
-O0 -lm -o d0.exe
-O1 -lm -o d1.exe
-O2 -lm -o d2.exe
-O0 -static -lm -o s0.exe
-O1 -static -lm -o s1.exe
-O2 -static -lm -o s2.exe
So, now I have 6 different arm64 executables created with different
optimization levels. O0 and O1 versions run ok.
Three sN.exe static executables I've also tried in qemu user mode (with same
-cpu), no issue in user mode.
And inside qemu-system I can see that
running "d2.exe" (attached) gives:
tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] == off'
failed.
And running "s2.exe" gives:
tcg/tcg.c:320: set_jmp_reset_offset: Assertion `s->tb_jmp_reset_offset[which]
== off' failed.
It seems like this test is an counter-example for logic that
"tcg_ctx->nb_ops < 4000" implies tcg will fit into 16-bit signed size
(see tcg_op_buf_full comments).
Richard's changes in abebf92597186 and 9f754620651d were not enough,
translation block must be smaller, or we have to find some proper way to bail
out when buffer overflows.
I don't know why this situation is not caught by code_gen_highwater logic in
tcg.c
I've also tried this "bail out" patch
diff --git a/tcg/tcg.c b/tcg/tcg.c
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -3949,7 +3949,8 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
size_t off = tcg_current_code_size(s);
s->gen_insn_end_off[num_insns] = off;
/* Assert that we do not overflow our stored offset. */
-assert(s->gen_insn_end_off[num_insns] == off);
+if (s->gen_insn_end_off[num_insns] != off)
+ return -1;
}
num_insns++;
for (i = 0; i < TARGET_INSN_START_WORDS; ++i) {
But then running "d2.exe" just hangs the whole qemu-system. It seems
that when tcg_gen_code return -1 (like in highwater logic mentioned
before), we just re-call it again and again.
** Affects: qemu
Importance: Undecided
Status: New
** Attachment added: "d2.exe"
https://bugs.launchpad.net/bugs/1824853/+attachment/5255996/+files/d2.exe
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1824853
Title:
4.0.0-rc3 crashes with tcg/tcg.c:3952: tcg_gen_code: Assertion
`s->gen_insn_end_off[num_insns] == off' failed
Status in QEMU:
New
Bug description:
I tried to bootstrap and regtested gcc trunk (gcc svn rev 270278,
datestamp 20190411) inside my arm64-gentoo installation under qemu-
system-aarch64.
Qemu version was 4.0.0-rc3 and -cpu cortex-a57. Qemu configured with
only --target-list=aarch64-softmmu,aarch64-linux-user and compiled
using gcc "version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1~16.04)".
Executable created from gcc/testsuite/gcc.target/aarch64/advsimd-
intrinsics/vldX.c compiled with -O2 crashed the whole qemu-system.
To investigate a bit I also manually run
~/gcc/inst/trunk/bin/gcc
~/gcc/src/trunk/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vldX.c
with different options like:
-O0 -lm -o d0.exe
-O1 -lm -o d1.exe
-O2 -lm -o d2.exe
-O0 -static -lm -o s0.exe
-O1 -static -lm -o s1.exe
-O2 -static -lm -o s2.exe
So, now I have 6 different arm64 executables created with different
optimization levels. O0 and O1 versions run ok.
Three sN.exe static executables I've also tried in qemu user mode (with same
-cpu), no issue in user mode.
And inside qemu-system I can see that
running "d2.exe" (attached) gives:
tcg/tcg.c:3952: tcg_gen_code: Assertion `s->gen_insn_end_off[num_insns] ==
off' failed.
And running "s2.exe" gives:
tcg/tcg.c:320: set_jmp_reset_offset: Assertion `s->tb_jmp_reset_offset[which]
== off' failed.
It seems like this test is an counter-example for logic that
"tcg_ctx->nb_ops < 4000" implies tcg will fit into 16-bit signed size
(see tcg_op_buf_full comments).
Richard's changes in abebf92597186 and 9f754620651d were not enough,
translation block must be smaller, or we have to find some proper way to bail
out when buffer overflows.
I don't know why this situation is not caught by code_gen_highwater logic in
tcg.c
I've also tried this "bail out" patch
diff --git a/tcg/tcg.c b/tcg/tcg.c
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -3949,7 +3949,8 @@ int tcg_gen_code(TCGCon
Re: [Qemu-devel] [PATCH for-4.0? 0/3] usb-mtp: fix ObjectInfo request handling
On Mon, 15 Apr 2019 at 18:10, Eric Blake wrote: > On 4/15/19 10:45 AM, Daniel P. Berrangé wrote: > > The 2nd patch in this series is a security flaw fix since the > > code was not correctly validating guest provided data length. > > Given that this is a security flaw, I've added this series to > https://wiki.qemu.org/Planning/4.0 in case you're hoping to get it in -rc4. What are the consequences of the flaw ? IIRC it's only one extra byte read? thanks -- PMM
Re: [Qemu-devel] [PATCH for-4.0? 0/3] usb-mtp: fix ObjectInfo request handling
On 4/15/19 10:45 AM, Daniel P. Berrangé wrote: > Two previous attempts to fix this due to GCC 9 highlighting > unaligned data access. My attempt: > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > And a previous one: > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > method handling the filename character set conversion. > > The 2nd patch in this series is a security flaw fix since the > code was not correctly validating guest provided data length. Given that this is a security flaw, I've added this series to https://wiki.qemu.org/Planning/4.0 in case you're hoping to get it in -rc4. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/3] usb-mtp: fix string length for filename when writing metadata
Daniel P. Berrangé writes:
> The ObjectInfo 'length' field provides the length of the
> wide character string filename. This is then converted to
> a multi-byte character string. This may have a different
> byte count to the wide character string. We should use the
> C string length of the multi-byte string instead.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> hw/usb/dev-mtp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index ebf210fbf8..838cd74da6 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1714,7 +1714,7 @@ static void usb_mtp_write_metadata(MTPState *s,
> uint64_t dlen)
> return;
> }
>
> -o = usb_mtp_object_lookup_name(p, filename, dataset->length);
> +o = usb_mtp_object_lookup_name(p, filename, -1);
Nit: Might as well just remove the "-1" argument and unconditionally use
strlen in usb_mtp_object_lookup_name
Bandan
> if (o != NULL) {
> next_handle = o->handle;
> }
Re: [Qemu-devel] [PATCH 0/3] usb-mtp: fix ObjectInfo request handling
On Mon, Apr 15, 2019 at 12:52:41PM -0400, Bandan Das wrote: > Daniel P. Berrangé writes: > > > Two previous attempts to fix this due to GCC 9 highlighting > > unaligned data access. My attempt: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > > > And a previous one: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > > method handling the filename character set conversion. > > > > The 2nd patch in this series is a security flaw fix since the > > code was not correctly validating guest provided data length. > > > > I've been unable to figure out how to exercise the codepath that > > calls usb_mtp_write_metadata. At a guess, it looks like something > > that should be called when writing to a file from a guest, but the > > GNOME GVFS MTP driver doesn't provide write support. Using the > > command line MTP tools "mtp-sendfile" command results in an > > protocol error > > > > # mtp-sendfile foo eek.txt > > libmtp version: 1.1.14 > > > > The store is read only by default. Are you trying something like: > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? Ah ha, I didn't realize I had to enable write support explicitly. Will retry with that. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 0/3] usb-mtp: fix ObjectInfo request handling
Daniel P. Berrangé writes: > Two previous attempts to fix this due to GCC 9 highlighting > unaligned data access. My attempt: > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > And a previous one: > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > method handling the filename character set conversion. > > The 2nd patch in this series is a security flaw fix since the > code was not correctly validating guest provided data length. > > I've been unable to figure out how to exercise the codepath that > calls usb_mtp_write_metadata. At a guess, it looks like something > that should be called when writing to a file from a guest, but the > GNOME GVFS MTP driver doesn't provide write support. Using the > command line MTP tools "mtp-sendfile" command results in an > protocol error > > # mtp-sendfile foo eek.txt > libmtp version: 1.1.14 > The store is read only by default. Are you trying something like: -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? BTW, I also found a bug introduced by a recent commit which will return an incomplete transfer for smaller file sizes. > Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.14. > Please report this VID/PID and the device model to the libmtp development > team > PTP_ERROR_IO: failed to open session, trying again after resetting USB > interface > LIBMTP libusb: Attempt to reset device > Sending foo to eek.txt > type: , 44 > Sending file... > > Error sending file. > Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send > object info. > Error 2: Error 02ff: PTP I/O Error > ERROR: Could not close session! > > And QEMU tracing show unexpected requests > > 26582@1555340076151600935 usb_mtp_command dev 4, code 0x9803, trans 0x18, > args 0x11, 0xdc04, 0x0, 0x0, 0x0 > 26582@1555340076151619955 usb_mtp_xfer dev 4, ep 2, 20/20 > 26582@1555340076154138556 usb_mtp_data_in dev 4, trans 0x18, len 8 > 26582@1555340076154150689 usb_mtp_xfer dev 4, ep 1, 20/512 > 26582@1555340076156654311 usb_mtp_success dev 4, trans 0x18, args 0x0, 0x0 > 26582@1555340076156667764 usb_mtp_xfer dev 4, ep 1, 12/512 > 26582@1555340076159215930 usb_mtp_command dev 4, code 0x100c, trans 0x19, > args 0x10001, 0xc, 0x0, 0x0, 0x0 > 26582@1555340076159229610 usb_mtp_xfer dev 4, ep 2, 20/20 > 26582@1555340076164166196 usb_mtp_stall dev 4, reason: awaiting data-out > 26582@1555340076167156367 usb_mtp_stall dev 4, reason: transaction > inflight > 26582@1555340076170108336 usb_mtp_stall dev 4, reason: unknown control > request > 26582@1555340076172606798 usb_mtp_stall dev 4, reason: unknown control > request > > Perhaps a Windows guest can exercise this, but I don't have a modern > Windows install with MTP support. > > Thus this series is merely compile tested. > > Daniel P. Berrangé (3): > usb-mtp: fix string length for filename when writing metadata > usb-mtp: fix bounds check for guest provided filename > usb-mtp: fix alignment of access of ObjectInfo filename field > > hw/usb/dev-mtp.c | 19 +-- > 1 file changed, 13 insertions(+), 6 deletions(-)
Re: [Qemu-devel] [PATCH] usb-mtp: change default to success for usb_mtp_update_object
Forgot to cc people ...
Bandan Das writes:
> Commit c5ead51f90cf (usb-mtp: return incomplete transfer on a lstat
> failure) checks if lstat succeeded when updating attributes of a
> file. However, it also changed behavior to return an error by
> default. This is incorrect because for smaller file sizes, Qemu
> will attempt to write the file in one go and there won't be
> an object for it.
>
> Fixes: c5ead51f90cf
> Signed-off-by: Bandan Das
> ---
> hw/usb/dev-mtp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index ebf210fbf8..5de22738ce 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1587,7 +1587,7 @@ done:
>
> static int usb_mtp_update_object(MTPObject *parent, char *name)
> {
> -int ret = -1;
> +int ret = 0;
>
> MTPObject *o =
> usb_mtp_object_lookup_name(parent, name, strlen(name));
Re: [Qemu-devel] [PATCH 14/17] qom/cpu: Simplify how CPUClass:cpu_dump_state() prints
* Markus Armbruster (arm...@redhat.com) wrote:
> CPUClass method dump_statistics() takes an fprintf()-like callback and
> a FILE * to pass to it. Most callers pass fprintf() and stderr.
> log_cpu_state() passes fprintf() and qemu_log_file.
> hmp_info_registers() passes monitor_fprintf() and the current monitor
> cast to FILE *. monitor_fprintf() casts it right back, and is
> otherwise identical to monitor_printf().
>
> The callback gets passed around a lot, which is tiresome. The
> type-punning around monitor_fprintf() is ugly.
>
> Drop the callback, and call qemu_fprintf() instead. Also gets rid of
> the type-punning, since qemu_fprintf() takes NULL instead of the
> current monitor cast to FILE *.
>
> Signed-off-by: Markus Armbruster
Yes, I think so.
There seems to be a place which changes hmp_info_local_apic in a plce
that was changed in an earlier patch which seems a shame, but OK.
Reviewed-by: Dr. David Alan Gilbert
> ---
> accel/kvm/kvm-all.c | 4 +-
> bsd-user/main.c | 2 +-
> cpus.c | 2 +-
> exec.c | 2 +-
> include/exec/log.h | 2 +-
> include/qom/cpu.h| 11 +-
> linux-user/alpha/cpu_loop.c | 2 +-
> linux-user/cpu_loop-common.h | 2 +-
> linux-user/cris/cpu_loop.c | 2 +-
> linux-user/microblaze/cpu_loop.c | 4 +-
> linux-user/s390x/cpu_loop.c | 4 +-
> linux-user/sh4/cpu_loop.c| 2 +-
> linux-user/sparc/cpu_loop.c | 2 +-
> monitor.c| 4 +-
> qom/cpu.c| 6 +-
> target/alpha/cpu.h | 3 +-
> target/alpha/helper.c| 24 +--
> target/arm/arm-semi.c| 2 +-
> target/arm/cpu.h | 3 +-
> target/arm/translate-a64.c | 82 -
> target/arm/translate.c | 58 +++
> target/arm/translate.h | 7 +-
> target/cris/cpu.h| 3 +-
> target/cris/helper.c | 2 +-
> target/cris/translate.c | 36 ++--
> target/hppa/cpu.h| 2 +-
> target/hppa/helper.c | 24 +--
> target/i386/cpu.h| 5 +-
> target/i386/hax-all.c| 4 +-
> target/i386/helper.c | 281 +++
> target/i386/monitor.c| 2 +-
> target/lm32/cpu.h| 3 +-
> target/lm32/translate.c | 36 ++--
> target/m68k/cpu.h| 3 +-
> target/m68k/translate.c | 86 +-
> target/microblaze/cpu.h | 3 +-
> target/microblaze/helper.c | 2 +-
> target/microblaze/translate.c| 39 ++---
> target/mips/internal.h | 3 +-
> target/mips/translate.c | 76 -
> target/moxie/cpu.h | 3 +-
> target/moxie/helper.c| 2 +-
> target/moxie/translate.c | 22 +--
> target/nios2/cpu.h | 3 +-
> target/nios2/helper.c| 2 +-
> target/nios2/translate.c | 24 +--
> target/openrisc/cpu.h| 3 +-
> target/openrisc/translate.c | 11 +-
> target/ppc/cpu.h | 3 +-
> target/ppc/translate.c | 161 +-
> target/riscv/cpu.c | 37 ++--
> target/s390x/helper.c| 42 ++---
> target/s390x/internal.h | 3 +-
> target/sh4/cpu.h | 3 +-
> target/sh4/translate.c | 27 +--
> target/sparc/cpu.c | 84 +
> target/sparc/cpu.h | 3 +-
> target/tilegx/cpu.c | 14 +-
> target/tricore/cpu.h | 3 +-
> target/tricore/translate.c | 26 +--
> target/unicore32/cpu.h | 3 +-
> target/unicore32/translate.c | 39 +++--
> target/xtensa/cpu.h | 3 +-
> target/xtensa/translate.c| 40 ++---
> 64 files changed, 686 insertions(+), 715 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 241db496c3..524c4ddfbd 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1798,7 +1798,7 @@ static int kvm_handle_internal_error(CPUState *cpu,
> struct kvm_run *run)
> if (run->internal.suberror == KVM_INTERNAL_ERROR_EMULATION) {
> fprintf(stderr, "emulation failure\n");
> if (!kvm_arch_stop_on_emulation_error(cpu)) {
> -cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE);
> +cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
> return EXCP_INTERRUPT;
> }
> }
> @@ -2089,7 +2089,7 @@ int kvm_cpu_exec(CPUState *cpu)
> qemu_mutex_lock_iothread();
>
> if (ret < 0) {
> -cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE);
> +cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
> vm_stop(RUN_STATE_INTERNAL_ERROR);
> }
>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 1b4a2f8693..9997ca3794 100644
> ---
[Qemu-devel] [PATCH] usb-mtp: change default to success for usb_mtp_update_object
Commit c5ead51f90cf (usb-mtp: return incomplete transfer on a lstat
failure) checks if lstat succeeded when updating attributes of a
file. However, it also changed behavior to return an error by
default. This is incorrect because for smaller file sizes, Qemu
will attempt to write the file in one go and there won't be
an object for it.
Fixes: c5ead51f90cf
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index ebf210fbf8..5de22738ce 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1587,7 +1587,7 @@ done:
static int usb_mtp_update_object(MTPObject *parent, char *name)
{
-int ret = -1;
+int ret = 0;
MTPObject *o =
usb_mtp_object_lookup_name(parent, name, strlen(name));
--
2.19.2
Re: [Qemu-devel] [PATCH v4 0/6] vfio-ccw: support hsch/csch (kernel part)
On Fri, 1 Mar 2019 10:38:56 +0100
Cornelia Huck wrote:
> [This is the Linux kernel part, git tree is available at
> https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git
> vfio-ccw-eagain-caps-v3
>
> The companion QEMU patches are available at
> https://github.com/cohuck/qemu vfio-ccw-caps]
>
> Currently, vfio-ccw only relays START SUBCHANNEL requests to the real
> device. This tends to work well for the most common 'good path' scenarios;
> however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like
> clearing pending requests at the device is currently not supported.
> This may be a problem for e.g. error recovery.
>
> This patch series introduces capabilities (similar to what vfio-pci uses)
> and exposes a new async region for handling hsch/csch.
>
> Lightly tested (I can interact with a dasd as before, and reserve/release
> seems to work well.) Not sure if there is a better way to test this, ideas
> welcome.
>
> Changes v3->v4:
> - shrunk the capability offset, we don't need to accommodate as much as pci
> - make checks in patch 1 more consistent and less buggy
> - rebased on top of my current vfio-ccw branch
>
> Changes v2->v3:
> - Unb0rked patch 1, improved scope
> - Split out the new mutex from patch 2 into new patch 3; added missing
> locking and hopefully improved description
> - Patch 2 now reworks the state handling by splitting the BUSY state
> into CP_PROCESSING and CP_PENDING
> - Patches 3 and 5 adapted on top of the reworked patches; hsch/csch
> are allowed in CP_PENDING, but not in CP_PROCESSING (did not add
> any R-b due to that)
> - Added missing free in patch 5
> - Probably some small changes I forgot to note down
>
> Changes v1->v2:
> - New patch 1: make it safe to use the cp accessors at any time; this
> should avoid problems with unsolicited interrupt handling
> - New patch 2: handle concurrent accesses to the io region; the idea is
> to return -EAGAIN to userspace more often (so it can simply retry)
> - also handle concurrent accesses to the async io region
> - change VFIO_REGION_TYPE_CCW
> - merge events for halt and clear to a single async event; this turned out
> to make the code quite a bit simpler
> - probably some small changes I forgot to note down
>
> Cornelia Huck (6):
> vfio-ccw: make it safe to access channel programs
> vfio-ccw: rework ssch state handling
> vfio-ccw: protect the I/O region
> vfio-ccw: add capabilities chain
> s390/cio: export hsch to modules
> vfio-ccw: add handling for async channel instructions
>
> drivers/s390/cio/Makefile | 3 +-
> drivers/s390/cio/ioasm.c| 1 +
> drivers/s390/cio/vfio_ccw_async.c | 88
> drivers/s390/cio/vfio_ccw_cp.c | 21 ++-
> drivers/s390/cio/vfio_ccw_cp.h | 2 +
> drivers/s390/cio/vfio_ccw_drv.c | 57 ++--
> drivers/s390/cio/vfio_ccw_fsm.c | 143 +-
> drivers/s390/cio/vfio_ccw_ops.c | 215 +++-
> drivers/s390/cio/vfio_ccw_private.h | 48 ++-
> include/uapi/linux/vfio.h | 4 +
> include/uapi/linux/vfio_ccw.h | 12 ++
> 11 files changed, 537 insertions(+), 57 deletions(-)
> create mode 100644 drivers/s390/cio/vfio_ccw_async.c
>
Queued (with the touchups discussed in the thread) to the vfio-ccw
branch on kernel.org.
Re: [Qemu-devel] [PATCH v4] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 06:33:36PM +0200, Marc-André Lureau wrote:
> Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> is a bit too strict. Current libvirt always set wait=false, and will
> thus fail to add client chardev.
>
> Make the code more permissive, allowing wait=false with client socket
> chardevs. Deprecate usage of 'wait' with client sockets.
>
> Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
IOW, this is a regression in 4.0 and so desirable to fix in rc4
to avoid breaking with existing libvirt.
Meanwhile libvirt should also be fixed to not pass 'wait' for
client sockets.
> Cc: Daniel P. Berrangé
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 12
> qemu-deprecated.texi | 5 +
> 2 files changed, 13 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file
On 4/15/19 10:54 AM, Kevin Wolf wrote: > preallocate_co() already gave the data file the full size without > forwarding the requested preallocation mode to the protocol. When > bdrv_co_truncate() was called later with the preallocation mode, the > file didn't actually grow any more, so the data file stayed unallocated > even if full preallocation was requested. > > Pass the right preallocation mode to preallocate_co() and remove the > second bdrv_co_truncate() to fix this. As a side effect, the ugly > one-byte write in preallocate_co() is replaced with a truncate call, > now leaving the last block unallocated on the protocol level as it > should be. > > Cc: qemu-sta...@nongnu.org > Signed-off-by: Kevin Wolf > --- > block/qcow2.c | 41 +++-- > 1 file changed, 23 insertions(+), 18 deletions(-) > Reviewed-by: Eric Blake Worth iotest coverage? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v4] socket: allow wait=false for client socket
Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
is a bit too strict. Current libvirt always set wait=false, and will
thus fail to add client chardev.
Make the code more permissive, allowing wait=false with client socket
chardevs. Deprecate usage of 'wait' with client sockets.
Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
Cc: Daniel P. Berrangé
Signed-off-by: Marc-André Lureau
---
chardev/char-socket.c | 12
qemu-deprecated.texi | 5 +
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3916505d67..b2cf593107 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1263,10 +1263,14 @@ static bool qmp_chardev_validate_socket(ChardevSocket
*sock,
return false;
}
if (sock->has_wait) {
-error_setg(errp, "%s",
- "'wait' option is incompatible with "
- "socket in client connect mode");
-return false;
+warn_report("'wait' option is deprecated with "
+"socket in client connect mode");
+if (sock->wait) {
+error_setg(errp, "%s",
+ "'wait' option is incompatible with "
+ "socket in client connect mode");
+return false;
+}
}
}
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 2219386769..842e71b11d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -105,6 +105,11 @@ details.
The ``query-events'' command has been superseded by the more powerful
and accurate ``query-qmp-schema'' command.
+@subsection chardev client socket with 'wait' option (since 4.0)
+
+Character devices creating sockets in client mode should not specify
+the 'wait' field, which is only applicable to sockets in server mode
+
@section Human Monitor Protocol (HMP) commands
@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since
3.1)
--
2.21.0.313.ge35b8cb8e2
Re: [Qemu-devel] [PATCH v3] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 6:27 PM Daniel P. Berrangé wrote:
>
> On Mon, Apr 15, 2019 at 06:21:29PM +0200, Marc-André Lureau wrote:
> > Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> > is a bit too strict. Current libvirt always set wait=false, and will
> > thus fail to add client chardev.
> >
> > Make the code more permissive, allowing wait=false with client socket
> > chardevs. Deprecate usage of 'wait' with client sockets.
> >
> > Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> > Cc: Daniel P. Berrangé
> > Signed-off-by: Marc-André Lureau
> > ---
> > chardev/char-socket.c | 10 +++---
> > qemu-deprecated.texi | 5 +
> > 2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 3916505d67..ebc1adb83e 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1263,9 +1263,13 @@ static bool
> > qmp_chardev_validate_socket(ChardevSocket *sock,
> > return false;
> > }
> > if (sock->has_wait) {
> > -error_setg(errp, "%s",
> > - "'wait' option is incompatible with "
> > - "socket in client connect mode");
> > +warn_report("'wait' option is deprecated with "
> > +"socket in client connect mode");
> > +if (sock->wait) {
> > +error_setg(errp, "%s",
> > + "'wait' option is incompatible with "
> > + "socket in client connect mode");
> > +}
> > return false;
>
> Oh wait, don't you need to put the 'return false' inside the
> if branch above, otherwise we're still failing validation.
indeed, v4 on the way
>
> > }
> > }
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 2219386769..842e71b11d 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -105,6 +105,11 @@ details.
> > The ``query-events'' command has been superseded by the more powerful
> > and accurate ``query-qmp-schema'' command.
> >
> > +@subsection chardev client socket with 'wait' option (since 4.0)
> > +
> > +Character devices creating sockets in client mode should not specify
> > +the 'wait' field, which is only applicable to sockets in server mode
> > +
> > @section Human Monitor Protocol (HMP) commands
> >
> > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> > (since 3.1)
> > --
> > 2.21.0.313.ge35b8cb8e2
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v3] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 06:21:29PM +0200, Marc-André Lureau wrote:
> Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> is a bit too strict. Current libvirt always set wait=false, and will
> thus fail to add client chardev.
>
> Make the code more permissive, allowing wait=false with client socket
> chardevs. Deprecate usage of 'wait' with client sockets.
>
> Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> Cc: Daniel P. Berrangé
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 10 +++---
> qemu-deprecated.texi | 5 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3916505d67..ebc1adb83e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1263,9 +1263,13 @@ static bool qmp_chardev_validate_socket(ChardevSocket
> *sock,
> return false;
> }
> if (sock->has_wait) {
> -error_setg(errp, "%s",
> - "'wait' option is incompatible with "
> - "socket in client connect mode");
> +warn_report("'wait' option is deprecated with "
> +"socket in client connect mode");
> +if (sock->wait) {
> +error_setg(errp, "%s",
> + "'wait' option is incompatible with "
> + "socket in client connect mode");
> +}
> return false;
Oh wait, don't you need to put the 'return false' inside the
if branch above, otherwise we're still failing validation.
> }
> }
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2219386769..842e71b11d 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -105,6 +105,11 @@ details.
> The ``query-events'' command has been superseded by the more powerful
> and accurate ``query-qmp-schema'' command.
>
> +@subsection chardev client socket with 'wait' option (since 4.0)
> +
> +Character devices creating sockets in client mode should not specify
> +the 'wait' field, which is only applicable to sockets in server mode
> +
> @section Human Monitor Protocol (HMP) commands
>
> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since
> 3.1)
> --
> 2.21.0.313.ge35b8cb8e2
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH v3] socket: allow wait=false for client socket
Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
is a bit too strict. Current libvirt always set wait=false, and will
thus fail to add client chardev.
Make the code more permissive, allowing wait=false with client socket
chardevs. Deprecate usage of 'wait' with client sockets.
Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
Cc: Daniel P. Berrangé
Signed-off-by: Marc-André Lureau
---
chardev/char-socket.c | 10 +++---
qemu-deprecated.texi | 5 +
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3916505d67..ebc1adb83e 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1263,9 +1263,13 @@ static bool qmp_chardev_validate_socket(ChardevSocket
*sock,
return false;
}
if (sock->has_wait) {
-error_setg(errp, "%s",
- "'wait' option is incompatible with "
- "socket in client connect mode");
+warn_report("'wait' option is deprecated with "
+"socket in client connect mode");
+if (sock->wait) {
+error_setg(errp, "%s",
+ "'wait' option is incompatible with "
+ "socket in client connect mode");
+}
return false;
}
}
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 2219386769..842e71b11d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -105,6 +105,11 @@ details.
The ``query-events'' command has been superseded by the more powerful
and accurate ``query-qmp-schema'' command.
+@subsection chardev client socket with 'wait' option (since 4.0)
+
+Character devices creating sockets in client mode should not specify
+the 'wait' field, which is only applicable to sockets in server mode
+
@section Human Monitor Protocol (HMP) commands
@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since
3.1)
--
2.21.0.313.ge35b8cb8e2
Re: [Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()
On 4/15/19 10:54 AM, Kevin Wolf wrote: > We'll add a bdrv_co_truncate() call in the next patch which can return > an Error that we don't want to discard. So add an errp parameter to > preallocate_co(). > > Cc: qemu-sta...@nongnu.org > Signed-off-by: Kevin Wolf > --- > block/qcow2.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH for-4.0?] socket: allow wait=false for client socket
On 4/15/19 10:49 AM, Marc-André Lureau wrote:
> Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> is a bit too strict. Current libvirt always set wait=false, and will
> thus fail to add client chardev.
>
> Make the code more permissive, allowing wait=false with client socket
> chardevs.
>
> Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> Cc: Daniel P. Berrangé
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
According to Fixes, this is a regression in 4.0. Is it worth having this
fix in -rc4?
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3916505d67..2ad9c9eea5 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1262,7 +1262,7 @@ static bool qmp_chardev_validate_socket(ChardevSocket
> *sock,
> error_setg(errp, "%s", "Websocket client is not implemented");
> return false;
> }
> -if (sock->has_wait) {
> +if (sock->has_wait && sock->wait) {
> error_setg(errp, "%s",
> "'wait' option is incompatible with "
> "socket in client connect mode");
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 06:16:33PM +0200, Marc-André Lureau wrote:
> On Mon, Apr 15, 2019 at 6:08 PM Daniel P. Berrangé
> wrote:
> >
> > On Mon, Apr 15, 2019 at 06:06:09PM +0200, Marc-André Lureau wrote:
> > > Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> > > is a bit too strict. Current libvirt always set wait=false, and will
> > > thus fail to add client chardev.
> > >
> > > Make the code more permissive, allowing wait=false with client socket
> > > chardevs.
> > >
> > > Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> > > Cc: Daniel P. Berrangé
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > > chardev/char-socket.c | 10 +++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 3916505d67..ebc1adb83e 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -1263,9 +1263,13 @@ static bool
> > > qmp_chardev_validate_socket(ChardevSocket *sock,
> > > return false;
> > > }
> > > if (sock->has_wait) {
> > > -error_setg(errp, "%s",
> > > - "'wait' option is incompatible with "
> > > - "socket in client connect mode");
> > > +warn_report("'wait' option is deprecated with "
> > > +"socket in client connect mode");
> > > +if (sock->wait) {
> > > +error_setg(errp, "%s",
> > > + "'wait' option is incompatible with "
> > > + "socket in client connect mode");
> > > +}
> > > return false;
> > > }
> > > }
> >
> > NB, it should also update qemu-deprecated.texi
>
> Is that clear enough?
>
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2219386769..0f53d1c228 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -105,6 +105,12 @@ details.
> The ``query-events'' command has been superseded by the more powerful
> and accurate ``query-qmp-schema'' command.
>
> +@subsection chardev client socket with 'wait' option (since 4.0)
> +
> +The ``chardev-add'' and ``chardev-change'' should not be used to
> +create client sockets with a ``wait'' field, which is reserved for
> +server sockets only.
It applies to command line syntax too, so I would just generalize that
a little to
"Character devices creating sockets in client mode should not
specify the 'wait' field, which is only applicable to sockets
in server mode"
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file
On 4/15/19 10:54 AM, Kevin Wolf wrote: > With an external data file, preallocate_co() must write the final byte > to the external data file, not to the qcow2 image file. > > This is harmless for preallocation of newly created images (only the > qcow2 file size is increased to the virtual disk size while it should be > much smaller), but with preallocated resize, it could in theory cause > visible corruption if the metadata of the image is larger than the data > (e.g. lots of bitmaps). Can we come up with such an image - maybe one with 512-byte cluster sizing and only 1k in guest-visible length? Since each bitmap is cluster-aligned, it seems like you'd only need a couple of bitmaps to easily reach that point. We're awfully late for 4.0, but as we already have -rc4 coming due, and as this is a data-corruption bug in a new feature, I can buy the argument of getting this one into 4.0, particularly if you can design the iotest along the lines of my ideas to prove that yes, indeed, we are accidentally wiping out qcow2 metadata for visible image corruption. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 6:08 PM Daniel P. Berrangé wrote:
>
> On Mon, Apr 15, 2019 at 06:06:09PM +0200, Marc-André Lureau wrote:
> > Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> > is a bit too strict. Current libvirt always set wait=false, and will
> > thus fail to add client chardev.
> >
> > Make the code more permissive, allowing wait=false with client socket
> > chardevs.
> >
> > Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> > Cc: Daniel P. Berrangé
> > Signed-off-by: Marc-André Lureau
> > ---
> > chardev/char-socket.c | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 3916505d67..ebc1adb83e 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1263,9 +1263,13 @@ static bool
> > qmp_chardev_validate_socket(ChardevSocket *sock,
> > return false;
> > }
> > if (sock->has_wait) {
> > -error_setg(errp, "%s",
> > - "'wait' option is incompatible with "
> > - "socket in client connect mode");
> > +warn_report("'wait' option is deprecated with "
> > +"socket in client connect mode");
> > +if (sock->wait) {
> > +error_setg(errp, "%s",
> > + "'wait' option is incompatible with "
> > + "socket in client connect mode");
> > +}
> > return false;
> > }
> > }
>
> NB, it should also update qemu-deprecated.texi
Is that clear enough?
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 2219386769..0f53d1c228 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -105,6 +105,12 @@ details.
The ``query-events'' command has been superseded by the more powerful
and accurate ``query-qmp-schema'' command.
+@subsection chardev client socket with 'wait' option (since 4.0)
+
+The ``chardev-add'' and ``chardev-change'' should not be used to
+create client sockets with a ``wait'' field, which is reserved for
+server sockets only.
+
@section Human Monitor Protocol (HMP) commands
@subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
(since 3.1)
--
Marc-André Lureau
Re: [Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation
On 4/15/19 10:54 AM, Kevin Wolf wrote: > Limiting the allocation to INT_MAX bytes isn't particularly clever > because it means that the final cluster will be a partial cluster which > will be completed through a COW operation. This results in unnecessary > data read and write requests which lead to an unwanted non-sparse > filesystem block for metadata preallocation. > > Align the maximum allocation size down to the cluster size to avoid this > situation. > > Cc: qemu-sta...@nongnu.org > Signed-off-by: Kevin Wolf > --- > block/qcow2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 06:06:09PM +0200, Marc-André Lureau wrote:
> Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> is a bit too strict. Current libvirt always set wait=false, and will
> thus fail to add client chardev.
>
> Make the code more permissive, allowing wait=false with client socket
> chardevs.
>
> Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> Cc: Daniel P. Berrangé
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3916505d67..ebc1adb83e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1263,9 +1263,13 @@ static bool qmp_chardev_validate_socket(ChardevSocket
> *sock,
> return false;
> }
> if (sock->has_wait) {
> -error_setg(errp, "%s",
> - "'wait' option is incompatible with "
> - "socket in client connect mode");
> +warn_report("'wait' option is deprecated with "
> +"socket in client connect mode");
> +if (sock->wait) {
> +error_setg(errp, "%s",
> + "'wait' option is incompatible with "
> + "socket in client connect mode");
> +}
> return false;
> }
> }
NB, it should also update qemu-deprecated.texi
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH v2] socket: allow wait=false for client socket
Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
is a bit too strict. Current libvirt always set wait=false, and will
thus fail to add client chardev.
Make the code more permissive, allowing wait=false with client socket
chardevs.
Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
Cc: Daniel P. Berrangé
Signed-off-by: Marc-André Lureau
---
chardev/char-socket.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3916505d67..ebc1adb83e 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1263,9 +1263,13 @@ static bool qmp_chardev_validate_socket(ChardevSocket
*sock,
return false;
}
if (sock->has_wait) {
-error_setg(errp, "%s",
- "'wait' option is incompatible with "
- "socket in client connect mode");
+warn_report("'wait' option is deprecated with "
+"socket in client connect mode");
+if (sock->wait) {
+error_setg(errp, "%s",
+ "'wait' option is incompatible with "
+ "socket in client connect mode");
+}
return false;
}
}
--
2.21.0.313.ge35b8cb8e2
[Qemu-devel] [Bug 1788582] Re: Race condition during shutdown
It was fixed with commit 4cf077b59fc73eec29f8b7 (see patch series
https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00504.html).
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1788582
Title:
Race condition during shutdown
Status in QEMU:
New
Bug description:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1788582/+subscriptions
Re: [Qemu-devel] [PATCH] socket: allow wait=false for client socket
On Mon, Apr 15, 2019 at 05:49:06PM +0200, Marc-André Lureau wrote:
> Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
> is a bit too strict. Current libvirt always set wait=false, and will
> thus fail to add client chardev.
>
> Make the code more permissive, allowing wait=false with client socket
> chardevs.
>
> Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
> Cc: Daniel P. Berrangé
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3916505d67..2ad9c9eea5 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1262,7 +1262,7 @@ static bool qmp_chardev_validate_socket(ChardevSocket
> *sock,
> error_setg(errp, "%s", "Websocket client is not implemented");
> return false;
> }
> -if (sock->has_wait) {
> +if (sock->has_wait && sock->wait) {
> error_setg(errp, "%s",
> "'wait' option is incompatible with "
> "socket in client connect mode");
Reviewed-by: Daniel P. Berrangé
Though, I think we should issue a deprecation warning if 'has_wait' is
set, because this is still user error, and it would be good to make it
stricter again later.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()
We'll add a bdrv_co_truncate() call in the next patch which can return
an Error that we don't want to discard. So add an errp parameter to
preallocate_co().
Cc: qemu-sta...@nongnu.org
Signed-off-by: Kevin Wolf
---
block/qcow2.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index dfac74c264..b4f9f5a240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,7 +2721,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
* Returns: 0 on success, -errno on failure.
*/
static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
- uint64_t new_length)
+ uint64_t new_length, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
uint64_t bytes;
@@ -2738,6 +2738,7 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
&host_offset, &meta);
if (ret < 0) {
+error_setg_errno(errp, -ret, "Allocating clusters failed");
return ret;
}
@@ -2746,6 +2747,7 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
ret = qcow2_alloc_cluster_link_l2(bs, meta);
if (ret < 0) {
+error_setg_errno(errp, -ret, "Mapping clusters failed");
qcow2_free_any_clusters(bs, meta->alloc_offset,
meta->nb_clusters,
QCOW2_DISCARD_NEVER);
return ret;
@@ -2775,6 +2777,7 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
&data, 1);
if (ret < 0) {
+error_setg_errno(errp, -ret, "Writing to EOF failed");
return ret;
}
}
@@ -3748,9 +3751,8 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
break;
case PREALLOC_MODE_METADATA:
-ret = preallocate_co(bs, old_length, offset);
+ret = preallocate_co(bs, old_length, offset, errp);
if (ret < 0) {
-error_setg_errno(errp, -ret, "Preallocation failed");
goto fail;
}
break;
@@ -3766,9 +3768,8 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
/* With a data file, preallocation means just allocating the metadata
* and forwarding the truncate request to the data file */
if (has_data_file(bs)) {
-ret = preallocate_co(bs, old_length, offset);
+ret = preallocate_co(bs, old_length, offset, errp);
if (ret < 0) {
-error_setg_errno(errp, -ret, "Preallocation failed");
goto fail;
}
break;
--
2.20.1
[Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file
preallocate_co() already gave the data file the full size without
forwarding the requested preallocation mode to the protocol. When
bdrv_co_truncate() was called later with the preallocation mode, the
file didn't actually grow any more, so the data file stayed unallocated
even if full preallocation was requested.
Pass the right preallocation mode to preallocate_co() and remove the
second bdrv_co_truncate() to fix this. As a side effect, the ugly
one-byte write in preallocate_co() is replaced with a truncate call,
now leaving the last block unallocated on the protocol level as it
should be.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Kevin Wolf
---
block/qcow2.c | 41 +++--
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index b4f9f5a240..7fbef97aab 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,11 +2721,13 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
* Returns: 0 on success, -errno on failure.
*/
static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
- uint64_t new_length, Error **errp)
+ uint64_t new_length, PreallocMode mode,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
uint64_t bytes;
uint64_t host_offset = 0;
+int64_t file_length;
unsigned int cur_bytes;
int ret;
QCowL2Meta *meta;
@@ -2772,12 +2774,19 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
* all of the allocated clusters (otherwise we get failing reads after
* EOF). Extend the image to the last allocated sector.
*/
-if (host_offset != 0) {
-uint8_t data = 0;
-ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
- &data, 1);
+file_length = bdrv_getlength(s->data_file->bs);
+if (file_length < 0) {
+error_setg_errno(errp, -file_length, "Could not get file size");
+return file_length;
+}
+
+if (host_offset + cur_bytes > file_length) {
+if (mode == PREALLOC_MODE_METADATA) {
+mode = PREALLOC_MODE_OFF;
+}
+ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, mode,
+ errp);
if (ret < 0) {
-error_setg_errno(errp, -ret, "Writing to EOF failed");
return ret;
}
}
@@ -3748,10 +3757,16 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
switch (prealloc) {
case PREALLOC_MODE_OFF:
+if (has_data_file(bs)) {
+ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
+if (ret < 0) {
+goto fail;
+}
+}
break;
case PREALLOC_MODE_METADATA:
-ret = preallocate_co(bs, old_length, offset, errp);
+ret = preallocate_co(bs, old_length, offset, prealloc, errp);
if (ret < 0) {
goto fail;
}
@@ -3768,7 +3783,7 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
/* With a data file, preallocation means just allocating the metadata
* and forwarding the truncate request to the data file */
if (has_data_file(bs)) {
-ret = preallocate_co(bs, old_length, offset, errp);
+ret = preallocate_co(bs, old_length, offset, prealloc, errp);
if (ret < 0) {
goto fail;
}
@@ -3883,16 +3898,6 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
bs->total_sectors = offset / BDRV_SECTOR_SIZE;
-if (has_data_file(bs)) {
-if (prealloc == PREALLOC_MODE_METADATA) {
-prealloc = PREALLOC_MODE_OFF;
-}
-ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
-if (ret < 0) {
-goto fail;
-}
-}
-
/* write updated header.size */
offset = cpu_to_be64(offset);
ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
--
2.20.1
[Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation
Limiting the allocation to INT_MAX bytes isn't particularly clever
because it means that the final cluster will be a partial cluster which
will be completed through a COW operation. This results in unnecessary
data read and write requests which lead to an unwanted non-sparse
filesystem block for metadata preallocation.
Align the maximum allocation size down to the cluster size to avoid this
situation.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Kevin Wolf
---
block/qcow2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d507ee0686..c8400e9712 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2723,6 +2723,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
uint64_t new_length)
{
+BDRVQcow2State *s = bs->opaque;
uint64_t bytes;
uint64_t host_offset = 0;
unsigned int cur_bytes;
@@ -2733,7 +2734,7 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
bytes = new_length - offset;
while (bytes) {
-cur_bytes = MIN(bytes, INT_MAX);
+cur_bytes = MIN(bytes, QEMU_ALIGN_DOWN(INT_MAX, s->cluster_size));
ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
&host_offset, &meta);
if (ret < 0) {
--
2.20.1
[Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file
With an external data file, preallocate_co() must write the final byte
to the external data file, not to the qcow2 image file.
This is harmless for preallocation of newly created images (only the
qcow2 file size is increased to the virtual disk size while it should be
much smaller), but with preallocated resize, it could in theory cause
visible corruption if the metadata of the image is larger than the data
(e.g. lots of bitmaps).
Cc: qemu-sta...@nongnu.org
Signed-off-by: Kevin Wolf
---
block/qcow2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c8400e9712..dfac74c264 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2772,7 +2772,7 @@ static int coroutine_fn preallocate_co(BlockDriverState
*bs, uint64_t offset,
*/
if (host_offset != 0) {
uint8_t data = 0;
-ret = bdrv_pwrite(bs->file, (host_offset + cur_bytes) - 1,
+ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
&data, 1);
if (ret < 0) {
return ret;
--
2.20.1
[Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes
Kevin Wolf (4): qcow2: Avoid COW during metadata preallocation qcow2: Fix preallocation bdrv_pwrite to wrong file qcow2: Add errp to preallocate_co() qcow2: Fix full preallocation with external data file block/qcow2.c | 47 +++ 1 file changed, 27 insertions(+), 20 deletions(-) -- 2.20.1
Re: [Qemu-devel] [PATCH 10/17] target: Clean up how the dump_mmu() print
* Markus Armbruster (arm...@redhat.com) wrote:
> The various dump_mmu() take an fprintf()-like callback and a FILE * to
> pass to it, and so do their helper functions. Passing around callback
> and argument is rather tiresome.
>
> Most dump_mmu() are called only by the target's hmp_info_tlb(). These
> all pass monitor_printf() cast to fprintf_function and the current
> monitor cast to FILE *.
>
> SPARC's dump_mmu() gets also called from target/sparc/ldst_helper.c a
> few times #ifdef DEBUG_MMU. These calls pass fprintf() and stdout.
You might want to fixup the DPRINTF_MMU in ldst_helper.c to also
use qemu_printf.
> The type-punning is technically undefined behaviour, but works in
> practice. Clean up: drop the callback, and call qemu_printf()
> instead.
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Dr. David Alan Gilbert
> ---
> target/m68k/cpu.h | 3 +-
> target/m68k/helper.c | 115 ++---
> target/m68k/monitor.c | 2 +-
> target/nios2/cpu.h | 2 +-
> target/nios2/mmu.c | 7 ++-
> target/nios2/monitor.c | 2 +-
> target/ppc/cpu.h | 2 +-
> target/ppc/mmu-hash64.c| 7 ++-
> target/ppc/mmu-hash64.h| 2 +-
> target/ppc/mmu_helper.c| 70 +++---
> target/ppc/monitor.c | 2 +-
> target/sparc/cpu.h | 2 +-
> target/sparc/ldst_helper.c | 18 +++---
> target/sparc/mmu_helper.c | 97 +++
> target/sparc/monitor.c | 2 +-
> target/xtensa/cpu.h| 2 +-
> target/xtensa/mmu_helper.c | 24
> target/xtensa/monitor.c| 2 +-
> 18 files changed, 178 insertions(+), 183 deletions(-)
>
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 9c1f0a2458..73952f6ece 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -573,5 +573,6 @@ static inline void cpu_get_tb_cpu_state(CPUM68KState
> *env, target_ulong *pc,
> }
> }
>
> -void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUM68KState *env);
> +void dump_mmu(CPUM68KState *env);
> +
> #endif
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index bb64cf15c0..d958a34959 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -369,30 +369,28 @@ int m68k_cpu_handle_mmu_fault(CPUState *cs, vaddr
> address, int size, int rw,
>
> /* MMU: 68040 only */
>
> -static void print_address_zone(FILE *f, fprintf_function cpu_fprintf,
> - uint32_t logical, uint32_t physical,
> +static void print_address_zone(uint32_t logical, uint32_t physical,
> uint32_t size, int attr)
> {
> -cpu_fprintf(f, "%08x - %08x -> %08x - %08x %c ",
> +qemu_printf("%08x - %08x -> %08x - %08x %c ",
> logical, logical + size - 1,
> physical, physical + size - 1,
> attr & 4 ? 'W' : '-');
> size >>= 10;
> if (size < 1024) {
> -cpu_fprintf(f, "(%d KiB)\n", size);
> +qemu_printf("(%d KiB)\n", size);
> } else {
> size >>= 10;
> if (size < 1024) {
> -cpu_fprintf(f, "(%d MiB)\n", size);
> +qemu_printf("(%d MiB)\n", size);
> } else {
> size >>= 10;
> -cpu_fprintf(f, "(%d GiB)\n", size);
> +qemu_printf("(%d GiB)\n", size);
> }
> }
> }
>
> -static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
> - CPUM68KState *env, uint32_t root_pointer)
> +static void dump_address_map(CPUM68KState *env, uint32_t root_pointer)
> {
> int i, j, k;
> int tic_size, tic_shift;
> @@ -454,7 +452,7 @@ static void dump_address_map(FILE *f, fprintf_function
> cpu_fprintf,
> if (first_logical != 0x) {
> size = last_logical + (1 << tic_shift) -
> first_logical;
> -print_address_zone(f, cpu_fprintf, first_logical,
> +print_address_zone(first_logical,
> first_physical, size, last_attr);
> }
> first_logical = logical;
> @@ -465,126 +463,125 @@ static void dump_address_map(FILE *f,
> fprintf_function cpu_fprintf,
> }
> if (first_logical != logical || (attr & 4) != (last_attr & 4)) {
> size = logical + (1 << tic_shift) - first_logical;
> -print_address_zone(f, cpu_fprintf, first_logical, first_physical,
> size,
> - last_attr);
> +print_address_zone(first_logical, first_physical, size, last_attr);
> }
> }
>
> #define DUMP_CACHEFLAGS(a) \
> switch (a & M68K_DESC_CACHEMODE) { \
> case M68K_DESC_CM_WRTHRU: /* cachable, write-through */ \
> -cpu_fprintf(f, "T"); \
> +qemu_printf("T"); \
> break; \
> case M68K_DESC_CM_COPYBK: /* cachable, copyback */ \
> -
Re: [Qemu-devel] [PATCH v2] vl: set LC_CTYPE early in main() for all code
On Mon, Apr 15, 2019 at 10:25:03AM -0500, Eric Blake wrote:
> On 4/15/19 9:25 AM, Peter Maydell wrote:
> > On Mon, 15 Apr 2019 at 15:17, Daniel P. Berrangé
> > wrote:
> >>
>
> > A few typo nits below...
>
> Also:
>
> >
> >>
> >> +/*
> >> + * Ideally we would set LC_ALL, but QEMU currently isn't able to cope
> >> + * with arbitrary localization settings. In particular there are two
> >> + * known problems
>
> "two known problems" but...
>
> >> + *
> >> + * - The QMP monitor needs to use the C locale rules for numeric
> >> + * formatting. This would need a double/int -> string formatter
> >> + * that is locale independant.
> >
> > "independent"
> >
> >> + *
> >> + * - The QMP monitor needs to encode all data as UTF-8. This needs
> >> + * to be updated to use iconv(3) to explicitly convert the current
> >> + * locale's charset into utf-8
> >> + *
> >> + * - Lots of codes uses is{upper,lower,alnum,...} functions,
> >> expecting
> >
> > "code"
> >
> >> + * C locale sorting behaviour. Most QEMU usage should likely be
> >> + * changed to g_ascii_is{upper,lower,alnum...} to match code
> >> + * assumptions, without being broken by locale settnigs.
> >
> > "settings"
>
> ...three bullet points.
Heh, I knew I should have used "some" instead of "two" earlier when
first writing this. It was inevitable I'd think of another problem
to add :-)
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2] vl: set LC_CTYPE early in main() for all code
On Mon, 15 Apr 2019 at 16:25, Eric Blake wrote: > "two known problems" but... > > ...three bullet points. Nobody expects the Spanish Inquisition to be code reviewed :-) thanks -- PMM
[Qemu-devel] [PATCH] socket: allow wait=false for client socket
Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
is a bit too strict. Current libvirt always set wait=false, and will
thus fail to add client chardev.
Make the code more permissive, allowing wait=false with client socket
chardevs.
Fixes: 767abe7f49e8be14d29da5db3527817b5d696a52
Cc: Daniel P. Berrangé
Signed-off-by: Marc-André Lureau
---
chardev/char-socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3916505d67..2ad9c9eea5 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1262,7 +1262,7 @@ static bool qmp_chardev_validate_socket(ChardevSocket
*sock,
error_setg(errp, "%s", "Websocket client is not implemented");
return false;
}
-if (sock->has_wait) {
+if (sock->has_wait && sock->wait) {
error_setg(errp, "%s",
"'wait' option is incompatible with "
"socket in client connect mode");
--
2.21.0.313.ge35b8cb8e2
[Qemu-devel] [PATCH 1/3] usb-mtp: fix string length for filename when writing metadata
The ObjectInfo 'length' field provides the length of the
wide character string filename. This is then converted to
a multi-byte character string. This may have a different
byte count to the wide character string. We should use the
C string length of the multi-byte string instead.
Signed-off-by: Daniel P. Berrangé
---
hw/usb/dev-mtp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index ebf210fbf8..838cd74da6 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1714,7 +1714,7 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t
dlen)
return;
}
-o = usb_mtp_object_lookup_name(p, filename, dataset->length);
+o = usb_mtp_object_lookup_name(p, filename, -1);
if (o != NULL) {
next_handle = o->handle;
}
--
2.20.1
[Qemu-devel] [PATCH 2/3] usb-mtp: fix bounds check for guest provided filename
The ObjectInfo struct has a variable length array containing the UTF-16
encoded filename. The number of characters of trailing data is given by
the 'length' field in the struct and this must be validated against the
size of the data packet received from the guest.
Since the data is UTF-16, we must convert the byte count we have to a
character count before validating. This must take care to truncate if
a malicious guest sent an odd number of bytes.
Signed-off-by: Daniel P. Berrangé
---
hw/usb/dev-mtp.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 838cd74da6..6b7d1296e4 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1699,12 +1699,19 @@ static void usb_mtp_write_metadata(MTPState *s,
uint64_t dlen)
MTPObject *o;
MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle);
uint32_t next_handle = s->next_handle;
+size_t filename_chars = dlen - offsetof(ObjectInfo, filename);
+
+/*
+ * filename is utf-16. We're intentionally doing
+ * integer division to truncate if malicious guest
+ * sent an odd number of bytes.
+ */
+filename_chars /= 2;
assert(!s->write_pending);
assert(p != NULL);
-filename = utf16_to_str(MIN(dataset->length,
-dlen - offsetof(ObjectInfo, filename)),
+filename = utf16_to_str(MIN(dataset->length, filename_chars),
dataset->filename);
if (strchr(filename, '/')) {
--
2.20.1
[Qemu-devel] [PATCH 3/3] usb-mtp: fix alignment of access of ObjectInfo filename field
The ObjectInfo struct's "filename" field is following a uint8_t
field in a packed struct and thus has bad alignment for a 16-bit
field. Switch the field to to uint8_t and use the helper function
for accessing unaligned 16-bit data.
Note that although the MTP spec specifies big endian, when transported
over the USB protocol, data is little endian.
Signed-off-by: Daniel P. Berrangé
---
hw/usb/dev-mtp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6b7d1296e4..963449ec7d 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -226,7 +226,7 @@ typedef struct {
uint32_t assoc_desc;
uint32_t seq_no; /*unused*/
uint8_t length; /*part of filename field*/
-uint16_t filename[0];
+uint8_t filename[0]; /* UTF-16 encoded */
char date_created[0]; /*unused*/
char date_modified[0]; /*unused*/
char keywords[0]; /*unused*/
@@ -1551,7 +1551,7 @@ static void usb_mtp_cancel_packet(USBDevice *dev,
USBPacket *p)
fprintf(stderr, "%s\n", __func__);
}
-static char *utf16_to_str(uint8_t len, uint16_t *arr)
+static char *utf16_to_str(uint8_t len, uint8_t *str16)
{
wchar_t *wstr = g_new0(wchar_t, len + 1);
int count, dlen;
@@ -1559,7 +1559,7 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
for (count = 0; count < len; count++) {
/* FIXME: not working for surrogate pairs */
-wstr[count] = (wchar_t)arr[count];
+wstr[count] = lduw_le_p(str16 + (count * 2));
}
wstr[count] = 0;
--
2.20.1
[Qemu-devel] [PATCH 0/3] usb-mtp: fix ObjectInfo request handling
Two previous attempts to fix this due to GCC 9 highlighting unaligned data access. My attempt: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html And a previous one: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html There are a number of bugs in the USB MTP usb_mtp_write_metadata method handling the filename character set conversion. The 2nd patch in this series is a security flaw fix since the code was not correctly validating guest provided data length. I've been unable to figure out how to exercise the codepath that calls usb_mtp_write_metadata. At a guess, it looks like something that should be called when writing to a file from a guest, but the GNOME GVFS MTP driver doesn't provide write support. Using the command line MTP tools "mtp-sendfile" command results in an protocol error # mtp-sendfile foo eek.txt libmtp version: 1.1.14 Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.14. Please report this VID/PID and the device model to the libmtp development team PTP_ERROR_IO: failed to open session, trying again after resetting USB interface LIBMTP libusb: Attempt to reset device Sending foo to eek.txt type: , 44 Sending file... Error sending file. Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send object info. Error 2: Error 02ff: PTP I/O Error ERROR: Could not close session! And QEMU tracing show unexpected requests 26582@1555340076151600935 usb_mtp_command dev 4, code 0x9803, trans 0x18, args 0x11, 0xdc04, 0x0, 0x0, 0x0 26582@1555340076151619955 usb_mtp_xfer dev 4, ep 2, 20/20 26582@1555340076154138556 usb_mtp_data_in dev 4, trans 0x18, len 8 26582@1555340076154150689 usb_mtp_xfer dev 4, ep 1, 20/512 26582@1555340076156654311 usb_mtp_success dev 4, trans 0x18, args 0x0, 0x0 26582@1555340076156667764 usb_mtp_xfer dev 4, ep 1, 12/512 26582@1555340076159215930 usb_mtp_command dev 4, code 0x100c, trans 0x19, args 0x10001, 0xc, 0x0, 0x0, 0x0 26582@1555340076159229610 usb_mtp_xfer dev 4, ep 2, 20/20 26582@1555340076164166196 usb_mtp_stall dev 4, reason: awaiting data-out 26582@1555340076167156367 usb_mtp_stall dev 4, reason: transaction inflight 26582@1555340076170108336 usb_mtp_stall dev 4, reason: unknown control request 26582@1555340076172606798 usb_mtp_stall dev 4, reason: unknown control request Perhaps a Windows guest can exercise this, but I don't have a modern Windows install with MTP support. Thus this series is merely compile tested. Daniel P. Berrangé (3): usb-mtp: fix string length for filename when writing metadata usb-mtp: fix bounds check for guest provided filename usb-mtp: fix alignment of access of ObjectInfo filename field hw/usb/dev-mtp.c | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) -- 2.20.1
Re: [Qemu-devel] [PATCH 09/17] target: Simplify how the TARGET_cpu_list() print
* Markus Armbruster (arm...@redhat.com) wrote:
> The various TARGET_cpu_list() take an fprintf()-like callback and a
> FILE * to pass to it. Their callers (vl.c's main() via list_cpus(),
> bsd-user/main.c's main(), linux-user/main.c's main()) all pass
> fprintf() and stdout. Thus, the flexibility provided by the (rather
> tiresome) indirection isn't actually used.
>
> Drop the callback, and call qemu_fprintf() instead.
Actually calling qemu_printf
> Calling printf() would also work, but would make the code unsuitable
> for monitor context without making it simpler.
Gernally OK; but just checking - are there any flag combos that will
mean this ends up with the result going down a monitor rather than
stdout, and will that upset something like libvirt that might be using
this to enumerate a cpu list?
Dave
> Signed-off-by: Markus Armbruster
> ---
> bsd-user/main.c | 2 +-
> cpus.c | 4 +--
> include/exec/cpu-common.h| 13 -
> include/sysemu/cpus.h| 3 +-
> linux-user/main.c| 2 +-
> target/alpha/cpu.c | 15 --
> target/alpha/cpu.h | 2 +-
> target/arm/cpu.c | 1 -
> target/arm/cpu.h | 2 +-
> target/arm/helper.c | 15 --
> target/cris/cpu.c| 14 -
> target/cris/cpu.h| 2 +-
> target/hppa/cpu.c| 14 -
> target/hppa/cpu.h| 2 +-
> target/i386/cpu.c| 29 ---
> target/i386/cpu.h| 2 +-
> target/lm32/cpu.c| 14 -
> target/lm32/cpu.h| 2 +-
> target/m68k/cpu.h| 2 +-
> target/m68k/helper.c | 14 +++--
> target/mips/cpu.h| 2 +-
> target/mips/translate.c | 1 +
> target/mips/translate_init.inc.c | 5 ++--
> target/openrisc/cpu.c| 15 --
> target/openrisc/cpu.h| 2 +-
> target/ppc/cpu.h | 2 +-
> target/ppc/translate_init.inc.c | 26 +++--
> target/riscv/cpu.c | 17 +++
> target/riscv/cpu.h | 2 +-
> target/s390x/cpu.h | 2 +-
> target/s390x/cpu_models.c| 21 ++
> target/sh4/cpu.c | 17 +++
> target/sh4/cpu.h | 2 +-
> target/sparc/cpu.c | 49 +++-
> target/sparc/cpu.h | 2 +-
> target/tricore/cpu.h | 2 +-
> target/tricore/helper.c | 15 --
> target/xtensa/cpu.h | 2 +-
> target/xtensa/helper.c | 7 +++--
> vl.c | 2 +-
> 40 files changed, 129 insertions(+), 218 deletions(-)
>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 0d3156974c..1b4a2f8693 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -817,7 +817,7 @@ int main(int argc, char **argv)
> if (is_help_option(cpu_model)) {
> /* XXX: implement xxx_cpu_list for targets that still miss it */
> #if defined(cpu_list)
> -cpu_list(stdout, &fprintf);
> +cpu_list();
> #endif
> exit(1);
> }
> diff --git a/cpus.c b/cpus.c
> index 684aa9679a..b4eecf70f0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2181,11 +2181,11 @@ int vm_stop_force_state(RunState state)
> }
> }
>
> -void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
> +void list_cpus(const char *optarg)
> {
> /* XXX: implement xxx_cpu_list for targets that still miss it */
> #if defined(cpu_list)
> -cpu_list(f, cpu_fprintf);
> +cpu_list();
> #endif
> }
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index cef8b88a2a..848a4b94ab 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -9,19 +9,6 @@
>
> #include "qemu/bswap.h"
> #include "qemu/queue.h"
> -#include "qemu/fprintf-fn.h"
> -
> -/**
> - * CPUListState:
> - * @cpu_fprintf: Print function.
> - * @file: File to print to using @cpu_fprint.
> - *
> - * State commonly used for iterating over CPU models.
> - */
> -typedef struct CPUListState {
> -fprintf_function cpu_fprintf;
> -FILE *file;
> -} CPUListState;
>
> /* The CPU list lock nests outside page_(un)lock or mmap_(un)lock */
> void qemu_init_cpu_list(void);
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index ef13a120cc..32c05f27e7 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -1,7 +1,6 @@
> #ifndef QEMU_CPUS_H
> #define QEMU_CPUS_H
>
> -#include "qemu/fprintf-fn.h"
> #include "qemu/timer.h"
>
> /* cpus.c */
> @@ -39,7 +38,7 @@ extern int smp_cores;
> extern int smp_threads;
> #endif
>
> -void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> +void list_cpus(const char *optarg);
>
> void qemu
[Qemu-devel] [RFC] iotests: Fix iotests 110 and 126
A recent patch results in qemu-img reporting the backing file format of
vmdk images as vmdk. This broke iotests 110 and 126.
Fixes: "vmdk: Set vmdk parent backing_format to vmdk"
Signed-off-by: Max Reitz
---
RFC because:
(1) I'd prefer for this patch to be squashed into the patch mentioned
above so we avoid broken iotests.
(2) Otherwise, I'd like for the "Fixes" line to contain a commit ID,
which isn't stable yet, though.
I'd like to mention that these aren't the only broken iotests on
block-next; 059 (with vmdk) is broken, too. I won't send a patch for
that, though, because to me the whole patch doesn't seem quite right
when compiling qemu with -m32.
---
tests/qemu-iotests/110 | 10 +++---
tests/qemu-iotests/126 | 10 +++---
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index fad672c1ae..33b169ffd4 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -53,8 +53,12 @@ TEST_IMG="$TEST_IMG.base" _make_test_img 64M
_make_test_img -b "$TEST_IMG_REL.base" 64M
# qemu should be able to reconstruct the filename, so relative backing names
# should work
+# (We have to filter the backing file format because vmdk always
+# reports it (as vmdk), whereas other image formats would do so only
+# with the backing_fmt creation option, which neither vmdk nor qcow
+# support)
TEST_IMG="json:{'driver':'$IMGFMT','file':{'driver':'file','filename':'$TEST_IMG'}}"
\
-_img_info | _filter_img_info
+_img_info | _filter_img_info | grep -v 'backing file format'
echo
echo '=== Non-reconstructable filename ==='
@@ -78,7 +82,7 @@ TEST_IMG="json:{
}
]
}
-}" _img_info | _filter_img_info
+}" _img_info | _filter_img_info | grep -v 'backing file format'
echo
echo '=== Backing name is always relative to the backed image ==='
@@ -110,7 +114,7 @@ TEST_IMG="json:{
}
]
}
-}" _img_info | _filter_img_info
+}" _img_info | _filter_img_info | grep -v 'backing file format'
# success, all done
diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
index 96dc048d59..e3ee65c606 100755
--- a/tests/qemu-iotests/126
+++ b/tests/qemu-iotests/126
@@ -62,8 +62,12 @@ TOP_IMG="$TEST_DIR/image:top.$IMGFMT"
TEST_IMG=$BASE_IMG _make_test_img 64M
TEST_IMG=$TOP_IMG _make_test_img -b ./image:base.$IMGFMT
-# The default cluster size depends on the image format
-TEST_IMG=$TOP_IMG _img_info | grep -v 'cluster_size'
+# (1) The default cluster size depends on the image format
+# (2) vmdk only supports vmdk backing files, so it always reports the
+# format of its backing file as such (but neither it nor qcow
+# support the backing_fmt creation option, so we cannot use that to
+# harmonize the output across all image formats this test supports)
+TEST_IMG=$TOP_IMG _img_info | grep -ve 'cluster_size' -e 'backing file format'
_rm_test_img "$BASE_IMG"
_rm_test_img "$TOP_IMG"
@@ -79,7 +83,7 @@ TOP_IMG="file:image:top.$IMGFMT"
TEST_IMG=$BASE_IMG _make_test_img 64M
TEST_IMG=$TOP_IMG _make_test_img -b "$BASE_IMG"
-TEST_IMG=$TOP_IMG _img_info | grep -v 'cluster_size'
+TEST_IMG=$TOP_IMG _img_info | grep -ve 'cluster_size' -e 'backing file format'
_rm_test_img "$BASE_IMG"
_rm_test_img "image:top.$IMGFMT"
--
2.20.1
Re: [Qemu-devel] [PATCH v4 4/6] vfio-ccw: add capabilities chain
On 03/01/2019 04:39 AM, Cornelia Huck wrote: Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel. Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_ops.c | 186 drivers/s390/cio/vfio_ccw_private.h | 38 ++ include/uapi/linux/vfio.h | 2 + 3 files changed, 200 insertions(+), 26 deletions(-) Reviewed-by: Farhan Ali
Re: [Qemu-devel] [PATCH v2] vl: set LC_CTYPE early in main() for all code
On 4/15/19 9:25 AM, Peter Maydell wrote:
> On Mon, 15 Apr 2019 at 15:17, Daniel P. Berrangé wrote:
>>
> A few typo nits below...
Also:
>
>>
>> +/*
>> + * Ideally we would set LC_ALL, but QEMU currently isn't able to cope
>> + * with arbitrary localization settings. In particular there are two
>> + * known problems
"two known problems" but...
>> + *
>> + * - The QMP monitor needs to use the C locale rules for numeric
>> + * formatting. This would need a double/int -> string formatter
>> + * that is locale independant.
>
> "independent"
>
>> + *
>> + * - The QMP monitor needs to encode all data as UTF-8. This needs
>> + * to be updated to use iconv(3) to explicitly convert the current
>> + * locale's charset into utf-8
>> + *
>> + * - Lots of codes uses is{upper,lower,alnum,...} functions, expecting
>
> "code"
>
>> + * C locale sorting behaviour. Most QEMU usage should likely be
>> + * changed to g_ascii_is{upper,lower,alnum...} to match code
>> + * assumptions, without being broken by locale settnigs.
>
> "settings"
...three bullet points.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 6/6] vfio-ccw: add handling for async channel instructions
On 03/01/2019 04:39 AM, Cornelia Huck wrote: Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch. Interrupt status continues to be reported through the same channels as for ssch. Signed-off-by: Cornelia Huck Reviewed-by: Farhan Ali
Re: [Qemu-devel] [PATCH v1 3/5] hppa: drop usage of memory_region_allocate_system_memory() for ROM
On 4/15/19 3:27 PM, Igor Mammedov wrote: > machine_hppa_init() violates memory_region_allocate_system_memory() contract > by calling it multiple times which could break with -mem-path. Replace > the second usage (for 'rom') with memory_region_init_ram() instead. > > Signed-off-by: Igor Mammedov > --- > hw/hppa/machine.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c > index d1b1d3c..b413913 100644 > --- a/hw/hppa/machine.c > +++ b/hw/hppa/machine.c > @@ -158,9 +158,8 @@ static void machine_hppa_init(MachineState *machine) > g_free(firmware_filename); > > rom_region = g_new(MemoryRegion, 1); > -memory_region_allocate_system_memory(rom_region, OBJECT(machine), > - "firmware", > - (FIRMWARE_END - FIRMWARE_START)); > +memory_region_init_ram(rom_region, NULL, "firmware", > + (FIRMWARE_END - FIRMWARE_START), &error_fatal); > memory_region_add_subregion(addr_space, FIRMWARE_START, rom_region); > > /* Load kernel */ > Reviewed-by: Philippe Mathieu-Daudé
[Qemu-devel] [Bug 1788582] Re: Race condition during shutdown
Do you find the cause of the bug and fix it?
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1788582
Title:
Race condition during shutdown
Status in QEMU:
New
Bug description:
I ran into a bug when I started several VMs in parallel using
libvirt. The VMs are using only a kernel and a initrd (which includes a
minimal OS). The guest OS itself does a 'poweroff -f' as soon as the
login prompt shows up. So the expectaction is that the VMs will start,
the shutdown will be initiated, and the QEMU processes will then
end. But instead some of the QEMU processes get stuck in ppoll().
A bisect showed that the first bad commit was
0f12264e7a41458179ad10276a7c33c72024861a ("block: Allow graph changes in
bdrv_drain_all_begin/end sections").
I've already tried the current master
(13b7b188501d419a7d63c016e00065bcc693b7d4)
since the problem might be related
to the commit a1405acddeb0af6625dd9c30e8277b08e0885bd3 ("aio: Do
aio_notify_accept only during blocking aio_poll"). But the bug is still
there. I’ve reproduced the bug on x86_64 and on s390x.
The backtrace of a hanging QEMU process:
(gdb) bt
#0 0x7f5d0e251b36 in ppoll () from target:/lib64/libc.so.6
#1 0x560191052014 in qemu_poll_ns (fds=0x560193b23d60, nfds=5,
timeout=55774838936000) at /home/user/git/qemu/util/qemu-timer.c:334
#2 0x5601910531fa in os_host_main_loop_wait (timeout=55774838936000) at
/home/user/git/qemu/util/main-loop.c:233
#3 0x560191053119 in main_loop_wait (nonblocking=0) at
/home/user/git/qemu/util/main-loop.c:497
#4 0x560190baf454 in main_loop () at /home/user/git/qemu/vl.c:1866
#5 0x560190baa552 in main (argc=71, argv=0x7ffde10e41c8,
envp=0x7ffde10e4408) at /home/user/git/qemu/vl.c:4644
The used domain definition is:
test
716800
2
8
hvm
/var/lib/libvirt/images/vmlinuz-4.14.13-200.fc26.x86_64
/var/lib/libvirt/images/test-image-qemux86_64+modules-4.14.13-200.fc26.x86_64.cpio.gz
console=hvc0 STARTUP=shutdown.sh
destroy
restart
preserve
/usr/local/qemu/master/bin/qemu-system-x86_64
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1788582/+subscriptions
Re: [Qemu-devel] [PATCH v1 1/5] sparc64: use memory_region_allocate_system_memory() only for '-m' specified RAM
On 4/15/19 3:27 PM, Igor Mammedov wrote:
> memory_region_allocate_system_memory() was designed to be called for
> allocating inital RAM. Using it mutiple times within one board is not
> supported and could fail if -mem-path with non hugepage path is used.
>
> Keep using memory_region_allocate_system_memory() only for initial
> RAM and use memory_region_init_ram() for the rest fixed size regions.
>
> Signed-off-by: Igor Mammedov
> ---
> hw/sparc64/niagara.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> index f8a856f..39a4522 100644
> --- a/hw/sparc64/niagara.c
> +++ b/hw/sparc64/niagara.c
> @@ -37,6 +37,7 @@
> #include "sysemu/block-backend.h"
> #include "qemu/error-report.h"
> #include "sysemu/qtest.h"
> +#include "qapi/error.h"
>
>
> typedef struct NiagaraBoardState {
> @@ -108,8 +109,8 @@ static void niagara_init(MachineState *machine)
> /* init CPUs */
> sparc64_cpu_devinit(machine->cpu_type, NIAGARA_PROM_BASE);
> /* set up devices */
> -memory_region_allocate_system_memory(&s->hv_ram, NULL, "sun4v-hv.ram",
> - NIAGARA_HV_RAM_SIZE);
> +memory_region_init_ram(&s->hv_ram, NULL, "sun4v-hv.ram",
> + NIAGARA_HV_RAM_SIZE, &error_fatal);
> memory_region_add_subregion(sysmem, NIAGARA_HV_RAM_BASE, &s->hv_ram);
>
> memory_region_allocate_system_memory(&s->partition_ram, NULL,
Using the partition RAM as system memory rather than HV one seems the
correct choice.
Reviewed-by: Philippe Mathieu-Daudé
> @@ -118,17 +119,17 @@ static void niagara_init(MachineState *machine)
> memory_region_add_subregion(sysmem, NIAGARA_PARTITION_RAM_BASE,
> &s->partition_ram);
>
> -memory_region_allocate_system_memory(&s->nvram, NULL,
> - "sun4v.nvram", NIAGARA_NVRAM_SIZE);
> +memory_region_init_ram(&s->nvram, NULL, "sun4v.nvram",
> NIAGARA_NVRAM_SIZE,
> + &error_fatal);
> memory_region_add_subregion(sysmem, NIAGARA_NVRAM_BASE, &s->nvram);
> -memory_region_allocate_system_memory(&s->md_rom, NULL,
> - "sun4v-md.rom",
> NIAGARA_MD_ROM_SIZE);
> +memory_region_init_ram(&s->md_rom, NULL, "sun4v-md.rom",
> + NIAGARA_MD_ROM_SIZE, &error_fatal);
> memory_region_add_subregion(sysmem, NIAGARA_MD_ROM_BASE, &s->md_rom);
> -memory_region_allocate_system_memory(&s->hv_rom, NULL,
> - "sun4v-hv.rom",
> NIAGARA_HV_ROM_SIZE);
> +memory_region_init_ram(&s->hv_rom, NULL, "sun4v-hv.rom",
> + NIAGARA_HV_ROM_SIZE, &error_fatal);
> memory_region_add_subregion(sysmem, NIAGARA_HV_ROM_BASE, &s->hv_rom);
> -memory_region_allocate_system_memory(&s->prom, NULL,
> - "sun4v.prom", PROM_SIZE_MAX);
> +memory_region_init_ram(&s->prom, NULL, "sun4v.prom", PROM_SIZE_MAX,
> + &error_fatal);
> memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
>
> add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
> @@ -145,8 +146,8 @@ static void niagara_init(MachineState *machine)
> BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> int size = blk_getlength(blk);
> if (size > 0) {
> -memory_region_allocate_system_memory(&s->vdisk_ram, NULL,
> - "sun4v_vdisk.ram", size);
> +memory_region_init_ram(&s->vdisk_ram, NULL, "sun4v_vdisk.ram",
> size,
> + &error_fatal);
> memory_region_add_subregion(get_system_memory(),
> NIAGARA_VDISK_BASE, &s->vdisk_ram);
> dinfo->is_default = 1;
>
Re: [Qemu-devel] [PATCH v4 6/6] vfio-ccw: add handling for async channel instructions
On 3/1/19 4:39 AM, Cornelia Huck wrote:
Add a region to the vfio-ccw device that can be used to submit
asynchronous I/O instructions. ssch continues to be handled by the
existing I/O region; the new region handles hsch and csch.
Interrupt status continues to be reported through the same channels
as for ssch.
Signed-off-by: Cornelia Huck
This all looks pretty sensible to me. Sorry my interminable delays!
Acked-by: Eric Farman
---
drivers/s390/cio/Makefile | 3 +-
drivers/s390/cio/vfio_ccw_async.c | 88
drivers/s390/cio/vfio_ccw_drv.c | 46 ---
drivers/s390/cio/vfio_ccw_fsm.c | 119 +++-
drivers/s390/cio/vfio_ccw_ops.c | 13 ++-
drivers/s390/cio/vfio_ccw_private.h | 5 ++
include/uapi/linux/vfio.h | 2 +
include/uapi/linux/vfio_ccw.h | 12 +++
8 files changed, 270 insertions(+), 18 deletions(-)
create mode 100644 drivers/s390/cio/vfio_ccw_async.c
diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index f230516abb96..f6a8db04177c 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o
qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
obj-$(CONFIG_QDIO) += qdio.o
-vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o
+vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
+ vfio_ccw_async.o
obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o
diff --git a/drivers/s390/cio/vfio_ccw_async.c
b/drivers/s390/cio/vfio_ccw_async.c
new file mode 100644
index ..8c1d2357ef5b
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Async I/O region for vfio_ccw
+ *
+ * Copyright Red Hat, Inc. 2019
+ *
+ * Author(s): Cornelia Huck
+ */
+
+#include
+#include
+
+#include "vfio_ccw_private.h"
+
+static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
+ char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+ loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+ struct ccw_cmd_region *region;
+ int ret;
+
+ if (pos + count > sizeof(*region))
+ return -EINVAL;
+
+ mutex_lock(&private->io_mutex);
+ region = private->region[i].data;
+ if (copy_to_user(buf, (void *)region + pos, count))
+ ret = -EFAULT;
+ else
+ ret = count;
+ mutex_unlock(&private->io_mutex);
+ return ret;
+}
+
+static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+ loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+ struct ccw_cmd_region *region;
+ int ret;
+
+ if (pos + count > sizeof(*region))
+ return -EINVAL;
+
+ if (!mutex_trylock(&private->io_mutex))
+ return -EAGAIN;
+
+ region = private->region[i].data;
+ if (copy_from_user((void *)region + pos, buf, count)) {
+ ret = -EFAULT;
+ goto out_unlock;
+ }
+
+ vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ASYNC_REQ);
+
+ ret = region->ret_code ? region->ret_code : count;
+
+out_unlock:
+ mutex_unlock(&private->io_mutex);
+ return ret;
+}
+
+static void vfio_ccw_async_region_release(struct vfio_ccw_private *private,
+ struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_async_region_ops = {
+ .read = vfio_ccw_async_region_read,
+ .write = vfio_ccw_async_region_write,
+ .release = vfio_ccw_async_region_release,
+};
+
+int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private)
+{
+ return vfio_ccw_register_dev_region(private,
+ VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD,
+ &vfio_ccw_async_region_ops,
+ sizeof(struct ccw_cmd_region),
+ VFIO_REGION_INFO_FLAG_READ |
+ VFIO_REGION_INFO_FLAG_WRITE,
+ private->cmd_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 5ea0da1dd954..c39d01943a6a 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -3,9 +3,11 @@
* VFIO based Physical Subchannel device driver
*
* Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2019
*
* Author(s): Dong Jia Shi
*Xiao Fen
Re: [Qemu-devel] How live migration work for vhost-user
* fengyd (fengy...@gmail.com) wrote: > Hi, > > During live migration, the folloing log can see in nova-compute.log in my > environment: > ERROR nova.virt.libvirt.driver [req-039a85e1-e7a1-4a63-bc6d-c4b9a044aab6 > 0cdab20dc79f4bc6ae5790e7b4a898ac 3363c319773549178acc67f32c78310e - default > default] [instance: 5ec719f4-1865-4afe-a207-3d9fae22c410] Live Migration > failure: internal error: qemu unexpectedly closed the monitor: > 2019-04-15T02:58:22.213897Z qemu-kvm: VQ 0 > size 0x100 < last_avail_idx 0x1e - used_idx 0x23 > > It's OK for standard Linux VM, but not OK for our VM where virtio is > implemented by ourself. > KVM version as follow: > qemu-kvm-common-ev-2.12.0-18.el7_6.3.1.x86_64 > qemu-kvm-ev-2.12.0-18.el7_6.3.1.x86_64 > libvirt-daemon-kvm-3.9.0-14.2.el7.centos.ncir.8.x86_64 > > Do you know what's the difference between virtio and vhost-user during > migration? > The function virtio_load in Qemu is called for virtio and vhost-user during > migration. > For virtio, last_avail_idx and used_idx are stored in Qemu, Qemu is > responsible for updating their values accordingly > For vhost-user, last_avail_idx and used_idx are stored in vhost-user app, > eg. DPDK, not in Qemu? > How does migration work for vhost-user? I don't know the details, but my understanding is that vhost-user tells the vhost-user client about an area of 'log' memory, where the vhost-user client must mark pages as dirty. In the qemu source, see docs/interop/vhost-user.txt and see the VHOST_SET_LOG_BASE and VHOST_USER_SET_LOG_FD calls. If the client correctly marks the areas as dirty, then qemu should resend those pages across. Dave > Thanks in advance > Yafeng -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-devel] [Bug 1805256] Re: qemu-img hangs on high core count ARM system
sorry, I make a spelling mistake here（"Hi, I also found a problem that qemu-img convert hands in ARM."）.The right is "I also found a problem that qemu-img convert hangs in ARM". -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: New Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
[Qemu-devel] [PATCH v6 2/5] block/backup: move to copy_bitmap with granularity
We are going to share this bitmap between backup and backup-top filter
driver, so let's share something more meaningful. It also simplifies
some calculations.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
---
block/backup.c | 48 +++-
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index d9f5db18ac..510fc54f98 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -113,7 +113,8 @@ static int coroutine_fn
backup_cow_with_bounce_buffer(BackupBlockJob *job,
int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
-hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
nbytes = MIN(job->cluster_size, job->len - start);
if (!*bounce_buffer) {
*bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -147,7 +148,7 @@ static int coroutine_fn
backup_cow_with_bounce_buffer(BackupBlockJob *job,
return nbytes;
fail:
-hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+hbitmap_set(job->copy_bitmap, start, job->cluster_size);
return ret;
}
@@ -167,16 +168,15 @@ static int coroutine_fn
backup_cow_with_offload(BackupBlockJob *job,
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+assert(QEMU_IS_ALIGNED(start, job->cluster_size));
nbytes = MIN(job->copy_range_size, end - start);
nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
-hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
- nr_clusters);
+hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
read_flags, write_flags);
if (ret < 0) {
trace_backup_do_cow_copy_range_fail(job, start, ret);
-hbitmap_set(job->copy_bitmap, start / job->cluster_size,
-nr_clusters);
+hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
return ret;
}
@@ -204,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
cow_request_begin(&cow_request, job, start, end);
while (start < end) {
-if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
+if (!hbitmap_get(job->copy_bitmap, start)) {
trace_backup_do_cow_skip(job, start);
start += job->cluster_size;
continue; /* already copied */
@@ -300,6 +300,11 @@ static void backup_clean(Job *job)
assert(s->target);
blk_unref(s->target);
s->target = NULL;
+
+if (s->copy_bitmap) {
+hbitmap_free(s->copy_bitmap);
+s->copy_bitmap = NULL;
+}
}
static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -312,7 +317,6 @@ static void backup_attached_aio_context(BlockJob *job,
AioContext *aio_context)
void backup_do_checkpoint(BlockJob *job, Error **errp)
{
BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
-int64_t len;
assert(block_job_driver(job) == &backup_job_driver);
@@ -322,8 +326,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
return;
}
-len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
-hbitmap_set(backup_job->copy_bitmap, 0, len);
+hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
}
static void backup_drain(BlockJob *job)
@@ -378,16 +381,16 @@ static int coroutine_fn
backup_run_incremental(BackupBlockJob *job)
{
int ret;
bool error_is_read;
-int64_t cluster;
+int64_t offset;
HBitmapIter hbi;
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+while ((offset = hbitmap_iter_next(&hbi)) != -1) {
do {
if (yield_and_check(job)) {
return 0;
}
-ret = backup_do_cow(job, cluster * job->cluster_size,
+ret = backup_do_cow(job, offset,
job->cluster_size, &error_is_read, false);
if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
BLOCK_ERROR_ACTION_REPORT)
@@ -409,12 +412,9 @@ static void
backup_incremental_init_copy_bitmap(BackupBlockJob *job)
while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
&offset, &bytes))
{
-uint64_t cluster = offset / job->cluster_size;
-uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
+hbitmap_set(job->copy_bitmap, offset, bytes);
-hbitmap_set(job->copy_bitmap, cluster, end_c
[Qemu-devel] [PATCH v6 3/5] block/backup: refactor and tolerate unallocated cluster skipping
Split allocation checking to separate function and reduce nesting.
Consider bdrv_is_allocated() fail as allocated area, as copying more
than needed is not wrong (and we do it anyway) and seems better than
fail the whole job. And, most probably we will fail on the next read,
if there are real problem with source.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/backup.c | 60 +++---
1 file changed, 23 insertions(+), 37 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 510fc54f98..298e85f1a9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -377,6 +377,22 @@ static bool coroutine_fn yield_and_check(BackupBlockJob
*job)
return false;
}
+static bool bdrv_is_unallocated_range(BlockDriverState *bs,
+ int64_t offset, int64_t bytes)
+{
+int64_t end = offset + bytes;
+
+while (offset < end && !bdrv_is_allocated(bs, offset, bytes, &bytes)) {
+if (bytes == 0) {
+return true;
+}
+offset += bytes;
+bytes = end - offset;
+}
+
+return offset >= end;
+}
+
static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
{
int ret;
@@ -462,49 +478,19 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
for (offset = 0; offset < s->len;
offset += s->cluster_size) {
bool error_is_read;
-int alloced = 0;
if (yield_and_check(s)) {
break;
}
-if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
-int i;
-int64_t n;
-
-/* Check to see if these blocks are already in the
- * backing file. */
-
-for (i = 0; i < s->cluster_size;) {
-/* bdrv_is_allocated() only returns true/false based
- * on the first set of sectors it comes across that
- * are are all in the same state.
- * For that reason we must verify each sector in the
- * backup cluster length. We end up copying more than
- * needed but at some point that is always the case. */
-alloced =
-bdrv_is_allocated(bs, offset + i,
- s->cluster_size - i, &n);
-i += n;
-
-if (alloced || n == 0) {
-break;
-}
-}
-
-/* If the above loop never found any sectors that are in
- * the topmost image, skip this backup. */
-if (alloced == 0) {
-continue;
-}
-}
-/* FULL sync mode we copy the whole drive. */
-if (alloced < 0) {
-ret = alloced;
-} else {
-ret = backup_do_cow(s, offset, s->cluster_size,
-&error_is_read, false);
+if (s->sync_mode == MIRROR_SYNC_MODE_TOP &&
+bdrv_is_unallocated_range(bs, offset, s->cluster_size))
+{
+continue;
}
+
+ret = backup_do_cow(s, offset, s->cluster_size,
+&error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
BlockErrorAction action =
--
2.18.0
[Qemu-devel] [PATCH v6 1/5] block/backup: simplify backup_incremental_init_copy_bitmap
Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.
Note: move to job->len instead of bitmap size: it should not matter but
less code.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/backup.c | 40
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 9988753249..d9f5db18ac 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -403,43 +403,27 @@ static int coroutine_fn
backup_run_incremental(BackupBlockJob *job)
/* init copy_bitmap from sync_bitmap */
static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
{
-BdrvDirtyBitmapIter *dbi;
-int64_t offset;
-int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
- job->cluster_size);
-
-dbi = bdrv_dirty_iter_new(job->sync_bitmap);
-while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
-int64_t cluster = offset / job->cluster_size;
-int64_t next_cluster;
-
-offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
-if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
-hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-break;
-}
+uint64_t offset = 0;
+uint64_t bytes = job->len;
-offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
- UINT64_MAX);
-if (offset == -1) {
-hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-break;
-}
+while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+ &offset, &bytes))
+{
+uint64_t cluster = offset / job->cluster_size;
+uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
-next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
-hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
-if (next_cluster >= end) {
+hbitmap_set(job->copy_bitmap, cluster, end_cluster - cluster);
+
+offset = end_cluster * job->cluster_size;
+if (offset >= job->len) {
break;
}
-
-bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+bytes = job->len - offset;
}
/* TODO job_progress_set_remaining() would make more sense */
job_progress_update(&job->common.job,
job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
-bdrv_dirty_iter_free(dbi);
}
static int coroutine_fn backup_run(Job *job, Error **errp)
--
2.18.0
[Qemu-devel] [PATCH v6 0/5] backup-top: preparing refactoring
Hi all! Here some refactoring patches, as a first step for backup-top filter introduction. v6: 01: - use end_cluster instead of last_cluster and fix bug in calculation [Max] 02: only rebased on 01, keep r-b 03, 04: new 05: it's rewritten "[PATCH v5 10/11] block/backup: tiny refactor backup_job_create", to make it more useful in general than just code movement inside one function Vladimir Sementsov-Ogievskiy (5): block/backup: simplify backup_incremental_init_copy_bitmap block/backup: move to copy_bitmap with granularity block/backup: refactor and tolerate unallocated cluster skipping block/backup: unify different modes code path block/backup: refactor: split out backup_calculate_cluster_size block/backup.c | 245 + 1 file changed, 106 insertions(+), 139 deletions(-) -- 2.18.0
[Qemu-devel] [PATCH v6 5/5] block/backup: refactor: split out backup_calculate_cluster_size
Split out cluster_size calculation. Move copy-bitmap creation above
block-job creation, as we are going to share it with upcoming
backup-top filter, which also should be created before actual block job
creation.
Also, while being here, drop unnecessary "goto error" from
bdrv_getlength error path.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/backup.c | 84 +++---
1 file changed, 53 insertions(+), 31 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index b54386b699..79db4fbb6d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -507,6 +507,42 @@ static const BlockJobDriver backup_job_driver = {
.drain = backup_drain,
};
+static int64_t backup_calculate_cluster_size(BlockDriverState *target,
+ Error **errp)
+{
+int ret;
+BlockDriverInfo bdi;
+
+/*
+ * If there is no backing file on the target, we cannot rely on COW if our
+ * backup cluster size is smaller than the target cluster size. Even for
+ * targets with a backing file, try to avoid COW if possible.
+ */
+ret = bdrv_get_info(target, &bdi);
+if (ret == -ENOTSUP && !target->backing) {
+/* Cluster size is not defined */
+warn_report("The target block device doesn't provide "
+"information about the block size and it doesn't have a "
+"backing file. The default block size of %u bytes is "
+"used. If the actual block size of the target exceeds "
+"this default, the backup may be unusable",
+BACKUP_CLUSTER_SIZE_DEFAULT);
+return BACKUP_CLUSTER_SIZE_DEFAULT;
+} else if (ret < 0 && !target->backing) {
+error_setg_errno(errp, -ret,
+"Couldn't determine the cluster size of the target image, "
+"which has no backing file");
+error_append_hint(errp,
+"Aborting, since this may create an unusable destination image\n");
+return ret;
+} else if (ret < 0 && target->backing) {
+/* Not fatal; just trudge on ahead. */
+return BACKUP_CLUSTER_SIZE_DEFAULT;
+}
+
+return MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+}
+
BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
BlockDriverState *target, int64_t speed,
MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -518,9 +554,10 @@ BlockJob *backup_job_create(const char *job_id,
BlockDriverState *bs,
JobTxn *txn, Error **errp)
{
int64_t len;
-BlockDriverInfo bdi;
BackupBlockJob *job = NULL;
int ret;
+int64_t cluster_size;
+HBitmap *copy_bitmap = NULL;
assert(bs);
assert(target);
@@ -579,9 +616,16 @@ BlockJob *backup_job_create(const char *job_id,
BlockDriverState *bs,
if (len < 0) {
error_setg_errno(errp, -len, "unable to get length for '%s'",
bdrv_get_device_name(bs));
-goto error;
+return NULL;
+}
+
+cluster_size = backup_calculate_cluster_size(target, errp);
+if (cluster_size < 0) {
+return NULL;
}
+copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+
/* job->len is fixed, so we can't allow resize */
job = block_job_create(job_id, &backup_job_driver, txn, bs,
BLK_PERM_CONSISTENT_READ,
@@ -610,35 +654,9 @@ BlockJob *backup_job_create(const char *job_id,
BlockDriverState *bs,
/* Detect image-fleecing (and similar) schemes */
job->serialize_target_writes = bdrv_chain_contains(target, bs);
-
-/* If there is no backing file on the target, we cannot rely on COW if our
- * backup cluster size is smaller than the target cluster size. Even for
- * targets with a backing file, try to avoid COW if possible. */
-ret = bdrv_get_info(target, &bdi);
-if (ret == -ENOTSUP && !target->backing) {
-/* Cluster size is not defined */
-warn_report("The target block device doesn't provide "
-"information about the block size and it doesn't have a "
-"backing file. The default block size of %u bytes is "
-"used. If the actual block size of the target exceeds "
-"this default, the backup may be unusable",
-BACKUP_CLUSTER_SIZE_DEFAULT);
-job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
-} else if (ret < 0 && !target->backing) {
-error_setg_errno(errp, -ret,
-"Couldn't determine the cluster size of the target image, "
-"which has no backing file");
-error_append_hint(errp,
-"Aborting, since this may create an unusable destination image\n");
-goto error;
-} else if (ret < 0 && target->backing) {
-/* Not fatal; just trudge on ahead. */
-job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAU
