[Bug 1876678] Re: Ubuntu 20.04 KVM / QEMU Failure with nested FreeBSD bhyve
** Summary changed: - Ubuntu 20.04 QEMU Failure with nested FreeBSD bhyve + Ubuntu 20.04 KVM / QEMU Failure with nested FreeBSD bhyve -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1876678 Title: Ubuntu 20.04 KVM / QEMU Failure with nested FreeBSD bhyve Status in QEMU: New Bug description: BUG: Starting FreeBSD Layer 2 bhyve Guest within Layer 1 FreeBSD VM Host on Layer 0 Ubuntu 20.04 KVM / QEMU Host result in Layer 1 Guest / Host Pausing with "Emulation Failure" TESTING: My test scenario is nested virtualisation: Layer 0 - Ubuntu 20.04 Host Layer 1 - FreeBSD 12.1 with OVMF + bhyve hypervisor Guest/Host Layer 2 - FreeBSD 12.1 guest Layer 0 Host is: Ubuntu 20.04 LTS KVM / QEMU / libvirt <> $ virsh -c qemu:///system version --daemon Compiled against library: libvirt 6.0.0 Using library: libvirt 6.0.0 Using API: QEMU 6.0.0 Running hypervisor: QEMU 4.2.0 Running against daemon: 6.0.0 < <> $ cat /proc/cpuinfo | grep -c vmx 64 $ cat /sys/module/kvm_intel/parameters/nested Y <> Layer 1 Guest / Host is: FreeBSD Q35 v4.2 with OVMF: Pass Host VMX support to Layer 1 Guest via hvm /usr/share/OVMF/OVMF_CODE.fd /home/USER/swarm.bhyve.freebsd/OVMF_VARS.fd ... ... > Checked that Layer 1 - FreeBSD Quest / Host has VMX feature available: <> # uname -a FreeBSD swarm.DOMAIN.HERE 12.1-RELEASE FreeBSD 12.1-RELEASE GENERIC amd64 # grep Features /var/run/dmesg.boot Features=0xf83fbff Features2=0xfffa3223 AMD Features=0x2c100800 AMD Features2=0x121 Structured Extended Features=0x1c0fbb Structured Extended Features2=0x4 Structured Extended Features3=0xac000400 XSAVE Features=0x1 < On Layer 1 FreeBSD Guest / Host start up the Layer 2 guest.. <> # ls FreeBSD-11.2-RELEASE-amd64-bootonly.iso FreeBSD-12.1-RELEASE-amd64-dvd1.iso bee-hd1-01.img # /usr/sbin/bhyve -c 2 -m 2048 -H -A -s 0:0,hostbridge -s 1:0,lpc -s 2:0,e1000,tap0 -s 3:0,ahci-hd,bee-hd1-01.img -l com1,stdio -s 5:0,ahci-cd,./FreeBSD-12.1-RELEASE-amd64-dvd1.iso bee <> Result is that Layer 1 - FreeBSD Host guest "paused". To Layer 1 machines freezes I cannot get any further diagnostics from this machine, so I run tail on libvirt log from Layer 0 - Ubuntu Host <> char device redirected to /dev/pts/29 (label charserial0) 2020-05-04T06:09:15.310474Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12] 2020-05-04T06:09:15.310531Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13] 2020-05-04T06:09:15.312533Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12] 2020-05-04T06:09:15.312548Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13] 2020-05-04T06:09:15.313828Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12] 2020-05-04T06:09:15.313841Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13] 2020-05-04T06:09:15.315185Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12] 2020-05-04T06:09:15.315201Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13] KVM internal error. Suberror: 1 emulation failure EAX= EBX= ECX= EDX= ESI= EDI= EBP= ESP= EIP= EFL= [---] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES = 8000 DPL=0 CS = 8000 DPL=0 SS = 8000 DPL=0 DS = 8000 DPL=0 FS = 8000 DPL=0 GS = 8000 DPL=0 LDT= 8000 DPL=0 TR = 8000 DPL=0 GDT= IDT= CR0=80050033 CR2= CR3= CR4=00372060 DR0= DR1= DR2= DR3= DR6=0ff0 DR7=0400 EFER=0d01 Code= ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? 2020-05-04T06:35:39.186799Z qemu-system-x86_64: terminating on signal 15 from pid 2155 (/usr/sbin/libvirtd) 2020-05-04 06:35:39.386+: shutting down, reason=destroyed <> I am reporting this bug here as result is very similar to th
[Bug 1805256] Re: qemu-img hangs on rcu_call_ready_event logic in Aarch64 when converting images
Thanks. I will test 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/1805256 Title: qemu-img hangs on rcu_call_ready_event logic in Aarch64 when converting images Status in kunpeng920: Incomplete Status in QEMU: In Progress Status in qemu package in Ubuntu: Incomplete Status in qemu source package in Bionic: Incomplete Status in qemu source package in Disco: Incomplete Status in qemu source package in Eoan: Incomplete Status in qemu source package in Focal: Incomplete Bug description: Command: qemu-img convert -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2 Hangs indefinitely approximately 30% of the runs. Workaround: qemu-img convert -m 1 -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2 Run "qemu-img convert" with "a single coroutine" to avoid this issue. (gdb) thread 1 ... (gdb) bt #0 0xbf1ad81c in __GI_ppoll #1 0xaabcf73c in ppoll #2 qemu_poll_ns #3 0xaabd0764 in os_host_main_loop_wait #4 main_loop_wait ... (gdb) thread 2 ... (gdb) bt #0 syscall () #1 0xaabd41cc in qemu_futex_wait #2 qemu_event_wait (ev=ev@entry=0xaac86ce8 ) #3 0xaabed05c in call_rcu_thread #4 0xaabd34c8 in qemu_thread_start #5 0xbf25c880 in start_thread #6 0xbf1b6b9c in thread_start () (gdb) thread 3 ... (gdb) bt #0 0xbf11aa20 in __GI___sigtimedwait #1 0xbf2671b4 in __sigwait #2 0xaabd1ddc in sigwait_compat #3 0xaabd34c8 in qemu_thread_start #4 0xbf25c880 in start_thread #5 0xbf1b6b9c in thread_start (gdb) run Starting program: /usr/bin/qemu-img convert -f qcow2 -O qcow2 ./disk01.ext4.qcow2 ./output.qcow2 [New Thread 0xbec5ad90 (LWP 72839)] [New Thread 0xbe459d90 (LWP 72840)] [New Thread 0xbdb57d90 (LWP 72841)] [New Thread 0xacac9d90 (LWP 72859)] [New Thread 0xa7ffed90 (LWP 72860)] [New Thread 0xa77fdd90 (LWP 72861)] [New Thread 0xa6ffcd90 (LWP 72862)] [New Thread 0xa67fbd90 (LWP 72863)] [New Thread 0xa5ffad90 (LWP 72864)] [Thread 0xa5ffad90 (LWP 72864) exited] [Thread 0xa6ffcd90 (LWP 72862) exited] [Thread 0xa77fdd90 (LWP 72861) exited] [Thread 0xbdb57d90 (LWP 72841) exited] [Thread 0xa67fbd90 (LWP 72863) exited] [Thread 0xacac9d90 (LWP 72859) exited] [Thread 0xa7ffed90 (LWP 72860) exited] """ All the tasks left are blocked in a system call, so no task left to call qemu_futex_wake() to unblock thread #2 (in futex()), which would unblock thread #1 (doing poll() in a pipe with thread #2). Those 7 threads exit before disk conversion is complete (sometimes in the beginning, sometimes at the end). [ Original 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/kunpeng920/+bug/1805256/+subscriptions
RE: [PULL 04/20] crypto: Redundant type conversion for AES_KEY pointer
>-Original Message- >From: Daniel P. Berrangé [mailto:berra...@redhat.com] >Sent: Monday, May 4, 2020 8:58 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; Michael Tokarev ; qemu- >triv...@nongnu.org; Laurent Vivier ; Euler Robot > >Subject: Re: [PULL 04/20] crypto: Redundant type conversion for AES_KEY >pointer > >Hi Chen, > >This patch triggered a build failure in QEMU about discarding the "const" >qualifier. > >IOW, the type conversion is not redundant after all - it is required in order >to >explicitly discard "const". > Yes, you are right! Thank you for pointing it out ! It is my carelessness, this patch is not complete. >I believe we can probably fix this by changing >qcrypto_cipher_aes_ecb_(en|de)crypt() methods so that they also have a "const" >qualifier on the AES_KEY parameter. > It's a good point. I will update the patch later. Thanks.
[PATCH] tests/Makefile: Fix description of "make check"
The description of "make check" is out-of-date, so fix it by adding block and softfloat. Signed-off-by: Huacai Chen --- tests/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 03a74b6..5d32239 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -4,7 +4,7 @@ check-help: @echo "Regression testing targets:" @echo - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and decodetree" + @echo " $(MAKE) checkRun block, qapi-schema, unit, softfloat, qtest and decodetree" @echo @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" @echo " $(MAKE) check-qtest Run qtest tests" -- 2.7.0
Re: [PATCH for-5.1 V3 0/7] mips: Add Loongson-3 machine support (with KVM)
Hi, Aleksandar,
On Sun, May 3, 2020 at 6:50 PM Aleksandar Markovic
wrote:
>
> нед, 3. мај 2020. у 12:21 Huacai Chen је написао/ла:
> >
> > Loongson-3 CPU family include Loongson-3A R1/R2/R3/R4 and Loongson-3B
> > R1/R2. Loongson-3A R1 is the oldest and its ISA is the smallest, while
> > Loongson-3A R4 is the newest and its ISA is almost the superset of all
> > others. To reduce complexity, in QEMU we just define two CPU types:
> >
> > 1, "Loongson-3A1000" CPU which is corresponding to Loongson-3A R1. It is
> >suitable for TCG because Loongson-3A R1 has fewest ASE.
> > 2, "Loongson-3A4000" CPU which is corresponding to Loongson-3A R4. It is
> >suitable for KVM because Loongson-3A R4 has the VZ ASE.
> >
>
> Huacai, thanks for putting together v3, which is a little better than v2, and
> thanks for addressing my previous suggestions.
>
> Now, give us some time to digest new data on Loongson3. We will
> respond, but it won't happen immediately, which is, you'd agree,
> reasonable. Just be patient.
>
> But again, in general, I salute your efforts very much!
>
> Yours, Aleksandar
I'm sorry for this late response because I have done many tests to
reproduce the problem reported at
https://patchew.org/QEMU/1588501221-1205-1-git-send-email-che...@lemote.com/,
but I don't have such a failure...
What I have done:
1, "make check" on MIPS64 platform (distro is Fedora28 for Loongson);
2, "make check" on X86_64 platform (distro is RHEL8);
3, "make docker-test-quick@centos7 SHOW_ENV=1 NETWORK=1" on X86_64
platform (distro is RHEL8);
4, "make docker-test-quick@centos7 SHOW_ENV=1 J=n NETWORK=1" on X86_64
platform (distro is RHEL8 and I've tried n=2,3,414);
I always get the same result:
Not run: 259
Passed all 117 iotests
And, it seems that my patchset doesn't touch anything about iotests,
so I don't know why the build test fails on iotests 192 (Maybe your
build test has the same problem without my patches).
P.S.: I have found a problem that my patchset has a build failure with
CONFIG_KVM=n, but this is another problem and I will send V4 to fix it
(after collecting all problems in V3).
>
> > Loongson-3 lacks English documents. I've tried to translated them with
> > translate.google.com, and the machine translated documents (together
> > with their original Chinese versions) are available here.
> >
> > Loongson-3A R1 (Loongson-3A1000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A1000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P1.pdf
> > (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A1000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P2.pdf
> > (Chinese Version)
> >
> > Loongson-3A R2 (Loongson-3A2000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A2000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user1.pdf (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A2000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user2.pdf (Chinese Version)
> >
> > Loongson-3A R3 (Loongson-3A3000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A3000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual1.pdf
> > (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A3000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual2.pdf
> > (Chinese Version)
> >
> > Loongson-3A R4 (Loongson-3A4000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A4000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/3A4000user.pdf (Chinese Version)
> > User Manual Part 2:
> > I'm sorry that it is unavailable now.
> >
> > We are preparing to add QEMU's Loongson-3 support. MIPS VZ extension is
> > fully supported in Loongson-3A R4+, so we at first add QEMU/KVM support
> > in this series. And the next series will add QEMU/TCG support (it will
> > emulate Loongson-3A R1).
> >
> > We already have a full functional Linux kernel (based on Linux-5.4.x LTS
> > but not upstream yet) here:
> >
> > https://github.com/chenhuacai/linux
> >
> > How to use QEMU/Loongson-3?
> > 1, Download kernel source from the above URL;
> > 2, Build a kernel with arch/mips/configs/loongson3_{def,hpc}config;
> > 3, Boot a Loongson-3A4000 host with this kernel;
> > 4, Build QEMU-5.0.0 with this patchset;
> > 5, modprobe kvm;
> > 6, Use QEMU with TCG (available in future):
> >qemu-system-mips64el -M loongson3,accel=tcg -cpu Loongson-3A1000
> > -kernel -append ...
> >Use QEMU with KVM (available at present):
> >qemu-system-mips64el -M loongson3,accel=kvm -cpu Loongson-3A4000
> > -kernel -append ...
> >
> >The "-cpu" parameter can be omitted here and QEMU will use the correct
> > type for TCG/KVM automatically.
> >
> > V1 -> V2:
> > 1, Add a cover letter;
> > 2, Improve CPU definitions;
> > 3, Remove LS7A-related things (Use GPEX instead);
> > 4, Add a description of how to run QEMU/Loongson-3.
> >
Re: [PATCH v5 3/7] qcow: Tolerate backing_fmt=, but warn on backing_fmt=raw
Am 03.04.2020 um 19:58 hat Eric Blake geschrieben:
> qcow has no space in the metadata to store a backing format, and there
> are existing qcow images backed both by raw or by other formats
> (usually qcow) images, reliant on probing to tell the difference.
> While we don't recommend the creation of new qcow images (as qcow2 is
> hands-down better), we can at least insist that if the user does
> request a specific format without using -u, then it must be non-raw
> (as a raw backing file that gets inadvertently edited into some other
> format can form a security hole); if the user does not request a
> specific format or lies when using -u, then the status quo of probing
> for the backing format remains intact (although an upcoming patch will
> warn when omitting a format request). Thus, when this series is
> complete, the only way to use a backing file for qcow without
> triggering a warning is when using -F if the backing file is non-raw
> to begin with. Note that this is only for QemuOpts usage; there is no
> change to the QAPI to allow a format through -blockdev.
>
> Add a new iotest 290 just for qcow, to demonstrate the new warning.
>
> Signed-off-by: Eric Blake
Somehow this feels backwards. Not specifying the backing file format at
all isn't any safer than explicitly specifying raw.
If there is a difference at all, I would say that explicitly specifying
raw means that the user is aware what they are doing. So we would have
more reason to warn against raw images if the backing format isn't
specified at all because then the user might not be aware that they are
using a backing file that probes as raw.
> block/qcow.c | 16 -
> tests/qemu-iotests/290 | 72 ++
> tests/qemu-iotests/290.out | 42 ++
> tests/qemu-iotests/group | 1 +
> 4 files changed, 130 insertions(+), 1 deletion(-)
> create mode 100755 tests/qemu-iotests/290
> create mode 100644 tests/qemu-iotests/290.out
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 8973e4e565a1..bbe48b7db4d7 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -940,11 +940,12 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
> *drv,
> {
> BlockdevCreateOptions *create_options = NULL;
> BlockDriverState *bs = NULL;
> -QDict *qdict;
> +QDict *qdict = NULL;
> Visitor *v;
> const char *val;
> Error *local_err = NULL;
> int ret;
> +char *backing_fmt;
>
> static const QDictRenames opt_renames[] = {
> { BLOCK_OPT_BACKING_FILE, "backing-file" },
> @@ -953,6 +954,13 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
> *drv,
> };
>
> /* Parse options and convert legacy syntax */
> +backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> +if (backing_fmt && !strcmp(backing_fmt, "raw")) {
> +error_setg(errp, "qcow cannot store backing format; an explicit "
> + "backing format of raw is unsafe");
Does this message tell that an implicit backing format of raw is safe?
> +ret = -EINVAL;
> +goto fail;
> +}
The commit message promises a warning. This is not a warning, but a hard
error.
> qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qcow_create_opts, true);
>
> val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
> @@ -1018,6 +1026,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
> *drv,
>
> ret = 0;
> fail:
> +g_free(backing_fmt);
> qobject_unref(qdict);
> bdrv_unref(bs);
> qapi_free_BlockdevCreateOptions(create_options);
> @@ -1152,6 +1161,11 @@ static QemuOptsList qcow_create_opts = {
> .type = QEMU_OPT_STRING,
> .help = "File name of a base image"
> },
> +{
> +.name = BLOCK_OPT_BACKING_FMT,
> +.type = QEMU_OPT_STRING,
> +.help = "Format of the backing image (caution: raw backing is
> unsafe)",
> +},
> {
> .name = BLOCK_OPT_ENCRYPT,
> .type = QEMU_OPT_BOOL,
> diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
> new file mode 100755
> index ..8482de58cb4b
> --- /dev/null
> +++ b/tests/qemu-iotests/290
> @@ -0,0 +1,72 @@
> +#!/usr/bin/env bash
> +#
> +# Test qcow backing file warnings
> +#
> +# Copyright (C) 2020 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this p
Re: [PATCH v5 4/7] qcow2: Deprecate use of qemu-img amend to change backing file
Am 03.04.2020 um 19:58 hat Eric Blake geschrieben: > The use of 'qemu-img amend' to change qcow2 backing files is not > tested very well. In particular, our implementation has a bug where > if a new backing file is provided without a format, then the prior > format is blindly reused, even if this results in data corruption, but > this is not caught by iotests. > > There are also situations where amending other options needs access to > the original backing file (for example, on a downgrade to a v2 image, > knowing whether a v3 zero cluster must be allocated or may be left > unallocated depends on knowing whether the backing file already reads > as zero), but the command line does not have a nice way to tell us > both the backing file to use for opening the image as well as the > backing file to install after the operation is complete. > > Even if we do allow changing the backing file, it is redundant with > the existing ability to change backing files via 'qemu-img rebase -u'. > It is time to deprecate this support (leaving the existing behavior > intact, even if it is buggy), and at a point in the future, require > the use of only 'qemu-img rebase' for adjusting backing chain > relations, saving 'qemu-img amend' for changes unrelated to the > backing chain. > > Signed-off-by: Eric Blake I think the original idea was that in BlockDriver, special interfaces like the ones used by qemu-img rebase could eventually go away if we have a single consistent interface to change image options, which is amend. So ideally, bdrv_change_backing_file() should become a wrapper around amend. I'm not even sure if the behaviour you mention should be considered a bug because amend is a low-level interface. But even if that's the case, it would be a matter of simply adding a check. Kevin
Re: [PATCH] tests/Makefile: Fix description of "make check"
On 5/5/20 9:29 AM, Huacai Chen wrote: > The description of "make check" is out-of-date, so fix it by adding > block and softfloat. > > Signed-off-by: Huacai Chen > --- > tests/Makefile.include | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 03a74b6..5d32239 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -4,7 +4,7 @@ > check-help: > @echo "Regression testing targets:" > @echo > - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and > decodetree" > + @echo " $(MAKE) checkRun block, qapi-schema, unit, > softfloat, qtest and decodetree" > @echo > @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" > @echo " $(MAKE) check-qtest Run qtest tests" > added Cc: qemu-triv...@nongnu.org Reviewed-by: Claudio Fontana
Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size
On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote: > In nvme_create_queue_pair() we create a page list using > qemu_blockalign(), then map it with qemu_vfio_dma_map(): > > q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE); > r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, > s->page_size * NVME_QUEUE_SIZE, ...); > > With: > > s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); > > The qemu_vfio_dma_map() documentation says "The caller need > to make sure the area is aligned to page size". While we use > multiple s->page_size as alignment, it might be not sufficient > on some hosts. Use the qemu_real_host_page_size value to be > sure the host alignment is respected. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Cc: Cédric Le Goater > Cc: David Gibson > Cc: Laurent Vivier > --- > block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 7b7c0cc5d6..bde0d28b39 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char > *device, int namespace, > > s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); > s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t); > -bs->bl.opt_mem_alignment = s->page_size; > +bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size); Why don't you replace directly the "4096" in s->page_size by qemu_real_host_page_size? I think this was the purpose of MAX(4096, ...) to align to the host page size. Thanks, Laurnt
Re: [PATCH v5 7/7] qemu-img: Deprecate use of -b without -F
Am 03.04.2020 um 19:58 hat Eric Blake geschrieben: > Creating an image that requires format probing of the backing image is > inherently unsafe (we've had several CVEs over the years based on > probes leaking information to the guest on a subsequent boot, although > these days tools like libvirt are aware of the issue enough to prevent > the worst effects). However, if our probing algorithm ever changes, > or if other tools like libvirt determine a different probe result than > we do, then subsequent use of that backing file under a different > format will present corrupted data to the guest. Start a deprecation > clock so that future qemu-img can refuse to create unsafe backing > chains that would rely on probing. Most warnings are intentionally > emitted from bdrv_img_create() in the block layer, but qemu-img > convert uses bdrv_create() which cannot emit its own warning without > causing spurious warnings on other code paths. In the end, all > command-line image creation or backing file rewriting now performs a > check. > > However, there is one time where probing is safe: if we probe raw, > then it is safe to record that implicitly in the image (but we still > warn, as it's better to teach the user to supply -F always than to > make them guess when it is safe). You're not stating it explicitly, but I guess the thing that you mean that is actually unsafe is if you have a raw image, always pass format=raw to QEMU (so the guest could write e.g. a qcow2 header), but then create a backing file without -F, so it will be probed. This is as bad as specifying format=raw only sometimes. I don't like the idea of responding to this by making raw images more convenient to use than actual image formats. How about we approach it the other way around: The troublemaker is raw, so let's require specifying raw explicitly, and record the probed format implicitly in other cases. This is a bit weaker in the immediate effect in that it doesn't protect you when you actually deal with a malicious image, but in normal use it will point out where your scripts or management software is too careless. The final result should be that management tools are fixed and you'll be safe, while manual users who can usually trust their guests aren't inconvenienced too much. Kevin
Re: [PATCH v2] Fix iotest 153
Patchew URL: https://patchew.org/QEMU/20200504131959.9533-1-mlevi...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20200504131959.9533-1-mlevi...@redhat.com Subject: [PATCH v2] Fix iotest 153 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 fatal: unable to write new index file Traceback (most recent call last): File "patchew-tester/src/patchew-cli", line 521, in test_one git_clone_repo(clone, r["repo"], r["head"], logf, True) File "patchew-tester/src/patchew-cli", line 57, in git_clone_repo cwd=clone) File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'checkout', 'refs/tags/patchew/20200504131959.9533-1-mlevi...@redhat.com', '-b', 'test']' returned non-zero exit status 128. The full log is available at http://patchew.org/logs/20200504131959.9533-1-mlevi...@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2] Fix iotest 153
Patchew URL: https://patchew.org/QEMU/20200504131959.9533-1-mlevi...@redhat.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504131959.9533-1-mlevi...@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v5] audio/jack: add JACK client audiodev
Patchew URL: https://patchew.org/QEMU/20200504132300.1ae883c1...@aeryn.lan.ktmba/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504132300.1ae883c1...@aeryn.lan.ktmba/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL 00/39] target-arm queue
Patchew URL: https://patchew.org/QEMU/20200504123309.3808-1-peter.mayd...@linaro.org/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504123309.3808-1-peter.mayd...@linaro.org/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL 00/39] target-arm queue
Patchew URL: https://patchew.org/QEMU/20200504123309.3808-1-peter.mayd...@linaro.org/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504123309.3808-1-peter.mayd...@linaro.org/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Patchew URL: https://patchew.org/QEMU/20200504142308.10446-1-be...@igalia.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20200504142308.10446-1-be...@igalia.com Subject: [PATCH v2] qcow2: Avoid integer wraparound in qcow2_co_truncate() 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 fatal: unable to write new index file Traceback (most recent call last): File "patchew-tester/src/patchew-cli", line 521, in test_one git_clone_repo(clone, r["repo"], r["head"], logf, True) File "patchew-tester/src/patchew-cli", line 57, in git_clone_repo cwd=clone) File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'checkout', 'refs/tags/patchew/20200504142308.10446-1-be...@igalia.com', '-b', 'test']' returned non-zero exit status 128. The full log is available at http://patchew.org/logs/20200504142308.10446-1-be...@igalia.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2 0/1] target/arm: Remove access_el3_aa32ns()
Patchew URL: https://patchew.org/QEMU/20200504142125.31180-1-edgar.igles...@gmail.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504142125.31180-1-edgar.igles...@gmail.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size
On 05/05/2020 10:00, Laurent Vivier wrote: > On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote: >> In nvme_create_queue_pair() we create a page list using >> qemu_blockalign(), then map it with qemu_vfio_dma_map(): >> >> q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE); >> r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, >> s->page_size * NVME_QUEUE_SIZE, ...); >> >> With: >> >> s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); >> >> The qemu_vfio_dma_map() documentation says "The caller need >> to make sure the area is aligned to page size". While we use >> multiple s->page_size as alignment, it might be not sufficient >> on some hosts. Use the qemu_real_host_page_size value to be >> sure the host alignment is respected. >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> Cc: Cédric Le Goater >> Cc: David Gibson >> Cc: Laurent Vivier >> --- >> block/nvme.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/nvme.c b/block/nvme.c >> index 7b7c0cc5d6..bde0d28b39 100644 >> --- a/block/nvme.c >> +++ b/block/nvme.c >> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char >> *device, int namespace, >> >> s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); >> s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t); >> -bs->bl.opt_mem_alignment = s->page_size; >> +bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size); > > Why don't you replace directly the "4096" in s->page_size by > qemu_real_host_page_size? > > I think this was the purpose of MAX(4096, ...) to align to the host page > size. oh, I see, page_size is used for a lot of things than can be broken, and this is what is done in bdrv_opt_mem_align() for instance, 4096 is the sector size not the host page size. BTW, you need the same change as in nvme_init() in nvme_refresh_limits(), I think. Thanks, Laurent
Re: Infinite loop in bus_unparent(), qdev bug or qdev misuse?
Patchew URL: https://patchew.org/QEMU/87tv0vzrwj@dusky.pond.sub.org/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/87tv0vzrwj@dusky.pond.sub.org/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: Infinite loop in bus_unparent(), qdev bug or qdev misuse?
Patchew URL: https://patchew.org/QEMU/87tv0vzrwj@dusky.pond.sub.org/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/87tv0vzrwj@dusky.pond.sub.org/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2] tests/qht-bench: Fix Clang 'int-conversion' warning
Patchew URL: https://patchew.org/QEMU/20200504144125.22435-1-phi...@redhat.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504144125.22435-1-phi...@redhat.com/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3] tests/qht-bench: Fix Clang 'implicit-int-float-conversion' warning
Patchew URL: https://patchew.org/QEMU/20200504144352.23021-1-phi...@redhat.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504144352.23021-1-phi...@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL v2 0/4] Block patches
Patchew URL: https://patchew.org/QEMU/20200504151438.362702-1-stefa...@redhat.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504151438.362702-1-stefa...@redhat.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL v2 0/4] Block patches
Patchew URL: https://patchew.org/QEMU/20200504151438.362702-1-stefa...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504151438.362702-1-stefa...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL v2 0/4] Block patches
Patchew URL: https://patchew.org/QEMU/20200504151438.362702-1-stefa...@redhat.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504151438.362702-1-stefa...@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL v2 0/4] Block patches
Patchew URL: https://patchew.org/QEMU/20200504151438.362702-1-stefa...@redhat.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504151438.362702-1-stefa...@redhat.com/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2 00/10] Cadence GEM Fixes
Patchew URL: https://patchew.org/QEMU/1588601168-27576-1-git-send-email-sai.pavan.bo...@xilinx.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1588601168-27576-1-git-send-email-sai.pavan.bo...@xilinx.com Subject: [PATCH v2 00/10] Cadence GEM Fixes 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 fatal: unable to write new index file Traceback (most recent call last): File "patchew-tester/src/patchew-cli", line 521, in test_one git_clone_repo(clone, r["repo"], r["head"], logf, True) File "patchew-tester/src/patchew-cli", line 57, in git_clone_repo cwd=clone) File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'checkout', 'refs/tags/patchew/1588601168-27576-1-git-send-email-sai.pavan.bo...@xilinx.com', '-b', 'test']' returned non-zero exit status 128. The full log is available at http://patchew.org/logs/1588601168-27576-1-git-send-email-sai.pavan.bo...@xilinx.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Patchew URL: https://patchew.org/QEMU/20200504155217.10325-1-be...@igalia.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504155217.10325-1-be...@igalia.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3 00/12] user-mode: Prune build dependencies (part 1)
Patchew URL: https://patchew.org/QEMU/20200504152922.21365-1-phi...@redhat.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504152922.21365-1-phi...@redhat.com/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3 00/12] user-mode: Prune build dependencies (part 1)
Le 04/05/2020 à 17:29, Philippe Mathieu-Daudé a écrit : > This is the first part of a series reducing user-mode > dependencies. By stripping out unused code, the build > and testing time is reduced (as is space used by objects). > > Part 1 (generic): > - reduce user-mode object list > - remove some migration code from user-mode > - remove cpu_get_crash_info() > > This series is fully reviewed. > > Since v2: > - Rebased due to conflict when applying patch: > "util/Makefile: Reduce the user-mode object list" > because commit 01ef6b9e4e modified util/Makefile.objs: > "linux-user: factor out reading of /proc/self/maps" > > Since v1: > - Addressed Laurent/Richard review comments > - Removed 'exec: Drop redundant #ifdeffery' > - Removed 'target: Restrict write_elfXX_note() handlers to system-mode' > > $ git backport-diff -u v1 -r v2 > Key: > [] : patches are identical > [] : number of functional differences between upstream/downstream patch > [down] : patch is downstream-only > The flags [FC] indicate (F)unctional and (C)ontextual differences, > respectively > > 001/12:[] [--] 'Makefile: Only build virtiofsd if system-mode is enabled' > 002/12:[] [--] 'configure: Avoid building TCG when not needed' > 003/12:[] [--] 'tests/Makefile: Only display TCG-related tests when TCG > is available' > 004/12:[] [--] 'tests/Makefile: Restrict some softmmu-only tests' > 005/12:[] [-C] 'util/Makefile: Reduce the user-mode object list' > 006/12:[] [--] 'stubs/Makefile: Reduce the user-mode object list' > 007/12:[] [--] 'target/riscv/cpu: Restrict CPU migration to system-mode' > 008/12:[] [--] 'exec: Assert CPU migration is not used on user-only build' > 009/12:[] [--] 'arch_init: Remove unused 'qapi-commands-misc.h' include' > 010/12:[] [--] 'target/i386: Restrict CpuClass::get_crash_info() to > system-mode' > 011/12:[] [--] 'target/s390x: Restrict CpuClass::get_crash_info() to > system-mode' > 012/12:[] [--] 'hw/core: Restrict CpuClass::get_crash_info() to > system-mode' > > Philippe Mathieu-Daudé (12): > Makefile: Only build virtiofsd if system-mode is enabled > configure: Avoid building TCG when not needed > tests/Makefile: Only display TCG-related tests when TCG is available > tests/Makefile: Restrict some softmmu-only tests > util/Makefile: Reduce the user-mode object list > stubs/Makefile: Reduce the user-mode object list > target/riscv/cpu: Restrict CPU migration to system-mode > exec: Assert CPU migration is not used on user-only build > arch_init: Remove unused 'qapi-commands-misc.h' include > target/i386: Restrict CpuClass::get_crash_info() to system-mode > target/s390x: Restrict CpuClass::get_crash_info() to system-mode > hw/core: Restrict CpuClass::get_crash_info() to system-mode > > configure | 4 +++ > Makefile | 2 +- > include/hw/core/cpu.h | 7 - > arch_init.c| 1 - > exec.c | 4 ++- > hw/core/cpu.c | 2 ++ > target/i386/cpu.c | 6 - > target/riscv/cpu.c | 6 +++-- > target/s390x/cpu.c | 12 - > stubs/Makefile.objs| 52 + > tests/Makefile.include | 18 +++-- > util/Makefile.objs | 59 +++--- > 12 files changed, 108 insertions(+), 65 deletions(-) > Reviewed-by: Laurent Vivier Tested-by: Laurent Vivier
Re: [PATCH v5 7/7] qemu-img: Deprecate use of -b without -F
On Tue, May 05, 2020 at 10:11:03 +0200, Kevin Wolf wrote: > Am 03.04.2020 um 19:58 hat Eric Blake geschrieben: > > Creating an image that requires format probing of the backing image is > > inherently unsafe (we've had several CVEs over the years based on > > probes leaking information to the guest on a subsequent boot, although > > these days tools like libvirt are aware of the issue enough to prevent > > the worst effects). However, if our probing algorithm ever changes, > > or if other tools like libvirt determine a different probe result than > > we do, then subsequent use of that backing file under a different > > format will present corrupted data to the guest. Start a deprecation > > clock so that future qemu-img can refuse to create unsafe backing > > chains that would rely on probing. Most warnings are intentionally > > emitted from bdrv_img_create() in the block layer, but qemu-img > > convert uses bdrv_create() which cannot emit its own warning without > > causing spurious warnings on other code paths. In the end, all > > command-line image creation or backing file rewriting now performs a > > check. > > > > However, there is one time where probing is safe: if we probe raw, > > then it is safe to record that implicitly in the image (but we still > > warn, as it's better to teach the user to supply -F always than to > > make them guess when it is safe). > > You're not stating it explicitly, but I guess the thing that you mean > that is actually unsafe is if you have a raw image, always pass > format=raw to QEMU (so the guest could write e.g. a qcow2 header), but > then create a backing file without -F, so it will be probed. This is as > bad as specifying format=raw only sometimes. > > I don't like the idea of responding to this by making raw images more > convenient to use than actual image formats. > > How about we approach it the other way around: The troublemaker is raw, > so let's require specifying raw explicitly, and record the probed format > implicitly in other cases. This is a bit weaker in the immediate effect > in that it doesn't protect you when you actually deal with a malicious > image, but in normal use it will point out where your scripts or > management software is too careless. The final result should be that > management tools are fixed and you'll be safe, while manual users who > can usually trust their guests aren't inconvenienced too much. That is definitely better than just probing. In my opinion though the format should always be specified explicitly. No favorism of 'raw' or any other format. In libvirt we fill-in that the format of the image is 'raw' unless the user specifies something else rather than reporting an error. This causes continuous grief for users who forget to set the format. (one example is that if you create a fully-allocated qcow2 image but use it as raw and install your VM, you'll never notice until you want to use qcow2 features. Users usually notice once they try to create a sparse image though due to the size difference)
Re: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Am 04.05.2020 um 19:07 hat Alberto Garcia geschrieben: > On Mon 04 May 2020 06:01:19 PM CEST, Eric Blake wrote: > >> +_supported_fmt qcow2 > >> +_supported_proto file > > > > Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix > > for qcow2, but are there other formats that it doesn't hurt to have > > the extra testing? > > It doesn't work with any other format at the moment (meaning: reading > the tail of the image after growing it returns the data from the backing > file). > > Also, it seems that qemu-img's -F does not work with other formats > either. > > > Also, I don't see anything preventing this from working with non-file > > protocol. > > Right, that can be updated I guess (whoever commits this, feel free to > do it). I don't know for which protocols it works. I know that qcow2 over nbd doesn't work. But I think there is a more important problem with the test: It seems to pass even with old binaries that don't have the fix. Is this only on my system or do you get the same? Kevin
[PATCH v2] crypto: Redundant type conversion for AES_KEY pointer
We can delete the redundant type conversion if
we set the the AES_KEY parameter with 'const' in
qcrypto_cipher_aes_ecb_(en|de)crypt() function.
Reported-by: Euler Robot
Signed-off-by: Chen Qun
---
v1->v2:
Cc: "Daniel P. Berrangé"
Modify the AES_KEY parameter with 'const' in
qcrypto_cipher_aes_ecb_(en|de)crypt() methods.
(Base on Daniel P. Berrangé's suggestion)
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg00515.html
---
crypto/cipher-builtin.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c
index bf8413e71a..35cf7820d9 100644
--- a/crypto/cipher-builtin.c
+++ b/crypto/cipher-builtin.c
@@ -74,7 +74,7 @@ static void qcrypto_cipher_free_aes(QCryptoCipher *cipher)
}
-static void qcrypto_cipher_aes_ecb_encrypt(AES_KEY *key,
+static void qcrypto_cipher_aes_ecb_encrypt(const AES_KEY *key,
const void *in,
void *out,
size_t len)
@@ -100,7 +100,7 @@ static void qcrypto_cipher_aes_ecb_encrypt(AES_KEY *key,
}
-static void qcrypto_cipher_aes_ecb_decrypt(AES_KEY *key,
+static void qcrypto_cipher_aes_ecb_decrypt(const AES_KEY *key,
const void *in,
void *out,
size_t len)
@@ -133,8 +133,7 @@ static void qcrypto_cipher_aes_xts_encrypt(const void *ctx,
{
const QCryptoCipherBuiltinAESContext *aesctx = ctx;
-qcrypto_cipher_aes_ecb_encrypt((AES_KEY *)&aesctx->enc,
- src, dst, length);
+qcrypto_cipher_aes_ecb_encrypt(&aesctx->enc, src, dst, length);
}
@@ -145,8 +144,7 @@ static void qcrypto_cipher_aes_xts_decrypt(const void *ctx,
{
const QCryptoCipherBuiltinAESContext *aesctx = ctx;
-qcrypto_cipher_aes_ecb_decrypt((AES_KEY *)&aesctx->dec,
- src, dst, length);
+qcrypto_cipher_aes_ecb_decrypt(&aesctx->dec, src, dst, length);
}
--
2.23.0
[PATCH] aspeed: sdmc: Implement AST2600 locking behaviour
The AST2600 handles this differently with the extra 'hardlock' state, so
move the testing to the soc specific class' write callback.
Signed-off-by: Joel Stanley
---
hw/misc/aspeed_sdmc.c | 55 +++
1 file changed, 45 insertions(+), 10 deletions(-)
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 02940e7c2a76..7c688becf8c7 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -23,7 +23,12 @@
/* Protection Key Register */
#define R_PROT(0x00 / 4)
+#define PROT_UNLOCKED 0x01
+#define PROT_HARDLOCKED0x10 /* AST2600 */
+#define PROT_SOFTLOCKED0x00
+
#define PROT_KEY_UNLOCK 0xFC600309
+#define PROT_KEY_HARDLOCK 0xDEADDEAD /* AST2600 */
/* Configuration Register */
#define R_CONF(0x04 / 4)
@@ -130,16 +135,6 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr,
uint64_t data,
return;
}
-if (addr == R_PROT) {
-s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
-return;
-}
-
-if (!s->regs[R_PROT]) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
-return;
-}
-
asc->write(s, addr, data);
}
@@ -291,6 +286,16 @@ static uint32_t
aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
static void aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
uint32_t data)
{
+if (reg == R_PROT) {
+s->regs[reg] = (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
PROT_SOFTLOCKED;
+return;
+}
+
+if (!s->regs[R_PROT]) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+return;
+}
+
switch (reg) {
case R_CONF:
data = aspeed_2400_sdmc_compute_conf(s, data);
@@ -339,6 +344,16 @@ static uint32_t
aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
static void aspeed_2500_sdmc_write(AspeedSDMCState *s, uint32_t reg,
uint32_t data)
{
+if (reg == R_PROT) {
+s->regs[reg] = (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
PROT_SOFTLOCKED;
+return;
+}
+
+if (!s->regs[R_PROT]) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+return;
+}
+
switch (reg) {
case R_CONF:
data = aspeed_2500_sdmc_compute_conf(s, data);
@@ -395,7 +410,27 @@ static uint32_t
aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
uint32_t data)
{
+if (s->regs[R_PROT] == PROT_HARDLOCKED) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked until system
reset!\n",
+__func__);
+return;
+}
+
+if (reg != R_PROT && s->regs[R_PROT] == PROT_SOFTLOCKED) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+return;
+}
+
switch (reg) {
+case R_PROT:
+if (data == PROT_KEY_UNLOCK) {
+data = PROT_UNLOCKED;
+} else if (data == PROT_KEY_HARDLOCK) {
+data = PROT_HARDLOCKED;
+} else {
+data = PROT_SOFTLOCKED;
+}
+break;
case R_CONF:
data = aspeed_2600_sdmc_compute_conf(s, data);
break;
--
2.26.2
Re: [PATCH v9 8/9] virtio-iommu: Implement probe request
On Fri, Apr 24, 2020 at 7:22 PM Auger Eric wrote:
>
> Hi Bharat,
> On 4/23/20 6:09 PM, Jean-Philippe Brucker wrote:
> > Hi Bharat,
> >
> > A few more things found while rebasing
> >
> > On Mon, Mar 23, 2020 at 02:16:16PM +0530, Bharat Bhushan wrote:
> >> This patch implements the PROBE request. Currently supported
> >> page size mask per endpoint is returned. Also append a NONE
> >> property in the end.
> >>
> >> Signed-off-by: Bharat Bhushan
> >> Signed-off-by: Eric Auger
> >> ---
> >> include/standard-headers/linux/virtio_iommu.h | 6 +
> >> hw/virtio/virtio-iommu.c | 161 +-
> >> hw/virtio/trace-events| 2 +
> >> 3 files changed, 166 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/standard-headers/linux/virtio_iommu.h
> >> b/include/standard-headers/linux/virtio_iommu.h
> >> index b9443b83a1..8a0d47b907 100644
> >> --- a/include/standard-headers/linux/virtio_iommu.h
> >> +++ b/include/standard-headers/linux/virtio_iommu.h
> >> @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap {
> >>
> >> #define VIRTIO_IOMMU_PROBE_T_NONE 0
> >> #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
> >> +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2
> >>
> >> #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
> >>
> >> @@ -130,6 +131,11 @@ struct virtio_iommu_probe_resv_mem {
> >> uint64_tend;
> >> };
> >>
> >> +struct virtio_iommu_probe_pgsize_mask {
> >> +struct virtio_iommu_probe_property head;
> >> +uint64_tpgsize_bitmap;
> >> +};
> >> +
> >> struct virtio_iommu_req_probe {
> >> struct virtio_iommu_req_headhead;
> >> uint32_tendpoint;
> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> >> index 747e3cf1da..63fbacdcdc 100644
> >> --- a/hw/virtio/virtio-iommu.c
> >> +++ b/hw/virtio/virtio-iommu.c
> >> @@ -38,6 +38,10 @@
> >>
> >> /* Max size */
> >> #define VIOMMU_DEFAULT_QUEUE_SIZE 256
> >> +#define VIOMMU_PROBE_SIZE 512
> >> +
> >> +#define SUPPORTED_PROBE_PROPERTIES (\
> >> +1 << VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK)
> >>
> >> typedef struct VirtIOIOMMUDomain {
> >> uint32_t id;
> >> @@ -62,6 +66,13 @@ typedef struct VirtIOIOMMUMapping {
> >> uint32_t flags;
> >> } VirtIOIOMMUMapping;
> >>
> >> +typedef struct VirtIOIOMMUPropBuffer {
> >> +VirtIOIOMMUEndpoint *endpoint;
> >> +size_t filled;
> >> +uint8_t *start;
> >> +bool error;
> >
> > It doesn't seem like bufstate->error gets used anywhere
> maybe rebase your work on
> [PATCH for-4.2 v10 10/15] virtio-iommu: Implement probe request
> which tests it.
This was the staring point for me, As of now i moved away from "error"
from above struct.
>
> Also in
> [Qemu-devel] [PATCH for-4.2 v10 10/15] virtio-iommu: Implement probe request
> I changed the implementation to keep it simpler.
>
> Thanks
>
> Eric
> >
> >> +} VirtIOIOMMUPropBuffer;
> >> +
> >> static inline uint16_t virtio_iommu_get_bdf(IOMMUDevice *dev)
> >> {
> >> return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
> >> @@ -490,6 +501,114 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
> >> return ret;
> >> }
> >>
> >> +static int virtio_iommu_fill_none_prop(VirtIOIOMMUPropBuffer *bufstate)
> >> +{
> >> +struct virtio_iommu_probe_property *prop;
> >> +
> >> +prop = (struct virtio_iommu_probe_property *)
> >> +(bufstate->start + bufstate->filled);
> >> +prop->type = 0;
> >> +prop->length = 0;
> >> +bufstate->filled += sizeof(*prop);
> >> +trace_virtio_iommu_fill_none_property(bufstate->endpoint->id);
> >> +return 0;
> >> +}
> >> +
> >> +static int virtio_iommu_fill_page_size_mask(VirtIOIOMMUPropBuffer
> >> *bufstate)
> >> +{
> >> +struct virtio_iommu_probe_pgsize_mask *page_size_mask;
> >> +size_t prop_size = sizeof(*page_size_mask);
> >> +VirtIOIOMMUEndpoint *ep = bufstate->endpoint;
> >> +VirtIOIOMMU *s = ep->viommu;
> >> +IOMMUDevice *sdev;
> >> +
> >> +if (bufstate->filled + prop_size >= VIOMMU_PROBE_SIZE) {
> >> +bufstate->error = true;
> >> +/* get the traversal stopped by returning true */
> >> +return true;
> >> +}
> >> +
> >> +page_size_mask = (struct virtio_iommu_probe_pgsize_mask *)
> >> + (bufstate->start + bufstate->filled);
> >> +
> >> +page_size_mask->head.type = VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK;
> >> +page_size_mask->head.length = prop_size;
> >> +QLIST_FOREACH(sdev, &s->notifiers_list, next) {
> >> +if (ep->id == sdev->devfn) {
> >> +page_size_mask->pgsize_bitmap = sdev->page_size_mask;
> >
> > Do we need a cpu_to_le64 here?
Ack, yes, even head.type and head.length needed cpu_to_le16().
> >
> >> +}
> >> +}
> >> +bufstate->filled += sizeof(*page_size_mask);
> >> +trace_virtio_iommu_fill_pgsize_mask_property(b
Re: [PATCH] aspeed: sdmc: Implement AST2600 locking behaviour
Patchew URL: https://patchew.org/QEMU/20200505090136.341426-1-j...@jms.id.au/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200505090136.341426-1-j...@jms.id.au/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2] crypto: Redundant type conversion for AES_KEY pointer
On Tue, May 05, 2020 at 04:59:40PM +0800, Chen Qun wrote: > We can delete the redundant type conversion if > we set the the AES_KEY parameter with 'const' in > qcrypto_cipher_aes_ecb_(en|de)crypt() function. > > Reported-by: Euler Robot > Signed-off-by: Chen Qun > --- > v1->v2: > Cc: "Daniel P. Berrangé" > > Modify the AES_KEY parameter with 'const' in > qcrypto_cipher_aes_ecb_(en|de)crypt() methods. > (Base on Daniel P. Berrangé's suggestion) > > https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg00515.html > > --- > crypto/cipher-builtin.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé and queued - will send a PR with this and some other crypto fixes later today most likely. 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: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Am 05.05.2020 um 10:54 hat Kevin Wolf geschrieben: > Am 04.05.2020 um 19:07 hat Alberto Garcia geschrieben: > > On Mon 04 May 2020 06:01:19 PM CEST, Eric Blake wrote: > > >> +_supported_fmt qcow2 > > >> +_supported_proto file > > > > > > Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix > > > for qcow2, but are there other formats that it doesn't hurt to have > > > the extra testing? > > > > It doesn't work with any other format at the moment (meaning: reading > > the tail of the image after growing it returns the data from the backing > > file). > > > > Also, it seems that qemu-img's -F does not work with other formats > > either. > > > > > Also, I don't see anything preventing this from working with non-file > > > protocol. > > > > Right, that can be updated I guess (whoever commits this, feel free to > > do it). > > I don't know for which protocols it works. I know that qcow2 over nbd > doesn't work. > > But I think there is a more important problem with the test: It seems to > pass even with old binaries that don't have the fix. Is this only on my > system or do you get the same? Ah, I do get the overflow in the calculation of the length for qcow2_cluster_zeroize(), but size_to_clusters() inside the function overflows back the other direction, so this ends up with nb_clusters = 0 and we don't do anything bad. We could probably trigger a bad case with data_file_raw=on, but then we don't have a backing file, so nothing sets BDRV_REQ_ZERO_WRITE. So I guess the bug isn't even really testable, but we just add the test in case something else in the same scenario breaks? Kevin
Re: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()
On Tue 05 May 2020 10:54:12 AM CEST, Kevin Wolf wrote: > But I think there is a more important problem with the test: It seems > to pass even with old binaries that don't have the fix. Is this only > on my system or do you get the same? With old binaries when qcow2_cluster_zeroize() is called it receives bytes = (UINT64_MAX - 9216), however that number is then used to calculate the number of affected clusters, so it's rounded up, wraps around again and back to zero. There's no visible sign of the error, it just happens to work fine. If there was a raw data file then we would try to write UINT64_MAX-9216 bytes to it, but in this case there's no backing file allowed and therefore the image is not zeroed, so qcow2_cluster_zeroize() never happens. Why the test case then? There was a mistake with my first patch and there it crashed (due to an assertion), that's why Eric thought it would be a good idea to add a test case anyway, in case we have to change that code in the future and we screw up. Berto
Re: [PATCH v2] Fix iotest 153
Patchew URL: https://patchew.org/QEMU/20200504131959.9533-1-mlevi...@redhat.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504131959.9533-1-mlevi...@redhat.com/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Am 05.05.2020 um 11:16 hat Alberto Garcia geschrieben: > On Tue 05 May 2020 10:54:12 AM CEST, Kevin Wolf wrote: > > But I think there is a more important problem with the test: It seems > > to pass even with old binaries that don't have the fix. Is this only > > on my system or do you get the same? > > With old binaries when qcow2_cluster_zeroize() is called it receives > bytes = (UINT64_MAX - 9216), however that number is then used to > calculate the number of affected clusters, so it's rounded up, wraps > around again and back to zero. There's no visible sign of the error, it > just happens to work fine. > > If there was a raw data file then we would try to write UINT64_MAX-9216 > bytes to it, but in this case there's no backing file allowed and > therefore the image is not zeroed, so qcow2_cluster_zeroize() never > happens. > > Why the test case then? There was a mistake with my first patch and > there it crashed (due to an assertion), that's why Eric thought it would > be a good idea to add a test case anyway, in case we have to change that > code in the future and we screw up. Thanks for the explanation, this makes sense. I'll apply the patch now. Kevin
Re: [PATCH v2 0/1] target/arm: Remove access_el3_aa32ns()
Patchew URL: https://patchew.org/QEMU/20200504142125.31180-1-edgar.igles...@gmail.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20200504142125.31180-1-edgar.igles...@gmail.com Subject: [PATCH v2 0/1] target/arm: Remove access_el3_aa32ns() 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 fatal: unable to write new index file warning: Clone succeeded, but checkout failed. You can inspect what was checked out with 'git status' and retry the checkout with 'git checkout -f HEAD' Traceback (most recent call last): File "patchew-tester/src/patchew-cli", line 521, in test_one git_clone_repo(clone, r["repo"], r["head"], logf, True) File "patchew-tester/src/patchew-cli", line 53, in git_clone_repo subprocess.check_call(clone_cmd, stderr=logf, stdout=logf) File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'clone', '-q', '/home/patchew/.cache/patchew-git-cache/httpsgithubcompatchewprojectqemu-3c8cf5a9c21ff8782164d1def7f44bd888713384', '/var/tmp/patchew-tester-tmp-jts7e35j/src']' returned non-zero exit status 128. The full log is available at http://patchew.org/logs/20200504142125.31180-1-edgar.igles...@gmail.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on mmio region translation by viommu
Hi Eric,
On Fri, Apr 24, 2020 at 7:47 PM Auger Eric wrote:
>
> Hi Bharat,
>
> On 4/2/20 11:01 AM, Bharat Bhushan wrote:
> > Hi Eric/Alex,
> >
> >> -Original Message-
> >> From: Alex Williamson
> >> Sent: Thursday, March 26, 2020 11:23 PM
> >> To: Auger Eric
> >> Cc: Bharat Bhushan ; peter.mayd...@linaro.org;
> >> pet...@redhat.com; eric.auger@gmail.com; kevin.t...@intel.com;
> >> m...@redhat.com; Tomasz Nowicki [C] ;
> >> drjo...@redhat.com; linuc.dec...@gmail.com; qemu-devel@nongnu.org; qemu-
> >> a...@nongnu.org; bharatb.li...@gmail.com; jean-phili...@linaro.org;
> >> yang.zh...@intel.com; David Gibson
> >> Subject: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on
> >> mmio
> >> region translation by viommu
> >>
> >> External Email
> >>
> >> --
> >> On Thu, 26 Mar 2020 18:35:48 +0100
> >> Auger Eric wrote:
> >>
> >>> Hi Alex,
> >>>
> >>> On 3/24/20 12:08 AM, Alex Williamson wrote:
> [Cc +dwg who originated this warning]
>
> On Mon, 23 Mar 2020 14:16:09 +0530
> Bharat Bhushan wrote:
>
> > On ARM, the MSI doorbell is translated by the virtual IOMMU.
> > As such address_space_translate() returns the MSI controller MMIO
> > region and we get an "iommu map to non memory area"
> > message. Let's remove this latter.
> >
> > Signed-off-by: Eric Auger
> > Signed-off-by: Bharat Bhushan
> > ---
> > hw/vfio/common.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
> > 5ca11488d6..c586edf47a 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -426,8 +426,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb,
> >> void **vaddr,
> > &xlat, &len, writable,
> > MEMTXATTRS_UNSPECIFIED);
> > if (!memory_region_is_ram(mr)) {
> > -error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > - xlat);
> > return false;
> > }
> >
>
> I'm a bit confused here, I think we need more justification beyond
> "we hit this warning and we don't want to because it's ok in this
> one special case, therefore remove it". I assume the special case
> is that the device MSI address is managed via the SET_IRQS ioctl and
> therefore we won't actually get DMAs to this range.
> >>> Yes exactly. The guest creates a mapping between one giova and this
> >>> gpa (corresponding to the MSI controller doorbell) because MSIs are
> >>> mapped on ARM. But practically the physical device is programmed with
> >>> an host chosen iova that maps onto the physical MSI controller's
> >>> doorbell. so the device never performs DMA accesses to this range.
> >>>
> >>> But I imagine the case that
> was in mind when adding this warning was general peer-to-peer
> between and assigned and emulated device.
> >>> yes makes sense.
> >>>
> >>> Maybe there's an argument to be made
> that such a p2p mapping might also be used in a non-vIOMMU case. We
> skip creating those mappings and drivers continue to work, maybe
> because nobody attempts to do p2p DMA with the types of devices we
> emulate, maybe because p2p DMA is not absolutely reliable on bare
> metal and drivers test it before using it.
> >>> MSI doorbells are mapped using the IOMMU_MMIO flag (dma-iommu.c
> >>> iommu_dma_get_msi_page).
> >>> One idea could be to pass that flag through the IOMMU Notifier
> >>> mechanism into the iotlb->perm. Eventually when we get this in
> >>> vfio_get_vaddr() we would not print the warning. Could that make sense?
> >>
> >> Yeah, if we can identify a valid case that doesn't need a warning, that's
> >> fine by me.
> >> Thanks,
> >
> > Let me know if I understood the proposal correctly:
> >
> > virtio-iommu driver in guest will make map (VIRTIO_IOMMU_T_MAP) with
> > VIRTIO_IOMMU_MAP_F_MMIO flag for MSI mapping.
> > In qemu, virtio-iommu device will set a new defined flag (say IOMMU_MMIO)
> > in iotlb->perm in memory_region_notify_iommu(). vfio_get_vaddr() will check
> > same flag and will not print the warning.>
> > Is above correct?
> Yes that's what I had in mind.
In that case virtio-iommu driver in guest should not make map
(VIRTIO_IOMMU_T_MAP) call as it known nothing to be mapped.
Stay Safe
Thanks
-Bharat
>
> Thanks
>
> Eric
> >
> > Thanks
> > -Bharat
> >
> >>
> >> Alex
> >
> >
>
Re: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on mmio region translation by viommu
Hi Bharat,
On 5/5/20 11:25 AM, Bharat Bhushan wrote:
> Hi Eric,
>
> On Fri, Apr 24, 2020 at 7:47 PM Auger Eric wrote:
>>
>> Hi Bharat,
>>
>> On 4/2/20 11:01 AM, Bharat Bhushan wrote:
>>> Hi Eric/Alex,
>>>
-Original Message-
From: Alex Williamson
Sent: Thursday, March 26, 2020 11:23 PM
To: Auger Eric
Cc: Bharat Bhushan ; peter.mayd...@linaro.org;
pet...@redhat.com; eric.auger@gmail.com; kevin.t...@intel.com;
m...@redhat.com; Tomasz Nowicki [C] ;
drjo...@redhat.com; linuc.dec...@gmail.com; qemu-devel@nongnu.org; qemu-
a...@nongnu.org; bharatb.li...@gmail.com; jean-phili...@linaro.org;
yang.zh...@intel.com; David Gibson
Subject: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on
mmio
region translation by viommu
External Email
--
On Thu, 26 Mar 2020 18:35:48 +0100
Auger Eric wrote:
> Hi Alex,
>
> On 3/24/20 12:08 AM, Alex Williamson wrote:
>> [Cc +dwg who originated this warning]
>>
>> On Mon, 23 Mar 2020 14:16:09 +0530
>> Bharat Bhushan wrote:
>>
>>> On ARM, the MSI doorbell is translated by the virtual IOMMU.
>>> As such address_space_translate() returns the MSI controller MMIO
>>> region and we get an "iommu map to non memory area"
>>> message. Let's remove this latter.
>>>
>>> Signed-off-by: Eric Auger
>>> Signed-off-by: Bharat Bhushan
>>> ---
>>> hw/vfio/common.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
>>> 5ca11488d6..c586edf47a 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -426,8 +426,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb,
void **vaddr,
>>> &xlat, &len, writable,
>>> MEMTXATTRS_UNSPECIFIED);
>>> if (!memory_region_is_ram(mr)) {
>>> -error_report("iommu map to non memory area %"HWADDR_PRIx"",
>>> - xlat);
>>> return false;
>>> }
>>>
>>
>> I'm a bit confused here, I think we need more justification beyond
>> "we hit this warning and we don't want to because it's ok in this
>> one special case, therefore remove it". I assume the special case
>> is that the device MSI address is managed via the SET_IRQS ioctl and
>> therefore we won't actually get DMAs to this range.
> Yes exactly. The guest creates a mapping between one giova and this
> gpa (corresponding to the MSI controller doorbell) because MSIs are
> mapped on ARM. But practically the physical device is programmed with
> an host chosen iova that maps onto the physical MSI controller's
> doorbell. so the device never performs DMA accesses to this range.
>
> But I imagine the case that
>> was in mind when adding this warning was general peer-to-peer
>> between and assigned and emulated device.
> yes makes sense.
>
> Maybe there's an argument to be made
>> that such a p2p mapping might also be used in a non-vIOMMU case. We
>> skip creating those mappings and drivers continue to work, maybe
>> because nobody attempts to do p2p DMA with the types of devices we
>> emulate, maybe because p2p DMA is not absolutely reliable on bare
>> metal and drivers test it before using it.
> MSI doorbells are mapped using the IOMMU_MMIO flag (dma-iommu.c
> iommu_dma_get_msi_page).
> One idea could be to pass that flag through the IOMMU Notifier
> mechanism into the iotlb->perm. Eventually when we get this in
> vfio_get_vaddr() we would not print the warning. Could that make sense?
Yeah, if we can identify a valid case that doesn't need a warning, that's
fine by me.
Thanks,
>>>
>>> Let me know if I understood the proposal correctly:
>>>
>>> virtio-iommu driver in guest will make map (VIRTIO_IOMMU_T_MAP) with
>>> VIRTIO_IOMMU_MAP_F_MMIO flag for MSI mapping.
>>> In qemu, virtio-iommu device will set a new defined flag (say IOMMU_MMIO)
>>> in iotlb->perm in memory_region_notify_iommu(). vfio_get_vaddr() will check
>>> same flag and will not print the warning.>
>>> Is above correct?
>> Yes that's what I had in mind.
>
> In that case virtio-iommu driver in guest should not make map
> (VIRTIO_IOMMU_T_MAP) call as it known nothing to be mapped.
sorry I don't catch what you meant. Please can you elaborate?
Thanks
Eric
>
> Stay Safe
>
> Thanks
> -Bharat
>
>>
>> Thanks
>>
>> Eric
>>>
>>> Thanks
>>> -Bharat
>>>
Alex
>>>
>>>
>>
>
Re: [PATCH] tests/Makefile: Fix description of "make check"
On 5/5/20 9:29 AM, Huacai Chen wrote: The description of "make check" is out-of-date, so fix it by adding block and softfloat. Signed-off-by: Huacai Chen --- tests/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 03a74b6..5d32239 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -4,7 +4,7 @@ check-help: @echo "Regression testing targets:" @echo - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and decodetree" + @echo " $(MAKE) checkRun block, qapi-schema, unit, softfloat, qtest and decodetree" Maybe end with " tests"? @echo @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" @echo " $(MAKE) check-qtest Run qtest tests"
Re: [PATCH v5 00/18] nvme: refactoring and cleanups
On 5/5/20 7:48 AM, Klaus Jensen wrote: From: Klaus Jensen Changes since v5 No functional changes, just updated Reviewed-by tags. Also, I screwed up the CC list when sending v4. Philippe and Keith, please add a Reviewed-by to * "nvme: factor out pmr setup" and * "do cmb/pmr init as part of pci init" since the first one was added and the second one was changed in v4 when rebasing on Kevins block-next tree which had the PMR work that was not in master at the time. With those in place, it should be ready for Kevin to merge. Klaus Jensen (18): nvme: fix pci doorbell size calculation nvme: rename trace events to pci_nvme nvme: remove superfluous breaks nvme: move device parameters to separate struct nvme: use constants in identify nvme: refactor nvme_addr_read nvme: add max_ioqpairs device parameter nvme: remove redundant cmbloc/cmbsz members nvme: factor out property/constraint checks nvme: factor out device state setup nvme: factor out block backend setup nvme: add namespace helpers nvme: factor out namespace setup nvme: factor out pci setup nvme: factor out cmb setup nvme: factor out pmr setup nvme: do cmb/pmr init as part of pci init nvme: factor out controller identify setup Thinking loudly, it would be easier to differentiate emulated device vs block driver using 's,^nvme,hw/nvme,' in patches (and series) title. Kevin, if you are motivated... hw/block/nvme.c | 543 -- hw/block/nvme.h | 31 ++- hw/block/trace-events | 180 +++--- include/block/nvme.h | 8 + 4 files changed, 429 insertions(+), 333 deletions(-)
Re: [PATCH v18 QEMU 05/18] vfio: Add migration region initialization and finalize function
On Tue, 5 May 2020 04:14:40 +0530
Kirti Wankhede wrote:
> - Migration functions are implemented for VFIO_DEVICE_TYPE_PCI device in this
> patch series.
I would drop this sentence; people looking at this patch in the future
are unlikely to care.
> - VFIO device supports migration or not is decided based of migration region
"Whether the VFIO device..."
s/based of/based on/
> query. If migration region query is successful and migration region
> initialization is successful then migration is supported else migration is
> blocked.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> hw/vfio/Makefile.objs | 2 +-
> hw/vfio/migration.c | 138
> ++
> hw/vfio/trace-events | 3 +
> include/hw/vfio/vfio-common.h | 9 +++
> 4 files changed, 151 insertions(+), 1 deletion(-)
> create mode 100644 hw/vfio/migration.c
> +int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
> +{
> +struct vfio_region_info *info;
> +Error *local_err = NULL;
> +int ret;
> +
> +ret = vfio_get_dev_region_info(vbasedev, VFIO_REGION_TYPE_MIGRATION,
> + VFIO_REGION_SUBTYPE_MIGRATION, &info);
> +if (ret) {
> +goto add_blocker;
> +}
> +
> +ret = vfio_migration_init(vbasedev, info);
> +if (ret) {
> +goto add_blocker;
> +}
> +
> +trace_vfio_migration_probe(vbasedev->name, info->index);
> +return 0;
> +
> +add_blocker:
> +error_setg(&vbasedev->migration_blocker,
> + "VFIO device doesn't support migration");
> +ret = migrate_add_blocker(vbasedev->migration_blocker, &local_err);
> +if (local_err) {
Rather check for ret?
> +error_propagate(errp, local_err);
> +error_free(vbasedev->migration_blocker);
vbasedev->migration_blocker = NULL; ?
> +}
> +return ret;
I think you also need to free info somewhere?
> +}
Re: [PATCH 0/6] vmdk: Fix zero cluster handling
Am 30.04.2020 um 15:30 hat Kevin Wolf geschrieben: > What I was really investigating is why 055 was so slow. I couldn't solve > that, but instead I found out that our VMDK code for zero clusters and > write_zeroes was completely broken. Apart from segfaults when zero > clusters were actually enabled, this caused a compressed backup target > to result in a bigger file than uncompressed with VMDK. > > This series tries to fix it (with one bonus performance patch). Thanks for the review, fixed up the commit messages and applied. If you were curious about the VMDK terminology, I looked it up and the basic terms translate to qcow2 like this: * grain directory = L1 table * grain table = L2 table * grain = cluster "zeroed-grain GTE (grain table entry)" is the exact term used in the VMDK spec for what we would call a zero cluster in qcow2. Kevin
Re: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on mmio region translation by viommu
hi Eric,
On Tue, May 5, 2020 at 3:00 PM Auger Eric wrote:
>
> Hi Bharat,
>
> On 5/5/20 11:25 AM, Bharat Bhushan wrote:
> > Hi Eric,
> >
> > On Fri, Apr 24, 2020 at 7:47 PM Auger Eric wrote:
> >>
> >> Hi Bharat,
> >>
> >> On 4/2/20 11:01 AM, Bharat Bhushan wrote:
> >>> Hi Eric/Alex,
> >>>
> -Original Message-
> From: Alex Williamson
> Sent: Thursday, March 26, 2020 11:23 PM
> To: Auger Eric
> Cc: Bharat Bhushan ; peter.mayd...@linaro.org;
> pet...@redhat.com; eric.auger@gmail.com; kevin.t...@intel.com;
> m...@redhat.com; Tomasz Nowicki [C] ;
> drjo...@redhat.com; linuc.dec...@gmail.com; qemu-devel@nongnu.org; qemu-
> a...@nongnu.org; bharatb.li...@gmail.com; jean-phili...@linaro.org;
> yang.zh...@intel.com; David Gibson
> Subject: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on
> mmio
> region translation by viommu
>
> External Email
>
> --
> On Thu, 26 Mar 2020 18:35:48 +0100
> Auger Eric wrote:
>
> > Hi Alex,
> >
> > On 3/24/20 12:08 AM, Alex Williamson wrote:
> >> [Cc +dwg who originated this warning]
> >>
> >> On Mon, 23 Mar 2020 14:16:09 +0530
> >> Bharat Bhushan wrote:
> >>
> >>> On ARM, the MSI doorbell is translated by the virtual IOMMU.
> >>> As such address_space_translate() returns the MSI controller MMIO
> >>> region and we get an "iommu map to non memory area"
> >>> message. Let's remove this latter.
> >>>
> >>> Signed-off-by: Eric Auger
> >>> Signed-off-by: Bharat Bhushan
> >>> ---
> >>> hw/vfio/common.c | 2 --
> >>> 1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
> >>> 5ca11488d6..c586edf47a 100644
> >>> --- a/hw/vfio/common.c
> >>> +++ b/hw/vfio/common.c
> >>> @@ -426,8 +426,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb,
> void **vaddr,
> >>> &xlat, &len, writable,
> >>> MEMTXATTRS_UNSPECIFIED);
> >>> if (!memory_region_is_ram(mr)) {
> >>> -error_report("iommu map to non memory area %"HWADDR_PRIx"",
> >>> - xlat);
> >>> return false;
> >>> }
> >>>
> >>
> >> I'm a bit confused here, I think we need more justification beyond
> >> "we hit this warning and we don't want to because it's ok in this
> >> one special case, therefore remove it". I assume the special case
> >> is that the device MSI address is managed via the SET_IRQS ioctl and
> >> therefore we won't actually get DMAs to this range.
> > Yes exactly. The guest creates a mapping between one giova and this
> > gpa (corresponding to the MSI controller doorbell) because MSIs are
> > mapped on ARM. But practically the physical device is programmed with
> > an host chosen iova that maps onto the physical MSI controller's
> > doorbell. so the device never performs DMA accesses to this range.
> >
> > But I imagine the case that
> >> was in mind when adding this warning was general peer-to-peer
> >> between and assigned and emulated device.
> > yes makes sense.
> >
> > Maybe there's an argument to be made
> >> that such a p2p mapping might also be used in a non-vIOMMU case. We
> >> skip creating those mappings and drivers continue to work, maybe
> >> because nobody attempts to do p2p DMA with the types of devices we
> >> emulate, maybe because p2p DMA is not absolutely reliable on bare
> >> metal and drivers test it before using it.
> > MSI doorbells are mapped using the IOMMU_MMIO flag (dma-iommu.c
> > iommu_dma_get_msi_page).
> > One idea could be to pass that flag through the IOMMU Notifier
> > mechanism into the iotlb->perm. Eventually when we get this in
> > vfio_get_vaddr() we would not print the warning. Could that make sense?
>
> Yeah, if we can identify a valid case that doesn't need a warning,
> that's fine by me.
> Thanks,
> >>>
> >>> Let me know if I understood the proposal correctly:
> >>>
> >>> virtio-iommu driver in guest will make map (VIRTIO_IOMMU_T_MAP) with
> >>> VIRTIO_IOMMU_MAP_F_MMIO flag for MSI mapping.
> >>> In qemu, virtio-iommu device will set a new defined flag (say IOMMU_MMIO)
> >>> in iotlb->perm in memory_region_notify_iommu(). vfio_get_vaddr() will
> >>> check same flag and will not print the warning.>
> >>> Is above correct?
> >> Yes that's what I had in mind.
> >
> > In that case virtio-iommu driver in guest should not make map
> > (VIRTIO_IOMMU_T_MAP) call as it known nothing to be mapped.
> sorry I don't catch what you meant. Please can you elaborate?
What I understood of the proposal is:
Linux:
1) MSI doorbells are mapped using the IOMMU_MMIO flag (dma-iommu.c
io
Re: [PATCH v4 00/18] target/arm: sve load/store improvements
On Mon, 4 May 2020 at 17:03, Richard Henderson wrote: > > On 5/4/20 2:43 AM, Peter Maydell wrote: > > I've reviewed patch 13, but I still don't understand why you've > > made the size-related changes in patch 4, so I've continued > > our conversation in the thread on the v3 version of that patch. > > I've changed that here in v4. Please have another look at this one. The page_check_range() call still seems to be passing a fixed size of '1' ? thanks -- PMM
Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size
On 5/5/20 10:23 AM, Laurent Vivier wrote: On 05/05/2020 10:00, Laurent Vivier wrote: On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote: In nvme_create_queue_pair() we create a page list using qemu_blockalign(), then map it with qemu_vfio_dma_map(): q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE); r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, s->page_size * NVME_QUEUE_SIZE, ...); With: s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); The qemu_vfio_dma_map() documentation says "The caller need to make sure the area is aligned to page size". While we use multiple s->page_size as alignment, it might be not sufficient on some hosts. Use the qemu_real_host_page_size value to be sure the host alignment is respected. Signed-off-by: Philippe Mathieu-Daudé --- Cc: Cédric Le Goater Cc: David Gibson Cc: Laurent Vivier --- block/nvme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/nvme.c b/block/nvme.c index 7b7c0cc5d6..bde0d28b39 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t); -bs->bl.opt_mem_alignment = s->page_size; +bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size); Why don't you replace directly the "4096" in s->page_size by qemu_real_host_page_size? I think this was the purpose of MAX(4096, ...) to align to the host page size. oh, I see, page_size is used for a lot of things than can be broken, and this is what is done in bdrv_opt_mem_align() for instance, 4096 is the sector size not the host page size. BTW, you need the same change as in nvme_init() in nvme_refresh_limits(), I think. nvme_init() seems OK, but you are right, I missed nvme_refresh_limits()... Thanks! Thanks, Laurent
[RFC PATCH] hw/arm/musicpal: Map the UART devices unconditionally
I can't find proper documentation or datasheet, but it is likely
a MMIO mapped serial device mapped in the 0x8000..0x8000
range belongs to the SoC address space, thus is always mapped in
the memory bus.
Map the devices on the bus regardless a chardev is attached to it.
Signed-off-by: Philippe Mathieu-Daudé
---
from 2019... found while doing housekeeping
---
hw/arm/musicpal.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index b2d0cfdac8..92f33ed87e 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1619,14 +1619,10 @@ static void musicpal_init(MachineState *machine)
pic[MP_TIMER2_IRQ], pic[MP_TIMER3_IRQ],
pic[MP_TIMER4_IRQ], NULL);
-if (serial_hd(0)) {
-serial_mm_init(address_space_mem, MP_UART1_BASE, 2, pic[MP_UART1_IRQ],
- 1825000, serial_hd(0), DEVICE_NATIVE_ENDIAN);
-}
-if (serial_hd(1)) {
-serial_mm_init(address_space_mem, MP_UART2_BASE, 2, pic[MP_UART2_IRQ],
- 1825000, serial_hd(1), DEVICE_NATIVE_ENDIAN);
-}
+serial_mm_init(address_space_mem, MP_UART1_BASE, 2, pic[MP_UART1_IRQ],
+ 1825000, serial_hd(0), DEVICE_NATIVE_ENDIAN);
+serial_mm_init(address_space_mem, MP_UART2_BASE, 2, pic[MP_UART2_IRQ],
+ 1825000, serial_hd(1), DEVICE_NATIVE_ENDIAN);
/* Register flash */
dinfo = drive_get(IF_PFLASH, 0, 0);
--
2.21.3
Re: [PATCH v2 3/4] backup: Make sure that source and target size match
Am 30.04.2020 um 20:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.04.2020 17:27, Kevin Wolf wrote:
> > Since the introduction of a backup filter node in commit 00e30f05d, the
> > backup block job crashes when the target image is smaller than the
> > source image because it will try to write after the end of the target
> > node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
> > would have caught this and errored out gracefully.)
> >
> > We can fix this and even do better than the old behaviour: Check that
> > source and target have the same image size at the start of the block job
> > and unshare BLK_PERM_RESIZE. (This permission was already unshared
> > before the same commit 00e30f05d, but the BlockBackend that was used to
> > make the restriction was removed without a replacement.) This will
> > immediately error out when starting the job instead of only when writing
> > to a block that doesn't exist in the target.
> >
> > Longer target than source would technically work because we would never
> > write to blocks that don't exist, but semantically these are invalid,
> > too, because a backup is supposed to create a copy, not just an image
> > that starts with a copy.
> >
> > Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Kevin Wolf
>
> I'm OK with it as is, as it fixes bug:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
> still, some notes below
>
>
> > ---
> > block/backup-top.c | 14 +-
> > block/backup.c | 14 +-
> > 2 files changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/backup-top.c b/block/backup-top.c
> > index 3b50c06e2c..79b268e6dc 100644
> > --- a/block/backup-top.c
> > +++ b/block/backup-top.c
> > @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState
> > *bs, BdrvChild *c,
> >*
> >* Share write to target (child_file), to not interfere
> >* with guest writes to its disk which may be in target backing
> > chain.
> > + * Can't resize during a backup block job because we check the size
> > + * only upfront.
> >*/
> > -*nshared = BLK_PERM_ALL;
> > +*nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
> > *nperm = BLK_PERM_WRITE;
> > } else {
> > /* Source child */
> > @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs,
> > BdrvChild *c,
> > if (perm & BLK_PERM_WRITE) {
> > *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> > }
> > -*nshared &= ~BLK_PERM_WRITE;
> > +*nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > }
> > }
> > @@ -192,11 +194,13 @@ BlockDriverState
> > *bdrv_backup_top_append(BlockDriverState *source,
> > {
> > Error *local_err = NULL;
> > BDRVBackupTopState *state;
> > -BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
> > - filter_node_name,
> > - BDRV_O_RDWR, errp);
> > +BlockDriverState *top;
> > bool appended = false;
> > +assert(source->total_sectors == target->total_sectors);
>
> May be better to error-out, just to keep backup-top independent. Still, now
> it's not
> really needed, as we have only one caller. And this function have to be
> refactored
> anyway, when publishing this filter (open() and close() should appear, so
> this code
> will be rewritten anyway.)
Yes, the whole function only works because it's used in this restricted
context today. For example, we only know that total_sectors is up to
date because the caller has called bdrv_getlength() just a moment ago.
I think fixing this would be beyond the scope of this patch, but
certainly a good idea anyway.
> And the other thought: the permissions we declared above, will be activated
> only after
> successful bdrv_child_refresh_perms(). I think some kind of race is possible,
> so that
> size is changed actual permission activation. So, may be good to double check
> sizes after
> bdrv_child_refresh_perms().. But it's a kind of paranoia.
We're not in coroutine context, so we can't yield. I don't see who could
change the size in parallel (apart from an external process, but an
external process can mess up anything).
When we make backup-top an independent driver, instead of double
checking (what would you do on error?), maybe we could move the size
initialisation (then with bdrv_getlength()) to after
bdrv_child_refresh_perms().
> Also, third thought: the restricted permissions doesn't save us from resizing
> of the source through exactly this node, does it? Hmm, but your test works
> somehow. But
> (I assume) it worked in a previous patch version without unsharing on source..
>
> Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't
> have
Re: [PATCH] tests/Makefile: Fix description of "make check"
Hi, Philippe, On Tue, May 5, 2020 at 5:33 PM Philippe Mathieu-Daudé wrote: > > On 5/5/20 9:29 AM, Huacai Chen wrote: > > The description of "make check" is out-of-date, so fix it by adding > > block and softfloat. > > > > Signed-off-by: Huacai Chen > > --- > > tests/Makefile.include | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index 03a74b6..5d32239 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -4,7 +4,7 @@ > > check-help: > > @echo "Regression testing targets:" > > @echo > > - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and > > decodetree" > > + @echo " $(MAKE) checkRun block, qapi-schema, unit, > > softfloat, qtest and decodetree" > > Maybe end with " tests"? Thank you, let me add "tests" in V2 > > > @echo > > @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" > > @echo " $(MAKE) check-qtest Run qtest tests" > > -- Huacai Chen
[PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
Use the generic AUDIO_HOST_ENDIANNESS definition instead of a custom one. Signed-off-by: Philippe Mathieu-Daudé --- Who/what machine is using this device anyway? --- hw/audio/gus.c | 8 +--- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/hw/audio/gus.c b/hw/audio/gus.c index eb4a803fb5..c8df2bde6b 100644 --- a/hw/audio/gus.c +++ b/hw/audio/gus.c @@ -41,12 +41,6 @@ #define ldebug(...) #endif -#ifdef HOST_WORDS_BIGENDIAN -#define GUS_ENDIANNESS 1 -#else -#define GUS_ENDIANNESS 0 -#endif - #define TYPE_GUS "gus" #define GUS(obj) OBJECT_CHECK (GUSState, (obj), TYPE_GUS) @@ -256,7 +250,7 @@ static void gus_realizefn (DeviceState *dev, Error **errp) as.freq = s->freq; as.nchannels = 2; as.fmt = AUDIO_FORMAT_S16; -as.endianness = GUS_ENDIANNESS; +as.endianness = AUDIO_HOST_ENDIANNESS; s->voice = AUD_open_out ( &s->card, -- 2.21.3
Re: [PATCH v2 0/4] backup: Make sure that source and target size match
Am 30.04.2020 um 16:27 hat Kevin Wolf geschrieben: > v2: > - Fixed iotest 283 > - Corrected commit message for patch 3 [Vladimir] > - Fixed permissions for the source node, too > - Refactored the test case to avoid some duplication [Vladimir] Thanks for the review, applied to the block branch. Kevin
Re: [RFC PATCH] hw/arm/musicpal: Map the UART devices unconditionally
On 05.05.20 11:59, Philippe Mathieu-Daudé wrote:
I can't find proper documentation or datasheet, but it is likely
a MMIO mapped serial device mapped in the 0x8000..0x8000
range belongs to the SoC address space, thus is always mapped in
the memory bus.
Map the devices on the bus regardless a chardev is attached to it.
Signed-off-by: Philippe Mathieu-Daudé
---
from 2019... found while doing housekeeping
---
hw/arm/musicpal.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index b2d0cfdac8..92f33ed87e 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1619,14 +1619,10 @@ static void musicpal_init(MachineState *machine)
pic[MP_TIMER2_IRQ], pic[MP_TIMER3_IRQ],
pic[MP_TIMER4_IRQ], NULL);
-if (serial_hd(0)) {
-serial_mm_init(address_space_mem, MP_UART1_BASE, 2, pic[MP_UART1_IRQ],
- 1825000, serial_hd(0), DEVICE_NATIVE_ENDIAN);
-}
-if (serial_hd(1)) {
-serial_mm_init(address_space_mem, MP_UART2_BASE, 2, pic[MP_UART2_IRQ],
- 1825000, serial_hd(1), DEVICE_NATIVE_ENDIAN);
-}
+serial_mm_init(address_space_mem, MP_UART1_BASE, 2, pic[MP_UART1_IRQ],
+ 1825000, serial_hd(0), DEVICE_NATIVE_ENDIAN);
+serial_mm_init(address_space_mem, MP_UART2_BASE, 2, pic[MP_UART2_IRQ],
+ 1825000, serial_hd(1), DEVICE_NATIVE_ENDIAN);
/* Register flash */
dinfo = drive_get(IF_PFLASH, 0, 0);
I don't recall details anymore either (more than 10 year ago now...),
but this looks reasonable.
Reviewed-by: Jan Kiszka
Jan
Re: [PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
On 05/05/20 12:07, Philippe Mathieu-Daudé wrote: > Use the generic AUDIO_HOST_ENDIANNESS definition instead > of a custom one. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Who/what machine is using this device anyway? PC, like all old ISA audio cards. Paolo
Re: [PATCH for-5.1 V3 0/7] mips: Add Loongson-3 machine support (with KVM)
уторак, 05. мај 2020., chen huacai је написао/ла:
> Hi, Aleksandar,
>
> On Sun, May 3, 2020 at 6:50 PM Aleksandar Markovic
> wrote:
> >
> > нед, 3. мај 2020. у 12:21 Huacai Chen је
> написао/ла:
> > >
> > > Loongson-3 CPU family include Loongson-3A R1/R2/R3/R4 and Loongson-3B
> > > R1/R2. Loongson-3A R1 is the oldest and its ISA is the smallest, while
> > > Loongson-3A R4 is the newest and its ISA is almost the superset of all
> > > others. To reduce complexity, in QEMU we just define two CPU types:
> > >
> > > 1, "Loongson-3A1000" CPU which is corresponding to Loongson-3A R1. It
> is
> > >suitable for TCG because Loongson-3A R1 has fewest ASE.
> > > 2, "Loongson-3A4000" CPU which is corresponding to Loongson-3A R4. It
> is
> > >suitable for KVM because Loongson-3A R4 has the VZ ASE.
> > >
> >
> > Huacai, thanks for putting together v3, which is a little better than
> v2, and
> > thanks for addressing my previous suggestions.
> >
> > Now, give us some time to digest new data on Loongson3. We will
> > respond, but it won't happen immediately, which is, you'd agree,
> > reasonable. Just be patient.
> >
> > But again, in general, I salute your efforts very much!
> >
> > Yours, Aleksandar
> I'm sorry for this late response because I have done many tests to
> reproduce the problem reported at
> https://patchew.org/QEMU/1588501221-1205-1-git-send-
> email-che...@lemote.com/,
> but I don't have such a failure...
>
> What I have done:
> 1, "make check" on MIPS64 platform (distro is Fedora28 for Loongson);
> 2, "make check" on X86_64 platform (distro is RHEL8);
> 3, "make docker-test-quick@centos7 SHOW_ENV=1 NETWORK=1" on X86_64
> platform (distro is RHEL8);
> 4, "make docker-test-quick@centos7 SHOW_ENV=1 J=n NETWORK=1" on X86_64
> platform (distro is RHEL8 and I've tried n=2,3,414);
>
> I always get the same result:
> Not run: 259
> Passed all 117 iotests
>
> And, it seems that my patchset doesn't touch anything about iotests,
> so I don't know why the build test fails on iotests 192 (Maybe your
> build test has the same problem without my patches).
>
>
>From time to time, there is some instability in our automatic iotests. You
shouldn't bother too much about it, of course you retest in your
environments, that is good. But, in all likelyhood, your patchset doesn't
really have anything to do with the reported iotest failure.
Truly yours,
Aleksandar
> P.S.: I have found a problem that my patchset has a build failure with
> CONFIG_KVM=n, but this is another problem and I will send V4 to fix it
> (after collecting all problems in V3).
>
>
> >
> > > Loongson-3 lacks English documents. I've tried to translated them with
> > > translate.google.com, and the machine translated documents (together
> > > with their original Chinese versions) are available here.
> > >
> > > Loongson-3A R1 (Loongson-3A1000)
> > > User Manual Part 1:
> > > http://ftp.godson.ac.cn/lemote/3A1000_p1.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A1000_
> processor_user_manual_P1.pdf (Chinese Version)
> > > User Manual Part 2:
> > > http://ftp.godson.ac.cn/lemote/3A1000_p2.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A1000_
> processor_user_manual_P2.pdf (Chinese Version)
> > >
> > > Loongson-3A R2 (Loongson-3A2000)
> > > User Manual Part 1:
> > > http://ftp.godson.ac.cn/lemote/3A2000_p1.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user1.pdf (Chinese
> Version)
> > > User Manual Part 2:
> > > http://ftp.godson.ac.cn/lemote/3A2000_p2.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user2.pdf (Chinese
> Version)
> > >
> > > Loongson-3A R3 (Loongson-3A3000)
> > > User Manual Part 1:
> > > http://ftp.godson.ac.cn/lemote/3A3000_p1.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual1.pdf
> (Chinese Version)
> > > User Manual Part 2:
> > > http://ftp.godson.ac.cn/lemote/3A3000_p2.pdf
> > > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual2.pdf
> (Chinese Version)
> > >
> > > Loongson-3A R4 (Loongson-3A4000)
> > > User Manual Part 1:
> > > http://ftp.godson.ac.cn/lemote/3A4000_p1.pdf
> > > http://ftp.godson.ac.cn/lemote/3A4000user.pdf (Chinese Version)
> > > User Manual Part 2:
> > > I'm sorry that it is unavailable now.
> > >
> > > We are preparing to add QEMU's Loongson-3 support. MIPS VZ extension is
> > > fully supported in Loongson-3A R4+, so we at first add QEMU/KVM support
> > > in this series. And the next series will add QEMU/TCG support (it will
> > > emulate Loongson-3A R1).
> > >
> > > We already have a full functional Linux kernel (based on Linux-5.4.x
> LTS
> > > but not upstream yet) here:
> > >
> > > https://github.com/chenhuacai/linux
> > >
> > > How to use QEMU/Loongson-3?
> > > 1, Download kernel source from the above URL;
> > > 2, Build a kernel with arch/mips/configs/loongson3_{def,hpc}config;
> > > 3, Boot a Loongson-3A4000 host with this kernel;
> > > 4, Build QEMU-5.0.0 with this patchset;
> > > 5, modprobe kvm;
> > > 6, Use QEMU with TCG (available in
Re: [RFC PATCH] hw/arm/musicpal: Map the UART devices unconditionally
On Tue, 5 May 2020 at 11:09, Jan Kiszka wrote: > > On 05.05.20 11:59, Philippe Mathieu-Daudé wrote: > I don't recall details anymore either (more than 10 year ago now...), > but this looks reasonable. My guess is that it dates back to when the serial code would crash if passed a NULL pointer for the backend rather than treating it as "same as if connected to a /dev/null backend". thanks -- PMM
Re: [PATCH v18 QEMU 06/18] vfio: Add VM state change handler to know state of VM
On Tue, 5 May 2020 04:14:41 +0530
Kirti Wankhede wrote:
> VM state change handler gets called on change in VM's state. This is used to
> set
> VFIO device state to _RUNNING.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> hw/vfio/migration.c | 87
> +++
> hw/vfio/trace-events | 2 +
> include/hw/vfio/vfio-common.h | 4 ++
> 3 files changed, 93 insertions(+)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bf9384907ec0..e79b34003079 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -10,6 +10,7 @@
> #include "qemu/osdep.h"
> #include
>
> +#include "sysemu/runstate.h"
> #include "hw/vfio/vfio-common.h"
> #include "cpu.h"
> #include "migration/migration.h"
> @@ -74,6 +75,85 @@ err:
> return ret;
> }
>
> +static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask,
> +uint32_t value)
I find 'mask' and 'value' a bit confusing. 'mask' seems to be all the
bits you want to keep, and 'value' the bits you want to add?
> +{
> +VFIOMigration *migration = vbasedev->migration;
> +VFIORegion *region = &migration->region;
> +uint32_t device_state;
> +int ret;
> +
> +ret = pread(vbasedev->fd, &device_state, sizeof(device_state),
> +region->fd_offset + offsetof(struct
> vfio_device_migration_info,
> + device_state));
> +if (ret < 0) {
> +error_report("%s: Failed to read device state %d %s",
> + vbasedev->name, ret, strerror(errno));
> +return ret;
> +}
> +
> +device_state = (device_state & mask) | value;
> +
> +if (!VFIO_DEVICE_STATE_VALID(device_state)) {
> +return -EINVAL;
> +}
> +
> +ret = pwrite(vbasedev->fd, &device_state, sizeof(device_state),
> + region->fd_offset + offsetof(struct
> vfio_device_migration_info,
> + device_state));
> +if (ret < 0) {
> +error_report("%s: Failed to set device state %d %s",
> + vbasedev->name, ret, strerror(errno));
> +
> +ret = pread(vbasedev->fd, &device_state, sizeof(device_state),
> +region->fd_offset + offsetof(struct
> vfio_device_migration_info,
> +device_state));
> +if (ret < 0) {
> +error_report("%s: On failure, failed to read device state %d %s",
> +vbasedev->name, ret, strerror(errno));
> +return ret;
> +}
> +
> +if (VFIO_DEVICE_STATE_IS_ERROR(device_state)) {
> +error_report("%s: Device is in error state 0x%x",
> + vbasedev->name, device_state);
> +return -EFAULT;
Why -EFAULT?
Also, if the device is in an error state, don't you want to propagate
that state into the vbasedev as well? It does not look usable in that
state, but that information is only available in the migration region.
> +}
> +}
> +
> +vbasedev->device_state = device_state;
> +trace_vfio_migration_set_state(vbasedev->name, device_state);
> +return 0;
> +}
> +
> +static void vfio_vmstate_change(void *opaque, int running, RunState state)
> +{
> +VFIODevice *vbasedev = opaque;
> +
> +if ((vbasedev->vm_running != running)) {
> +int ret;
> +uint32_t value = 0, mask = 0;
> +
> +if (running) {
> +value = VFIO_DEVICE_STATE_RUNNING;
> +if (vbasedev->device_state & VFIO_DEVICE_STATE_RESUMING) {
> +mask = ~VFIO_DEVICE_STATE_RESUMING;
> +}
> +} else {
> +mask = ~VFIO_DEVICE_STATE_RUNNING;
> +}
I think the issue might be that you are starting to fiddle with the
target state before you know what the actual device state is (you only
know the state in the vbasedev, which might be out of sync.) But you do
know what the transition is supposed to look like depending on the
vmstate change, so what about the following:
- read the state from the region
- figure out the transition that is supposed to be happening
- write the target state
> +
> +ret = vfio_migration_set_state(vbasedev, mask, value);
> +if (ret) {
> +error_report("%s: Failed to set device state 0x%x",
> + vbasedev->name, value & mask);
If the transition failed, what does that mean? I assume that the device
might actually be in an unusable state (like the error state referenced
above)? Does it make sense to continue, or should the device rather be
flagged broken in some way?
> +}
> +vbasedev->vm_running = running;
> +trace_vfio_vmstate_change(vbasedev->name, running,
> RunState_str(state),
> + value & mask);
> +}
> +}
Re: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print on mmio region translation by viommu
Hi Eric,
On Tue, May 5, 2020 at 3:16 PM Bharat Bhushan wrote:
>
> hi Eric,
>
> On Tue, May 5, 2020 at 3:00 PM Auger Eric wrote:
> >
> > Hi Bharat,
> >
> > On 5/5/20 11:25 AM, Bharat Bhushan wrote:
> > > Hi Eric,
> > >
> > > On Fri, Apr 24, 2020 at 7:47 PM Auger Eric wrote:
> > >>
> > >> Hi Bharat,
> > >>
> > >> On 4/2/20 11:01 AM, Bharat Bhushan wrote:
> > >>> Hi Eric/Alex,
> > >>>
> > -Original Message-
> > From: Alex Williamson
> > Sent: Thursday, March 26, 2020 11:23 PM
> > To: Auger Eric
> > Cc: Bharat Bhushan ; peter.mayd...@linaro.org;
> > pet...@redhat.com; eric.auger@gmail.com; kevin.t...@intel.com;
> > m...@redhat.com; Tomasz Nowicki [C] ;
> > drjo...@redhat.com; linuc.dec...@gmail.com; qemu-devel@nongnu.org;
> > qemu-
> > a...@nongnu.org; bharatb.li...@gmail.com; jean-phili...@linaro.org;
> > yang.zh...@intel.com; David Gibson
> > Subject: [EXT] Re: [PATCH v9 1/9] hw/vfio/common: Remove error print
> > on mmio
> > region translation by viommu
> >
> > External Email
> >
> > --
> > On Thu, 26 Mar 2020 18:35:48 +0100
> > Auger Eric wrote:
> >
> > > Hi Alex,
> > >
> > > On 3/24/20 12:08 AM, Alex Williamson wrote:
> > >> [Cc +dwg who originated this warning]
> > >>
> > >> On Mon, 23 Mar 2020 14:16:09 +0530
> > >> Bharat Bhushan wrote:
> > >>
> > >>> On ARM, the MSI doorbell is translated by the virtual IOMMU.
> > >>> As such address_space_translate() returns the MSI controller MMIO
> > >>> region and we get an "iommu map to non memory area"
> > >>> message. Let's remove this latter.
> > >>>
> > >>> Signed-off-by: Eric Auger
> > >>> Signed-off-by: Bharat Bhushan
> > >>> ---
> > >>> hw/vfio/common.c | 2 --
> > >>> 1 file changed, 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
> > >>> 5ca11488d6..c586edf47a 100644
> > >>> --- a/hw/vfio/common.c
> > >>> +++ b/hw/vfio/common.c
> > >>> @@ -426,8 +426,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb,
> > void **vaddr,
> > >>> &xlat, &len, writable,
> > >>> MEMTXATTRS_UNSPECIFIED);
> > >>> if (!memory_region_is_ram(mr)) {
> > >>> -error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > >>> - xlat);
> > >>> return false;
> > >>> }
> > >>>
> > >>
> > >> I'm a bit confused here, I think we need more justification beyond
> > >> "we hit this warning and we don't want to because it's ok in this
> > >> one special case, therefore remove it". I assume the special case
> > >> is that the device MSI address is managed via the SET_IRQS ioctl and
> > >> therefore we won't actually get DMAs to this range.
> > > Yes exactly. The guest creates a mapping between one giova and this
> > > gpa (corresponding to the MSI controller doorbell) because MSIs are
> > > mapped on ARM. But practically the physical device is programmed with
> > > an host chosen iova that maps onto the physical MSI controller's
> > > doorbell. so the device never performs DMA accesses to this range.
> > >
> > > But I imagine the case that
> > >> was in mind when adding this warning was general peer-to-peer
> > >> between and assigned and emulated device.
> > > yes makes sense.
> > >
> > > Maybe there's an argument to be made
> > >> that such a p2p mapping might also be used in a non-vIOMMU case. We
> > >> skip creating those mappings and drivers continue to work, maybe
> > >> because nobody attempts to do p2p DMA with the types of devices we
> > >> emulate, maybe because p2p DMA is not absolutely reliable on bare
> > >> metal and drivers test it before using it.
> > > MSI doorbells are mapped using the IOMMU_MMIO flag (dma-iommu.c
> > > iommu_dma_get_msi_page).
> > > One idea could be to pass that flag through the IOMMU Notifier
> > > mechanism into the iotlb->perm. Eventually when we get this in
> > > vfio_get_vaddr() we would not print the warning. Could that make
> > > sense?
> >
> > Yeah, if we can identify a valid case that doesn't need a warning,
> > that's fine by me.
> > Thanks,
> > >>>
> > >>> Let me know if I understood the proposal correctly:
> > >>>
> > >>> virtio-iommu driver in guest will make map (VIRTIO_IOMMU_T_MAP) with
> > >>> VIRTIO_IOMMU_MAP_F_MMIO flag for MSI mapping.
> > >>> In qemu, virtio-iommu device will set a new defined flag (say
> > >>> IOMMU_MMIO) in iotlb->perm in memory_region_notify_iommu().
> > >>> vfio_get_vaddr() will check same flag and will not print the warning.>
> > >>> Is above correct?
> > >> Yes that's what I had in mind.
> > >
[PATCH v2 08/10] mips/boston: Plug memory leak in boston_mach_init()
Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
Cc: Paul Burton
Cc: Aleksandar Rikalo
Signed-off-by: Markus Armbruster
---
hw/mips/boston.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 2832dfa6ae..a896056be1 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -426,7 +426,6 @@ static void boston_mach_init(MachineState *machine)
{
DeviceState *dev;
BostonState *s;
-Error *err = NULL;
MemoryRegion *flash, *ddr_low_alias, *lcd, *platreg;
MemoryRegion *sys_mem = get_system_memory();
XilinxPCIEHost *pcie2;
@@ -467,7 +466,8 @@ static void boston_mach_init(MachineState *machine)
sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
flash = g_new(MemoryRegion, 1);
-memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB, &err);
+memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
+ &error_fatal);
memory_region_add_subregion_overlap(sys_mem, 0x1800, flash, 0);
memory_region_add_subregion_overlap(sys_mem, 0x8000, machine->ram, 0);
--
2.21.1
[PATCH v2 00/10] More miscellaneous error handling fixes
v2: * PATCH 2: missing return [Paul] * PATCH 3: commit message typo [David] * PATCH 5: error message tidied up [Eric, Philippe] * PATCH 7: commit message pasto * Old PATCH 4 dropped [Matthew] Cc: Paul Durrant Cc: David Hildenbrand Cc: Eric Blake Cc: Philippe Mathieu-Daudé Markus Armbruster (10): nvdimm: Plug memory leak in uuid property setter xen: Fix and improve handling of device_add usb-host errors s390x/cpumodel: Fix harmless misuse of visit_check_struct() tests/migration: Tighten error checking error: Use error_reportf_err() where appropriate mips/malta: Fix create_cps() error handling mips/boston: Fix boston_mach_init() error handling mips/boston: Plug memory leak in boston_mach_init() arm/sabrelite: Consistently use &error_fatal in sabrelite_init() i386: Fix x86_cpu_load_model() error API violation chardev/char-socket.c| 5 +++-- hw/arm/sabrelite.c | 7 +-- hw/mem/nvdimm.c | 1 - hw/mips/boston.c | 17 +++-- hw/mips/mips_malta.c | 15 ++- hw/sd/pxa2xx_mmci.c | 4 ++-- hw/sd/sd.c | 4 ++-- hw/usb/dev-mtp.c | 9 + hw/usb/xen-usb.c | 19 +-- qemu-nbd.c | 7 +++ scsi/qemu-pr-helper.c| 4 ++-- target/i386/cpu.c| 25 - target/s390x/cpu_models.c| 2 +- tests/qtest/migration-test.c | 4 ++-- 14 files changed, 59 insertions(+), 64 deletions(-) -- 2.21.1
[PATCH v2 10/10] i386: Fix x86_cpu_load_model() error API violation
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
x86_cpu_load_model() is wrong that way. Harmless, because its @errp
is always &error_abort. To fix, cut out the @errp middleman.
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Signed-off-by: Markus Armbruster
---
target/i386/cpu.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9c256ab159..16ed95e8da 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5078,7 +5078,7 @@ static void x86_cpu_apply_version_props(X86CPU *cpu,
X86CPUModel *model)
/* Load data from X86CPUDefinition into a X86CPU object
*/
-static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
+static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
{
X86CPUDefinition *def = model->cpudef;
CPUX86State *env = &cpu->env;
@@ -5092,13 +5092,19 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel
*model, Error **errp)
*/
/* CPU models only set _minimum_ values for level/xlevel: */
-object_property_set_uint(OBJECT(cpu), def->level, "min-level", errp);
-object_property_set_uint(OBJECT(cpu), def->xlevel, "min-xlevel", errp);
+object_property_set_uint(OBJECT(cpu), def->level, "min-level",
+ &error_abort);
+object_property_set_uint(OBJECT(cpu), def->xlevel, "min-xlevel",
+ &error_abort);
-object_property_set_int(OBJECT(cpu), def->family, "family", errp);
-object_property_set_int(OBJECT(cpu), def->model, "model", errp);
-object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
-object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+object_property_set_int(OBJECT(cpu), def->family, "family",
+&error_abort);
+object_property_set_int(OBJECT(cpu), def->model, "model",
+&error_abort);
+object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
+&error_abort);
+object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
+&error_abort);
for (w = 0; w < FEATURE_WORDS; w++) {
env->features[w] = def->features[w];
}
@@ -5135,7 +5141,8 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel
*model, Error **errp)
vendor = host_vendor;
}
-object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
+object_property_set_str(OBJECT(cpu), vendor, "vendor",
+&error_abort);
x86_cpu_apply_version_props(cpu, model);
}
@@ -6981,7 +6988,7 @@ static void x86_cpu_initfn(Object *obj)
object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort);
if (xcc->model) {
-x86_cpu_load_model(cpu, xcc->model, &error_abort);
+x86_cpu_load_model(cpu, xcc->model);
}
}
--
2.21.1
[PATCH v2 04/10] tests/migration: Tighten error checking
migrate_get_socket_address() neglects to check
visit_type_SocketAddressList() failure. This smells like a leak, but
it actually will crash dereferencing @addrs. Pass &error_abort to
remove the code smell.
Signed-off-by: Markus Armbruster
---
tests/qtest/migration-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2568c9529c..dc3490c9fa 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "libqtest.h"
+#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qemu/module.h"
#include "qemu/option.h"
@@ -301,7 +302,6 @@ static char *migrate_get_socket_address(QTestState *who,
const char *parameter)
{
QDict *rsp;
char *result;
-Error *local_err = NULL;
SocketAddressList *addrs;
Visitor *iv = NULL;
QObject *object;
@@ -310,7 +310,7 @@ static char *migrate_get_socket_address(QTestState *who,
const char *parameter)
object = qdict_get(rsp, parameter);
iv = qobject_input_visitor_new(object);
-visit_type_SocketAddressList(iv, NULL, &addrs, &local_err);
+visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
visit_free(iv);
/* we are only using a single address */
--
2.21.1
[PATCH v2 05/10] error: Use error_reportf_err() where appropriate
Replace
error_report("...: %s", ..., error_get_pretty(err));
by
error_reportf_err(err, "...: ", ...);
One of the replaced messages lacked a colon. Add it.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Reviewed-by: Philippe Mathieu-Daudé
---
chardev/char-socket.c | 5 +++--
hw/sd/pxa2xx_mmci.c | 4 ++--
hw/sd/sd.c| 4 ++--
hw/usb/dev-mtp.c | 9 +
qemu-nbd.c| 7 +++
scsi/qemu-pr-helper.c | 4 ++--
6 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..e5ee685f8c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -138,8 +138,9 @@ static void check_report_connect_error(Chardev *chr,
SocketChardev *s = SOCKET_CHARDEV(chr);
if (!s->connect_err_reported) {
-error_report("Unable to connect character device %s: %s",
- chr->label, error_get_pretty(err));
+error_reportf_err(err,
+ "Unable to connect character device %s: ",
+ chr->label);
s->connect_err_reported = true;
}
qemu_chr_socket_restart_timer(chr);
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 8f9ab0ec16..f9c50ddda5 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -497,12 +497,12 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
qdev_prop_set_drive(carddev, "drive", blk, &err);
if (err) {
-error_report("failed to init SD card: %s", error_get_pretty(err));
+error_reportf_err(err, "failed to init SD card: ");
return NULL;
}
object_property_set_bool(OBJECT(carddev), true, "realized", &err);
if (err) {
-error_report("failed to init SD card: %s", error_get_pretty(err));
+error_reportf_err(err, "failed to init SD card: ");
return NULL;
}
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 71a9af09ab..3c06a0ac6d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -703,13 +703,13 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
dev = DEVICE(obj);
qdev_prop_set_drive(dev, "drive", blk, &err);
if (err) {
-error_report("sd_init failed: %s", error_get_pretty(err));
+error_reportf_err(err, "sd_init failed: ");
return NULL;
}
qdev_prop_set_bit(dev, "spi", is_spi);
object_property_set_bool(obj, true, "realized", &err);
if (err) {
-error_report("sd_init failed: %s", error_get_pretty(err));
+error_reportf_err(err, "sd_init failed: ");
return NULL;
}
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 20717f026b..168428156b 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -631,8 +631,9 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject
*o)
int64_t id = qemu_file_monitor_add_watch(s->file_monitor, o->path,
NULL,
file_monitor_event, s, &err);
if (id == -1) {
-error_report("usb-mtp: failed to add watch for %s: %s", o->path,
- error_get_pretty(err));
+error_reportf_err(err,
+ "usb-mtp: failed to add watch for %s: ",
+ o->path);
error_free(err);
} else {
trace_usb_mtp_file_monitor_event(s->dev.addr, o->path,
@@ -1276,8 +1277,8 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
s->file_monitor = qemu_file_monitor_new(&err);
if (err) {
-error_report("usb-mtp: file monitoring init failed: %s",
- error_get_pretty(err));
+error_reportf_err(err,
+ "usb-mtp: file monitoring init failed: ");
error_free(err);
} else {
QTAILQ_INIT(&s->events);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4aa005004e..fe8b0052a2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -856,8 +856,7 @@ int main(int argc, char **argv)
}
tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
if (local_err) {
-error_report("Failed to get TLS creds %s",
- error_get_pretty(local_err));
+error_reportf_err(local_err, "Failed to get TLS creds: ");
exit(EXIT_FAILURE);
}
} else {
@@ -979,8 +978,8 @@ int main(int argc, char **argv)
&local_err);
if (sioc == NULL) {
object_unref(OBJECT(server));
-error_report("Failed to use socket activation: %s",
- error_get_pretty(local_err));
+error_reportf_err(local_err,
+ "Failed to use socket activation: ");
exit(EXIT_FAILURE);
}
qio_net_listener_add(server, sioc);
diff --git a/scs
[PATCH v2 09/10] arm/sabrelite: Consistently use &error_fatal in sabrelite_init()
Cc: Peter Maydell
Cc: Jean-Christophe Dubois
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
---
hw/arm/sabrelite.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index e31694bb92..04f4b96591 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -41,7 +41,6 @@ static void sabrelite_reset_secondary(ARMCPU *cpu,
static void sabrelite_init(MachineState *machine)
{
FslIMX6State *s;
-Error *err = NULL;
/* Check the amount of memory is compatible with the SOC */
if (machine->ram_size > FSL_IMX6_MMDC_SIZE) {
@@ -52,11 +51,7 @@ static void sabrelite_init(MachineState *machine)
s = FSL_IMX6(object_new(TYPE_FSL_IMX6));
object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
-object_property_set_bool(OBJECT(s), true, "realized", &err);
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
-}
+object_property_set_bool(OBJECT(s), true, "realized", &error_fatal);
memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
machine->ram);
--
2.21.1
[PATCH v2 07/10] mips/boston: Fix boston_mach_init() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
boston_mach_init() is wrong that way. The last calls treats an error
as fatal. Do that for the prior ones, too.
Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
Cc: Paul Burton
Cc: Aleksandar Rikalo
Signed-off-by: Markus Armbruster
---
hw/mips/boston.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 98ecd25e8e..2832dfa6ae 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -458,14 +458,11 @@ static void boston_mach_init(MachineState *machine)
sysbus_init_child_obj(OBJECT(machine), "cps", OBJECT(&s->cps),
sizeof(s->cps), TYPE_MIPS_CPS);
object_property_set_str(OBJECT(&s->cps), machine->cpu_type, "cpu-type",
-&err);
-object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp",
&err);
-object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
-}
+&error_fatal);
+object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp",
+&error_fatal);
+object_property_set_bool(OBJECT(&s->cps), true, "realized",
+ &error_fatal);
sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
--
2.21.1
[PATCH v2 03/10] s390x/cpumodel: Fix harmless misuse of visit_check_struct()
Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
violations" neglected to change visit_check_struct()'s Error **
argument along with the others. If visit_check_struct() failed, we'd
take the success path. Fortunately, it can't fail here:
qobject_input_check_struct() checks we consumed the whole dictionary,
and to get here, we did. Fix it anyway.
Cc: David Hildenbrand
Cc: Cornelia Huck
Signed-off-by: Markus Armbruster
Reviewed-by: David Hildenbrand
Reviewed-by: Cornelia Huck
---
target/s390x/cpu_models.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7c32180269..87a8028ad3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -524,7 +524,7 @@ static void cpu_model_from_info(S390CPUModel *model, const
CpuModelInfo *info,
}
}
if (!err) {
-visit_check_struct(visitor, errp);
+visit_check_struct(visitor, &err);
}
visit_end_struct(visitor, NULL);
visit_free(visitor);
--
2.21.1
[PATCH v2 06/10] mips/malta: Fix create_cps() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
create_cps() is wrong that way. The last calls treats an error as
fatal. Do that for the prior ones, too.
Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
Cc: Aleksandar Markovic
Cc: "Philippe Mathieu-Daudé"
Cc: Aurelien Jarno
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
---
hw/mips/mips_malta.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..17bf41616b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
static void create_cps(MachineState *ms, MaltaState *s,
qemu_irq *cbus_irq, qemu_irq *i8259_irq)
{
-Error *err = NULL;
-
sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
TYPE_MIPS_CPS);
-object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
-object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
-object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
-}
+object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
+&error_fatal);
+object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
+&error_fatal);
+object_property_set_bool(OBJECT(&s->cps), true, "realized",
+ &error_fatal);
sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
--
2.21.1
Re: [PATCH v25 02/10] hw/arm/virt: Introduce a RAS machine option
On Fri, 10 Apr 2020 19:46:31 +0800
Dongjiu Geng wrote:
> RAS Virtualization feature is not supported now, so
> add a RAS machine option and disable it by default.
>
> Reviewed-by: Peter Maydell
> Signed-off-by: Dongjiu Geng
> Signed-off-by: Xiang Zheng
> Reviewed-by: Jonathan Cameron
Reviewed-by: Igor Mammedov
> ---
> hw/arm/virt.c | 23 +++
> include/hw/arm/virt.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7dc96ab..20409b9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1960,6 +1960,20 @@ static void virt_set_acpi(Object *obj, Visitor *v,
> const char *name,
> visit_type_OnOffAuto(v, name, &vms->acpi, errp);
> }
>
> +static bool virt_get_ras(Object *obj, Error **errp)
> +{
> +VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +return vms->ras;
> +}
> +
> +static void virt_set_ras(Object *obj, bool value, Error **errp)
> +{
> +VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +vms->ras = value;
> +}
> +
> static char *virt_get_gic_version(Object *obj, Error **errp)
> {
> VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -2284,6 +2298,15 @@ static void virt_instance_init(Object *obj)
> "Valid values are none and smmuv3",
> NULL);
>
> +/* Default disallows RAS instantiation */
> +vms->ras = false;
> +object_property_add_bool(obj, "ras", virt_get_ras,
> + virt_set_ras, NULL);
> +object_property_set_description(obj, "ras",
> +"Set on/off to enable/disable reporting
> host memory errors "
> +"to a KVM guest using ACPI and guest
> external abort exceptions",
> +NULL);
> +
> vms->irqmap = a15irqmap;
>
> virt_flash_create(vms);
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 60b2f52..6401662 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -131,6 +131,7 @@ typedef struct {
> bool highmem_ecam;
> bool its;
> bool virt;
> +bool ras;
> OnOffAuto acpi;
> VirtGICType gic_version;
> VirtIOMMUType iommu;
[PATCH v2 01/10] nvdimm: Plug memory leak in uuid property setter
nvdimm_set_uuid() leaks memory on qemu_uuid_parse() failure. Fix
that.
Fixes: 6c5627bb24dcd68c997857a8b671617333b1289f
Cc: Xiao Guangrong
Cc: Shivaprasad G Bhat
Signed-off-by: Markus Armbruster
---
hw/mem/nvdimm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 8e426d24bb..d5752f7bf6 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -97,7 +97,6 @@ static void nvdimm_set_uuid(Object *obj, Visitor *v, const
char *name,
if (qemu_uuid_parse(value, &nvdimm->uuid) != 0) {
error_setg(errp, "Property '%s.%s' has invalid value",
object_get_typename(obj), name);
-goto out;
}
g_free(value);
--
2.21.1
[PATCH V2] tests/Makefile: Fix description of "make check"
The description of "make check" is out-of-date, so fix it by adding block and softfloat. Reviewed-by: Claudio Fontana Signed-off-by: Huacai Chen --- tests/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 03a74b6..5d32239 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -4,7 +4,7 @@ check-help: @echo "Regression testing targets:" @echo - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and decodetree" + @echo " $(MAKE) checkRun block, qapi-schema, unit, softfloat, qtest and decodetree tests" @echo @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" @echo " $(MAKE) check-qtest Run qtest tests" -- 2.7.0
[PATCH v2 02/10] xen: Fix and improve handling of device_add usb-host errors
usbback_portid_add() leaks the error when qdev_device_add() fails.
Fix that. While there, use the error to improve the error message.
The qemu_opts_from_qdict() similarly leaks on failure. But any
failure there is a programming error. Pass &error_abort.
Fixes: 816ac92ef769f9ffc534e49a1bb6177bddce7aa2
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: Gerd Hoffmann
Cc: xen-de...@lists.xenproject.org
Signed-off-by: Markus Armbruster
---
hw/usb/xen-usb.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 961190d0f7..4d266d7bb4 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -30,6 +30,7 @@
#include "hw/usb.h"
#include "hw/xen/xen-legacy-backend.h"
#include "monitor/qdev.h"
+#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qstring.h"
@@ -755,13 +756,16 @@ static void usbback_portid_add(struct usbback_info
*usbif, unsigned port,
qdict_put_int(qdict, "port", port);
qdict_put_int(qdict, "hostbus", atoi(busid));
qdict_put_str(qdict, "hostport", portname);
-opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
-if (local_err) {
-goto err;
-}
+opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
+&error_abort);
usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
if (!usbif->ports[port - 1].dev) {
-goto err;
+qobject_unref(qdict);
+xen_pv_printf(&usbif->xendev, 0,
+ "device %s could not be opened: %s\n",
+ busid, error_get_pretty(local_err));
+error_free(local_err);
+return;
}
qobject_unref(qdict);
speed = usbif->ports[port - 1].dev->speed;
@@ -793,11 +797,6 @@ static void usbback_portid_add(struct usbback_info *usbif,
unsigned port,
usbback_hotplug_enq(usbif, port);
TR_BUS(&usbif->xendev, "port %d attached\n", port);
-return;
-
-err:
-qobject_unref(qdict);
-xen_pv_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
}
static void usbback_process_port(struct usbback_info *usbif, unsigned port)
--
2.21.1
Re: [PATCH] tests/Makefile: Fix description of "make check"
уторак, 05. мај 2020., Huacai Chen је написао/ла: > The description of "make check" is out-of-date, so fix it by adding > block and softfloat. > > Signed-off-by: Huacai Chen > --- Reviewed-by: Aleksandar Markovic > tests/Makefile.include | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 03a74b6..5d32239 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -4,7 +4,7 @@ > check-help: > @echo "Regression testing targets:" > @echo > - @echo " $(MAKE) checkRun unit, qapi-schema, qtest > and decodetree" > + @echo " $(MAKE) checkRun block, qapi-schema, unit, > softfloat, qtest and decodetree" > @echo > @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given > target" > @echo " $(MAKE) check-qtest Run qtest tests" > -- > 2.7.0 > > >
Re: [PATCH v25 05/10] ACPI: Build Hardware Error Source Table
On Fri, 10 Apr 2020 19:46:34 +0800
Dongjiu Geng wrote:
> This patch builds Hardware Error Source Table(HEST) via fw_cfg blobs.
> Now it only supports ARMv8 SEA, a type of Generic Hardware Error
> Source version 2(GHESv2) error source. Afterwards, we can extend
> the supported types if needed. For the CPER section, currently it
> is memory section because kernel mainly wants userspace to handle
> the memory errors.
>
> This patch follows the spec ACPI 6.2 to build the Hardware Error
> Source table. For more detailed information, please refer to
> document: docs/specs/acpi_hest_ghes.rst
>
> build_ghes_hw_error_notification() helper will help to add Hardware
> Error Notification to ACPI tables without using packed C structures
> and avoid endianness issues as API doesn't need explicit conversion.
>
> Signed-off-by: Xiang Zheng
> Signed-off-by: Dongjiu Geng
Reviewed-by: Igor Mammedov
> ---
> change since v24:
> 1. Add acpi_add_table() before acpi_build_hest()
> 2. Pass NULL for oem_table_id in build_header() to build Hardware
>Error Source Table header
> ---
> hw/acpi/ghes.c | 126
> +++
> hw/arm/virt-acpi-build.c | 2 +
> include/hw/acpi/ghes.h | 39 +++
> 3 files changed, 167 insertions(+)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index e1b3f8f..091fd87 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -23,6 +23,7 @@
> #include "qemu/units.h"
> #include "hw/acpi/ghes.h"
> #include "hw/acpi/aml-build.h"
> +#include "qemu/error-report.h"
>
> #define ACPI_GHES_ERRORS_FW_CFG_FILE"etc/hardware_errors"
> #define ACPI_GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> @@ -33,6 +34,42 @@
> /* Now only support ARMv8 SEA notification type error source */
> #define ACPI_GHES_ERROR_SOURCE_COUNT1
>
> +/* Generic Hardware Error Source version 2 */
> +#define ACPI_GHES_SOURCE_GENERIC_ERROR_V2 10
> +
> +/* Address offset in Generic Address Structure(GAS) */
> +#define GAS_ADDR_OFFSET 4
> +
> +/*
> + * Hardware Error Notification
> + * ACPI 4.0: 17.3.2.7 Hardware Error Notification
> + * Composes dummy Hardware Error Notification descriptor of specified type
> + */
> +static void build_ghes_hw_error_notification(GArray *table, const uint8_t
> type)
> +{
> +/* Type */
> +build_append_int_noprefix(table, type, 1);
> +/*
> + * Length:
> + * Total length of the structure in bytes
> + */
> +build_append_int_noprefix(table, 28, 1);
> +/* Configuration Write Enable */
> +build_append_int_noprefix(table, 0, 2);
> +/* Poll Interval */
> +build_append_int_noprefix(table, 0, 4);
> +/* Vector */
> +build_append_int_noprefix(table, 0, 4);
> +/* Switch To Polling Threshold Value */
> +build_append_int_noprefix(table, 0, 4);
> +/* Switch To Polling Threshold Window */
> +build_append_int_noprefix(table, 0, 4);
> +/* Error Threshold Value */
> +build_append_int_noprefix(table, 0, 4);
> +/* Error Threshold Window */
> +build_append_int_noprefix(table, 0, 4);
> +}
> +
> /*
> * Build table for the hardware error fw_cfg blob.
> * Initialize "etc/hardware_errors" and "etc/hardware_errors_addr" fw_cfg
> blobs.
> @@ -87,3 +124,92 @@ void build_ghes_error_table(GArray *hardware_errors,
> BIOSLinker *linker)
> bios_linker_loader_write_pointer(linker, ACPI_GHES_DATA_ADDR_FW_CFG_FILE,
> 0, sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE, 0);
> }
> +
> +/* Build Generic Hardware Error Source version 2 (GHESv2) */
> +static void build_ghes_v2(GArray *table_data, int source_id, BIOSLinker
> *linker)
> +{
> +uint64_t address_offset;
> +/*
> + * Type:
> + * Generic Hardware Error Source version 2(GHESv2 - Type 10)
> + */
> +build_append_int_noprefix(table_data, ACPI_GHES_SOURCE_GENERIC_ERROR_V2,
> 2);
> +/* Source Id */
> +build_append_int_noprefix(table_data, source_id, 2);
> +/* Related Source Id */
> +build_append_int_noprefix(table_data, 0x, 2);
> +/* Flags */
> +build_append_int_noprefix(table_data, 0, 1);
> +/* Enabled */
> +build_append_int_noprefix(table_data, 1, 1);
> +
> +/* Number of Records To Pre-allocate */
> +build_append_int_noprefix(table_data, 1, 4);
> +/* Max Sections Per Record */
> +build_append_int_noprefix(table_data, 1, 4);
> +/* Max Raw Data Length */
> +build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4);
> +
> +address_offset = table_data->len;
> +/* Error Status Address */
> +build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
> + 4 /* QWord access */, 0);
> +bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> +address_offset + GAS_ADDR_OFFSET, sizeof(uint64_t),
> +ACPI_GHES_ERRORS_FW_CFG_FILE, source_id * sizeof(uint64_t));
> +
> +switch (source_id) {
> +case ACPI_HEST_SRC_ID_SEA:
> +/*
> +
Re: [PATCH v23 0/4] implement zstd cluster compression method
On 30.04.20 12:19, Denis Plotnikov wrote: > v23: >Undecided: whether to add zstd(zlib) compression > details to the qcow2 spec >03: tighten assertion on zstd decompression [Eric] >04: use _rm_test_img appropriately [Max] Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v25 06/10] ACPI: Record the Generic Error Status Block address
On Fri, 10 Apr 2020 19:46:35 +0800
Dongjiu Geng wrote:
> Record the GHEB address via fw_cfg file, when recording
> a error to CPER, it will use this address to find out
> Generic Error Data Entries and write the error.
>
> In order to avoid migration failure, make hardware
> error table address to a part of GED device instead
> of global variable, then this address will be migrated
> to target QEMU.
>
> Acked-by: Xiang Zheng
> Signed-off-by: Dongjiu Geng
Reviewed-by: Igor Mammedov
> ---
> change since v24:
> 1. Use s->ghes_state.ghes_addr_le to check in ghes_needed()
> 2. Using hardware_error->len instead of request_block_size to calculate in
> acpi_ghes_add_fw_cfg()
> 3. Remove assert(vms->acpi_dev) be build APEI table
> 4. Directly use ACPI_GED(vms->acpi_dev) instead of ACPI_GED(vms->acpi_dev)
> ---
> hw/acpi/generic_event_device.c | 19 +++
> hw/acpi/ghes.c | 14 ++
> hw/arm/virt-acpi-build.c | 8
> include/hw/acpi/generic_event_device.h | 2 ++
> include/hw/acpi/ghes.h | 6 ++
> 5 files changed, 49 insertions(+)
>
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 021ed2b..1491291 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -234,6 +234,24 @@ static const VMStateDescription vmstate_ged_state = {
> }
> };
>
> +static bool ghes_needed(void *opaque)
> +{
> +AcpiGedState *s = opaque;
> +return s->ghes_state.ghes_addr_le;
> +}
> +
> +static const VMStateDescription vmstate_ghes_state = {
> +.name = "acpi-ged/ghes",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = ghes_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
> + vmstate_ghes_state, AcpiGhesState),
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
> static const VMStateDescription vmstate_acpi_ged = {
> .name = "acpi-ged",
> .version_id = 1,
> @@ -244,6 +262,7 @@ static const VMStateDescription vmstate_acpi_ged = {
> },
> .subsections = (const VMStateDescription * []) {
> &vmstate_memhp_state,
> +&vmstate_ghes_state,
> NULL
> }
> };
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 091fd87..e74af23 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -24,6 +24,8 @@
> #include "hw/acpi/ghes.h"
> #include "hw/acpi/aml-build.h"
> #include "qemu/error-report.h"
> +#include "hw/acpi/generic_event_device.h"
> +#include "hw/nvram/fw_cfg.h"
>
> #define ACPI_GHES_ERRORS_FW_CFG_FILE"etc/hardware_errors"
> #define ACPI_GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> @@ -213,3 +215,15 @@ void acpi_build_hest(GArray *table_data, BIOSLinker
> *linker)
> build_header(linker, table_data, (void *)(table_data->data + hest_start),
> "HEST", table_data->len - hest_start, 1, NULL, NULL);
> }
> +
> +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> + GArray *hardware_error)
> +{
> +/* Create a read-only fw_cfg file for GHES */
> +fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
> +hardware_error->len);
> +
> +/* Create a read-write fw_cfg file for Address */
> +fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
> +NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false);
> +}
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f611bce..2726aac 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -911,6 +911,7 @@ void virt_acpi_setup(VirtMachineState *vms)
> {
> AcpiBuildTables tables;
> AcpiBuildState *build_state;
> +AcpiGedState *acpi_ged_state;
>
> if (!vms->fw_cfg) {
> trace_virt_acpi_setup();
> @@ -941,6 +942,13 @@ void virt_acpi_setup(VirtMachineState *vms)
> fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> tables.tcpalog->data,
> acpi_data_len(tables.tcpalog));
>
> +if (vms->ras) {
> +assert(vms->acpi_dev);
> +acpi_ged_state = ACPI_GED(vms->acpi_dev);
> +acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state,
> + vms->fw_cfg, tables.hardware_errors);
> +}
> +
> build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
> build_state, tables.rsdp,
> ACPI_BUILD_RSDP_FILE, 0);
> diff --git a/include/hw/acpi/generic_event_device.h
> b/include/hw/acpi/generic_event_device.h
> index d157eac..037d2b5 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -61,6 +61,7 @@
>
> #include "hw/sysbus.h"
> #include "hw/acpi/memory_hotplug.h"
> +#include "hw/acpi/ghes.h"
>
> #define ACPI_POWER_BUTTON_DEVICE "P
Re: [PATCH V2] tests/Makefile: Fix description of "make check"
On 5/5/20 12:24 PM, Huacai Chen wrote: The description of "make check" is out-of-date, so fix it by adding block and softfloat. Reviewed-by: Claudio Fontana Signed-off-by: Huacai Chen --- tests/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 03a74b6..5d32239 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -4,7 +4,7 @@ check-help: @echo "Regression testing targets:" @echo - @echo " $(MAKE) checkRun unit, qapi-schema, qtest and decodetree" + @echo " $(MAKE) checkRun block, qapi-schema, unit, softfloat, qtest and decodetree tests" @echo @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target" @echo " $(MAKE) check-qtest Run qtest tests" Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé
Re: [PATCH v2 00/15] target/arm: partial vector cleanup
On Sat, 2 May 2020 at 23:45, Richard Henderson wrote: > > This is not complete, but shows the direction I'd like to go. > > It may well help what Peter is doing with the neon decodetree > conversion. It may be helpful to apply before the conversion > in order to reduce the number of special cases. As may > continuing with the cleanup; I'll probably work on that more > next week. > > Version 2 extracts more bits from my sve2 branch. There's > still more to pull back, especially for crypto_helper.c, where > there are also tail clearing bugs to fix. This doesn't apply on master any more as a result of the first lump of decodetree stuff going in. Also patch 2 doesn't seem to compile: /home/petmay01/linaro/qemu-from-laptop/qemu/target/arm/translate.c: In function ‘gen_gvec_srshr’: /home/petmay01/linaro/qemu-from-laptop/qemu/target/arm/translate.c:4107:9: error: implicit declaration of function ‘tcg_gen_gvec_dup_imm’; did you mean ‘tcg_gen_gvec_dup_i64’? [-Werror=implicit-function-declaration] tcg_gen_gvec_dup_imm(vece, rd_ofs, opr_sz, max_sz, 0); ^~~~ tcg_gen_gvec_dup_i64 /home/petmay01/linaro/qemu-from-laptop/qemu/target/arm/translate.c:4107:9: error: nested extern declaration of ‘tcg_gen_gvec_dup_imm’ [-Werror=nested-externs] Any chance you could do a rebase and resend? thanks -- PMM
Re: [PATCH v2 04/10] tests/migration: Tighten error checking
On 5/5/20 12:19 PM, Markus Armbruster wrote:
migrate_get_socket_address() neglects to check
visit_type_SocketAddressList() failure. This smells like a leak, but
it actually will crash dereferencing @addrs. Pass &error_abort to
remove the code smell.
Signed-off-by: Markus Armbruster
---
tests/qtest/migration-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2568c9529c..dc3490c9fa 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "libqtest.h"
+#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qemu/module.h"
#include "qemu/option.h"
@@ -301,7 +302,6 @@ static char *migrate_get_socket_address(QTestState *who,
const char *parameter)
{
QDict *rsp;
char *result;
-Error *local_err = NULL;
SocketAddressList *addrs;
Visitor *iv = NULL;
QObject *object;
@@ -310,7 +310,7 @@ static char *migrate_get_socket_address(QTestState *who,
const char *parameter)
object = qdict_get(rsp, parameter);
iv = qobject_input_visitor_new(object);
-visit_type_SocketAddressList(iv, NULL, &addrs, &local_err);
+visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
visit_free(iv);
/* we are only using a single address */
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 08/10] mips/boston: Plug memory leak in boston_mach_init()
уторак, 05. мај 2020., Markus Armbruster је написао/ла:
> Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
> Cc: Paul Burton
> Cc: Aleksandar Rikalo
> Signed-off-by: Markus Armbruster
> ---
> hw/mips/boston.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>
Thank you, Markus, for spotting this error.
Reviewed-by: Aleksandar Markovic
I am fine with you selecting this and another mips-related patch in this
series in your pull request, that will be result of this series.
Truly yours,
Aleksandar
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 2832dfa6ae..a896056be1 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -426,7 +426,6 @@ static void boston_mach_init(MachineState *machine)
> {
> DeviceState *dev;
> BostonState *s;
> -Error *err = NULL;
> MemoryRegion *flash, *ddr_low_alias, *lcd, *platreg;
> MemoryRegion *sys_mem = get_system_memory();
> XilinxPCIEHost *pcie2;
> @@ -467,7 +466,8 @@ static void boston_mach_init(MachineState *machine)
> sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>
> flash = g_new(MemoryRegion, 1);
> -memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB, &err);
> +memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
> + &error_fatal);
> memory_region_add_subregion_overlap(sys_mem, 0x1800, flash, 0);
>
> memory_region_add_subregion_overlap(sys_mem, 0x8000,
> machine->ram, 0);
> --
> 2.21.1
>
>
>
Re: [PATCH v2 07/10] mips/boston: Fix boston_mach_init() error handling
уторак, 05. мај 2020., Markus Armbruster је написао/ла:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL. Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
>
> boston_mach_init() is wrong that way. The last calls treats an error
> as fatal. Do that for the prior ones, too.
>
> Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
> Cc: Paul Burton
> Cc: Aleksandar Rikalo
> Signed-off-by: Markus Armbruster
> ---
Reviewed-by: Aleksandar Markovic
> hw/mips/boston.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 98ecd25e8e..2832dfa6ae 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -458,14 +458,11 @@ static void boston_mach_init(MachineState *machine)
> sysbus_init_child_obj(OBJECT(machine), "cps", OBJECT(&s->cps),
>sizeof(s->cps), TYPE_MIPS_CPS);
> object_property_set_str(OBJECT(&s->cps), machine->cpu_type,
> "cpu-type",
> -&err);
> -object_property_set_int(OBJECT(&s->cps), machine->smp.cpus,
> "num-vp", &err);
> -object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
> -
> -if (err != NULL) {
> -error_report("%s", error_get_pretty(err));
> -exit(1);
> -}
> +&error_fatal);
> +object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp",
> +&error_fatal);
> +object_property_set_bool(OBJECT(&s->cps), true, "realized",
> + &error_fatal);
>
> sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>
> --
> 2.21.1
>
>
>
Re: [PATCH v2 06/10] mips/malta: Fix create_cps() error handling
уторак, 05. мај 2020., Markus Armbruster је написао/ла:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL. Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
>
> create_cps() is wrong that way. The last calls treats an error as
> fatal. Do that for the prior ones, too.
>
> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
> Cc: Aleksandar Markovic
> Cc: "Philippe Mathieu-Daudé"
> Cc: Aurelien Jarno
> Signed-off-by: Markus Armbruster
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
> ---
> hw/mips/mips_malta.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
>
Reviewed-by: Aleksandar Markovic
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index e4c4de1b4e..17bf41616b 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState
> *ms,
> static void create_cps(MachineState *ms, MaltaState *s,
> qemu_irq *cbus_irq, qemu_irq *i8259_irq)
> {
> -Error *err = NULL;
> -
> sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps),
> sizeof(s->cps),
>TYPE_MIPS_CPS);
> -object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
> &err);
> -object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
> &err);
> -object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
> -if (err != NULL) {
> -error_report("%s", error_get_pretty(err));
> -exit(1);
> -}
> +object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
> +&error_fatal);
> +object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
> +&error_fatal);
> +object_property_set_bool(OBJECT(&s->cps), true, "realized",
> + &error_fatal);
>
> sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>
> --
> 2.21.1
>
>
Re: [PATCH v25 08/10] ACPI: Record Generic Error Status Block(GESB) table
On Fri, 10 Apr 2020 19:46:37 +0800
Dongjiu Geng wrote:
> kvm_arch_on_sigbus_vcpu() error injection uses source_id as
> index in etc/hardware_errors to find out Error Status Data
> Block entry corresponding to error source. So supported source_id
> values should be assigned here and not be changed afterwards to
> make sure that guest will write error into expected Error Status
> Data Block.
>
> Before QEMU writes a new error to ACPI table, it will check whether
> previous error has been acknowledged. If not acknowledged, the new
> errors will be ignored and not be recorded. For the errors section
> type, QEMU simulate it to memory section error.
>
> Signed-off-by: Dongjiu Geng
> Signed-off-by: Xiang Zheng
Reviewed-by: Igor Mammedov
Also we should ratelimit error messages that could be triggered at runtime
from acpi_ghes_record_errors() and functions it's calling.
It could be a patch on top.
> ---
> change since v24:
> 1. Using g_array_append_vals() to replace build_append_int_noprefix() to
> build FRU Text.
> 2. Remove the judgement that judge whether acpi_ged_state is NULL.
> 3. Add le64_to_cpu() to error_block_address
> ---
> hw/acpi/ghes.c | 219
> +
> include/hw/acpi/ghes.h | 1 +
> 2 files changed, 220 insertions(+)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index e74af23..a3ab2e4 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -26,6 +26,7 @@
> #include "qemu/error-report.h"
> #include "hw/acpi/generic_event_device.h"
> #include "hw/nvram/fw_cfg.h"
> +#include "qemu/uuid.h"
>
> #define ACPI_GHES_ERRORS_FW_CFG_FILE"etc/hardware_errors"
> #define ACPI_GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> @@ -43,6 +44,36 @@
> #define GAS_ADDR_OFFSET 4
>
> /*
> + * The total size of Generic Error Data Entry
> + * ACPI 6.1/6.2: 18.3.2.7.1 Generic Error Data,
> + * Table 18-343 Generic Error Data Entry
> + */
> +#define ACPI_GHES_DATA_LENGTH 72
> +
> +/* The memory section CPER size, UEFI 2.6: N.2.5 Memory Error Section */
> +#define ACPI_GHES_MEM_CPER_LENGTH 80
> +
> +/* Masks for block_status flags */
> +#define ACPI_GEBS_UNCORRECTABLE 1
> +
> +/*
> + * Total size for Generic Error Status Block except Generic Error Data
> Entries
> + * ACPI 6.2: 18.3.2.7.1 Generic Error Data,
> + * Table 18-380 Generic Error Status Block
> + */
> +#define ACPI_GHES_GESB_SIZE 20
> +
> +/*
> + * Values for error_severity field
> + */
> +enum AcpiGenericErrorSeverity {
> +ACPI_CPER_SEV_RECOVERABLE = 0,
> +ACPI_CPER_SEV_FATAL = 1,
> +ACPI_CPER_SEV_CORRECTED = 2,
> +ACPI_CPER_SEV_NONE = 3,
> +};
> +
> +/*
> * Hardware Error Notification
> * ACPI 4.0: 17.3.2.7 Hardware Error Notification
> * Composes dummy Hardware Error Notification descriptor of specified type
> @@ -73,6 +104,138 @@ static void build_ghes_hw_error_notification(GArray
> *table, const uint8_t type)
> }
>
> /*
> + * Generic Error Data Entry
> + * ACPI 6.1: 18.3.2.7.1 Generic Error Data
> + */
> +static void acpi_ghes_generic_error_data(GArray *table,
> +const uint8_t *section_type, uint32_t error_severity,
> +uint8_t validation_bits, uint8_t flags,
> +uint32_t error_data_length, QemuUUID fru_id,
> +uint64_t time_stamp)
> +{
> +const uint8_t fru_text[20] = {0};
> +
> +/* Section Type */
> +g_array_append_vals(table, section_type, 16);
> +
> +/* Error Severity */
> +build_append_int_noprefix(table, error_severity, 4);
> +/* Revision */
> +build_append_int_noprefix(table, 0x300, 2);
> +/* Validation Bits */
> +build_append_int_noprefix(table, validation_bits, 1);
> +/* Flags */
> +build_append_int_noprefix(table, flags, 1);
> +/* Error Data Length */
> +build_append_int_noprefix(table, error_data_length, 4);
> +
> +/* FRU Id */
> +g_array_append_vals(table, fru_id.data, ARRAY_SIZE(fru_id.data));
> +
> +/* FRU Text */
> +g_array_append_vals(table, fru_text, sizeof(fru_text));
> +
> +/* Timestamp */
> +build_append_int_noprefix(table, time_stamp, 8);
> +}
> +
> +/*
> + * Generic Error Status Block
> + * ACPI 6.1: 18.3.2.7.1 Generic Error Data
> + */
> +static void acpi_ghes_generic_error_status(GArray *table, uint32_t
> block_status,
> +uint32_t raw_data_offset, uint32_t raw_data_length,
> +uint32_t data_length, uint32_t error_severity)
> +{
> +/* Block Status */
> +build_append_int_noprefix(table, block_status, 4);
> +/* Raw Data Offset */
> +build_append_int_noprefix(table, raw_data_offset, 4);
> +/* Raw Data Length */
> +build_append_int_noprefix(table, raw_data_length, 4);
> +/* Data Length */
> +build_append_int_noprefix(table, data_length, 4);
> +/* Error Severity */
> +build_append_int_noprefix(table, error_severity, 4);
> +}
> +
> +/* UEFI 2.6: N.2.5 Memory Error
Re: [PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
On 5/5/20 12:10 PM, Paolo Bonzini wrote: On 05/05/20 12:07, Philippe Mathieu-Daudé wrote: Use the generic AUDIO_HOST_ENDIANNESS definition instead of a custom one. Signed-off-by: Philippe Mathieu-Daudé --- Who/what machine is using this device anyway? PC, like all old ISA audio cards. I imagined, but any particular project in mind? I'm wondering if we should add a test for it, and what kind of testing.
Re: [PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
On 05/05/20 12:45, Philippe Mathieu-Daudé wrote: > On 5/5/20 12:10 PM, Paolo Bonzini wrote: >> On 05/05/20 12:07, Philippe Mathieu-Daudé wrote: >>> Use the generic AUDIO_HOST_ENDIANNESS definition instead >>> of a custom one. >>> >>> Signed-off-by: Philippe Mathieu-Daudé >>> --- >>> Who/what machine is using this device anyway? >> >> PC, like all old ISA audio cards. > > I imagined, but any particular project in mind? I'm wondering if we > should add a test for it, and what kind of testing. Old games and demos use it. Most demos don't work that well on QEMU though. Paolo
Re: [PATCH v18 QEMU 07/18] vfio: Add migration state change notifier
On Tue, 5 May 2020 04:14:42 +0530
Kirti Wankhede wrote:
> Added migration state change notifier to get notification on migration state
> change. These states are translated to VFIO device state and conveyed to
> vendor
> driver.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> hw/vfio/migration.c | 30 ++
> hw/vfio/trace-events | 5 +++--
> include/hw/vfio/vfio-common.h | 1 +
> 3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index e79b34003079..c2f5564b51c3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -154,6 +154,28 @@ static void vfio_vmstate_change(void *opaque, int
> running, RunState state)
> }
> }
>
> +static void vfio_migration_state_notifier(Notifier *notifier, void *data)
> +{
> +MigrationState *s = data;
> +VFIODevice *vbasedev = container_of(notifier, VFIODevice,
> migration_state);
> +int ret;
> +
> +trace_vfio_migration_state_notifier(vbasedev->name,
> +MigrationStatus_str(s->state));
> +
> +switch (s->state) {
> +case MIGRATION_STATUS_CANCELLING:
> +case MIGRATION_STATUS_CANCELLED:
> +case MIGRATION_STATUS_FAILED:
> +ret = vfio_migration_set_state(vbasedev,
> + ~(VFIO_DEVICE_STATE_SAVING |
> VFIO_DEVICE_STATE_RESUMING),
> + VFIO_DEVICE_STATE_RUNNING);
> +if (ret) {
> +error_report("%s: Failed to set state RUNNING", vbasedev->name);
> +}
What is the actually desired state transition here? Again, I find the
interface hard to read... and my comments regarding state propagation
and error states from the previous patch apply here as well.
> +}
> +}
> +
Re: [PATCH V2] tests/Makefile: Fix description of "make check"
уторак, 05. мај 2020., Huacai Chen је написао/ла: > The description of "make check" is out-of-date, so fix it by adding > block and softfloat. > > Reviewed-by: Claudio Fontana > Signed-off-by: Huacai Chen > --- > tests/Makefile.include | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > For this version too: Reviewed-by: Aleksandar Markovic > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 03a74b6..5d32239 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -4,7 +4,7 @@ > check-help: > @echo "Regression testing targets:" > @echo > - @echo " $(MAKE) checkRun unit, qapi-schema, qtest > and decodetree" > + @echo " $(MAKE) checkRun block, qapi-schema, unit, > softfloat, qtest and decodetree tests" > @echo > @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given > target" > @echo " $(MAKE) check-qtest Run qtest tests" > -- > 2.7.0 > > >
Re: [PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
On 5/5/20 12:49 PM, Paolo Bonzini wrote: On 05/05/20 12:45, Philippe Mathieu-Daudé wrote: On 5/5/20 12:10 PM, Paolo Bonzini wrote: On 05/05/20 12:07, Philippe Mathieu-Daudé wrote: Use the generic AUDIO_HOST_ENDIANNESS definition instead of a custom one. Signed-off-by: Philippe Mathieu-Daudé --- Who/what machine is using this device anyway? PC, like all old ISA audio cards. I imagined, but any particular project in mind? I'm wondering if we should add a test for it, and what kind of testing. Old games and demos use it. Most demos don't work that well on QEMU though. Good. Cc'ing Max in case he knows a such demo we can use for testing. Thanks! Paolo
Re: [PATCH v4 0/5] block-copy: use aio-task-pool
On 29.04.20 15:08, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > v4 > 01: add Max's r-b > 04: move variable definition to the top of the block, add Max's r-b > 05: - change error-codes in block_copy_task_run(), document them > and be more accurate about error code in block_copy_dirty_clusters(). > - s/g_free(aio)/aio_task_pool_free(aio)/ Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
Today KVM external call canceled
Hi As there are no topics, we cancel that call. Happy hacking.
KVM call for agenda for 2020-05-19
Hi Please, send any topic that you are interested in covering. At the end of Monday I will send an email with the agenda or the cancellation of the call, so hurry up. After discussions on the QEMU Summit, we are going to have always open a KVM call where you can add topics. Call details: By popular demand, a google calendar public entry with it https://www.google.com/calendar/embed?src=dG9iMXRqcXAzN3Y4ZXZwNzRoMHE4a3BqcXNAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ (Let me know if you have any problems with the calendar entry. I just gave up about getting right at the same time CEST, CET, EDT and DST). If you need phone number details, contact me privately Thanks, Juan.
Re: [PATCH] hw/audio/gus: Use AUDIO_HOST_ENDIANNESS definition from 'audio/audio.h'
On 05.05.20 12:55, Philippe Mathieu-Daudé wrote: > On 5/5/20 12:49 PM, Paolo Bonzini wrote: >> On 05/05/20 12:45, Philippe Mathieu-Daudé wrote: >>> On 5/5/20 12:10 PM, Paolo Bonzini wrote: On 05/05/20 12:07, Philippe Mathieu-Daudé wrote: > Use the generic AUDIO_HOST_ENDIANNESS definition instead > of a custom one. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Who/what machine is using this device anyway? PC, like all old ISA audio cards. >>> >>> I imagined, but any particular project in mind? I'm wondering if we >>> should add a test for it, and what kind of testing. >> >> Old games and demos use it. Most demos don't work that well on QEMU >> though. > > Good. Cc'ing Max in case he knows a such demo we can use for testing. I don’t know how that impression could have manifested, but I’m actually not really an expert on old demos or games. (I just happened to write some 512 byte stuff at some point, but the only sound I ever used there was over the PC speaker...) Max
Re: [PATCH v3 03/33] block: Add BdrvChildRole and BdrvChildRoleBits
Am 18.02.2020 um 13:42 hat Max Reitz geschrieben:
> This mask will supplement BdrvChildClass when it comes to what role (or
> combination of roles) a child takes for its parent. It consists of
> BdrvChildRoleBits values (which is an enum).
>
> Because empty enums are not allowed, let us just start with it filled.
>
> Signed-off-by: Max Reitz
> ---
> include/block/block.h | 38 ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index fd89eb6c75..8c23948d08 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -268,6 +268,44 @@ enum {
> DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
> };
>
> +enum BdrvChildRoleBits {
> +/* Child stores data */
> +BDRV_CHILD_DATA = (1 << 0),
> +
> +/* Child stores metadata */
> +BDRV_CHILD_METADATA = (1 << 1),
> +
> +/*
> + * A child to which the parent forwards all reads and writes. It
Is "_all_ reads and writes" really required? Imagine a caching block
driver, should it not be considered a filter because it may just
complete the requests from its cache rather than asking the child?
> + * must present exactly the same visible data as the parent.
> + * Any node may have at most one filtered child at a time.
> + */
> +BDRV_CHILD_FILTERED = (1 << 2),
> +
> +/*
> + * Child from which to read all data that isn’t allocated in the
> + * parent (i.e., the backing child); such data is copied to the
> + * parent through COW (and optionally COR).
> + */
> +BDRV_CHILD_COW = (1 << 3),
> +
> +/*
> + * The primary child. For most drivers, this is the child whose
> + * filename applies best to the parent node.
> + * Each parent must give this flag to no more than one child at a
> + * time.
> + */
> +BDRV_CHILD_PRIMARY = (1 << 4),
And I assume some drivers like quorum don't set it on any child.
> +/* Useful combination of flags */
> +BDRV_CHILD_IMAGE= BDRV_CHILD_DATA
> + | BDRV_CHILD_METADATA
> + | BDRV_CHILD_PRIMARY,
> +};
> +
> +/* Mask of BdrvChildRoleBits values */
> +typedef unsigned int BdrvChildRole;
> +
> char *bdrv_perm_names(uint64_t perm);
> uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
The list intuitively makes sense to me. Let me try to think of some
interesting cases to see whether the documentation is complete or
whether it could be improved.
qcow2 is what everyone has in mind, so it should be obvious:
* Without a data file:
* file: BDRV_CHILD_IMAGE
* backing: BDRV_CHILD_COW
* With a data file:
* file: BDRV_CHILD_PRIMARY | BDRV_CHILD_METADATA
* data-file: BDRV_CHILD_DATA
* backing: BDRV_CHILD_COW
We can use VMDK to make things a bit more interesting:
* file: BDRV_CHILD_PRIMARY | BDRV_CHILD_METADATA
* extents.*: BDRV_CHILD_METADATA | BDRV_CHILD_DATA
* backing: BDRV_CHILD_COW
In other words, we can have multiple data and metadata children. Is this
correct or should extents not be marked as metadata? (Checked the final
code: yes we do have multiple of them in vmdk.) Should this be mentioned
in the documentation?
Do we then also want to allow multiple BDRV_CHILD_COW children? We don't
currently have a driver that needs it, but maybe it would be consistent
with DATA and METADATA then. However, it would contradict the
documentation that it's the "Child from which to read all data".
blkverify:
* x-image: BDRV_CHILD_PRIMARY | BDRV_CHILD_DATA | BDRV_CHILD_FILTERED
* x-raw: BDRV_CHILD_DATA | BDRV_CHILD_FILTERED
Hm, according to the documentation, this doesn't work, FILTERED can be
set only for one node. But the condition ("the parent forwards all reads
and writes") applies to both children. I think the documentation should
mention what needs to be done in such cases. For blkverify, both
children are not equal in intention, so I guess the "real" filtered
child is x-image. But for quorum, you can't make any such distinction. I
assume the recommendation should be not to set FILTERED for any child
then.
Looking at the final code... Hm, your choice looks quite different: You
don't have DATA for x-raw, but you make it the PRIMARY and FILTERED
child. I think PRIMARY/FILTERED is just a bug (e.g. getlength and flush
being forwarded only to x-image show that it's primary). I do wonder
whether I have a different interpretation of DATA than you, though.
Also, the comparison makes me wonder whether FILTERED always implies
PRIMARY? Would there ever be a scenario where a child is FILTERED, but
not PRIMARY?
So I'm not completely sure if I understand the roles correctly, but I
guess my understanding is good enough to continue with the rest of the
series.
Kevin
Re: [PATCH v25 09/10] target-arm: kvm64: handle SIGBUS signal from kernel or KVM
On Fri, 10 Apr 2020 19:46:38 +0800
Dongjiu Geng wrote:
> Add a SIGBUS signal handler. In this handler, it checks the SIGBUS type,
> translates the host VA delivered by host to guest PA, then fills this PA
> to guest APEI GHES memory, then notifies guest according to the SIGBUS
> type.
>
> When guest accesses the poisoned memory, it will generate a Synchronous
> External Abort(SEA). Then host kernel gets an APEI notification and calls
> memory_failure() to unmapped the affected page in stage 2, finally
> returns to guest.
>
> Guest continues to access the PG_hwpoison page, it will trap to KVM as
> stage2 fault, then a SIGBUS_MCEERR_AR synchronous signal is delivered to
> Qemu, Qemu records this error address into guest APEI GHES memory and
> notifes guest using Synchronous-External-Abort(SEA).
>
> In order to inject a vSEA, we introduce the kvm_inject_arm_sea() function
> in which we can setup the type of exception and the syndrome information.
> When switching to guest, the target vcpu will jump to the synchronous
> external abort vector table entry.
>
> The ESR_ELx.DFSC is set to synchronous external abort(0x10), and the
> ESR_ELx.FnV is set to not valid(0x1), which will tell guest that FAR is
> not valid and hold an UNKNOWN value. These values will be set to KVM
> register structures through KVM_SET_ONE_REG IOCTL.
>
> Signed-off-by: Dongjiu Geng
> Signed-off-by: Xiang Zheng
> Reviewed-by: Michael S. Tsirkin
> Acked-by: Xiang Zheng
> Reviewed-by: Peter Maydell
Reviewed-by: Igor Mammedov
> ---
> include/sysemu/kvm.h| 3 +-
> target/arm/cpu.h| 4 +++
> target/arm/helper.c | 2 +-
> target/arm/internals.h | 5 ++--
> target/arm/kvm64.c | 77
> +
> target/arm/tlb_helper.c | 2 +-
> target/i386/cpu.h | 2 ++
> 7 files changed, 89 insertions(+), 6 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 141342d..3b22504 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -379,8 +379,7 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
> /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>
> -#ifdef TARGET_I386
> -#define KVM_HAVE_MCE_INJECTION 1
> +#ifdef KVM_HAVE_MCE_INJECTION
> void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> #endif
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8b9f296..6a9838d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -28,6 +28,10 @@
> /* ARM processors have a weak memory model */
> #define TCG_GUEST_DEFAULT_MO (0)
>
> +#ifdef TARGET_AARCH64
> +#define KVM_HAVE_MCE_INJECTION 1
> +#endif
> +
> #define EXCP_UDEF1 /* undefined instruction */
> #define EXCP_SWI 2 /* software interrupt */
> #define EXCP_PREFETCH_ABORT 3
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 163c91a..b2c30f2 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3517,7 +3517,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t
> value,
> * Report exception with ESR indicating a fault due to a
> * translation table walk for a cache maintenance instruction.
> */
> -syn = syn_data_abort_no_iss(current_el == target_el,
> +syn = syn_data_abort_no_iss(current_el == target_el, 0,
> fi.ea, 1, fi.s1ptw, 1, fsc);
> env->exception.vaddress = value;
> env->exception.fsr = fsr;
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index e633aff..37c22a9 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -451,13 +451,14 @@ static inline uint32_t syn_insn_abort(int same_el, int
> ea, int s1ptw, int fsc)
> | ARM_EL_IL | (ea << 9) | (s1ptw << 7) | fsc;
> }
>
> -static inline uint32_t syn_data_abort_no_iss(int same_el,
> +static inline uint32_t syn_data_abort_no_iss(int same_el, int fnv,
> int ea, int cm, int s1ptw,
> int wnr, int fsc)
> {
> return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> | ARM_EL_IL
> - | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
> + | (fnv << 10) | (ea << 9) | (cm << 8) | (s1ptw << 7)
> + | (wnr << 6) | fsc;
> }
>
> static inline uint32_t syn_data_abort_with_iss(int same_el,
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index be5b31c..d53f7f2 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -28,6 +28,9 @@
> #include "sysemu/kvm_int.h"
> #include "kvm_arm.h"
> #include "internals.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/ghes.h"
> +#include "hw/arm/virt.h"
>
> static bool have_guest_debug;
>
> @@ -893,6 +896,30 @@ int kvm_arm_cpreg_level(uint64_t regidx)
> return KVM_PUT_RUNTIME_ST
[PATCH v4 00/13] acpi: i386 tweaks
First batch of microvm patches, some generic acpi stuff. Split the acpi-build.c monster, specifically split the pc and q35 and pci bits into a separate file which we can skip building at some point in the future. v2 changes: leave acpi-build.c largely as-is, move useful bits to other places to allow them being reused, specifically: * move isa device generator functions to individual isa devices. * move fw_cfg generator function to fw_cfg.c v3 changes: fix rtc, support multiple lpt devices. v4 changes: * drop merged patches. * split rtc crs change to separata patch. * added two cleanup patches. * picked up ack & review tags. take care, Gerd Gerd Hoffmann (13): qtest: allow DSDT acpi table changes acpi: move aml builder code for rtc device acpi: rtc: use a single crs range acpi: serial: don't use _STA method acpi: move aml builder code for serial device acpi: parallel: don't use _STA method acpi: move aml builder code for parallel device acpi: move aml builder code for floppy device acpi: move aml builder code for i8042 (kbd+mouse) device acpi: factor out fw_cfg_add_acpi_dsdt() acpi: simplify build_isa_devices_aml() acpi: drop serial/parallel enable bits from dsdt floppy: make isa_fdc_get_drive_max_chs static hw/i386/fw_cfg.h| 1 + include/hw/block/fdc.h | 2 - tests/qtest/bios-tables-test-allowed-diff.h | 17 ++ hw/block/fdc.c | 87 +- hw/char/parallel.c | 32 +++ hw/char/serial-isa.c| 32 +++ hw/i386/acpi-build.c| 285 +--- hw/i386/fw_cfg.c| 28 ++ hw/input/pckbd.c| 31 +++ hw/rtc/mc146818rtc.c| 20 ++ stubs/cmos.c| 7 + stubs/Makefile.objs | 1 + 12 files changed, 260 insertions(+), 283 deletions(-) create mode 100644 stubs/cmos.c -- 2.18.4
[PATCH v4 02/13] acpi: move aml builder code for rtc device
Signed-off-by: Gerd Hoffmann
---
hw/i386/acpi-build.c | 17 -
hw/rtc/mc146818rtc.c | 22 ++
2 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2e15f6848e7e..0bfa2dd23fcc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1137,22 +1137,6 @@ static Aml *build_fdc_device_aml(ISADevice *fdc)
return dev;
}
-static Aml *build_rtc_device_aml(void)
-{
-Aml *dev;
-Aml *crs;
-
-dev = aml_device("RTC");
-aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
-crs = aml_resource_template();
-aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
-aml_append(crs, aml_irq_no_flags(8));
-aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-return dev;
-}
-
static Aml *build_kbd_device_aml(void)
{
Aml *dev;
@@ -1278,7 +1262,6 @@ static void build_isa_devices_aml(Aml *table)
Aml *scope = aml_scope("_SB.PCI0.ISA");
Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, &ambiguous);
-aml_append(scope, build_rtc_device_aml());
aml_append(scope, build_kbd_device_aml());
aml_append(scope, build_mouse_device_aml());
if (fdc) {
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index d18c09911be2..2104e0aa3b14 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -27,6 +27,7 @@
#include "qemu/cutils.h"
#include "qemu/module.h"
#include "qemu/bcd.h"
+#include "hw/acpi/aml-build.h"
#include "hw/irq.h"
#include "hw/qdev-properties.h"
#include "qemu/timer.h"
@@ -1007,13 +1008,34 @@ static void rtc_resetdev(DeviceState *d)
}
}
+static void rtc_build_aml(ISADevice *isadev, Aml *scope)
+{
+Aml *dev;
+Aml *crs;
+
+crs = aml_resource_template();
+aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
+ 0x10, 0x02));
+aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
+aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE + 2,
+ 0x02, 0x06));
+
+dev = aml_device("RTC");
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+aml_append(scope, dev);
+}
+
static void rtc_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
dc->realize = rtc_realizefn;
dc->reset = rtc_resetdev;
dc->vmsd = &vmstate_rtc;
+isa->build_aml = rtc_build_aml;
device_class_set_props(dc, mc146818rtc_properties);
}
--
2.18.4
[PATCH v4 11/13] acpi: simplify build_isa_devices_aml()
x86 machines can have a single ISA bus only.
Signed-off-by: Gerd Hoffmann
---
hw/i386/acpi-build.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index cb3913d2ee76..1922868f3401 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1061,19 +1061,14 @@ static void build_hpet_aml(Aml *table)
static void build_isa_devices_aml(Aml *table)
{
bool ambiguous;
-
-Aml *scope = aml_scope("_SB.PCI0.ISA");
Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, &ambiguous);
+Aml *scope;
-if (ambiguous) {
-error_report("Multiple ISA busses, unable to define IPMI ACPI data");
-} else if (!obj) {
-error_report("No ISA bus, unable to define IPMI ACPI data");
-} else {
-build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
-isa_build_aml(ISA_BUS(obj), scope);
-}
+assert(obj && !ambiguous);
+scope = aml_scope("_SB.PCI0.ISA");
+build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
+isa_build_aml(ISA_BUS(obj), scope);
aml_append(table, scope);
}
--
2.18.4
