Re: [Qemu-devel] [PATCH] ui/gtk.c: Fix *BSD build of Gtk+ UI
On 05/21/13 18:14, Brad Smith wrote: Fix the build of the Gtk+ UI on *BSD systems. Signed-off-by: Brad Smith b...@comstyle.com diff --git a/include/qemu-common.h b/include/qemu-common.h index af769f5..c944bb7 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -45,6 +45,7 @@ #if defined(__GLIBC__) # include pty.h #elif defined CONFIG_BSD +# include termios.h # if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__) # include libutil.h # else Can't see how it could hurt. Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On 05/22/13 00:32, Michael Roth wrote:
When this VMSD was introduced it's version fields were set to
sizeof(I6300State), making them essentially random from build to build,
version to version.
To fix this, we lock in a high version id and low minimum version id to
support old-new migration from all prior versions of this device's
state. This should work since the device state has not changed since
its introduction.
The potentially breaks migration from 1.5+ to 1.5, but since the
versioning was essentially random prior to this patch, new-old
migration was not consistently functional to begin with.
Reported-by: Nicholas Thomas n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
hw/watchdog/wdt_i6300esb.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..851b664 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
-.version_id = sizeof(I6300State),
-.minimum_version_id = sizeof(I6300State),
-.minimum_version_id_old = sizeof(I6300State),
+/* With this VMSD's introduction, version_id/minimum_version_id were
+ * erroneously set to sizeof(I6300State), causing a somewhat random
+ * version_id to be set for every build. This eventually broke
+ * migration.
+ *
+ * To correct this without breaking old-new migration for older versions
+ * of QEMU, we've set version_id to a value high enough to exceed all
past
+ * values of sizeof(I6300State) across various build environments, and
have
+ * reset minimum_version_id_old/minimum_version_id to 1, since this VMSD
+ * has never changed and thus can except all past versions.
As a non-native speaker I think you mean accept.
+ *
+ * For future changes we can treat these values as we normally would.
+ */
+.version_id = 1,
+.minimum_version_id = 1,
+.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
Otherwise looks good to me (which may not mean much :))
Laszlo
Re: [Qemu-devel] [PATCH v2] wdt_i6300esb: fix vmstate versioning
On 05/22/13 18:32, Michael Roth wrote: When this VMSD was introduced it's version fields were set to sizeof(I6300State), making them essentially random from build to build, version to version. To fix this, we lock in a high version id and low minimum version id to support old-new migration from all prior versions of this device's state. This should work since the device state has not changed since its introduction. The potentially breaks migration from 1.5+ to 1.5, but since the versioning was essentially random prior to this patch, new-old migration was not consistently functional to begin with. Reported-by: Nicholas Thomas n...@bytemark.co.uk Suggested-by: Peter Maydell peter.mayd...@linaro.org Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com --- v2: * Fixed s/except/accept/ typo (Laszlo) hw/watchdog/wdt_i6300esb.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) Always alert to make a difference :), Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [RFC PATCH v3 00/11] qemu-ga: fsfreeze on Windows using VSS
Sekiyama-san, On 05/21/13 17:33, Tomoki Sekiyama wrote: * About errors in Windows 7 with patch v2 VSS requires to write to snapshot volumes just before making them read-only at final commit phase. This feature is called `auto-recovery' (See http://msdn.microsoft.com/en-us/library/windows/desktop/aa384651(v=vs.85).aspx#base.vssgloss_auto_recoved_shadow_copy for details). Since qemu and libvirt don't have feature to handle writable snapshots, this patchset just disables auto-recovery by specifying VSS_VOLSNAP_ATTR_NO_AUTORECOVERY flag to SetContext. Unfortunately, this flag seems unsupported in Windows 7 or earlier. It tries to look up the snapshot volumes to write and fails in VSS_E_OBJECT_NOT_FOUND. For fundamental resolution we need a framework for guests to query snapshots and to mount them as writable snapshots, but we just ignore the error in this patchset. I've been trying to understand VSS as documented in http://technet.microsoft.com/en-us/library/ee923636%28v=ws.10%29.aspx I'll be referring to the steps detailed there as VSS steps. Currently I'm under the impression that the guest-fsfreeze-freeze and guest-fsfreeze-thaw commands do not match VSS from a who has control when perspective. VSS steps 1 to 5 correspond to guest-fsfreeze-freeze. In the POSIX flavor of qemu-ga, including fsfreeze hook scripts, at this point qemu-ga gets back and returns control to the management stack, and backup can be done. VSS step 6 corresponds to the management stack preparing the snapshot(s) or other means of backup. VSS steps 7 to 8 correspond to guest-fsfreeze-thaw. The POSIX flavor of qemu-ga thaws filesystems and reactivates applications / enables them to dirty their own data stores. This action is initiated by the management stack which has full control over its timing and conditions etc. I believe to discover the following (orthogonal) mismatches between VSS and our current freeze/thaw APIs: (i) Once VSS starts running at step 1, it doesn't relinquish control after step 5, and it doesn't allow an external controller to relaunch it from step 7. In other words, VSS step 6 is a callout / callback initiated by Windows logic that doesn't match our current control ownership. In theory VSS step 6 should issue a QMP event (?) that allows libvirt to make the snapshot, and until libvirt is done with that, VSS step 6 should block. (ii) Somewhat similar is the control over backup storage: An fsfreeze hook script causes the corresponding application to clean up its live data store. The application doesn't encounter the notion of a separate / detached copy, and no additional storage is visible (needs to be visible) in the guest. The management stack exploits this guarantee when it (basically blindly) snapshots the disk image. Under VSS, in-guest components (VSS providers, basically drivers) are responsible for incremental backup (copy-on-write etc.) features. I'm not sure how this can be combined with qcow2 features managed by libvirt. The VSS provider could target a separate disk, update it incrementally to mirror the current live data, and libvirt could keep snapshots of this backup disk. (iii) VSS step 10 exists because in fact I cheated in (i) and VSS steps 1 to 5 do not correspond to guest-fsfreeze-freeze; the VSS design has aspects that are completely absent from guest-fsfreeze-freeze. guest-fsfreeze-freeze, including the hook scripts, gives as much time to applications to quiesce as they need. When the main hook script returns control to qemu-qa, and when qemu-ga returns control to libvirt, the layer taking back control is assured that the lower layer has completed all of its freezing responsibilities. This is not so with VSS steps 1-5, and the root cause is performance / avoidance of live service interruption. VSS puts a 60 second limit on application data store quiescing, and a 10 second limit on actual snapshotting / backup (VSS step 6). Live service should resume in no more than 70 seconds apparently. VSS realizes that many applications won't be able to satisfy this requirement. Therefore the application level quiescing is split in a synchronous handler and an asynchronous bottom half (which is VSS step 10). - Up to and including VSS step 5, an application prepares but a separate dirty context (in 60 seconds). - In VSS step 6, this context (shadow copy) is detached (move to other storage, probably). - In VSS steps 7 and 8, the application resumes live service. Also, - VSS step 10, the bottom half of the application (a subprocess or a dedicated thread, maybe), cleans up the now detached dirty context, *in parallel* to the live service of the application. If we disable VSS step 10 (VSS_VOLSNAP_ATTR_NO_AUTORECOVERY), I'm still confused / unsure about: - the control ownership problem in (i) and the storage problem in (ii) -- I believe your series must already solve this; can you please describe it in a few words? - the time limits for VSS steps 5
Re: [Qemu-devel] [RFC PATCH v3 02/11] Fix errors and warnings while compiling with c++ compilier
On 05/21/13 17:33, Tomoki Sekiyama wrote:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..b174acb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -156,9 +156,16 @@ def c_var(name, protect=True):
# GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
# excluding _.*
gcc_words = set(['asm', 'typeof'])
+# C++ ISO/IEC 14882:2003 2.11
+cpp_words = set(['bool', 'catch', 'class', 'const_cast', 'delete',
+ 'dynamic_cast', 'explicit', 'false', 'friend',
'mutable',
+ 'namespace', 'new', 'operator', 'private', 'protected',
+ 'public', 'reinterpret_cast', 'static_cast', 'template',
+ 'this', 'throw', 'true', 'try', 'typeid', 'typename',
+ 'using', 'virtual', 'wchar_t'])
# namespace pollution:
polluted_words = set(['unix'])
-if protect and (name in c89_words | c99_words | c11_words | gcc_words |
polluted_words):
+if protect and (name in c89_words | c99_words | c11_words | gcc_words |
cpp_words | polluted_words):
return q_ + name
return name.replace('-', '_').lstrip(*)
Since you're respinning anyway, perhaps consider adding these lovely
alternative representations from just one paragraph below (they are
reserved and shall not be used otherwise than the operators they stand
for):
and bitand compl not_eq or_eq xor_eq
and_eq bitor not or xor
although probably noone would use these as identifiers or otherwise...
So just mentioning it for completeness.
Laszlo
Re: [Qemu-devel] [RFC PATCH v3 03/11] Add a script to extract VSS SDK headers on POSIX system
On 05/21/13 23:02, Tomoki Sekiyama wrote: Maybe it also should have an additional check and a message to install Msitools for the case msiextract isn't exectable. Right. One POSIX-y way would be command -v msiextract /dev/null and checking the exit status. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html Alternatively, just go ahead calling it, and if the exit status is 126 or 127, assume it is not (correctly) installed and emit an extra hint afterwards: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01 Laszlo
Re: [Qemu-devel] [RFC PATCH v3 03/11] Add a script to extract VSS SDK headers on POSIX system
On 05/21/13 17:33, Tomoki Sekiyama wrote: VSS SDK(*) setup.exe is only runnable on Windows. This adds a script to extract VSS SDK headers on POSIX-systems using msitools. * http://www.microsoft.com/en-us/download/details.aspx?id=23490 From: Paolo Bonzini pbonz...@redhat.com Signed-off-by: Tomoki Sekiyama tomoki.sekiy...@hds.com --- scripts/extract-vsssdk-headers | 25 + 1 file changed, 25 insertions(+) create mode 100755 scripts/extract-vsssdk-headers diff --git a/scripts/extract-vsssdk-headers b/scripts/extract-vsssdk-headers new file mode 100755 index 000..5877137 --- /dev/null +++ b/scripts/extract-vsssdk-headers @@ -0,0 +1,25 @@ +#! /bin/bash + +# extract-vsssdk-headers +# Author: Paolo Bonzini pbonz...@redhat.com + +set -e +if test $# = 0 || ! test -f $1; then + echo 'Usage: extract-vsssdk-headers /path/to/setup.exe' + exit 1 +fi Error message to 2 ? (Would not be worth a repost but you're already doing one...) + +# Extract .MSI file in the .exe, looking for the OLE compound +# document signature. Extra data at the end does not matter. +export LC_ALL=C +MAGIC=$'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1' Can't help mentioning the following portable (alas, octal) equivalent :) MAGIC=$(printf '%b' '\0320\0317\0021\0340\0241\0261\0032\0341') http://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html#tag_20_94 +offset=`grep -abom1 $MAGIC $1 | sed -n 's/:/\n/; P' ` Of these grep options, none are portable, hence I'm supposing dependency on GNU grep. In that case, see (*). +(dd of=/dev/null skip=$offset bs=1 count=0; cat) $1 vsssdk.msi In place of dd for seeking + cat, suggest tail -c +$((offset+1)) -- $1 http://pubs.opengroup.org/onlinepubs/9699919799/utilities/tail.html + +# Now extract the files. +tmpdir=tmp$$ +mkdir $tmpdir +msiextract -C $tmpdir vsssdk.msi +mv $tmpdir/Program Files/Microsoft/VSSSDK72/inc inc +rm -rf $tmpdir vsssdk.msi +exit 0 (*) Since we rely on GNU utilities anyway, I propose the mktemp utility (Written by Jim Meyering and Eric Blake, heh) from GNU coreutils: tmpdir=$(mktemp -d) Feel free to ignore any of the above, of course :) Thanks, Laszlo
Re: [Qemu-devel] [RFC PATCH v3 04/11] qemu-ga: Add an configure option to specify path to Windows VSS SDK
On 05/21/13 18:53, Eric Blake wrote: On 05/21/2013 09:33 AM, Tomoki Sekiyama wrote: To enable VSS support in qemu-ga for Windows, header files included in VSS SDK is required. The VSS support is enabled when the option like below: ./configure --with-vss-sdk=/pass/to/VSS SDK VSS SDK is available from: http://www.microsoft.com/en-us/download/details.aspx?id=23490 To cross-compilie using mingw32 for Linux, you need to setup the SDK on s/compilie/compile/ There are two mingw projects: mingw (older, 32-bit only) and mingw64 (newer, both 32- and 64-bit support). The name 'mingw32' is misleading, as neither project uses it. Fedora uses mingw64. RHEL-6 has packages named mingw32-* (from rhel-6-workstation-optional-rpms -- on my laptop anyway). I believe saying just mingw should be fine, as proposed. Laszlo
Re: [Qemu-devel] [RFC PATCH v3 04/11] qemu-ga: Add an configure option to specify path to Windows VSS SDK
On 05/21/13 17:33, Tomoki Sekiyama wrote: +if test $vss_win32_sdk != ; then + echo ERROR: Please download and install Microsoft VSS SDK: + echo ERROR: http://www.microsoft.com/en-us/download/details.aspx?id=23490; + echo ERROR: On POSIX-systems, you can extract the SDK headers by: + echo ERROR: scripts/extract-vsssdk-headers setup.exe + echo ERROR: The headers are extracted in the directory \`inc'. + feature_not_found VSS support +fi Please consider adding 2 after the fi. (... although even feature_not_found() -- error_exit() misses that redirection (and the behavior would change with my suggestion), so maybe it's more consistent the way you submitted it.) Laszlo
Re: [Qemu-devel] [RFC PATCH v3 03/11] Add a script to extract VSS SDK headers on POSIX system
On 05/24/13 17:59, Eric Blake wrote: On 05/24/2013 07:38 AM, Laszlo Ersek wrote: +++ b/scripts/extract-vsssdk-headers @@ -0,0 +1,25 @@ +#! /bin/bash + +MAGIC=$'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1' Can't help mentioning the following portable (alas, octal) equivalent :) MAGIC=$(printf '%b' '\0320\0317\0021\0340\0241\0261\0032\0341') Yeah, but as long as the she-bang is (correctly) requiring bash, Yes, that's why I edited my original please consider using to can't help mentioning :) I recalled that you had mentioned dash in one of your reviews (*), I checked your reply to this v3 03/11 and saw that it wasn't it -- here you'd written Since you are using bash so I edited the above paragraph. Clearly insufficiently :) (*) ... Apparently you mentioned dash in a MALLOC_PERTURB_ thread. L.
Re: [Qemu-devel] tcg: Windows guests don't boot
On 05/24/13 21:05, Luiz Capitulino wrote: Hi, Today I accidentally started qemu w/o -enable-kvm to run a Windows guest and noticed it didn't boot: sometimes it hangs on a blue screen and sometimes it keeps rebooting in a loop. I tried with Windows 2008 and Windows 8, and went back to qemu v1.2.0 to see if it's a bisectable regression, but no luck. I'm reporting case someone is interested in debugging this. Yes. I think https://bugs.launchpad.net/qemu/+bug/1180970 is related. See also these threads: [Qemu-devel] [Bug 1180970] [NEW] qemu: fatal: Trying to execute code outside RAM or ROM; worked in 1.4.0, fails in 1.4.92 [Qemu-devel] [Bug 1180970] *** affects all x86_64 soft emulation I'm going to respond with my initial analysys (lol) in the latter. Thanks, Laszlo
Re: [Qemu-devel] [Bug 1180970] *** affects all x86_64 soft emulation
On 05/24/13 19:25, Duane Voth wrote:
qemu: fatal: Trying to execute code outside RAM or ROM; worked in
1.4.0, fails in 1.4.92
Want to bring a little attention to this bug - the break is in
target-i386/translate.c which affects all x86_64 soft emulation in a
fairly subtle way (ie. users will report a wide variety of problems
none of which seem to be related). I can't find a way to elevate bug
importance in launchpad.
4a6fd938f5457ee161d2acbd9364608a2a68b7a1 is the offending commit.
There have been numerous changes after this commit over top of the
change that broke emulation, so backing out this commit is not
trivial.
I can reproduce the problem that is the subject of bug 1180970 for
testing easily.
I can also reproduce this bug with my OVMF build, when KVM is disabled
(current master).
x86_64-softmmu/qemu-system-x86_64 -S -monitor stdio -m 1024 \
-vga cirrus -debugcon file:debug.log \
-global isa-debugcon.iobase=0x402 \
-bios /home/lacos/src/upstream/edk2-git-svn/out/OVMF.fd
Again, this is how qemu aborts:
(qemu) qemu: fatal: Trying to execute code outside RAM or ROM at
0x0001
RAX=3e084da8 RBX=3e084868 RCX=
RDX=3e084f00
RSI=0001 RDI=3e085000 RBP=3e084708
RSP=3fac8510
R8 = R9 =3e14c3e3 R10=0033
R11=00d3
R12=3e0848a0 R13= R14=
R15=
RIP=ffe4 RFL=0046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0008 00cf9300 DPL=0 DS [-WA]
CS =0028 00af9b00 DPL=0 CS64 [-RA]
SS =0008 00cf9300 DPL=0 DS [-WA]
DS =0008 00cf9300 DPL=0 DS [-WA]
FS =0008 00cf9300 DPL=0 DS [-WA]
GS =0008 00cf9300 DPL=0 DS [-WA]
LDT= 8200 DPL=0 LDT
TR = 8b00 DPL=0 TSS64-busy
GDT= 3fa50e98 003f
IDT= 3f9d6e20 0fff
CR0=8033 CR2= CR3=3fa67000 CR4=0668
[...]
Repeating from last time, we found it interesting that
RIP=ffe4 but the problem address is 0x0001.
I made some lame attempts to find out what code is running there, and --
since I've read the term nop slide recently --, I'll call it just that:
---[ debug patch]---
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..0e0356f 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7197,6 +7197,7 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
/* misc */
case 0x90: /* nop */
/* XXX: correct lock test for all insn */
+fprintf(stderr, nop @ %016lx\n, pc_start);
if (prefixes PREFIX_LOCK) {
goto illegal_op;
}
---[ debug patch]---
The output it produces leading up to the abort quoted above is:
nop @ ffe4
nop @ ffe5
nop @ ffe6
nop @ ffe7
nop @ fff5
nop @ fff6
nop @ fff7
nop @ fff8
nop @ fff9
nop @ fffa
nop @ fffb
nop @ fffc
nop @ fffd
nop @ fffe
nop @
qemu: fatal: Trying to execute code outside RAM or ROM at 0x0001
Hence nop slide.
Peeking into the coredump triggered by abort(), the backtrace is as follows:
#0 0x7fd53c02b8a5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x7fd53c02d085 in abort () at abort.c:92
#2 0x7fd5428d0333 in cpu_abort (env=0x7fd544b89c10, fmt=
0x7fd542a47440 Trying to execute code outside RAM or ROM at 0x%016lx\n)
at /home/lacos/src/upstream/qemu/exec.c:542
#3 0x7fd5428c9aa4 in get_page_addr_code (env1=0x7fd544b89c10,
addr=4294967296)
at /home/lacos/src/upstream/qemu/cputlb.c:338
#4 0x7fd5429de266 in tb_gen_code (env=0x7fd544b89c10, pc=4294967268,
cs_base=0, flags=4244148, cflags=0)
at /home/lacos/src/upstream/qemu/translate-all.c:966
#5 0x7fd5428c431b in tb_find_slow (env=0x7fd544b89c10, pc=4294967268,
cs_base=0, flags=4244148)
at /home/lacos/src/upstream/qemu/cpu-exec.c:139
#6 0x7fd5428c44c4 in tb_find_fast (env=0x7fd544b89c10) at
/home/lacos/src/upstream/qemu/cpu-exec.c:166
#7 0x7fd5428c4c78 in cpu_x86_exec (env=0x7fd544b89c10) at
/home/lacos/src/upstream/qemu/cpu-exec.c:593
#8 0x7fd5428c8058 in tcg_cpu_exec (env=0x7fd544b89c10) at
/home/lacos/src/upstream/qemu/cpus.c:1144
#9 0x7fd5428c81a3 in tcg_exec_all () at
/home/lacos/src/upstream/qemu/cpus.c:1177
#10 0x7fd5428c7321 in qemu_tcg_cpu_thread_fn (arg=0x7fd544b89b00)
[Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 CODE64()
The code reorganization in commit 4a6fd938 broke handling of PREFIX_ADR.
Restore the previous behavior:
If TARGET_X86_64 *and* CODE64():
(a) PREFIX_ADR set: no effect, aflag should stay at the original
s-code32 value,
(b) PREFIX_ADR clear: aflag should be set to constant 2.
Otherwise:
(c) PREFIX_ADR set: the least significant bit in aflag (originally
initialized form s-code32) should be negated,
(d) PREFIX_ADR clear: no effect, aflag should stay at the original
s-code32 value.
Currently branch (a) is mishandled as branch (c).
Please-review: Richard Henderson r...@twiddle.net
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
target-i386/translate.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..86f2678 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
/* 0x66 is ignored if rex.w is set */
dflag = 2;
}
-if (!(prefixes PREFIX_ADR)) {
+if (prefixes PREFIX_ADR) {
+/* flip it back, 0x67 should have no effect */
+aflag ^= 1;
+}
+else {
aflag = 2;
}
}
--
1.7.1
[Qemu-devel] [Bug 1180970] Re: qemu: fatal: Trying to execute code outside RAM or ROM; worked in 1.4.0, fails in 1.4.92
** Changed in: qemu Status: New = In Progress -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1180970 Title: qemu: fatal: Trying to execute code outside RAM or ROM; worked in 1.4.0, fails in 1.4.92 Status in QEMU: In Progress Bug description: I'm using qemu to run and debug the EDK2 uEFI environment. OVMF is being built out of the EDK2 tree I've checked out (r14367). (Reproducing all this could be tedious so I am available for debugging/testing.) qemu 1.4.0 was able to execute this guest environment with no trouble, qemu 1.4.92 however issues an error message and aborts. The command line I use to start qemu is: $ /usr/local/bin/qemu-system-x86_64 -m 1024 -bios OVMF.fd -monitor stdio 1.4.92 gives the following register dump: QEMU 1.4.92 monitor - type 'help' for more information (qemu) qemu: fatal: Trying to execute code outside RAM or ROM at 0x0001 RAX=3e084da8 RBX=3e084868 RCX= RDX=3e084f00 RSI=0001 RDI=3e085000 RBP=3e084708 RSP=3fac8510 R8 = R9 =3e14c3e3 R10=0033 R11=00d3 R12=3e0848a0 R13= R14= R15= RIP=ffe4 RFL=0046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES =0008 00cf9300 DPL=0 DS [-WA] CS =0028 00af9b00 DPL=0 CS64 [-RA] SS =0008 00cf9300 DPL=0 DS [-WA] DS =0008 00cf9300 DPL=0 DS [-WA] FS =0008 00cf9300 DPL=0 DS [-WA] GS =0008 00cf9300 DPL=0 DS [-WA] LDT= 8200 DPL=0 LDT TR = 8b00 DPL=0 TSS64-busy GDT= 3fa50e98 003f IDT= 3f9d6e20 0fff CR0=8033 CR2= CR3=3fa67000 CR4=0668 ... Questions: 1) Is this problem relevant? (is full backward compatability to be supported?) 2) Are there new guest execution controls in 1.4.9x that might cause this? 3) If #2, can they be disabled by a qemu command line switch? 4) If not #2, in what qemu source file specifically can I find the logic causing the abort? (help me help you :) 5) If guest memory is corrupted or improperly mapped, how can I keep qemu alive to examime/dump guest memory? To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1180970/+subscriptions
[Qemu-devel] [Bug 1180970] Re: qemu: fatal: Trying to execute code outside RAM or ROM; worked in 1.4.0, fails in 1.4.92
Proposed patch: http://thread.gmane.org/gmane.comp.emulators.qemu/213023 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1180970 Title: qemu: fatal: Trying to execute code outside RAM or ROM; worked in 1.4.0, fails in 1.4.92 Status in QEMU: In Progress Bug description: I'm using qemu to run and debug the EDK2 uEFI environment. OVMF is being built out of the EDK2 tree I've checked out (r14367). (Reproducing all this could be tedious so I am available for debugging/testing.) qemu 1.4.0 was able to execute this guest environment with no trouble, qemu 1.4.92 however issues an error message and aborts. The command line I use to start qemu is: $ /usr/local/bin/qemu-system-x86_64 -m 1024 -bios OVMF.fd -monitor stdio 1.4.92 gives the following register dump: QEMU 1.4.92 monitor - type 'help' for more information (qemu) qemu: fatal: Trying to execute code outside RAM or ROM at 0x0001 RAX=3e084da8 RBX=3e084868 RCX= RDX=3e084f00 RSI=0001 RDI=3e085000 RBP=3e084708 RSP=3fac8510 R8 = R9 =3e14c3e3 R10=0033 R11=00d3 R12=3e0848a0 R13= R14= R15= RIP=ffe4 RFL=0046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES =0008 00cf9300 DPL=0 DS [-WA] CS =0028 00af9b00 DPL=0 CS64 [-RA] SS =0008 00cf9300 DPL=0 DS [-WA] DS =0008 00cf9300 DPL=0 DS [-WA] FS =0008 00cf9300 DPL=0 DS [-WA] GS =0008 00cf9300 DPL=0 DS [-WA] LDT= 8200 DPL=0 LDT TR = 8b00 DPL=0 TSS64-busy GDT= 3fa50e98 003f IDT= 3f9d6e20 0fff CR0=8033 CR2= CR3=3fa67000 CR4=0668 ... Questions: 1) Is this problem relevant? (is full backward compatability to be supported?) 2) Are there new guest execution controls in 1.4.9x that might cause this? 3) If #2, can they be disabled by a qemu command line switch? 4) If not #2, in what qemu source file specifically can I find the logic causing the abort? (help me help you :) 5) If guest memory is corrupted or improperly mapped, how can I keep qemu alive to examime/dump guest memory? To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1180970/+subscriptions
Re: [Qemu-devel] [PATCH] Remove OSS support for OpenBSD
On 05/08/13 13:39, Brad Smith wrote:
Remove the OSS support for OpenBSD. The OSS API has not been usable
for quite some time.
Signed-off-by: Brad Smith b...@comstyle.com
diff --git a/audio/ossaudio.c b/audio/ossaudio.c
index 00be9c9..007c641 100644
--- a/audio/ossaudio.c
+++ b/audio/ossaudio.c
@@ -25,11 +25,7 @@
#include sys/mman.h
#include sys/types.h
#include sys/ioctl.h
-#ifdef __OpenBSD__
-#include soundcard.h
-#else
#include sys/soundcard.h
-#endif
#include qemu-common.h
#include qemu/main-loop.h
#include qemu/host-utils.h
diff --git a/configure b/configure
index 9439f1c..89dda5b 100755
--- a/configure
+++ b/configure
@@ -468,8 +468,7 @@ OpenBSD)
bsd=yes
make=${MAKE-gmake}
audio_drv_list=oss
- audio_possible_drivers=oss sdl esd
- oss_lib=-lossaudio
+ audio_possible_drivers=sdl esd
;;
Darwin)
bsd=yes
Shouldn't the default audio driver list ($audio_drv_list) be a subset of
the possible driver list?
Thanks,
Laszlo
Re: [Qemu-devel] [PATCH] Remove OSS support for OpenBSD
On 05/25/13 01:01, Brad Smith wrote:
Remove the OSS support for OpenBSD. The OSS API has not been usable
for quite some time.
Signed-off-by: Brad Smith b...@comstyle.com
diff --git a/audio/ossaudio.c b/audio/ossaudio.c
index 00be9c9..007c641 100644
--- a/audio/ossaudio.c
+++ b/audio/ossaudio.c
@@ -25,11 +25,7 @@
#include sys/mman.h
#include sys/types.h
#include sys/ioctl.h
-#ifdef __OpenBSD__
-#include soundcard.h
-#else
#include sys/soundcard.h
-#endif
#include qemu-common.h
#include qemu/main-loop.h
#include qemu/host-utils.h
diff --git a/configure b/configure
index 5ae7e4a..eb74510 100755
--- a/configure
+++ b/configure
@@ -468,9 +468,8 @@ NetBSD)
OpenBSD)
bsd=yes
make=${MAKE-gmake}
- audio_drv_list=oss
- audio_possible_drivers=oss sdl esd
- oss_lib=-lossaudio
+ audio_drv_list=sdl
+ audio_possible_drivers=sdl esd
;;
Darwin)
bsd=yes
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 CODE64()
On 05/26/13 10:33, Paolo Bonzini wrote:
Il 26/05/2013 01:23, Richard Henderson ha scritto:
On 2013-05-24 14:37, Laszlo Ersek wrote:
@@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State
*env, DisasContext *s,
/* 0x66 is ignored if rex.w is set */
dflag = 2;
}
-if (!(prefixes PREFIX_ADR)) {
+if (prefixes PREFIX_ADR) {
+/* flip it back, 0x67 should have no effect */
+aflag ^= 1;
+}
+else {
aflag = 2;
}
}
Agreed that there's a bug here. I'm thinking it would be clearer to not
write this as yet another flip, but understand that unlike dflag, aflag
can only be either 1 or 2 in 64-bit mode.
I'm thinking of something more like this:
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..bf772aa 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4813,9 +4813,8 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
/* 0x66 is ignored if rex.w is set */
dflag = 2;
}
-if (!(prefixes PREFIX_ADR)) {
-aflag = 2;
-}
+/* 0x67 toggles between 64-bit and 32-bit addressing. */
+aflag = (prefixes PREFIX_ADR ? 1 : 2);
Isn't that just aflag++? Needs a comment of course (toggle between
32- and 64-bit, not 16- and 32-bit.).
I finally looked up in the SDM what the 0x67 (address-size override)
prefix does. Apparently,
2.1.1 Instruction Prefixes
[...] The address-size override prefix (67H) allows programs to
switch between 16- and 32-bit addressing. Either size can be the
default; the prefix selects the non-default size. [...]
CMPS/CMPSB/CMPSW/CMPSD/CMPSQ -- Compare String Operands
[...] In 64-bit mode, the instruction's default address size is 64
bits, 32 bit address size is supported using the prefix 67H. [...]
Assuming that
- aflag==0 means 16-bit address,
- aflag==1 means 32-bit address,
- aflag==2 means 64-bit address,
- and bit0 in s-code32 carries aflag corresponding to the default
address size in 32-bit mode,
I think Richard's patch is correct (my approach was only to restore the
pre-patch logic without having any clue about the variables' contents).
I believe aflag++ is incorrect if the current default address size for
32-bit is 16-bit (ie. (s-code32 1) == 0). In this case the first XOR
(seeing the 0x67 prefix) flips it to 1, and the increment would change
it to 2. aflag==2 corresponds to 64-bit address, but in 64-bit mode with
the 0x67 prefix we must choose 32-bit.
(IOW in 32-bit mode the meaning of the 0x67 prefix is not absolute but
relative.)
Laszlo
Re: [Qemu-devel] [PATCH stable-1.1] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 05/26/13 15:34, Andreas Färber wrote:
From: Laszlo Ersek ler...@redhat.com
The qemu guest agent creates a bunch of files with insecure permissions
when started in daemon mode. For example:
-rw-rw-rw- 1 root root /var/log/qemu-ga.log
-rw-rw-rw- 1 root root /var/run/qga.state
-rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
In addition, at least all files created with the guest-file-open QMP
command, and all files created with shell output redirection (or
otherwise) by utilities invoked by the fsfreeze hook script are affected.
For now mask all file mode bits for group and others in
become_daemon().
Temporarily, for compatibility reasons, stick with the 0666 file-mode in
case of files newly created by the guest-file-open QMP call. Do so
without changing the umask temporarily.
Signed-off-by: Laszlo Ersek ler...@redhat.com
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
(cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67)
[AF: Use error_set() instead of error_setg*()]
Signed-off-by: Andreas Färber afaer...@suse.de
---
qemu-ga.c| 2 +-
qga/commands-posix.c | 117
+--
2 files changed, 115 insertions(+), 4 deletions(-)
diff --git a/qemu-ga.c b/qemu-ga.c
index 1b00c2f..086b1b9 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -424,7 +424,7 @@ static void become_daemon(const char *pidfile)
}
}
-umask(0);
+umask(S_IRWXG | S_IRWXO);
sid = setsid();
if (sid 0) {
goto fail;
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 00d035d..c0a1bd4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,9 @@
#include sys/types.h
#include sys/ioctl.h
#include sys/wait.h
+#include stdio.h
+#include string.h
+#include sys/stat.h
#include qga/guest-agent-core.h
#include qga-qmp-commands.h
#include qerror.h
@@ -122,9 +125,117 @@ static GuestFileHandle *guest_file_handle_find(int64_t
id)
return NULL;
}
+typedef const char * const ccpc;
+
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+ccpc *forms;
+int oflag_base;
+} guest_file_open_modes[] = {
+{ (ccpc[]){ r, rb, NULL }, O_RDONLY },
+{ (ccpc[]){ w, wb, NULL }, O_WRONLY | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a, ab, NULL }, O_WRONLY | O_CREAT | O_APPEND },
+{ (ccpc[]){ r+, rb+, r+b, NULL }, O_RDWR},
+{ (ccpc[]){ w+, wb+, w+b, NULL }, O_RDWR | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a+, ab+, a+b, NULL }, O_RDWR | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+unsigned mode;
+
+for (mode = 0; mode ARRAY_SIZE(guest_file_open_modes); ++mode) {
+ccpc *form;
+
+form = guest_file_open_modes[mode].forms;
+while (*form != NULL strcmp(*form, mode_str) != 0) {
+++form;
+}
+if (*form != NULL) {
+break;
+}
+}
+
+if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+error_set(err, QERR_UNSUPPORTED);
+return -1;
+}
+return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+ S_IRGRP | S_IWGRP | \
+ S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+Error *local_err = NULL;
+int oflag;
+
+oflag = find_open_flag(mode, local_err);
+if (local_err == NULL) {
+int fd;
+
+/* If the caller wants / allows creation of a new file, we implement
it
+ * with a two step process: open() + (open() / fchmod()).
+ *
+ * First we insist on creating the file exclusively as a new file. If
+ * that succeeds, we're free to set any file-mode bits on it. (The
+ * motivation is that we want to set those file-mode bits
independently
+ * of the current umask.)
+ *
+ * If the exclusive creation fails because the file already exists
+ * (EEXIST is not possible for any other reason), we just attempt to
+ * open the file, but in this case we won't be allowed to change the
+ * file-mode bits on the preexistent file.
+ *
+ * The pathname should never disappear between the two open()s in
+ * practice. If it happens, then someone very likely tried to race
us.
+ * In this case just go ahead and report the ENOENT from the second
+ * open() to the caller.
+ *
+ * If the caller wants to open a preexistent file, then the first
+ * open() is decisive and its third argument is ignored, and the
second
+ * open() and the fchmod
Re: [Qemu-devel] [PATCH stable-1.1] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 05/27/13 02:11, Laszlo Ersek wrote:
On 05/26/13 15:34, Andreas Färber wrote:
From: Laszlo Ersek ler...@redhat.com
The qemu guest agent creates a bunch of files with insecure permissions
when started in daemon mode. For example:
-rw-rw-rw- 1 root root /var/log/qemu-ga.log
-rw-rw-rw- 1 root root /var/run/qga.state
-rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
In addition, at least all files created with the guest-file-open QMP
command, and all files created with shell output redirection (or
otherwise) by utilities invoked by the fsfreeze hook script are affected.
For now mask all file mode bits for group and others in
become_daemon().
Temporarily, for compatibility reasons, stick with the 0666 file-mode in
case of files newly created by the guest-file-open QMP call. Do so
without changing the umask temporarily.
Signed-off-by: Laszlo Ersek ler...@redhat.com
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
(cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67)
[AF: Use error_set() instead of error_setg*()]
Signed-off-by: Andreas Färber afaer...@suse.de
---
qemu-ga.c| 2 +-
qga/commands-posix.c | 117
+--
2 files changed, 115 insertions(+), 4 deletions(-)
diff --git a/qemu-ga.c b/qemu-ga.c
index 1b00c2f..086b1b9 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -424,7 +424,7 @@ static void become_daemon(const char *pidfile)
}
}
-umask(0);
+umask(S_IRWXG | S_IRWXO);
sid = setsid();
if (sid 0) {
goto fail;
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 00d035d..c0a1bd4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,9 @@
#include sys/types.h
#include sys/ioctl.h
#include sys/wait.h
+#include stdio.h
+#include string.h
+#include sys/stat.h
#include qga/guest-agent-core.h
#include qga-qmp-commands.h
#include qerror.h
@@ -122,9 +125,117 @@ static GuestFileHandle *guest_file_handle_find(int64_t
id)
return NULL;
}
+typedef const char * const ccpc;
+
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+ccpc *forms;
+int oflag_base;
+} guest_file_open_modes[] = {
+{ (ccpc[]){ r, rb, NULL }, O_RDONLY },
+{ (ccpc[]){ w, wb, NULL }, O_WRONLY | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a, ab, NULL }, O_WRONLY | O_CREAT | O_APPEND },
+{ (ccpc[]){ r+, rb+, r+b, NULL }, O_RDWR},
+{ (ccpc[]){ w+, wb+, w+b, NULL }, O_RDWR | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a+, ab+, a+b, NULL }, O_RDWR | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+unsigned mode;
+
+for (mode = 0; mode ARRAY_SIZE(guest_file_open_modes); ++mode) {
+ccpc *form;
+
+form = guest_file_open_modes[mode].forms;
+while (*form != NULL strcmp(*form, mode_str) != 0) {
+++form;
+}
+if (*form != NULL) {
+break;
+}
+}
+
+if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+error_set(err, QERR_UNSUPPORTED);
+return -1;
+}
+return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+ S_IRGRP | S_IWGRP | \
+ S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+Error *local_err = NULL;
+int oflag;
+
+oflag = find_open_flag(mode, local_err);
+if (local_err == NULL) {
+int fd;
+
+/* If the caller wants / allows creation of a new file, we
implement it
+ * with a two step process: open() + (open() / fchmod()).
+ *
+ * First we insist on creating the file exclusively as a new file.
If
+ * that succeeds, we're free to set any file-mode bits on it. (The
+ * motivation is that we want to set those file-mode bits
independently
+ * of the current umask.)
+ *
+ * If the exclusive creation fails because the file already exists
+ * (EEXIST is not possible for any other reason), we just attempt to
+ * open the file, but in this case we won't be allowed to change the
+ * file-mode bits on the preexistent file.
+ *
+ * The pathname should never disappear between the two open()s in
+ * practice. If it happens, then someone very likely tried to race
us.
+ * In this case just go ahead and report the ENOENT from the second
+ * open() to the caller.
+ *
+ * If the caller wants to open a preexistent file, then the first
+ * open() is decisive and its third argument is ignored, and the
second
Re: [Qemu-devel] [PATCH stable-1.1] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 05/27/13 02:19, Andreas Färber wrote: Am 27.05.2013 02:11, schrieb Laszlo Ersek: Do you plan to backport 8fe6bbc qga: distinguish binary modes in guest_file_open_modes map 2b72001 qga: unlink just created guest-file if fchmod() or fdopen() fails on it too? These are considered polish for the CVE fix. I did backport both to openSUSE 12.2 - they apply without conflicts. :) I mainly posted this one to check if there are better QERRs to use. I think your choices were apt. Also, a side-note: existing world-writable log files etc. are not recreated nor have their modes changed, so maybe a release note or some such would be useful for admins (delete your previous logfile optional unix domain socket, or change their modes manually). Feel free to add a note to the 1.5 Release Notes - it can then be copied to the previous releases we backport this fix to. Do you mean in the wiki? I'll need an account first :) Thanks, Laszlo
Re: [Qemu-devel] [PATCH stable-1.1] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 05/27/13 02:19, Andreas Färber wrote: Am 27.05.2013 02:11, schrieb Laszlo Ersek: Also, a side-note: existing world-writable log files etc. are not recreated nor have their modes changed, so maybe a release note or some such would be useful for admins (delete your previous logfile optional unix domain socket, or change their modes manually). Feel free to add a note to the 1.5 Release Notes - it can then be copied to the previous releases we backport this fix to. Done: http://wiki.qemu.org/ChangeLog/1.5#Guest_agent If too verbose or not precise enough, please go ahead and edit it. Thanks, Laszlo
Re: [Qemu-devel] [PATCH] boot: fix path pattern of scsi device
On 05/28/13 10:06, Paolo Bonzini wrote: Il 28/05/2013 09:40, Amos Kong ha scritto: bootindex parameter of scsi device doesn't work, it causes by wrong pattern in seabios. qemu passes the following firmware dev_path to seabios: /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 No, this is another unexpected change due to the virtio refactoring in QEMU. The right fix is in QEMU, by adding a get_fw_dev_path implementation in hw/virtio/virtio-bus.c. We fixed it already for migration paths, it should be easy to do the same for this. Please Cc qemu-sta...@nongnu.org when sending the QEMU patch. Thanks, Paolo Ahhh. I was super confused by this patch initially. Amos, when posting a patch to both lists, please add the project name to the bracketed bag-of-tags in the subject, like [SeaBIOS PATCH] boot: fix path pattern of scsi device I saw this message first on qemu-devel, and until I noticed src/boot.c I was kind of confused whom you want to adapt to whom, and in what direction Paolo argues against it. So, virtio refactoring in QEMU (care to name a commit or release?) changed the OpenFirmware device path exported for virtio-scsi devices under the boot order fw_cfg key. This patch intended to adapt SeaBIOS to recognize the new OFW devpath. Under this approach I would have to update QemuBootOrder.c in OVMF in parallel, so that it accepts both old and new style OFW devpaths for virtio-scsi. However Paolo says the new style OFW devpath should be fixed (eliminated) in qemu, and consumers shouldn't notice any change in the long term. And I won't have to change QemuBootOrder.c. Right? Thanks! Laszlo
[Qemu-devel] anyone willing to review a virtio-net guest driver for OVMF?
Hi, the driver in question is intended as a fallback driver when the iPXE EFI driver for virtio-net is not present as an oprom. The series starts with technical notes that should help the virtio-net expert catch any errors more easily. This review could easily take up a lot of time; my hope is in the fact that people here are both more skilled and faster than my humble self. http://thread.gmane.org/gmane.comp.bios.tianocore.devel/2804 Thanks, Laszlo
Re: [Qemu-devel] [QEMU PATCH v3] qdev: fix get_fw_dev_path to support to add nothing to fw_dev_path
On 05/29/13 09:56, Amos Kong wrote:
Recent virtio refactoring in QEMU made virtio-bus become the parent bus
of scsi-bus, and virtio-bus doesn't have get_fw_dev_path implementation,
typename will be added to fw_dev_path by default, the new fw_dev_path
could not be identified by seabios. It causes that bootindex parameter
of scsi device doesn't work.
This patch implements get_fw_dev_path() in BusClass, it will be called
if bus doesn't implement the method, tyename will be added to
fw_dev_path. If the implemented method returns NULL, nothing will be
added to fw_dev_path.
It also implements virtio_bus_get_fw_dev_path() to return NULL. Then
QEMU will still pass original style of fw_dev_path to seabios.
Signed-off-by: Amos Kong ak...@redhat.com
--
v2: only add nothing to fw_dev_path when get_fw_dev_path() is
implemented and returns NULL. then it will not effect other devices
don't have get_fw_dev_path() implementation.
v3: implement default get_fw_dev_path() in BusClass
---
hw/core/qdev.c | 10 +-
hw/virtio/virtio-bus.c | 6 ++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6985ad8..9190a7e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -515,7 +515,7 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev,
char *p, int size)
l += snprintf(p + l, size - l, %s, d);
g_free(d);
} else {
-l += snprintf(p + l, size - l, %s,
object_get_typename(OBJECT(dev)));
+return l;
}
}
l += snprintf(p + l , size - l, /);
@@ -867,9 +867,17 @@ static void qbus_initfn(Object *obj)
QTAILQ_INIT(bus-children);
}
+static char *default_bus_get_fw_dev_path(DeviceState *dev)
+{
+return g_strdup(object_get_typename(OBJECT(dev)));
+}
+
static void bus_class_init(ObjectClass *class, void *data)
{
+BusClass *bc = BUS_CLASS(class);
+
class-unparent = bus_unparent;
+bc-get_fw_dev_path = default_bus_get_fw_dev_path;
}
static void qbus_finalize(Object *obj)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index ea2e11a..6849a01 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -161,10 +161,16 @@ static char *virtio_bus_get_dev_path(DeviceState *dev)
return qdev_get_dev_path(proxy);
}
+static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
+{
+return NULL;
+}
+
static void virtio_bus_class_init(ObjectClass *klass, void *data)
{
BusClass *bus_class = BUS_CLASS(klass);
bus_class-get_dev_path = virtio_bus_get_dev_path;
+bus_class-get_fw_dev_path = virtio_bus_get_fw_dev_path;
}
static const TypeInfo virtio_bus_info = {
Ah, the happiness of finally understanding what Paolo suggested. (Or at
least believing so.)
My R-b is superfluous after Paolo's, but I can't resist.
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [SeaBIOS] KVM call agenda for 2013-05-28
On 05/30/13 11:23, David Woodhouse wrote: On Wed, 2013-05-29 at 11:18 -0500, Anthony Liguori wrote: Certainly an option, but that is a long-term project. Out of curiousity, are there other benefits to using coreboot as a core firmware in QEMU? Is there a payload we would ever plausibly use besides OVMF and SeaBIOS? I like the idea of using Coreboot on the UEFI side — if the most actively used TianoCore platform is CorebootPkg instead of OvmfPkg, that makes it a lot easier for people using *real* hardware with Coreboot to use TianoCore. Where is CorebootPkg available from? And it helps to dispel the stupid misconception in some quarters that Coreboot *competes* with UEFI and thus cannot possibly be supported because helping something that competes with UEFI would be bad. I'm not sure who do you mean by some quarters, but for some distributions Coreboot would be yet another component (package) to support, for no obvious benefit. (Gerd said it better than I possibly could: http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5685/focus=5705.) Is there a payload we would ever plausibly use besides OVMF and SeaBIOS? For my part I want to get to the point where the default firmware shipped with qemu can be OVMF. For some distributions this is a licensing question as well. See FatBinPkg/License.txt. (The same applies if you rebuild it from source (FatPkgDev), based on FatBinPkg/ReadMe.txt.) For example Fedora can't ship OVMF because of it. If you implement a UEFI FAT driver based on Microsoft's official specification, you're bound by the same restrictions on use and redistribution. If you implement a private UEFI FAT driver from scratch, or port a free software FAT implementation (eg. the r/o one in grub or the r/w one in mtools), you could still run into legal problems, I've been told. If you rip out the FAT driver, then OVMF won't be UEFI compliant and no installer media will boot on it. Interestingly, Ubuntu has OVMF in Universe http://packages.ubuntu.com/raring/ovmf. I think they missed the FatBinPkg license (I would have missed it too, after all you have to track down the licenses of every module included in the FDF file -- it was Paolo who told me about it) and I believe they should actually ship OVMF in Multiverse or Restricted https://help.ubuntu.com/community/Repositories/Ubuntu. We have SeaBIOS-as-CSM working, which was one of the biggest barriers. Agreed, and I could have never done that. Thanks for implementing it with Kevin. We need at least one out-of-tree edk2 patch for now (from you) but apparently that's no problem. There are a few more things (like NV variable storage, in particular) that I need to fix before I can actually make that suggestion with a straight face though... As far as I understand: - Jordan is nearing completion of flash support on KVM, - he also has WIP NvVar storage on top of qemu flash. http://thread.gmane.org/gmane.comp.emulators.qemu/213690 http://thread.gmane.org/gmane.comp.bios.tianocore.devel/2781/focus=2798 (Please coordinate I guess :)) Laszlo
Re: [Qemu-devel] [PVSCSI]How to unplug scsi disk simulated by Qemu, just like unplug the ide disk?
On 05/30/13 13:23, Gonglei (Arei) wrote: Hi all, My environment is xen-4.1.2 + qemu-1.2.2 I made a pvscsi driver for Redhat guest, but I encountered a problem that I could see two scsi disks, one was simulated by QEMU, another was passthrough. Actually I want to unplug the scsi disk simulated. Any methods can solve the problem on the qemu upstream. Thanks! Unless I'm misunderstanding you (which is more likely than not): emulated devices (eg. NIC and IDE disks) are unplugged by the guest kernel. See arch/x86/xen/platform-pci-unplug.c. The qemu side seems to be in hw/xen/xen_platform.c. Laszlo
Re: [Qemu-devel] [PATCH] gdbstub: do not restart crashed guest
On 05/30/13 13:20, Paolo Bonzini wrote:
If a guest has crashed with an internal error or similar, detaching
gdb (or any other debugger action) should not restart it.
Cc: Jan Kiszka jan.kis...@siemens.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
gdbstub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gdbstub.c b/gdbstub.c
index e80e1d3..90e54cb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -371,7 +371,9 @@ static inline void gdb_continue(GDBState *s)
#ifdef CONFIG_USER_ONLY
s-running_state = 1;
#else
-vm_start();
+if (runstate_check(RUN_STATE_DEBUG)) {
+vm_start();
+}
#endif
}
I sought to check the gdb_continue() call sites, and uses of
RUN_STATE_DEBUG. Seems reasonable.
Reviewed-by: Laszlo Ersek ler...@redhat.com
(
FWIW I wonder why in commit ad02b96a Luiz allowed DEBUG - SUSPENDED. As
far as I understand, when the debugger is attached, the guest is not
running, hence it can't go directly to RUN_STATE_SUSPENDED. Maybe due to
a concurrent monitor command?
Technically it does seem possible; from main_loop_should_exit():
if (qemu_debug_requested()) {
vm_stop(RUN_STATE_DEBUG);
}
if (qemu_suspend_requested()) {
qemu_system_suspend();
}
Both requests could become pending during one iteration of the loop, and
the next iteration will see both of them. OK.
)
Laszlo
Re: [Qemu-devel] [PATCH v2 2/5] pci: store PCI hole ranges in guestinfo structure
I can't offer any opinion about the values you put into w32 and w64, but I have
some remarks.
First, please consider passing -O/path/to/some/order_file to git-format-patch,
so that .h files show up at the top of each patch.
On 05/30/13 13:07, Michael S. Tsirkin wrote:
Will be used to pass hole ranges to guests.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/i386/pc.c | 39 ++-
hw/i386/pc_piix.c | 14 +-
hw/i386/pc_q35.c | 6 +-
hw/pci-host/q35.c | 4
include/hw/i386/pc.h | 19 ++-
include/hw/pci-host/q35.h | 2 ++
include/qemu/typedefs.h | 1 +
7 files changed, 81 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4844a6b..c233161 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -978,6 +978,41 @@ void pc_cpus_init(const char *cpu_model, DeviceState
*icc_bridge)
}
}
+typedef struct PcGuestInfoState {
+PcGuestInfo info;
+Notifier machine_done;
+} PcGuestInfoState;
+
+static
+void pc_guest_info_machine_done(Notifier *notifier, void *data)
+{
+PcGuestInfoState *guest_info_state = container_of(notifier,
+ PcGuestInfoState,
+ machine_done);
+}
+
+PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
+ram_addr_t above_4g_mem_size)
+{
+PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
+PcGuestInfo *guest_info = guest_info_state-info;
+
+guest_info-pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
+if (sizeof(hwaddr) == 4) {
+guest_info-pci_info.w64.begin = 0;
+guest_info-pci_info.w64.end = 0;
+} else {
+guest_info-pci_info.w64.begin = 0x1ULL + above_4g_mem_size;
+guest_info-pci_info.w64.end = guest_info-pci_info.w64.begin +
+(0x1ULL 62);
+assert(guest_info-pci_info.w64.begin =
guest_info-pci_info.w64.end);
+}
+
+guest_info_state-machine_done.notify = pc_guest_info_machine_done;
+qemu_add_machine_init_done_notifier(guest_info_state-machine_done);
+return guest_info;
+}
+
void pc_acpi_init(const char *default_dsdt)
{
char *filename;
@@ -1019,7 +1054,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
ram_addr_t below_4g_mem_size,
ram_addr_t above_4g_mem_size,
MemoryRegion *rom_memory,
- MemoryRegion **ram_memory)
+ MemoryRegion **ram_memory,
+ PcGuestInfo *guest_info)
{
int linux_boot, i;
MemoryRegion *ram, *option_rom_mr;
@@ -1071,6 +1107,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
for (i = 0; i nb_option_roms; i++) {
rom_add_option(option_rom[i].name, option_rom[i].bootindex);
}
+guest_info-fw_cfg = fw_cfg;
return fw_cfg;
}
I find this suboptimal style, ie. passing guest_info to pc_memory_init() just
so that pc_memory_init() can set guest_info-fw_cfg to fw_cfg, when
pc_memory_init() returns fw_cfg anyway.
More philosophically:
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b4c8a74..1bf5219 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -9,8 +9,20 @@
#include net/net.h
#include hw/i386/ioapic.h
+#include qemu/range.h
+
/* PC-style peripherals (also used by other machines). */
+typedef struct PcPciInfo {
+Range w32;
+Range w64;
+} PcPciInfo;
+
+struct PcGuestInfo {
+PcPciInfo pci_info;
+FWCfgState *fw_cfg;
+};
Are you sure you need a private link to the global fw_cfg object? The pvpanic
series added a global lookup possibility in commit 10a584b2
(object_resolve_path()).
Anyway these are just subjective style notes.
+
/* parallel.c */
static inline bool parallel_init(ISABus *bus, int index, CharDriverState
*chr)
{
@@ -80,6 +92,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
level);
void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
void pc_hot_add_cpu(const int64_t id, Error **errp);
void pc_acpi_init(const char *default_dsdt);
+
+PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
+ram_addr_t above_4g_mem_size);
+
FWCfgState *pc_memory_init(MemoryRegion *system_memory,
const char *kernel_filename,
const char *kernel_cmdline,
@@ -87,7 +103,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
ram_addr_t below_4g_mem_size,
ram_addr_t above_4g_mem_size,
MemoryRegion *rom_memory,
- MemoryRegion **ram_memory);
+
Re: [Qemu-devel] [SeaBIOS] KVM call agenda for 2013-05-28
On 05/30/13 14:19, David Woodhouse wrote: Yeah, but if we're shoving a lot of hardware-specific ACPI table generation into the guest's firmware, instead of just doing it on the qemu side where a number of us seem to think it belongs, then there *is* a benefit to using Coreboot. When stuff changes on the qemu side and we have to update the table generation to match, you end up having to update just the Coreboot package, and *not* having to patch both SeaBIOS and OVMF. The extra package in the distro really isn't painful to handle, and I suspect it would end up *reducing* the amount of work that we have to do to update. You update *just* Coreboot, not *both* of SeaBIOS and OVMF. I can't deny there's logic in that, but it still feels like tying ourselves up in some intricate bondage choreography. We'd like to move ACPI tables out of firmware, but we can't move them to qemu due to project direction disagreement, so let's adopt a middleman. (I'm not trying to denigrate coreboot -- I don't know it at all --, but introducing it in a (granted, distro-specific) stack just for this purpose seems quite arbitrary.) If you implement a private UEFI FAT driver from scratch, or port a free software FAT implementation (eg. the r/o one in grub or the r/w one in mtools), you could still run into legal problems, I've been told. That has been said, and it's been said for the FAT implementation in the kernel too. If a distribution is happy to ship the kernel without ripping out its FAT support, then I see no reason why that distribution wouldn't also be happy to ship a version of OVMF with a clean implementation of FAT support. It doesn't make sense to be happy with one but not the other. Under my *personal* impression, logic and Common Law don't really mix, especially not in the US. Still under my *personal* impression, someone might not feel convenient suing you for redistributing code that already exists in the upstream Linux kernel, but might happily drag you to court for an original clean implementation, and then you can explain it's illogical for them to do so. The best I can do with your suggestion is to take it to our legal dept. I would be happy to work on a new FAT driver. (I used to feel differently earlier but I've changed my mind.) We need at least one out-of-tree edk2 patch for now (from you) but apparently that's no problem. That'll get merged soon. We are working on the corresponding spec update... Great news! Thanks, Laszlo
Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
On 05/30/13 14:59, Luiz Capitulino wrote:
On Tue, 28 May 2013 14:19:22 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xf000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Ping? Wen?
Would be nice get a Reviewed-by before merging...
I didn't miss your submission and did find it OK, I just felt unsure
about stating so, because simple patches like this are prime territory
to burn someone's R-b's worth (ie. to expose a reviewer's lack of
information / experience). But hey, what can I lose? The patch does look
good to me, so
Reviewed-by: Laszlo Ersek ler...@redhat.com
---
PS: I (obviously) don't any more core dumps with this patch applied, but
I couldn't check if the Windows dump is correct (does anyone know
how to do this?). I did quickly check on Linux though.
target-i386/arch_memory_mapping.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr
pde_start_addr,
continue;
}
-pte_start_addr = (pde ~0xfff) a20_mask;
+pte_start_addr = (pde PLM4_ADDR_MASK) a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
-pde_start_addr = (pdpe ~0xfff) a20_mask;
+pde_start_addr = (pdpe PLM4_ADDR_MASK) a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i 0x1ffULL) 39) | (0xULL 48);
-pdpe_start_addr = (pml4e ~0xfff) a20_mask;
+pdpe_start_addr = (pml4e PLM4_ADDR_MASK) a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
if (env-hflags HF_LMA_MASK) {
hwaddr pml4e_addr;
-pml4e_addr = (env-cr[3] ~0xfff) env-a20_mask;
+pml4e_addr = (env-cr[3] PLM4_ADDR_MASK) env-a20_mask;
walk_pml4e(list, pml4e_addr, env-a20_mask);
} else
#endif
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 15:27, Michael S. Tsirkin wrote:
Use the type-safe FWCfgState structure instead
of the unsafe void *.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/misc/pvpanic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 31e1b1d..1483f27 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
{
PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
static bool port_configured;
-void *fw_cfg;
+FWCfgState *fw_cfg;
memory_region_init_io(s-io, pvpanic_ops, s, pvpanic, 1);
isa_register_ioport(dev, s-io, s-ioport);
Doesn't this break your build? Lower down in the function there's
fw_cfg = object_resolve_path(/machine/fw_cfg, NULL);
and object_resolve_path() returns a pointer-to-Object, not
pointer-to-FWCfgState.
But for starters I'm quite confused about how the unpatched function
works. What it does amounts to:
fw_cfg_add_file(object_resolve_path(...), ...);
But, again object_resolve_path() returns pointer-to-Object. I'm checking
struct Object in include/qom/object.h, and it suggests that derived
structs should embed Object as first member. However FWCfgState is *not*
such a derived member. What's going on here?
http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450
Laszlo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 17:41, Paolo Bonzini wrote: Il 30/05/2013 17:05, Laszlo Ersek ha scritto: But, again object_resolve_path() returns pointer-to-Object. I'm checking struct Object in include/qom/object.h, and it suggests that derived structs should embed Object as first member. However FWCfgState is *not* such a derived member. What's going on here? http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564 http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450 FWCfgState embeds Object via SysBusDevice and DeviceState. Clearly an evil trick by a devious mind. Thanks :) Laszlo
Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly
On 05/30/13 17:05, Laszlo Ersek wrote:
On 05/30/13 15:27, Michael S. Tsirkin wrote:
Use the type-safe FWCfgState structure instead
of the unsafe void *.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/misc/pvpanic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 31e1b1d..1483f27 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
{
PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
static bool port_configured;
-void *fw_cfg;
+FWCfgState *fw_cfg;
memory_region_init_io(s-io, pvpanic_ops, s, pvpanic, 1);
isa_register_ioport(dev, s-io, s-ioport);
Doesn't this break your build? Lower down in the function there's
fw_cfg = object_resolve_path(/machine/fw_cfg, NULL);
and object_resolve_path() returns a pointer-to-Object, not
pointer-to-FWCfgState.
Paolo explained the guts, but don't we still need a downcast here? (No
idea how to do that nicely in the object model du jour -- maybe
OBJECT_CHECK() or similar?)
Laszlo
Re: [Qemu-devel] [PATCH 0/3] fw_cfg: misc fixes
On 05/30/13 15:27, Michael S. Tsirkin wrote: Here are some misc fixes to the fw cfg object handling. I've queued them on my pci branch temporarily as it's useful to cleanup some pci things. I'm using this with Laszlo's cleanup patch that got rid of void * in pc.c too - that's queued there as well. Please review and comment. Michael S. Tsirkin (3): pvpanic: use FWCfgState explicitly fw_cfg: add API to find FW cfg object fw_cfg: fw_cfg is a singleton hw/misc/pvpanic.c | 4 ++-- hw/nvram/fw_cfg.c | 18 +- include/hw/nvram/fw_cfg.h | 2 ++ 3 files changed, 17 insertions(+), 7 deletions(-) series Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH 2/3] fw_cfg: add API to find FW cfg object
On 05/30/13 15:28, Michael S. Tsirkin wrote:
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 1483f27..910e44f 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -96,7 +96,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
isa_register_ioport(dev, s-io, s-ioport);
if (!port_configured) {
-fw_cfg = object_resolve_path(/machine/fw_cfg, NULL);
+fw_cfg = fw_cfg_find();
if (fw_cfg) {
fw_cfg_add_file(fw_cfg, etc/pvpanic-port,
g_memdup(s-ioport, sizeof(s-ioport)),
Explains 1/3 too, OK.
Laszlo
Re: [Qemu-devel] [PATCH v2 5/5] pc: pci-info add compat support
On 05/30/13 13:07, Michael S. Tsirkin wrote: /* PC hardware initialisation */ static void pc_init1(MemoryRegion *system_memory, @@ -122,6 +122,7 @@ static void pc_init1(MemoryRegion *system_memory, } guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); +guest_info-compat_v1_5 = guest_info_compat_v1_5; I believe I can see the advantage of delaying this compat_v1_5 until init-done-notifier time: init code gradually building up / rewriting guest_info doesn't have to tiptoe around conditions. Style: would it be worth passing guest_info_compat_v1_5 as a parameter to pc_guest_info_init()? Currently you have an _init() function that partially initializes the struct, and right after _init() returns you fill in what's still missing form basic initialization. No more comments for the series. Thanks, Laszlo
Re: [Qemu-devel] [SeaBIOS] KVM call agenda for 2013-05-28
On 05/30/13 18:20, Jordan Justen wrote: I think ACPI table generation lives in firmware on real products, because on real products the firmware is the point that best understands the actual hardware layout for the machine. In qemu, I would say that qemu best knows the hardware layout, given that the firmware is generally a slightly separate project from qemu. I don't think adding a coreboot layer into the picture helps, if it brings along the coreboot payload boot interface as a requirement. Then again, I don't really understand how firmware could be swapped out in this case. What would -bios do? How would the coreboot ACPI shim layer be specified to qemu? I guess -bios would load coreboot. Coreboot would siphon the data necessary for ACPI table building through the current (same) fw_cfg bottleneck, build the tables, load the boot firmware (SeaBIOS or OVMF or something else -- not sure how to configure that), and pass down the tables to the firmware (through a now unspecified interface -- perhaps the tables could even be installed at this point). This could introduce another interface (fw_cfg+something rather than just fw_cfg), but ACPI table preparation would be concentrated in one project. I guess. Laszlo
Re: [Qemu-devel] [SeaBIOS] KVM call agenda for 2013-05-28
On 05/30/13 18:57, Jordan Justen wrote: On Thu, May 30, 2013 at 9:41 AM, Laszlo Ersek ler...@redhat.com wrote: On 05/30/13 18:20, Jordan Justen wrote: I think ACPI table generation lives in firmware on real products, because on real products the firmware is the point that best understands the actual hardware layout for the machine. In qemu, I would say that qemu best knows the hardware layout, given that the firmware is generally a slightly separate project from qemu. I don't think adding a coreboot layer into the picture helps, if it brings along the coreboot payload boot interface as a requirement. Then again, I don't really understand how firmware could be swapped out in this case. What would -bios do? How would the coreboot ACPI shim layer be specified to qemu? I guess -bios would load coreboot. Coreboot would siphon the data necessary for ACPI table building through the current (same) fw_cfg bottleneck, build the tables, load the boot firmware (SeaBIOS or OVMF or something else -- not sure how to configure that), and pass down the tables to the firmware (through a now unspecified interface -- perhaps the tables could even be installed at this point). This could introduce another interface (fw_cfg+something rather than just fw_cfg), but ACPI table preparation would be concentrated in one project. I guess. For reference, I believe that both Xen and virtualbox build ACPI table in the VMM rather than firmware. They both dump the tables into the 0xe000 segment (yuck) where firmware finds and publishes it to the OS. I think fw-cfg would be a reasonable alternative to this for communicating the tables. I think Xen uses a separate utility called hvmloader that runs in the domU. - http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5453/focus=5668 - http://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=tools/firmware/hvmloader - http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/6255/focus=110562 Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 09:09, Jordan Justen wrote: Why is updating the ACPI tables in seabios viewed as such a burden? Either qemu does it, or seabios... (And, OVMF too, but I don't think you guys are concerned with that. :) I am :) On the flip side, why is moving the ACPI tables to QEMU such an issue? It seems like Xen and virtualbox both already do this. Why is running iasl not an issue for them? I think something was mentioned about iasl having problems on BE machines? I could be easily wrong but I *guess* qemu's hosts x targets (emulate what on what) set is a proper superset of xen's and virtualbox's. Presumably if you want to run an x86 guest on a MIPS host, and also want to build qemu on the same MIPS (or SPARC) host, you'd have to run iasl there too. Maybe we are doing lots of things horribly wrong in our OVMF ACPI tables :) Impossible. :) In earnest, I think what we have now is (mostly) correct, just not extensive / flexible enough. No support for PCI hotplug or CPU hotplug, none for S3 (although all of these tie into UEFI deeply), no MTRR setup, no MPTABLE; let alone a non-PIIX chipset. (Well maybe I shouldn't lump these under the ACPI umbrella.) but I haven't seen it as much of a burden. (Of course, Laszlo has helped out with many of the ACPI changes in OVMF, so his opinion should be taken into consideration too. :) It hasn't been a burden in the sense of me not liking the activity; I actually like fiddling with knobs. It has certainly been extra work to bring OVMF's ACPI tables closer to SeaBIOS's functionality / flexibility (and we still lag behind it quite.). Due to licensing differences I can't just port code from SeaBIOS to OVMF (and I never have without explicit permission), so it's been a lot of back and forth with acpidump / iasl -d in guests (massage OVMF, boot guest, check guest dmesg / lspci, dump tables, compare, repeat), brain picking colleagues, the ACPI and PIIX specs and so on. I have a page on the RH intranet dedicated to this. When something around these parts is being changed (or looks like it could be changed) in SeaBIOS, or between qemu and SeaBIOS, I always must be alert and consider reimplementing it in, or porting it with permission to, OVMF. (Most recent example: pvpanic device -- currently only in SeaBIOS.) It worries me that if I slack off, or am busy with something else, or simply don't notice, then the gap will widen again. I appreciate learning a bunch about ACPI, and don't mind the days of work that went into some of my simple-looking ACPI patches for OVMF, but had the tables come from a common (programmatic) source, none of this would have been an issue, and I wouldn't have felt even occasionally that ACPI patches for OVMF were both duplicate work *and* futile (considering how much ahead SeaBIOS was). I don't mind reimplementing stuff, or porting it with permission, going forward, but the sophisticated parts in SeaBIOS are a hard nut. For example I'll never be able to auto-extract offsets from generated AML and patch the AML using those offsets; the edk2 build tools (a project separate from edk2) don't support this, and it takes several months to get a thing as simple as gcc-47 build flags into edk2-buildtools. Instead I have to write template ASL, compile it to AML, hexdump the result, verify it against the AML grammar in the ACPI spec (offsets aren't obvious, BytePrefix and friends are a joy), define initialize a packed struct or array in OVMF, and patch the template AML using fixed field names or array subscripts. Workable, but dog slow. If the ACPI payload came from up above, we might be as well provided with a list of (canonical name, offset, size) triplets, and could perhaps blindly patch the contents. (Not unlike Michael's linker code for connecting tables into a hierarchy.) AFAIK most recently iasl got built-in support for offset extraction (and in the process the current SeaBIOS build method was broken...), so that part might get easier in the future. Oh well it's Friday, sorry about this rant! :) I'll happily do what I can in the current status quo, but frequently, it won't amount to much. Thanks, Laszlo
Re: [Qemu-devel] [PATCH v7 1/4] isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)
On 05/31/13 06:41, Jordan Justen wrote: On Thu, May 30, 2013 at 7:06 PM, Kevin O'Connor ke...@koconnor.net wrote: On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: The isapc machine with seabios currently requires the BIOS region to be read/write memory rather than read-only memory. KVM currently cannot support the BIOS as a ROM region, but qemu in non-KVM mode can. Based on this, isapc machine currently only works with KVM. To work-around this isapc issue, this change avoids marking the BIOS as readonly for isapc. How about changing the code to always make the ROM read/write (instead of doing it only on isapc). Currently, if the rom is marked as read-only, then SeaBIOS makes it read/write as the first thing it does. So, making it read-only doesn't really serve any purpose. Are you talking about PAM registers? I think these should only affect 0xe-0xf and 0xfffe000-0x. So, the PAM registers should only impact part of the potential ROM range. Also, I think the PAM registers should default to ROM mode after reset and on power-on. I think we've been here before... - always resetting the PAM registers broke S3 resume: http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=195932 http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=196081 - there was a patch to distinguish soft reset from hard reset: http://thread.gmane.org/gmane.comp.emulators.qemu/195351 - another approach for soft vs. hard, and how it relates to resume: http://thread.gmane.org/gmane.comp.emulators.qemu/198545/focus=198546 I believe currently qemu doesn't distinguish soft from hard reset. Hard reset would have to reset PAM registers, soft reset must not. Since we only have one (mixed?) kind, and S3 resume depends on PAM registers being intact, we can't clear them during the one kind of reset that we currently have. No idea what the final resolution was, but I vaguely believe none of the distinguish soft from hard reset submissions got all aspects right, or some such. Thanks, Laszlo
Re: [Qemu-devel] [SeaBIOS] KVM call agenda for 2013-05-28
On 05/31/13 10:13, Peter Stuge wrote: ACPI bytes are obviously a function of QEMU configuration. Precisely! When we evaluate that (mathematical-sense) function in boot firmware, we need to retrieve the function's arguments. Those arguments are bits of QEMU configuration, as you say, and fw_cfg is extremely inconvenient for fetching them. Whenever the domain or arity of said mathematical function changes (we need more arguments, or different kinds of arguments), we have to extend fw_cfg with yet another ad-hoc key or file entry. The set of arguments going into ACPI tables *is* ad-hoc and arbitrary, there's nothing to do about it. But at least we shouldn't impede the retrieval of said arguments with artificial obstacles, such as half-assedly serializing them over fw_cfg (and not documenting them, naturally). In qemu the entire pool of arguments, current and future, would be at just an arm's length, in immediately consumable form. I've said the same about the acpi_build_madt() prototype that was proposed for qemu: http://thread.gmane.org/gmane.comp.emulators.qemu/207171/focus=208719. Thanks, Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 16:08, David Woodhouse wrote: On Fri, 2013-05-31 at 08:04 -0500, Anthony Liguori wrote: soapbox Fork OVMF, drop the fat module, and just add GPL code. It's an easily solvable problem. Heh. Actually it doesn't need to be a fork. It's modular, and the FAT driver is just a single module. Which is actually included in *binary* form in the EDK2 repository, I believe, and its source code is elsewhere. Correct. We could happily make a GPL¹ or LGPL implementation of a FAT module and build our OVMF with that instead, and we wouldn't need to fork OVMF at all. Yes, that's one plan, *if* someone can sort out, or is willing to shoulder, the perhaps illogical but still worrisome surroundings of FatPkg / FatBinPkg. (I don't intend to spread FUD!) For example, if your employer authorizes you to implement GplFatPkg from scratch, and distribute it as an external module, I -- as someone without any education in law though -- will give you a standing ovation and buy you a case of beer at KVM Forum 2013. Deal? :) (You proved to have great leverage by getting the efi compat table extended, so... :)) ¹ If it's GPL, of course, then we mustn't include any *other* binary blobs in our OVMF build. But the whole point in this conversation is that we don't *want* to do that. So that's fine. Right. Eg. Shell1 is embedded as a pre-built binary, but that's just convenience, you can build the in-tree Shell2 from source afresh and embed that instead (and ship its source too). Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 17:43, Anthony Liguori wrote: David Woodhouse dw...@infradead.org writes: On Fri, 2013-05-31 at 08:04 -0500, Anthony Liguori wrote: soapbox Fork OVMF, drop the fat module, and just add GPL code. It's an easily solvable problem. Heh. Actually it doesn't need to be a fork. It's modular, and the FAT driver is just a single module. Which is actually included in *binary* form in the EDK2 repository, I believe, and its source code is elsewhere. We could happily make a GPL¹ or LGPL implementation of a FAT module and build our OVMF with that instead, and we wouldn't need to fork OVMF at all. So can't we have GPL virtio modules too? I don't think there's any problem there except for the FAT module. I share your assessment. I would propose more of a virtual fork. It could consist of a git repo with the GPL modules + a submodule for edk2. Ideally, there would be no need to actually fork edk2. Indeed. edk2 is extremely modular. But in order to get a useful firmware image ultimately, you need a FAT driver. My assumption is that edk2 won't take GPL code. Correct, see eg. OvmfPkg/Contributions.txt. Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 16:38, Anthony Liguori wrote: It's either Open Source or it's not. It's currently not. I disagree with this binary representation of Open Source or Not. If it weren't (mostly) Open Source, how could we fork (most of) it as you're suggesting (from the soapbox :))? I have a hard time sympathesizing with trying to work with a proprietary upstream. My experience has been positive. First of all, whether UEFI is a good thing or not is controversial. I won't try to address that. However UEFI is here to stay, machines are being shipped with it, Linux and other OSen try to support it. Developing (or running) an OS in combination with a specific firmware is sometimes easier / more economic in a virtual environment, hence there should be support for qemu + UEFI. It is this mindset that I operate in. (Oh, I also forgot to mention that this task has been assigned to me by my superiors as well :)) Jordan, the OvmfPkg maintainer is responsive and progressive in the true FLOSS manner (*), which was a nice surprise for a project whose coding standards for example are made 100% after Windows source code, and whose mailing list is mostly subscribed to by proprietary vendors. Really when it comes to OvmfPkg patches the process follows the normal FLOSS development model. (*) Jordan, I hope this will prompt you to merge VirtioNetDxe v4 real soon now :) I personally think the 2-clause BSDL for 99% of the project was a very sane and practical one from Intel et al. FatPkg is a sad exception. One might even consider it a bad accident: http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1861/focus=1878 I have no idea how that selection process went down, but I assume if FLOSS people had been screaming very loud at that time and had offered a *simple* (which ext2 is not, I gather), wide-spread and unencumbered filesystem, things would be different today. In terms of creating a FAT module, the most likely source would seem to be the kernel code and since that's GPL, I don't think it's terribly avoidable to end up with a GPL'd uefi implementation. If that's inevitable, then we're wasting effort by rewriting stuff under a BSD license. Please ask your employer if they'd be willing to put their name on an original, clean-room, GPL-licensed implementation of FAT32 for UEFI. Thus far we've been talking copyright rather than patents, but there's also this: http://en.wikipedia.org/wiki/FAT_filesystem#Challenge http://en.wikipedia.org/wiki/FAT_filesystem#Patent_infringement_lawsuits It almost doesn't matter who prevails in such a lawsuit; the *possibility* of such a lawsuit gives people cold feet. Blame the USPTO. Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 18:33, David Woodhouse wrote: On Fri, 2013-05-31 at 10:43 -0500, Anthony Liguori wrote: It's even more fundamental. OVMF as a whole (at least in it's usable form) is not Open Source. The FAT module is required to make EDK2 usable, and yes, that's not Open Source. So in a sense you're right. But we're talking here about *replacing* the FAT module with something that *is* open source. And the FAT module isn't a fundamental part of EDK2; it's just an optional module that happens to be bundled with the repository. Yes. *Some* FAT module is a hard requirement. So I think you're massively overstating the issue. OVMF/EDK2 *is* Open Source, Agreed, and replacing the FAT module really isn't that hard. technically it's not hard; for a seasoned file system developer (which I'm not, of course), even possibly missing UEFI bits, it should be children's play actually, considering the high quality of UEFI documentation and the responsiveness of edk2-devel. Considering US legal climate however, it appears *extremely* hard to replace the FAT module, in my unwashed personal opinion. Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-05-28
On 05/31/13 23:03, Jordan Justen wrote: Of course, the fact that the current FAT driver is exclusionary for free software projects is a point that is not lost on me. I just don't agree that the best response to this is a GPL'd FAT driver. What would you suggest? Thank you, Laszlo
Re: [Qemu-devel] [PATCH] target-i386: Fix aflag logic for CODE64 and the 0x67 prefix
On 05/29/13 21:30, Richard Henderson wrote:
The code reorganization in commit 4a6fd938 broke handling of PREFIX_ADR.
While fixing this, tidy and comment the code so that it's more obvious
what's going on in setting both aflag and dflag.
The TARGET_X86_64 ifdef can be eliminated because CODE64 expands to the
constant zero when TARGET_X86_64 is undefined.
Cc: Paolo Bonzini pbonz...@redhat.com
Reported-by: Laszlo Ersek ler...@redhat.com
Signed-off-by: Richard Henderson r...@twiddle.net
---
target-i386/translate.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..14b0298 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4677,8 +4677,6 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
}
s-pc = pc_start;
prefixes = 0;
-aflag = s-code32;
-dflag = s-code32;
s-override = -1;
rex_w = -1;
rex_r = 0;
@@ -4801,23 +4799,25 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
}
/* Post-process prefixes. */
-if (prefixes PREFIX_DATA) {
-dflag ^= 1;
-}
-if (prefixes PREFIX_ADR) {
-aflag ^= 1;
-}
-#ifdef TARGET_X86_64
if (CODE64(s)) {
-if (rex_w == 1) {
-/* 0x66 is ignored if rex.w is set */
-dflag = 2;
+/* In 64-bit mode, the default data size is 32-bit. Select 64-bit
+ data with rex_w, and 16-bit data with 0x66; rex_w takes precedence
+ over 0x66 if both are present. */
+dflag = (rex_w 0 ? 2 : prefixes PREFIX_DATA ? 0 : 1);
+/* In 64-bit mode, 0x67 selects 32-bit addressing. */
+aflag = (prefixes PREFIX_ADR ? 1 : 2);
+} else {
+/* In 16/32-bit mode, 0x66 selects the opposite data size. */
+dflag = s-code32;
+if (prefixes PREFIX_DATA) {
+dflag ^= 1;
}
-if (!(prefixes PREFIX_ADR)) {
-aflag = 2;
+/* In 16/32-bit mode, 0x67 selects the opposite addressing. */
+aflag = s-code32;
+if (prefixes PREFIX_ADR) {
+aflag ^= 1;
}
}
-#endif
s-prefix = prefixes;
s-aflag = aflag;
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap
comments below
On 06/03/13 11:04, Jason Wang wrote:
This is because vhostfdname were passed as NULL to net_init_tap_one() when
vhostfd were not specified, but net_init_tap_one() will still pass it to
monitor_handle_fd_param() when tap-has_vhostfds is true. Since file
descriptor
(fd, vhostfd) and file descriptor set (fds, vhostfds) were not compatible, so
this patch forbids passing them to tap in the same time.
This solve the segfault when passing the command line like:
./qemu-system-x86_64 -netdev tap,fd=2,vhost=on,vhostfds=baz,id=xyz
Cc: Paolo Bonzini pbonz...@redhat.com
Cc: Stefan Hajnoczi shajn...@redhat.com
Cc: Laszlo Ersek ler...@redhat.com
Cc: qemu-sta...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
---
net/tap.c | 10 ++
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index e0b7a2a..477505f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -698,9 +698,10 @@ int net_init_tap(const NetClientOptions *opts, const
char *name,
if (tap-has_fd) {
if (tap-has_ifname || tap-has_script || tap-has_downscript ||
tap-has_vnet_hdr || tap-has_helper || tap-has_queues ||
-tap-has_fds) {
+tap-has_fds || tap-has_vhostfds) {
error_report(ifname=, script=, downscript=, vnet_hdr=,
- helper=, queues=, and fds= are invalid with fd=);
+ helper=, queues=, fds=, and vhostfds=
+ are invalid with fd=);
return -1;
}
This seems OK, since
has_fd precludeshas_fds
- optional: has_vhostfd - optional: has_vhostfds
@@ -725,9 +726,10 @@ int net_init_tap(const NetClientOptions *opts, const
char *name,
if (tap-has_ifname || tap-has_script || tap-has_downscript ||
tap-has_vnet_hdr || tap-has_helper || tap-has_queues ||
-tap-has_fd) {
+tap-has_fd || tap-has_vhostfd) {
error_report(ifname=, script=, downscript=, vnet_hdr=,
- helper=, queues=, and fd= are invalid with fds=);
+ helper=, queues=, fd=, and vhostfd=
+ are invalid with fds=);
return -1;
}
I think this us OK too (implementing the above exclusion from the other
side), but in the second hunk tap-has_fd can never be true actually.
I think relying on that in both the condition and the error message here
would simplify them.
Finally, I think the has_helper, and the outermost else branch
should both be updated as well, for consistency with hunk #1 here. These
branches use vhostfdname too. In these branches,
- both has_fd and has_fds are false (should simplify check errmsg),
- has_vhostfds should be rejected.
Thanks
Laszlo
Re: [Qemu-devel] [PATCH trivial] acpi: actually require either data= or file= for -acpitable
On 06/03/13 14:42, Michael Tokarev wrote:
03.06.2013 16:34, Eric Blake wrote:
On 06/03/2013 03:20 AM, Michael Tokarev wrote:
Initially the code ensured that we have exactly one of data= or file=
option for -acpitable. But after some transformations, the condition
becomes
if (has_data == has_file) { error }
to mean, probably, that both should not be set at the same time. But this
condition does not cover the case when neither is set, and we generate
bogus acpi table in this case.
Instead, check if sum of the two is exactly 1.
Signed-off-by: Michael Tokarev m...@tls.msk.ru --- hw/acpi/core.c |2
+- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 42eeace..2815d8c 100644
--- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -249,7 +249,7 @@ void
acpi_table_add(const QemuOpts *opts, Error **errp) if (err) { goto out; } -
if (hdrs-has_file == hdrs-has_data) {
NACK.
hdrs-has_file and hdrs-has_data are both bool.
Yup.
Pre-patch, the table of possible combinations is:
false == false = error message, zero given false == true = okay, exactly
one given true == false = okay, exactly one given true == true = error
message, two given
which is already what you want.
Ok, you're right. Thank you for spotting this!
This function has another error -- if the file specified
(either for data= or file=) can't be read, it happily
continues instead of erroring out. _That_ is the bug
I tried to hunt but catched something else entirely ;)
Will send a real fix later today... :)
Please CC me; IIRC I sought to test this code reasonably deeply when I
wrote / transformed it. I'm curious.
Thanks,
Laszlo
Re: [Qemu-devel] [PATCH] script: git script to compile every commit in a range of commits
comments below
On 05/31/13 18:39, Jeff Cody wrote:
+usage() {
+echo
+echo $0 [OPTIONS]
+echo $desc
+echo
+echo OPTIONS:
+echo -r git range
+optional; default is '$range'
+
+echo -c configure options
+optional; default is '$config_opt'
+
+echo -m make options
+optional; default is '$make_opt'
+
+echo -d log dir
+optional; default is '$logdir'
+
+echo -l log filename
+optional; default is output-PROCID, where PROCID is the bash
process id
+note: you may specify a full path for the log filename here, and
exclude the
+-d option.
+
+echo -f force a git reset and clean
+this will cause a 'git reset --hard; git clean -fdx' to be run
between checkouts.
+!! WARNING: This may cause data loss in your git tree.
+READ THE git-clean and git-reset man pages and make
+sure you understand the implications of
+'git clean -fdx' and 'git reset --hard' before using
!!
+
+echo -h help
+}
Sorry for not noticing this before: is there any reason for the trailing
spaces on most lines in the usage text?
Plus I suggest lower-casing the THE in READ THE.
+
+while getopts :r:c:m:l:d:hf opt
+do
+case $opt in
+r) range=$OPTARG
+;;
+c) config_opt=$OPTARG
+;;
+m) make_opt=$OPTARG
+;;
+d) logdir=$OPTARG
+;;
+l) logfile=$OPTARG
+;;
+f) force_clean='y'
+;;
+h) usage
+ exit
+;;
+\?) echo Unknown option: -$OPTARG 2
+usage
+exit 1
+;;
+esac
+done
+
+# append a '/' to logdir if $logdir was specified without one
+[[ -n $logdir ]] [[ ${logdir:${#logdir}-1} != / ]]
logdir=${logdir}/
+
+logfile=${logdir}${logfile}
+
+head=`git rev-parse HEAD`
+total=`git rev-list $range |wc -l`
+
+echo log output: $logfile
+
+rm -f $logfile
rm -f -- $logfile is safer, but I doubt anyone would pass a pathname
starting with -...
+date $logfile
+echo git compile check for $range. $logfile
+echo * configure options='$config_opt' $logfile
+echo * make options='$make_opt' $logfile
+echo Performing a test compile on $total patches | tee -a $logfile
+echo -
$logfile
+echo | tee -a $logfile
+
+clean_repo() {
+if [[ $force_clean == 'y' ]]
+then
+git reset --hard $logfile 21 || true
+git clean -fdx -e $logfile $logfile 21 || true
+fi
+}
Does -e mean except? It's not supported on RHEL-6.
+
+# we want to cleanup and return the git tree back to the previous head
+trap cleanup EXIT
+
+cleanup() {
+echo
+echo -n Cleaning up...
+clean_repo
+git checkout $head /dev/null 21
+echo done.
+}
+
+cnt=1
+# don't pipe the git job into read, to avoid subshells
+while read hash
+do
+txt=`git log --pretty=tformat:%h: %s $hash^!`
+echo ${cnt}/${total}: compiling: $txt | tee -a $logfile
+let cnt=$cnt+1;
+echo $logfile
+clean_repo
+make clean /dev/null 21 || true
+git checkout $hash $logfile 21 \
+./configure $config_opt $logfile 21 \
+make $make_opt $logfile 21 ||
+(
+echo | tee -a $logfile
+echo ERROR: commit $hash failed to build! | tee -a $logfile
+git show --stat $hash | tee -a $logfile
+exit 1
+)
+done (git log $range --pretty=tformat:%H --reverse)
+
+echo
+All patches in $range compiled successfully! | tee -a $logfile
+exit 0
I think my remarks are not important, the script should work in any
practical environment. We should get this merged and small fixes can be
posted incrementally if needed.
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH 2/7] log.h: Supply missing includes
On 06/06/13 18:27, Markus Armbruster wrote: stdio.h has always been missing. Rest missed in commit eeacee4. Signed-off-by: Markus Armbruster arm...@redhat.com --- include/qemu/log.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/qemu/log.h b/include/qemu/log.h index 6b0db02..fd76f91 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -2,6 +2,9 @@ #define QEMU_LOG_H #include stdarg.h +#include stdbool.h +#include stdio.h +#include qemu/compiler.h #ifdef NEED_CPU_H #include disas/disas.h #endif Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH 3/7] smbios: Convert to error_report()
On 06/06/13 18:27, Markus Armbruster wrote: Improves diagnistics from ad hoc messages like Invalid SMBIOS UUID string to qemu-system-x86_64: -smbios type=1,uuid=gaga: Invalid UUID Signed-off-by: Markus Armbruster arm...@redhat.com --- arch_init.c | 1 - hw/i386/smbios.c | 24 2 files changed, 12 insertions(+), 13 deletions(-) Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH 4/7] Use sizeof(qemu_uuid) instead of literal 16
On 06/06/13 18:27, Markus Armbruster wrote: Signed-off-by: Markus Armbruster arm...@redhat.com --- arch_init.c | 3 ++- hw/nvram/fw_cfg.c | 2 +- include/sysemu/sysemu.h | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/arch_init.c b/arch_init.c index 5d71870..aa24660 100644 --- a/arch_init.c +++ b/arch_init.c @@ -1029,7 +1029,8 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid) return -1; } #ifdef TARGET_I386 -smbios_add_field(1, offsetof(struct smbios_type_1, uuid), 16, uuid); +smbios_add_field(1, offsetof(struct smbios_type_1, uuid), + sizeof(uuid), uuid); #endif return 0; } I believe this is wrong, uuid is not an array here but a pointer. I guess you mistyped sizeof(qemu_uuid) as sizeof(uuid) in the third arg. diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 3c255ce..f1d3861 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -509,7 +509,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port, sysbus_mmio_map(d, 1, data_addr); } fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)QEMU, 4); -fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16); +fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, sizeof(qemu_uuid)); fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC)); fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu); diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 2fb71af..b969e56 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -15,7 +15,7 @@ extern const char *bios_name; extern const char *qemu_name; -extern uint8_t qemu_uuid[]; +extern uint8_t qemu_uuid[16]; int qemu_uuid_parse(const char *str, uint8_t *uuid); #define UUID_FMT %02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx Rest seems OK. Laszlo
Re: [Qemu-devel] [PATCH 1/7] error-report.h: Supply missing include
On 06/06/13 18:27, Markus Armbruster wrote:
Missed in commit e5924d8.
Signed-off-by: Markus Armbruster arm...@redhat.com
---
include/qemu/error-report.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c902cc1..14c1719 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,7 @@
#define QEMU_ERROR_H
#include stdarg.h
+#include qemu/compiler.h
typedef struct Location {
/* all members are private to qemu-error.c */
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH 5/7] smbios: Clean up smbios_add_field() parameters
On 06/06/13 18:27, Markus Armbruster wrote:
Having size preceed the associated pointer is odd. Swap them, and fix
up the types.
Can you proceed to fix the spelling of precede? :)
Signed-off-by: Markus Armbruster arm...@redhat.com
---
arch_init.c | 2 +-
hw/i386/smbios.c | 26 ++
include/hw/i386/smbios.h | 2 +-
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index aa24660..06b65a2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1030,7 +1030,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
}
#ifdef TARGET_I386
smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
- sizeof(uuid), uuid);
+ uuid, sizeof(uuid));
Still wrong I think.
#endif
return 0;
}
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index a67a328..322f0a0 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -99,7 +99,7 @@ static void smbios_check_collision(int type, int entry)
}
}
-void smbios_add_field(int type, int offset, int len, void *data)
+void smbios_add_field(int type, int offset, const void *data, size_t len)
{
struct smbios_field *field;
@@ -130,21 +130,23 @@ static void smbios_build_type_0_fields(const char *t)
if (get_param_value(buf, sizeof(buf), vendor, t))
smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), version, t))
smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), date, t))
smbios_add_field(0, offsetof(struct smbios_type_0,
bios_release_date_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), release, t)) {
int major, minor;
sscanf(buf, %d.%d, major, minor);
smbios_add_field(0, offsetof(struct smbios_type_0,
- system_bios_major_release), 1, major);
+ system_bios_major_release),
+ major, 1);
smbios_add_field(0, offsetof(struct smbios_type_0,
- system_bios_minor_release), 1, minor);
+ system_bios_minor_release),
+ minor, 1);
}
}
@@ -154,16 +156,16 @@ static void smbios_build_type_1_fields(const char *t)
if (get_param_value(buf, sizeof(buf), manufacturer, t))
smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), product, t))
smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), version, t))
smbios_add_field(1, offsetof(struct smbios_type_1, version_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), serial, t))
smbios_add_field(1, offsetof(struct smbios_type_1,
serial_number_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), uuid, t)) {
if (qemu_uuid_parse(buf, qemu_uuid) != 0) {
error_report(Invalid UUID);
@@ -172,10 +174,10 @@ static void smbios_build_type_1_fields(const char *t)
}
if (get_param_value(buf, sizeof(buf), sku, t))
smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), family, t))
smbios_add_field(1, offsetof(struct smbios_type_1, family_str),
- strlen(buf) + 1, buf);
+ buf, strlen(buf) + 1);
}
int smbios_entry_add(const char *t)
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 94e3641..9babeaf 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -14,7 +14,7 @@
*/
int smbios_entry_add(const char *t);
-void smbios_add_field(int type, int offset, int len, void *data);
+void smbios_add_field(int type, int offset, const void *data, size_t len);
uint8_t *smbios_get_table(size_t *length);
/*
I trust gcc would have caught any missed calls (creates pointer from
integer
Re: [Qemu-devel] [PATCH 6/7] smbios: Fix -smbios type=0, release=... for big endian hosts
On 06/06/13 18:27, Markus Armbruster wrote:
Classic endianness bug due to careless dirty coding: assuming reading
a byte from an int variable gets the least significant byte.
Signed-off-by: Markus Armbruster arm...@redhat.com
---
hw/i386/smbios.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 322f0a0..68bd6d0 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -127,6 +127,7 @@ void smbios_add_field(int type, int offset, const void
*data, size_t len)
static void smbios_build_type_0_fields(const char *t)
{
char buf[1024];
+unsigned char major, minor;
if (get_param_value(buf, sizeof(buf), vendor, t))
smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
@@ -139,8 +140,7 @@ static void smbios_build_type_0_fields(const char *t)
bios_release_date_str),
buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), release, t)) {
-int major, minor;
-sscanf(buf, %d.%d, major, minor);
+sscanf(buf, %hhd.%hhd, major, minor);
smbios_add_field(0, offsetof(struct smbios_type_0,
system_bios_major_release),
major, 1);
Strictly speaking these should be %hhu, if it's not much of a bother.
Otherwise:
Reviewed-by: Laszlo Ersek ler...@redhat.com
(BTW what was wrong with the definitions being in the narrowest scope? :))
Re: [Qemu-devel] [PATCH 7/7] smbios: Check R in -smbios type=0, release=R parses okay
On 06/06/13 18:27, Markus Armbruster wrote:
Signed-off-by: Markus Armbruster arm...@redhat.com
---
hw/i386/smbios.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 68bd6d0..88a1360 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
bios_release_date_str),
buf, strlen(buf) + 1);
if (get_param_value(buf, sizeof(buf), release, t)) {
-sscanf(buf, %hhd.%hhd, major, minor);
+if (sscanf(buf, %hhd.%hhd, major, minor) != 2) {
+error_report(Invalid release);
+exit(1);
+}
smbios_add_field(0, offsetof(struct smbios_type_0,
system_bios_major_release),
major, 1);
Right. OTOH if any of the decimal strings provided doesn't fit into the
space provided (eg. you pass 256 for an unsigned char which happens
to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
used with untrusted data. (... if the result of the conversion cannot
be represented in the space provided, the behavior is undefined.)
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH] fix Coverity scan SIGN_EXTENSION error
On 06/06/13 17:12, Gerd Hoffmann wrote:
Cc: Markus Armbruster arm...@redhat.com
Signed-off-by: Gerd Hoffmann kra...@redhat.com
---
hw/display/qxl-render.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index f511a62..269b1a7 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -199,7 +199,7 @@ static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl,
QXLCursor *cursor)
c-hot_y = cursor-header.hot_spot_y;
switch (cursor-header.type) {
case SPICE_CURSOR_TYPE_ALPHA:
-size = cursor-header.width * cursor-header.height *
sizeof(uint32_t);
+size = sizeof(uint32_t) * cursor-header.width *
cursor-header.height;
memcpy(c-data, cursor-chunk.data, size);
if (qxl-debug 2) {
cursor_print_ascii_art(c, qxl/alpha);
Right. width and height are both uint16_t (short unsigned) and
they are promoted to int. The binding is then
(int * int) * size_t
int * size_t
size_t * size_t
size_t
By the time the product would be converted to size_t (unsigned or
unsigned long in practice, dependent on ILP32 or LP64), it may have
overflowed (undefined behavior for signed int).
The above changes it to
(size_t * int)* int
(size_t * size_t) * int
size_t* int
size_t* size_t
size_t
(BTW this is not order of evaluation, just how the types are determined
by operator binding.)
Reviewed-by: Laszlo Ersek ler...@redhat.com
Re: [Qemu-devel] RFH: boot from virtio cdrom?
On 06/07/13 08:12, Gerd Hoffmann wrote: On 06/06/13 17:10, Philipp Hahn wrote: Hello, I'm using libvirt to manage my VMs and configured one VM to boot from a CDROM connected via virtio. This does neither work with QEMU-1.1.2 nor with QEMU-1.5; neither with SeaBIOS is 1.7.0 nor 1.7.2. Doesn't work with virtio-blk, virtio-scsi can handle that just fine though. ... just make sure your installer media has immediate support for virtio-scsi (or, in case of Windows, at least access to a driver disk, but virtio-win-0.1-30.iso in your command line implied that). Example: https://bugzilla.redhat.com/show_bug.cgi?id=864012. Laszlo
Re: [Qemu-devel] [PATCH v2 0/6] Some -smbios work
On 06/07/13 15:00, Markus Armbruster wrote: v2: Address Hawkeye Laszlo's review You're too kind, but it did crack me up :) (Next time I'll miss something I'll have to hang my head in shame all the more!) * 1-3/7 unchanged * Drop 4/7 because it's buggy, and the fixed version isn't worthwhile * Spelling fix in commit message of 5/7 * Correct scanf format in 5-6/7 series Reviewed-by: Laszlo ever the optimist Ersek ler...@redhat.com
Re: [Qemu-devel] [PATCH] script: git script to compile every commit in a range of commits
On 06/07/13 16:44, Jeff Cody wrote: Thanks. I can either do the above changes for a v2, or as follow on patches. Whichever is easier for you, certainly! I'm fine with the script going-in as is. Cheers, Laszlo
Re: [Qemu-devel] [PATCH] qemu: piix: PCI bridge ACPI hotplug support
On 06/10/13 20:58, Anthony Liguori wrote:
Michael S. Tsirkin m...@redhat.com writes:
This adds support for device hotplug behind
pci bridges. Bridge devices themselves need
to be pre-configured on qemu command line.
One of the goals of this project was to
demonstrate the advantages of generating
ACPI tables in QEMU.
In my opinion, it does this successfully.
Since you've gone out of your way to make this difficult to actually
review...
I haven't tried to review the patch yet, but why would you say such a
thing? Michael wants to convince you. There's no way he could sneak past
you in this patch. You surely won't say it's too hard to review, I'll
yield, and he's obviously aware.
[snip]
* No need for yet another interface to bios to detect qemu version
to check if it's safe to activate new code,
and to ship multiple table versions:
We only care about supporting one version of SeaBIOS. SeaBIOS should
only care about supporting one version of QEMU. We're not asking it to
support multiple versions.
Kevin requires cross-compat between {old, new} qemu and {old, new} SeaBIOS:
http://thread.gmane.org/gmane.comp.emulators.qemu/202005/focus=202072
we generated code so we know this is new qemu
* Use of glib's GArray makes it much easier to build
up tables in code without need for iasl and code patching
Adding a dynamic array to SeaBIOS isn't rocket science.
Please take a look at my series for OVMF that adds basic support for
SMBIOS tables. It took me three days of basically uninterrupted coding
and two approaches to throw away until I got something submittable (with
default tables for only type 0 and type 1).
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/3037
I don't want to invite accusations like this is a strawman argument,
noone was speaking about SMBIOS, but the selective, dynamic patching is
somewhat similar between AML and SMBIOS in any given boot firmware. You
got a big bunch of named offsets that must be mangled individually.
Allowing the firmware to only install blobs verbatim, or maybe patch
them but only in a completely programmatic manner (ie. without specific
field names offsets in the firmware, which I did try for SMBIOS in
OVMF, and failed, (*)), would be a big help.
((*) because the fw_cfg format of individual SMBIOS fields doesn't
distinguish formatted fields from strings in the unformatted area)
Not rocket science, just churn and busywork.
[snip]
So TL;DR, you break a bunch of stuff and introduce a mess of code. It
would be less code and wouldn't break anything to add this logic to
SeaBIOS.
How is this even a discussion?
Well it isn't for me any more; count me out. I'll go with the flow.
Cheers,
Laszlo
Re: [Qemu-devel] [PATCH] qemu: piix: PCI bridge ACPI hotplug support
On 06/10/13 21:57, Anthony Liguori wrote: Laszlo Ersek ler...@redhat.com writes: Please take a look at my series for OVMF that adds basic support for SMBIOS tables. It took me three days of basically uninterrupted coding and two approaches to throw away until I got something submittable (with default tables for only type 0 and type 1). http://thread.gmane.org/gmane.comp.bios.tianocore.devel/3037 I don't want to invite accusations like this is a strawman argument, noone was speaking about SMBIOS, but the selective, dynamic patching is somewhat similar between AML and SMBIOS in any given boot firmware. You got a big bunch of named offsets that must be mangled individually. Allowing the firmware to only install blobs verbatim, or maybe patch them but only in a completely programmatic manner (ie. without specific field names offsets in the firmware, which I did try for SMBIOS in OVMF, and failed, (*)), would be a big help. ((*) because the fw_cfg format of individual SMBIOS fields doesn't distinguish formatted fields from strings in the unformatted area) I don't understand this piece. Other than TianoCore being a weird environment, what made this more difficult than it is to generate the tables in QEMU? QEMU can pass down two kinds of SMBIOS config items over fw_cfg: verbatim tables and individual fields. The verbatim table kind means install this table as-is. I'm not sure how SeaBIOS solves it, in OVMF you call Smbios-Add(). Regarding the field kind config item: for each table type required by the SMBIOS spec, if qemu doesn't provide such a table in whole / in verbatim, then the firmware must install a default table for that type, *and* patch it with any referring field kind config items. Given the current format of the field kind config item, it is currently not possible to just iterate over the field kind config items, and patch whatever field in whatever table they refer to. The information contained in the field kind config item is insufficient to do this. The field kind config item designates a (table, field offset) pair. If that offset points at a string-typed SMBIOS field (basically string serial number into the unformatted area), one must update the unformatted string area after the actual table fields. (In OVMF one does this with a different call, Smbios-UpdateStrings(), and at that point the table even must have been installed already.) If the offset points at a non-string-typed (= in-formatted-area) field, the field must be overwritten in-place. (In OVMF this must happen *before* table installation, ie. before Smbios-Add().) So, the current fw_cfg representation is insufficient, and the firmware must extend (or qemu should have extended) the info contents with this distinction between formatted and unformatted. Again this differs from field to field and the origin of that classification is the SMBIOS spec. Until this point in my email there has been no clear indication whether qemu or the firmware should do this. *Some* side will have to suffer shoveling the field/offset info, and a table template for each type, from the SMBIOS spec into code. I described the above solely as background, to set the landscape. (a) The first thing that tips the scale is: of qemu there is one, and of firmware, there exist at least two, *wildly different* implementations. (Apologies, I forgot about coreboot; not sure if it's affected.) It's not a design purity argument, just what's cheaper. (b) The second thing that further unbalances the scale: suppose you decide to add further field-kind overrides to qemu. For example, currently Type 3 (chassis information, table required by SMBIOS spec) cannot be overridden field-wise. If a user is unhappy with her firmware's default Type 3 table, she's allowed to hex-edit a Type 3 blob, and pass it with -smbios file=type3.blob on the qemu command line. If you as developer want to provide her with more flexibility, you need to (i) patch qemu (of course), to recognize individual fields on the command line, and to populate fw_cfg with them, (ii) patch *every single firmware* supported by qemu to *look for* those (type=3,offset) field-kind config entries, and to effectuate them. If you pass down only verbatim tables, then (ii) falls away. You don't have to update SeaBIOS, nor OVMF. As a bonus, you don't have to care about versions: age-old firmware that only knows how to install verbatim tables will immediately benefit from qemu's new-fangled Type 3 flexibility. Case in point: https://bugzilla.redhat.com/show_bug.cgi?id=788142. (Non-public bug, sorry about that. I think I can disclose that it is about supporting the field kind config entry for Type 3 tables.) For ACPI tables, especially for tables containing AML, multiply the cost of (ii) by twenty. Many more offsets to patch and vastly more dependencies via fw_cfg. Laszlo
Re: [Qemu-devel] [PATCH] qemu: piix: PCI bridge ACPI hotplug support
Sorry about replying to myself... On 06/10/13 22:43, Laszlo Ersek wrote: It's not a design purity argument, just what's cheaper. You probably consider SMBIOS/ACPI table generation guest code that has no place in the machine emulator. You're likely willing to expose the needed bits on a case-by-case basis, in their barest representation. I must consider the entire stack, from the libvirt top, down to the guest kernel or root shell, and see where it is cheapest to effect changes. The higher I can hoist something the better. Modulo, it depends on what element of the stack people think of as central / indispensable -- that element should support the feature directly. (Returning to my earlier example, it would be theoretically viable *I guess* to equip libvirt to pre-format a Type 3 SMBIOS blob in a file, and pass it with -smbios file=..., but many people don't use libvirt. It also might not apply to other emulators supported by libvirt.) Thanks, Laszlo
Re: [Qemu-devel] KVM call agenda for 2013-06-11
On 06/11/13 17:45, Michael S. Tsirkin wrote: To summarize, there's a concensus now that generating ACPI tables in QEMU is a good idea. Two issues that need to be addressed: - original patches break cross-version migration. Need to fix that. - Anthony requested that patchset is merged together with some new feature. I'm not sure the reasoning is clear: current a version intentionally generates tables that are bug for bug compatible with seabios, to simplify testing. Sorry about not following the series more closely -- is there now a qemu interface available that allows any firmware just take the tables, maybe to fix them up blindly / algorithmically, and to install them? IOW, is the interface at such a point that in OVMF we could start looking throwing out specific code, in favor of implementing the generic fw-side algorithm? It seems clear we have users for this such as hotplug of devices behind pci bridges, so why keep the infrastructure out of tree? Looking for something additional, smaller as the hotplug patch is a bit big, so might delay merging. Going forward - would we want to move smbios as well? Everyone seems to think it's a good idea. I think the current fw_cfg interface for SMBIOS tables is already good enough to save a lot of work in OVMF. Namely, if all required tables were generated (table template + field-wise patching) in qemu, and then all exported over fw_cfg as verbatim tables, my SMBIOS series currently pending for OVMF should be able to install them. This would save OVMF the coding of templates (and any necessary patching) for types 3, 4 (especially nasty), 9, 16, 17, 19, and 32. (Basically all except type 0 and type 1, which are already implemented (but verbatim tables from qemu would take priority even for type 0 and type 1). Type 7 can be left out apparently; IIRC dmidecode doesn't report it even under SeaBIOS.) I'm not implying anyone should start working on this (myself included :)), but yeah, moving SMBIOS would save work in OVMF. (Provided there was any reason to support said SMBIOS tables in OVMF :)) Thanks, Laszlo
Re: [Qemu-devel] [PATCH trivial] gitignore: unignore *.patch
On 06/12/13 13:51, Michael Tokarev wrote: 06.06.2013 01:22, Peter Maydell wrote: Personally I think a lot of the random rubbish in our .gitignore is bogus and should be removed. Basically anything that's an editor dropping or .patch or TAGS file or similar is a local workflow thing and should be dealt with by setting up a global ignorefile in your local git config. The only things in the .gitignore in the public repo should be files which QEMU's build process itself creates. But I know not everybody agrees with this, which is why all this stuff is in the .gitignore :-) I'm one of the few who completely agrees, there's no place for various random rubbery in there, only things which are really generated during build should be there. I'd really love to remove all the cruft. But who else disagrees? I agree that .gitignore should list only files produced by the build process. Anything else can be added to a global ignore file (which I haven't heard of before, but it probably does exist :)), *or* to .git/info/exclude (recommended by Eric Blake to me some months back) This file is not tracked like .gitignore, but it doesn't need tracking. Laszlo
Re: [Qemu-devel] guest-dump-memory
On 10/14/13 10:18, Phi Debian wrote: Hi All, I tried to subscribe to this list, but never got teh confirm mail. So I write here non subscribed, so add my email addr in replies. I am trying to use guest-dump-memory on arm (arm32, armv7* name it) with qemu 1.6.1. The command is 'implemented' i.e listed in the help, but it create an empty file and a replis saying dump-gues-memory is not implemented. The logs says that ARM implement dump-guest-memory since 1.2 http://wiki.qemu.org/ChangeLog/1.2 Is it working ? or did I missed a configuration option? There are some target-specific functions that qmp_dump_guest_memory() (and its callees) depend on. For example, see stubs/dump.c: - cpu_get_dump_info() - cpu_get_note_size() and qom/cpu.c: - cpu_write_elf64_note() - etc. These have a real implementation for the i386 and s390x targets only. I don't know why the 1.2 changelog stated otherwise. Probably a typo / unwanted paste from the x86 section. Laszlo
[Qemu-devel] [PATCH] move test-* from .gitignore to tests/.gitignore
Also sort the test-* entries in the latter. Signed-off-by: Laszlo Ersek ler...@redhat.com --- .gitignore | 9 - tests/.gitignore | 11 +-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 8e1b73f..7f4c106 100644 --- a/.gitignore +++ b/.gitignore @@ -45,15 +45,6 @@ qemu-bridge-helper qemu-monitor.texi vscclient QMP/qmp-commands.txt -test-bitops -test-coroutine -test-int128 -test-opts-visitor -test-qmp-input-visitor -test-qmp-output-visitor -test-string-input-visitor -test-string-output-visitor -test-visitor-serialization fsdev/virtfs-proxy-helper fsdev/virtfs-proxy-helper.1 fsdev/virtfs-proxy-helper.pod diff --git a/tests/.gitignore b/tests/.gitignore index 425757c..dbed60f 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -6,20 +6,27 @@ check-qlist check-qstring test-aio test-bitops -test-throttle +test-coroutine test-cutils test-hbitmap test-int128 test-iov test-mul64 +test-opts-visitor test-qapi-types.[ch] test-qapi-visit.[ch] test-qdev-global-props -test-qmp-commands.h test-qmp-commands +test-qmp-commands.h test-qmp-input-strict +test-qmp-input-visitor test-qmp-marshal.c +test-qmp-output-visitor +test-string-input-visitor +test-string-output-visitor test-thread-pool +test-throttle +test-visitor-serialization test-x86-cpuid test-xbzrle *-test -- 1.8.3.1
Re: [Qemu-devel] [PATCH] move test-* from .gitignore to tests/.gitignore
On 10/15/13 16:11, Eric Blake wrote: On 10/15/2013 07:10 AM, Laszlo Ersek wrote: Also sort the test-* entries in the latter. Signed-off-by: Laszlo Ersek ler...@redhat.com --- .gitignore | 9 - tests/.gitignore | 11 +-- 2 files changed, 9 insertions(+), 11 deletions(-) This feels backwards to me. I'd much rather have a single top-level .gitignore for everything. We are no longer in the CVS days of having a per-directory ignore file. Yes, I'm aware. I re-read your previous comment before posting: http://thread.gmane.org/gmane.comp.emulators.qemu/228886/focus=229185 and I agree with it in theory. However, current qemu practice (even recent patches, eg. 9dbb52e8) seem to point in the opposite direction. I don't feel strongly about this -- I just promised earlier to do it and I want it off my shouler. Thanks! Laszlo
[Qemu-devel] QueuePFN peculiarity in virtio-mmio
Hi, Appendix X: virtio-mmio in the virtio spec says • 0x040 | RW | QueuePFN [...] When the Guest stops using the queue it must write zero (0x0) to this register. [...] and Virtqueue Configuration [...] 2. Check if the queue is not already in use: read QueuePFN register, returned value should be zero (0x0). [...] I think this in itself is already suboptimal, because a guest that crashes and reboots (while the emulator itself survives) will not be able to use the device after said reboot (it has never re-set QueuePFN to zero). But, more importantly: I think that resetting the device (by writing 0 to its status register) should include (ie. *guarantee*) the effects of setting QueuePFN to zero for all imaginable queues of the device. This way, a defensive guest that starts up by resetting the device (*) after identifying it via MagicValue / Version / DeviceID / VendorID would be able to use the device regardless of the device's prior QueuePFN setting(s). (*) Resetting the device is the first step in 2.2.1 Device Initialization Sequence. It is not required on initial start up, but as a guest driver can never be sure whether the startup in question is the initial one, a defensive driver will always start with device reet. The question arises because Olivier has posted a series to edk2-devel that adds virtio-mmio support to TianoCore, and Mark tested it (using OVMF) with a Linux guest and found problems. Namely, OVMF itself can drive the virtio devices via virtio-mmio, but the Linux kernel booted from OVMF can not. The reason is the missing zeroing of QueuePFN when OVMF is exiting. (I'm just paraphrasing the analysis.) I think - that resetting the device (via its status register) should make the host forget *all* prior configuration, including QueuePFN, - and that the Linux driver should reset the device as first step. So: - What's the motivation for the acquire/release semantics of QueuePFN? - Am I right that device reset should force a QueuePFN release too? Thanks, Laszlo
Re: [Qemu-devel] QueuePFN peculiarity in virtio-mmio
My apologies, I used Anthony's previous (now obsolete) email. Updated it now keeping full context below. Sorry. On 10/22/13 19:49, Laszlo Ersek wrote: Hi, Appendix X: virtio-mmio in the virtio spec says • 0x040 | RW | QueuePFN [...] When the Guest stops using the queue it must write zero (0x0) to this register. [...] and Virtqueue Configuration [...] 2. Check if the queue is not already in use: read QueuePFN register, returned value should be zero (0x0). [...] I think this in itself is already suboptimal, because a guest that crashes and reboots (while the emulator itself survives) will not be able to use the device after said reboot (it has never re-set QueuePFN to zero). But, more importantly: I think that resetting the device (by writing 0 to its status register) should include (ie. *guarantee*) the effects of setting QueuePFN to zero for all imaginable queues of the device. This way, a defensive guest that starts up by resetting the device (*) after identifying it via MagicValue / Version / DeviceID / VendorID would be able to use the device regardless of the device's prior QueuePFN setting(s). (*) Resetting the device is the first step in 2.2.1 Device Initialization Sequence. It is not required on initial start up, but as a guest driver can never be sure whether the startup in question is the initial one, a defensive driver will always start with device reet. The question arises because Olivier has posted a series to edk2-devel that adds virtio-mmio support to TianoCore, and Mark tested it (using OVMF) with a Linux guest and found problems. Namely, OVMF itself can drive the virtio devices via virtio-mmio, but the Linux kernel booted from OVMF can not. The reason is the missing zeroing of QueuePFN when OVMF is exiting. (I'm just paraphrasing the analysis.) I think - that resetting the device (via its status register) should make the host forget *all* prior configuration, including QueuePFN, - and that the Linux driver should reset the device as first step. So: - What's the motivation for the acquire/release semantics of QueuePFN? - Am I right that device reset should force a QueuePFN release too? Thanks, Laszlo
Re: [Qemu-devel] [edk2] QueuePFN peculiarity in virtio-mmio
On 10/22/13 19:55, Laszlo Ersek wrote: The question arises because Olivier has posted a series to edk2-devel that adds virtio-mmio support to TianoCore, and Mark tested it (using OVMF) with a Linux guest and found problems. Namely, OVMF itself can drive the virtio devices via virtio-mmio, but the Linux kernel booted from OVMF can not. The reason is the missing zeroing of QueuePFN when OVMF is exiting. (I'm just paraphrasing the analysis.) s/OVMF/AArch64 foundation model/g http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4411 I should have followed my own advice, not to post when sick. I'll go now and hide in a cave. Laszlo /facepalm
Re: [Qemu-devel] [PATCH] Python-lang gdb script to extract x86_64 guest vmcore from qemu coredump
On 10/11/13 19:54, Stefan Hajnoczi wrote: On Thu, Sep 12, 2013 at 9:46 PM, Laszlo Ersek ler...@redhat.com wrote: When qemu dies unexpectedly, for example in response to an explicit abort() call, or (more importantly) when an external signal is delivered to it that results in a coredump, sometimes it is useful to extract the guest vmcore from the qemu process' memory image. The guest vmcore might help understand an emulation problem in qemu, or help debug the guest. This script reimplements (and cuts many features of) the qmp_dump_guest_memory() command in gdb/Python, https://sourceware.org/gdb/current/onlinedocs/gdb/Python-API.html working off the saved memory image of the qemu process. The docstring in the patch (serving as gdb help text) describes the limitations relative to the QMP command. Dependencies of qmp_dump_guest_memory() have been reimplemented as needed. I sought to follow the general structure, sticking to original function names where possible. However, keeping it simple prevailed in some places. The patch has been tested with a 4 VCPU, 768 MB, RHEL-6.4 (2.6.32-358.el6.x86_64) guest: I tried this out with qemu-kvm-1.1.2 and it worked after a few minor tweaks due to memory data structure changes. I did hit a problem with crash since the vmlinux was 32-bit and the guest dump was 64-bit. But from what I can tell dump-guest-memory works as advertised. Reviewed-by: Stefan Hajnoczi stefa...@redhat.com Could someone please pick up the patch? Thanks! Laszlo
Re: [Qemu-devel] Prohibit Windows from running in QEMU
On 10/29/13 11:50, Peter Lieven wrote: On 29.10.2013 11:48, Paolo Bonzini wrote: Il 29/10/2013 11:40, Peter Lieven ha scritto: The KVM signature should be at CPUID leaf 0x4100. If I enable hyperv for all vServers the signature is at KVM_CPUID_SIGNATURE_NEXT (0x4100) otherwise at KVM_CPUID_SIGNATURE (0x0). KVM_CPU_ID_SIGNATURE is 0x4000. Does this matter to Linux? For recent versions it doesn't. Older versions will not be able to use kvmclock (and other PV enhancements for KVM such as steal time or PV EOI). Ok, so this is not an option today - maybe later... Any other idea to detect Windows is running or trying to start? I don't know what I'm talking about. But: - Maybe tracing MSR accesses could give you a profile. - Windows' ACPI parser is super cranky. You could pass in a custom (but standardized) ACPI table on the command line (-acpitable) that only triggers some warnings in Linux's port of ACPICA, but crashes Windows (BSOD). Like, write compile a simple table to AML, then mess it up (eg. Package encoding or some such) with a hex editor. This would take some experimentation as well, but searching existing bug reports could help. Laszlo
Re: [Qemu-devel] [PATCH 2/2] add some virtio-scsi trace events
On 09/04/13 16:21, Stefan Hajnoczi wrote:
On Thu, Aug 29, 2013 at 03:37:50PM +0200, Laszlo Ersek wrote:
+static void dump_cmd_req(const VirtIOSCSIReq *req, uint32_t cdb_size)
+{
+const VirtIOSCSICmdReq *cr;
+char *cdb_hex;
+
+if (!trace_event_get_state(TRACE_VIRTIO_SCSI_DUMP_CMD_REQ)) {
+return;
+}
+cr = req-req.cmd;
+cdb_hex = qemu_hexstr(cr-cdb, cdb_size, NULL);
+trace_virtio_scsi_dump_cmd_req((void *)req, cr-tag,
+ virtio_scsi_get_lun((uint8_t *)cr-lun),
+ cdb_hex);
+g_free(cdb_hex);
There is a halfway solution to disable expensive trace events that works
across all backends (SystemTap, stderr, etc):
if (!TRACE_VIRTIO_SCSI_DUMP_CMD_REQ_ENABLED) {
return;
}
This is a compile-time constant which can be toggled with the disable
keyword in the ./trace-events file:
disable my_expensive_event(const char *foo) foo %s
See the bottom of docs/tracing.txt for full documentation on the
disable keyword.
Yes, I've read it :)
The compile-time constant is already used (just not in an if() but in a
surrounding #if, snipped from the context above), and the corresponding
tracepoints are already disabled in ./trace-events.
I do read documentation if it exists.
I guess I'll rework this sometime later.
Thanks,
Laszlo
Re: [Qemu-devel] [PATCH] vl.c: Implement SIGILL signal handler for triggering SIGSEGV
On 09/05/13 15:26, Paolo Bonzini wrote: Il 05/09/2013 14:19, Michal Novotny ha scritto: This is the patch to introduce SIGILL handler to be able to trigger SIGSEGV signal in qemu. This has been written to help debugging state when qemu crashes by SIGSEGV as a simple reproducer to emulate such situation in case of need. What's wrong with kill -11 or, within gdb, j *0x1234? Why do you need a SIGILL handler for this? In fact, SIGILL is a pretty bad choice: QEMU includes a JIT compiler, so a SIGILL is a relatively common thing to happen while debugging it. Also: (1) there is a known bug in qemu-thread-posix.c, which should not block SIGILL, SIGBUS, SIGSEGV, SIGFPE and SIGSYS. Without fixing that, this trick will only work for the iothread and not for the VCPU threads. If you can produce a patch for this, it would be very nice. +int *p = NULL; + +*p = 0xDEADBEEF; (2) This is undefined behavior. You probably want something like volatile int *p = (volatile int *)(intptr_t)4; instead. What's wrong with raise(SIGSEGV)? I don't understand the motivation BTW -- what sense does it make to turn SIGILL into SIGSEGV? If someone just wants to force a coredump due to signal interactively, SIGQUIT was invented *exactly* for that. You can even send it from the controlling terminal directly, with Ctrl-\. (More precisely, by entering QUIT character, see eg. the stty manual.) (Also, in this specific case it would be no problem if all but one threads blocked SIGQUIT -- the terminal driver or the kill utility would generate the signal for the entire process, not a specific thread, and then the signal would be delivered to some thread among those threads that are not blocking the signal.) Laszlo
Re: [Qemu-devel] [PATCH vgabios] Make windows8 work with high resolution when using -vga std in qmeu
On 09/10/13 05:24, Bo Yang wrote: This patch has been sent to upstream vgabios maillist, but there is no response. Since it is useful for windows8 resolution, I resend it to qemu maillist for review. Signed-off-by: Bo Yang boy...@suse.com --- vbe.c | 42 ++ 1 files changed, 38 insertions(+), 4 deletions(-) SeaBIOS / SeaVGABIOS is separate from the asm-ized vgabios; this patch doesn't apply AFAICS. Regarding the win8 resolution problem, see this seabios thread: http://www.seabios.org/pipermail/seabios/2013-September/006875.html Laszlo
[Qemu-devel] [PATCH] Python-lang gdb script to extract x86_64 guest vmcore from qemu coredump
was configured as x86_64-unknown-linux-gnu...
KERNEL: /usr/lib/debug/lib/modules/2.6.32-358.el6.x86_64/vmlinux
-DUMPFILE: /home/lacos/tmp/guest.vmcore
+DUMPFILE: /home/lacos/tmp/example.dump
CPUS: 4
-DATE: Thu Sep 12 17:16:11 2013
- UPTIME: 00:01:09
-LOAD AVERAGE: 0.07, 0.03, 0.00
+DATE: Thu Sep 12 17:17:41 2013
+ UPTIME: 00:00:38
+LOAD AVERAGE: 0.18, 0.05, 0.01
TASKS: 130
NODENAME: localhost.localdomain
RELEASE: 2.6.32-358.el6.x86_64
@@ -38,12 +38,12 @@
COMMAND: swapper
TASK: 81a8d020 (1 of 4) [THREAD_INFO: 81a0]
CPU: 0
- STATE: TASK_RUNNING (PANIC)
+ STATE: TASK_RUNNING (ACTIVE)
+ WARNING: panic task not found
crash bt
PID: 0 TASK: 81a8d020 CPU: 0 COMMAND: swapper
- #0 [81a01ed0] default_idle at 8101495d
- #1 [81a01ef0] cpu_idle at 81009fc6
+ #0 [81a01ef0] cpu_idle at 81009fc6
crash task 81a8d020
PID: 0 TASK: 81a8d020 CPU: 0 COMMAND: swapper
struct task_struct {
@@ -75,7 +75,7 @@
prev = 0x81a8d080
},
on_rq = 0,
-exec_start = 8618466836,
+exec_start = 7469214014,
sum_exec_runtime = 0,
vruntime = 0,
prev_sum_exec_runtime = 0,
@@ -149,7 +149,7 @@
},
tasks = {
next = 0x88002d621948,
-prev = 0x880029618f28
+prev = 0x880023b74488
},
pushable_tasks = {
prio = 140,
@@ -165,7 +165,7 @@
}
},
mm = 0x0,
- active_mm = 0x88002929b780,
+ active_mm = 0x8800297eb980,
exit_state = 0,
exit_code = 0,
exit_signal = 0,
@@ -177,7 +177,7 @@
sched_reset_on_fork = 0,
pid = 0,
tgid = 0,
- stack_canary = 2483693585637059287,
+ stack_canary = 7266362296181431986,
real_parent = 0x81a8d020,
parent = 0x81a8d020,
children = {
@@ -224,14 +224,14 @@
set_child_tid = 0x0,
clear_child_tid = 0x0,
utime = 0,
- stime = 3,
+ stime = 2,
utimescaled = 0,
- stimescaled = 3,
+ stimescaled = 2,
gtime = 0,
prev_utime = 0,
prev_stime = 0,
nvcsw = 0,
- nivcsw = 1000,
+ nivcsw = 1764,
start_time = {
tv_sec = 0,
tv_nsec = 0
- name_droppingI asked for Dave Anderson's help with verifying the
extracted vmcore, and his comments make me think I should post
this./name_dropping
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
scripts/dump-guest-memory.py | 336 ++
1 files changed, 336 insertions(+), 0 deletions(-)
create mode 100644 scripts/dump-guest-memory.py
diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
new file mode 100644
index 000..e66315a
--- /dev/null
+++ b/scripts/dump-guest-memory.py
@@ -0,0 +1,336 @@
+# This python script adds a new gdb command, dump-guest-memory. It
+# should be loaded with source dump-guest-memory.py at the (gdb)
+# prompt.
+#
+# Copyright (C) 2013, Red Hat, Inc.
+#
+# Authors:
+# Laszlo Ersek ler...@redhat.com
+#
+# The leading docstring doesn't have idiomatic Python formatting. It is
+# printed by gdb's help command (the first line is printed in the
+# help data summary), and it should match how other help texts look in
+# gdb.
+
+import struct
+
+class DumpGuestMemory(gdb.Command):
+Extract guest vmcore from qemu process coredump.
+
+The sole argument is FILE, identifying the target file to write the
+guest vmcore to.
+
+This GDB command reimplements the dump-guest-memory QMP command in
+python, using the representation of guest memory as captured in the qemu
+coredump. The qemu process that has been dumped must have had the
+command line option -machine dump-guest-core=on.
+
+For simplicity, the paging, begin and end parameters of the QMP
+command are not supported -- no attempt is made to get the guest's
+internal paging structures (ie. paging=false is hard-wired), and guest
+memory is always fully dumped.
+
+Only x86_64 guests are supported.
+
+The CORE/NT_PRSTATUS and QEMU notes (that is, the VCPUs' statuses) are
+not written to the vmcore. Preparing these would require context that is
+only present in the KVM host kernel module when the guest is alive. A
+fake ELF note is written instead, only to keep the ELF parser of crash
+happy.
+
+Dependent on how busted the qemu process was at the time of the
+coredump, this command might produce unpredictable results. If qemu
+deliberately called abort(), or it was dumped in response to a signal at
+a halfway fortunate point, then its coredump should be in reasonable
+shape and this command should mostly work.
+
+TARGET_PAGE_SIZE = 0x1000
+TARGET_PAGE_MASK = 0xF000
+
+# Various ELF constants
+EM_X86_64 = 62# AMD x86-64 target machine
+ELFDATA2LSB = 1 # little endian
+ELFCLASS64 = 2
+ELFMAG = \x7FELF
+EV_CURRENT = 1
+ET_CORE = 4
Re: [Qemu-devel] [PATCH] Python-lang gdb script to extract x86_64 guest vmcore from qemu coredump
On 09/12/13 21:46, Laszlo Ersek wrote: When qemu dies unexpectedly, for example in response to an explicit abort() call, or (more importantly) when an external signal is delivered to it that results in a coredump, sometimes it is useful to extract the guest vmcore from the qemu process' memory image. The guest vmcore might help understand an emulation problem in qemu, or help debug the guest. This script reimplements (and cuts many features of) the qmp_dump_guest_memory() command in gdb/Python, https://sourceware.org/gdb/current/onlinedocs/gdb/Python-API.html working off the saved memory image of the qemu process. The docstring in the patch (serving as gdb help text) describes the limitations relative to the QMP command. Dependencies of qmp_dump_guest_memory() have been reimplemented as needed. I sought to follow the general structure, sticking to original function names where possible. However, keeping it simple prevailed in some places. Ping. I'd like to get this upstream because that would help with keeping it in sync with the memory API / internals. Boring as hell to review (you'd have to follow through most of the dump-guest-memory implementation, down to the memory internals; and probably read the gdb python API docs). OTOH it can't hurt anything (separate python file in the scripts directory), so maybe it could go in just with an Acked-by from someone. Thanks Laszlo
Re: [Qemu-devel] [PATCH v4 12/23] acpi: add rules to compile ASL source
On 09/23/13 15:39, Michael S. Tsirkin wrote: On Mon, Sep 23, 2013 at 02:36:41PM +0200, Paolo Bonzini wrote: Il 22/09/2013 15:37, Michael S. Tsirkin ha scritto: Detect presence of IASL compiler and use it to process ASL source. If not there, use pre-compiled files in-tree. Add script to update the in-tree files. Note: distros are known to silently update iasl so detect correct iasl flags for the installed version on each run as opposed to at configure time. This is not any different from a C compiler, which is likely updated much much more often than iasl. Yes but it's not a theoretical issue: we did catch iasl changing flags semantics on the fly, once. I think compilers don't do this as a norm :) (Fully tangentially... gcc and clang emit warnings for a new set of code constructs every other week. When combined with -Werror, it's super annoying; valid and intentional code can stop to build unexpectedly. Good thing we have --disable-werror for this.) Laszlo
[Qemu-devel] [PATCH 0/8] OptsVisitor: support / flatten integer ranges for repeating options
rfc-v1:
- addressed Paolo's comments for patches 1 and 2,
- patches 7 and 8 are new (unit tests),
- updated the cover letter to take native lists into account, plus
cleaned it up.
Consider the following QAPI schema fragment, for the purpose of command
line parsing with OptsVisitor:
{ 'union': 'NumaOptions',
'data': {
'node': 'NumaNodeOptions',
'mem' : 'NumaMemOptions' }}
{ 'type': 'NumaNodeOptions',
'data': {
'*nodeid': 'int',
'*cpus' : ['uint16'] }}
{ 'type': 'NumaMemOptions',
'data': {
'*nodeid': 'int',
'*size' : 'size' }}
(Commit eb7ee2cb (qapi: introduce OptsVisitor) had originally
documented OptsVisitor's general schema requirements for parsing
repeated options such that the list element type had to be a struct with
one mandatory scalar field. Accordingly, the RFC version of this series
required for interval flattening that this underlying scalar type be an
integer type. However, since commit a678e26c (qapi: pad GenericList
value fields to 64 bits) we've had reliable native lists; OptsVisitor
turns out to support them automatically.)
OptsVisitor already accepts the following command line with the above
schema:
-numa node,nodeid=3,cpus=0,cpus=1,cpus=2,cpus=6,cpus=7,cpus=8
Paolo suggested in
http://thread.gmane.org/gmane.comp.emulators.qemu/222589/focus=222732
that OptsVisitor should allow the following shortcut:
-numa node,nodeid=3,cpus=0-2,cpus=6-8
and that the code processing the cpus list should encounter all six
elements (0, 1, 2, 6, 7, 8) individually.
The series implements this feature. Both signed and unsigned values and
intervals are supported in general:
* 0 (zero)
* 1-5 (one to five)
* 4-4 (four to four, range with one element)
* -2(minus two)
* -5-8 (minus five to plus eight)
* -9--6 (minus nine to minus six)
The restrictions imposed by the native list element's signedness and
size (in the above schema example, 'uint16') are enforced element-wise
as usual. That is, for 'uint16', the command line option
-numa node,nodeid=3,cpus=65534-65537
is equivalent to
-numa node,nodeid=3,cpus=65534,cpus=65535,cpus=65536,cpus=65537
and visit_type_uint16() [qapi/qapi-visit-core.c] will catch the first
element (= 65536) that has been parsed by opts_type_int() but cannot be
represented as 'uint16'.
Laszlo Ersek (8):
OptsVisitor: introduce basic list modes
OptsVisitor: introduce list modes for interval flattening
OptsVisitor: opts_type_int(): recognize intervals when LM_IN_PROGRESS
OptsVisitor: rebase opts_type_uint64() to parse_uint_full()
OptsVisitor: opts_type_uint64(): recognize intervals when
LM_IN_PROGRESS
OptsVisitor: don't try to flatten overlong integer ranges
add test-int128 to .gitignore
OptsVisitor: introduce unit tests, with test cases for range
flattening
tests/Makefile |6 +-
qapi-schema-test.json | 15 +++
include/qapi/opts-visitor.h |6 +
qapi/opts-visitor.c | 184 -
tests/test-opts-visitor.c | 275 +++
.gitignore |2 +
6 files changed, 456 insertions(+), 32 deletions(-)
create mode 100644 tests/test-opts-visitor.c
[Qemu-devel] [PATCH 3/8] OptsVisitor: opts_type_int(): recognize intervals when LM_IN_PROGRESS
When a well-formed range value, bounded by signed integers, is encountered
while processing a repeated option, enter LM_SIGNED_INTERVAL and return
the low bound.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
qapi/opts-visitor.c | 34 --
1 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index c2a57bd..90be583 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -367,15 +367,37 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name,
Error **errp)
}
str = opt-str ? opt-str : ;
+/* we've gotten past lookup_scalar() */
+assert(ov-list_mode == LM_NONE || ov-list_mode == LM_IN_PROGRESS);
+
errno = 0;
val = strtoll(str, endptr, 0);
-if (*str != '\0' *endptr == '\0' errno == 0 INT64_MIN = val
-val = INT64_MAX) {
-*obj = val;
-processed(ov, name);
-return;
+if (errno == 0 endptr str INT64_MIN = val val = INT64_MAX) {
+if (*endptr == '\0') {
+*obj = val;
+processed(ov, name);
+return;
+}
+if (*endptr == '-' ov-list_mode == LM_IN_PROGRESS) {
+long long val2;
+
+str = endptr + 1;
+val2 = strtoll(str, endptr, 0);
+if (errno == 0 endptr str *endptr == '\0'
+INT64_MIN = val2 val2 = INT64_MAX val = val2) {
+ov-range_next.s = val;
+ov-range_limit.s = val2;
+ov-list_mode = LM_SIGNED_INTERVAL;
+
+/* as if entering on the top */
+*obj = ov-range_next.s;
+return;
+}
+}
}
-error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt-name, an int64 value);
+error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt-name,
+ (ov-list_mode == LM_NONE) ? an int64 value :
+ an int64 value or range);
}
--
1.7.1
[Qemu-devel] [PATCH 1/8] OptsVisitor: introduce basic list modes
We're going to need more state while processing a list of repeated
options. This change eliminates repeated_opts_first and adds a new state
variable:
list_mode repeated_opts repeated_opts_first
-- - ---
LM_NONE NULL false
LM_STARTED non-NULL true
LM_IN_PROGRESS non-NULL false
Additionally, it is documented that lookup_scalar() and processed(), both
called by opts_type_XXX(), are invalid in LM_STARTED -- generated qapi
code calls opts_next_list() to allocate the very first link before trying
to parse a scalar into it. List mode restrictions are expressed in
positive / inclusive form.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
rfc-v1:
- replace assert(0) with abort() for when opts_next_list() encounters a
nonexistent / invalid list mode [Paolo]
qapi/opts-visitor.c | 45 +++--
1 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 174bd8b..29fd1ab 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -1,7 +1,7 @@
/*
* Options Visitor
*
- * Copyright Red Hat, Inc. 2012
+ * Copyright Red Hat, Inc. 2012, 2013
*
* Author: Laszlo Ersek ler...@redhat.com
*
@@ -18,6 +18,15 @@
#include qapi/visitor-impl.h
+enum ListMode
+{
+LM_NONE, /* not traversing a list of repeated options */
+LM_STARTED, /* opts_start_list() succeeded */
+LM_IN_PROGRESS /* opts_next_list() has been called */
+};
+
+typedef enum ListMode ListMode;
+
struct OptsVisitor
{
Visitor visitor;
@@ -35,8 +44,8 @@ struct OptsVisitor
/* The list currently being traversed with opts_start_list() /
* opts_next_list(). The list must have a struct element type in the
* schema, with a single mandatory scalar member. */
+ListMode list_mode;
GQueue *repeated_opts;
-bool repeated_opts_first;
/* If opts_root-id is set, reinstantiate it as a fake QemuOpt for
* uniformity. Only its name and str fields are set. fake_id_opt does
@@ -156,9 +165,11 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
/* we can't traverse a list in a list */
-assert(ov-repeated_opts == NULL);
+assert(ov-list_mode == LM_NONE);
ov-repeated_opts = lookup_distinct(ov, name, errp);
-ov-repeated_opts_first = (ov-repeated_opts != NULL);
+if (ov-repeated_opts != NULL) {
+ov-list_mode = LM_STARTED;
+}
}
@@ -168,10 +179,13 @@ opts_next_list(Visitor *v, GenericList **list, Error
**errp)
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
GenericList **link;
-if (ov-repeated_opts_first) {
-ov-repeated_opts_first = false;
+switch (ov-list_mode) {
+case LM_STARTED:
+ov-list_mode = LM_IN_PROGRESS;
link = list;
-} else {
+break;
+
+case LM_IN_PROGRESS: {
const QemuOpt *opt;
opt = g_queue_pop_head(ov-repeated_opts);
@@ -180,6 +194,11 @@ opts_next_list(Visitor *v, GenericList **list, Error
**errp)
return NULL;
}
link = (*list)-next;
+break;
+}
+
+default:
+abort();
}
*link = g_malloc0(sizeof **link);
@@ -192,14 +211,16 @@ opts_end_list(Visitor *v, Error **errp)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+assert(ov-list_mode == LM_STARTED || ov-list_mode == LM_IN_PROGRESS);
ov-repeated_opts = NULL;
+ov-list_mode = LM_NONE;
}
static const QemuOpt *
lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
{
-if (ov-repeated_opts == NULL) {
+if (ov-list_mode == LM_NONE) {
GQueue *list;
/* the last occurrence of any QemuOpt takes effect when queried by name
@@ -207,6 +228,7 @@ lookup_scalar(const OptsVisitor *ov, const char *name,
Error **errp)
list = lookup_distinct(ov, name, errp);
return list ? g_queue_peek_tail(list) : NULL;
}
+assert(ov-list_mode == LM_IN_PROGRESS);
return g_queue_peek_head(ov-repeated_opts);
}
@@ -214,9 +236,12 @@ lookup_scalar(const OptsVisitor *ov, const char *name,
Error **errp)
static void
processed(OptsVisitor *ov, const char *name)
{
-if (ov-repeated_opts == NULL) {
+if (ov-list_mode == LM_NONE) {
g_hash_table_remove(ov-unprocessed_opts, name);
+return;
}
+assert(ov-list_mode == LM_IN_PROGRESS);
+/* do nothing */
}
@@ -365,7 +390,7 @@ opts_start_optional(Visitor *v, bool *present, const char
*name,
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
/* we only support a single mandatory scalar field in a list node */
-assert(ov-repeated_opts == NULL);
+assert(ov-list_mode == LM_NONE);
*present = (lookup_distinct(ov, name, NULL) != NULL);
}
--
1.7.1
[Qemu-devel] [PATCH 6/8] OptsVisitor: don't try to flatten overlong integer ranges
Prevent mistyped command line options from incurring high memory and CPU
usage at startup. 64K elements in a range should be enough for everyone
(TM).
The OPTS_VISITOR_RANGE_MAX macro is public so that unit tests can
construct corner cases with it.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
include/qapi/opts-visitor.h |6 ++
qapi/opts-visitor.c |7 +--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
index 5939eee..fd48c14 100644
--- a/include/qapi/opts-visitor.h
+++ b/include/qapi/opts-visitor.h
@@ -16,6 +16,12 @@
#include qapi/visitor.h
#include qemu/option.h
+/* Inclusive upper bound on the size of any flattened range. This is a safety
+ * (= anti-annoyance) measure; wrong ranges should not cause long startup
+ * delays nor exhaust virtual memory.
+ */
+#define OPTS_VISITOR_RANGE_MAX 65536
+
typedef struct OptsVisitor OptsVisitor;
/* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's int
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index d54d373..96ed858 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -384,7 +384,9 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name,
Error **errp)
str = endptr + 1;
val2 = strtoll(str, endptr, 0);
if (errno == 0 endptr str *endptr == '\0'
-INT64_MIN = val2 val2 = INT64_MAX val = val2) {
+INT64_MIN = val2 val2 = INT64_MAX val = val2
+(val INT64_MAX - OPTS_VISITOR_RANGE_MAX ||
+ val2 val + OPTS_VISITOR_RANGE_MAX)) {
ov-range_next.s = val;
ov-range_limit.s = val2;
ov-list_mode = LM_SIGNED_INTERVAL;
@@ -435,7 +437,8 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char
*name, Error **errp)
str = endptr + 1;
if (parse_uint_full(str, val2, 0) == 0
-val2 = UINT64_MAX val = val2) {
+val2 = UINT64_MAX val = val2
+val2 - val OPTS_VISITOR_RANGE_MAX) {
ov-range_next.u = val;
ov-range_limit.u = val2;
ov-list_mode = LM_UNSIGNED_INTERVAL;
--
1.7.1
[Qemu-devel] [PATCH 5/8] OptsVisitor: opts_type_uint64(): recognize intervals when LM_IN_PROGRESS
When a well-formed range value, bounded by unsigned integers, is
encountered while processing a repeated option, enter LM_UNSIGNED_INTERVAL
and return the low bound.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
qapi/opts-visitor.c | 32 +++-
1 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index d8f9a0e..d54d373 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -408,6 +408,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char
*name, Error **errp)
const QemuOpt *opt;
const char *str;
unsigned long long val;
+char *endptr;
if (ov-list_mode == LM_UNSIGNED_INTERVAL) {
*obj = ov-range_next.u;
@@ -420,13 +421,34 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char
*name, Error **errp)
}
str = opt-str;
-if (parse_uint_full(str, val, 0) == 0 val = UINT64_MAX) {
-*obj = val;
-processed(ov, name);
-return;
+/* we've gotten past lookup_scalar() */
+assert(ov-list_mode == LM_NONE || ov-list_mode == LM_IN_PROGRESS);
+
+if (parse_uint(str, val, endptr, 0) == 0 val = UINT64_MAX) {
+if (*endptr == '\0') {
+*obj = val;
+processed(ov, name);
+return;
+}
+if (*endptr == '-' ov-list_mode == LM_IN_PROGRESS) {
+unsigned long long val2;
+
+str = endptr + 1;
+if (parse_uint_full(str, val2, 0) == 0
+val2 = UINT64_MAX val = val2) {
+ov-range_next.u = val;
+ov-range_limit.u = val2;
+ov-list_mode = LM_UNSIGNED_INTERVAL;
+
+/* as if entering on the top */
+*obj = ov-range_next.u;
+return;
+}
+}
}
error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt-name,
- an uint64 value);
+ (ov-list_mode == LM_NONE) ? a uint64 value :
+ a uint64 value or range);
}
--
1.7.1
[Qemu-devel] [PATCH 4/8] OptsVisitor: rebase opts_type_uint64() to parse_uint_full()
Simplify the code in preparation for the next patch.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
qapi/opts-visitor.c | 23 +--
1 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 90be583..d8f9a0e 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -407,6 +407,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char
*name, Error **errp)
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
const QemuOpt *opt;
const char *str;
+unsigned long long val;
if (ov-list_mode == LM_UNSIGNED_INTERVAL) {
*obj = ov-range_next.u;
@@ -417,26 +418,12 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char
*name, Error **errp)
if (!opt) {
return;
}
-
str = opt-str;
-if (str != NULL) {
-while (isspace((unsigned char)*str)) {
-++str;
-}
-
-if (*str != '-' *str != '\0') {
-unsigned long long val;
-char *endptr;
-/* non-empty, non-negative subject sequence */
-errno = 0;
-val = strtoull(str, endptr, 0);
-if (*endptr == '\0' errno == 0 val = UINT64_MAX) {
-*obj = val;
-processed(ov, name);
-return;
-}
-}
+if (parse_uint_full(str, val, 0) == 0 val = UINT64_MAX) {
+*obj = val;
+processed(ov, name);
+return;
}
error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt-name,
an uint64 value);
--
1.7.1
[Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
tests/Makefile|6 +-
qapi-schema-test.json | 15 +++
tests/test-opts-visitor.c | 275 +
.gitignore|1 +
4 files changed, 296 insertions(+), 1 deletions(-)
create mode 100644 tests/test-opts-visitor.c
diff --git a/tests/Makefile b/tests/Makefile
index 425a9a8..8bb290e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -23,6 +23,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
check-unit-y += tests/test-string-output-visitor$(EXESUF)
gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
+check-unit-y += tests/test-opts-visitor$(EXESUF)
+gcov-files-test-opts-visitor-y = qapi/opts-visitor.c
check-unit-y += tests/test-coroutine$(EXESUF)
gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c
check-unit-y += tests/test-visitor-serialization$(EXESUF)
@@ -81,7 +83,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o
tests/check-qdict.o \
tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
tests/test-qmp-commands.o tests/test-visitor-serialization.o \
- tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o
+ tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
+ tests/test-opts-visitor.o
test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -123,6 +126,7 @@ tests/test-qmp-input-visitor$(EXESUF):
tests/test-qmp-input-visitor.o $(test-qap
tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o
$(test-qapi-obj-y) libqemuutil.a libqemustub.a
tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o
tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o
libqemuutil.a libqemustub.a
tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o
$(test-qapi-obj-y) libqemuutil.a libqemustub.a
+tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
libqemuutil.a libqemustub.a
tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a
diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 4434fa3..fe5af75 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -51,3 +51,18 @@
{ 'command': 'user_def_cmd', 'data': {} }
{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
{ 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b':
'UserDefOne'}, 'returns': 'UserDefTwo' }
+
+# For testing integer range flattening in opts-visitor. The following schema
+# corresponds to the option format:
+#
+# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12
+#
+# For simplicity, this example doesn't use [type=]discriminator nor optargs
+# specific to discriminator values.
+{ 'type': 'UserDefOptions',
+ 'data': {
+'*i64' : [ 'int'],
+'*u64' : [ 'uint64' ],
+'*u16' : [ 'uint16' ],
+'*i64x': 'int' ,
+'*u64x': 'uint64' } }
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
new file mode 100644
index 000..9f902b5
--- /dev/null
+++ b/tests/test-opts-visitor.c
@@ -0,0 +1,275 @@
+/*
+ * Options Visitor unit-tests.
+ *
+ * Copyright (C) 2013 Red Hat, Inc.
+ *
+ * Authors:
+ * Laszlo Ersek ler...@redhat.com (based on test-string-output-visitor)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include glib.h
+
+#include qemu/config-file.h /* qemu_add_opts() */
+#include qemu/option.h /* qemu_opts_parse() */
+#include qapi/opts-visitor.h/* opts_visitor_new() */
+#include test-qapi-visit.h /* visit_type_UserDefOptions() */
+#include qapi/dealloc-visitor.h /* qapi_dealloc_visitor_new() */
+
+static QemuOptsList userdef_opts = {
+.name = userdef,
+.head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head),
+.desc = { { 0 } } /* validated with OptsVisitor */
+};
+
+/* fixture (= glib test case context) and test case manipulation */
+
+typedef struct OptsVisitorFixture {
+UserDefOptions *userdef;
+Error *err;
+} OptsVisitorFixture;
+
+
+static void
+setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+const char *opts_string = test_data;
+QemuOpts *opts;
+OptsVisitor *ov;
+
+opts = qemu_opts_parse(qemu_find_opts(userdef), opts_string, 0);
+g_assert(opts != NULL);
+
+ov = opts_visitor_new(opts);
+visit_type_UserDefOptions(opts_get_visitor(ov), f-userdef, NULL,
+ f-err);
+opts_visitor_cleanup(ov);
+qemu_opts_del(opts);
+}
+
+
+static void
+teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+if (f-userdef != NULL) {
+QapiDeallocVisitor *dv;
+
+dv = qapi_dealloc_visitor_new();
+visit_type_UserDefOptions
[Qemu-devel] [PATCH 7/8] add test-int128 to .gitignore
Probably missed in commit 6046c620 (int128: optimize and add test cases). Signed-off-by: Laszlo Ersek ler...@redhat.com --- .gitignore |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index 0fe114d..388cb45 100644 --- a/.gitignore +++ b/.gitignore @@ -46,6 +46,7 @@ qemu-monitor.texi vscclient QMP/qmp-commands.txt test-coroutine +test-int128 test-qmp-input-visitor test-qmp-output-visitor test-string-input-visitor -- 1.7.1
[Qemu-devel] [PATCH 2/8] OptsVisitor: introduce list modes for interval flattening
The new modes are equal-rank, exclusive alternatives of LM_IN_PROGRESS.
Teach opts_next_list(), opts_type_int() and opts_type_uint64() to handle
them.
Also enumerate explicitly what functions are valid to call in what modes:
- opts_next_list() is valid to call while flattening a range,
- opts_end_list(): ditto,
- lookup_scalar() is invalid to call during flattening; generated qapi
traversal code must continue asking for the same kind of signed/unsigned
list element until the interval is fully flattened,
- processed(): ditto.
List mode restrictions are always formulated in positive / inclusive
sense. The restrictions for lookup_scalar() and processed() are
automatically satisfied by current qapi traversals if the schema to build
is compatible with OptsVisitor.
The new list modes are not entered yet.
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
rfc-v1:
- replace sub-modes with alternatives in the commit message,
- document list mode enum constants extensively [Paolo].
qapi/opts-visitor.c | 67 +-
1 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 29fd1ab..c2a57bd 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -22,7 +22,32 @@ enum ListMode
{
LM_NONE, /* not traversing a list of repeated options */
LM_STARTED, /* opts_start_list() succeeded */
-LM_IN_PROGRESS /* opts_next_list() has been called */
+
+LM_IN_PROGRESS, /* opts_next_list() has been called.
+ *
+ * Generating the next list link will consume the most
+ * recently parsed QemuOpt instance of the repeated
+ * option.
+ *
+ * Parsing a value into the list link will examine the
+ * next QemuOpt instance of the repeated option, and
+ * possibly enter LM_SIGNED_INTERVAL or
+ * LM_UNSIGNED_INTERVAL.
+ */
+
+LM_SIGNED_INTERVAL, /* opts_next_list() has been called.
+ *
+ * Generating the next list link will consume the most
+ * recently stored element from the signed interval,
+ * parsed from the most recent QemuOpt instance of the
+ * repeated option. This may consume QemuOpt itself
+ * and return to LM_IN_PROGRESS.
+ *
+ * Parsing a value into the list link will store the
+ * next element of the signed interval.
+ */
+
+LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned interval. */
};
typedef enum ListMode ListMode;
@@ -47,6 +72,15 @@ struct OptsVisitor
ListMode list_mode;
GQueue *repeated_opts;
+/* When parsing a list of repeating options as integers, values of the form
+ * a-b, representing a closed interval, are allowed. Elements in the
+ * range are generated individually.
+ */
+union {
+int64_t s;
+uint64_t u;
+} range_next, range_limit;
+
/* If opts_root-id is set, reinstantiate it as a fake QemuOpt for
* uniformity. Only its name and str fields are set. fake_id_opt does
* not survive or escape the OptsVisitor object.
@@ -185,6 +219,22 @@ opts_next_list(Visitor *v, GenericList **list, Error
**errp)
link = list;
break;
+case LM_SIGNED_INTERVAL:
+case LM_UNSIGNED_INTERVAL:
+link = (*list)-next;
+
+if (ov-list_mode == LM_SIGNED_INTERVAL) {
+if (ov-range_next.s ov-range_limit.s) {
+++ov-range_next.s;
+break;
+}
+} else if (ov-range_next.u ov-range_limit.u) {
+++ov-range_next.u;
+break;
+}
+ov-list_mode = LM_IN_PROGRESS;
+/* range has been completed, fall through in order to pop option */
+
case LM_IN_PROGRESS: {
const QemuOpt *opt;
@@ -211,7 +261,10 @@ opts_end_list(Visitor *v, Error **errp)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
-assert(ov-list_mode == LM_STARTED || ov-list_mode == LM_IN_PROGRESS);
+assert(ov-list_mode == LM_STARTED ||
+ ov-list_mode == LM_IN_PROGRESS ||
+ ov-list_mode == LM_SIGNED_INTERVAL ||
+ ov-list_mode == LM_UNSIGNED_INTERVAL);
ov-repeated_opts = NULL;
ov-list_mode = LM_NONE;
}
@@ -303,6 +356,11 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name,
Error **errp)
long long val;
char *endptr;
+if (ov-list_mode == LM_SIGNED_INTERVAL) {
+*obj = ov-range_next.s;
+return;
+}
+
opt = lookup_scalar(ov, name, errp);
if (!opt) {
return;
@@ -328,6 +386,11
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/23/13 00:04, Eric Blake wrote: On 07/22/2013 03:07 PM, Laszlo Ersek wrote: Signed-off-by: Laszlo Ersek ler...@redhat.com --- tests/Makefile|6 +- qapi-schema-test.json | 15 +++ tests/test-opts-visitor.c | 275 + .gitignore|1 + 4 files changed, 296 insertions(+), 1 deletions(-) create mode 100644 tests/test-opts-visitor.c +add_test(/visitor/opts/i64/val1/errno,expect_fail, + i64=0x8000); +add_test(/visitor/opts/i64/val1/empty,expect_fail, i64=); +add_test(/visitor/opts/i64/val1/trailing, expect_fail, i64=5z); +add_test(/visitor/opts/i64/nonlist, expect_fail, i64x=5-6); +add_test(/visitor/opts/i64/val2/errno,expect_fail, + i64=0x7fff-0x8000); +add_test(/visitor/opts/i64/val2/empty,expect_fail, i64=5-); +add_test(/visitor/opts/i64/val2/trailing, expect_fail, i64=5-6z); +add_test(/visitor/opts/i64/range/empty, expect_fail, i64=6-5); +add_test(/visitor/opts/i64/range/minval, expect_i64_min, + i64=-0x8000--0x8000); +add_test(/visitor/opts/i64/range/maxval, expect_i64_max, + i64=0x7fff-0x7fff); Pretty thorough, although I thought of a couple other ideas to test: i64=5z-6 should fail; i64=5-6-7 should fail I can add them if you insist, but I wrote (and single-stepped all of) the test cases so that all branches added by patches 3, 5 and 6 would be covered. (Some of the final tests in this function are actually redundant, but I liked how they looked :)) For example, i64=5z-6 is no different from i64=5z, in patch 3 both the first added (*endptr == '\0') condition and the (*endptr == '-') fail the same way for both input strings: we never look past the z. Likewise, i64=5-6-7 is the same case as i64=5-6z: both characters after the 6 (ie. - and z) violate the second added (*endptr == '\0') condition in patch 3 the same way. Do you accept this argument? :) Thanks! Laszlo
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/23/13 00:26, Eric Blake wrote: On 07/22/2013 04:24 PM, Laszlo Ersek wrote: Pretty thorough, although I thought of a couple other ideas to test: i64=5z-6 should fail; i64=5-6-7 should fail I can add them if you insist, but I wrote (and single-stepped all of) the test cases so that all branches added by patches 3, 5 and 6 would be covered. (Some of the final tests in this function are actually redundant, but I liked how they looked :)) For example, i64=5z-6 is no different from i64=5z, in patch 3 both the first added (*endptr == '\0') condition and the (*endptr == '-') fail the same way for both input strings: we never look past the z. Likewise, i64=5-6-7 is the same case as i64=5-6z: both characters after the 6 (ie. - and z) violate the second added (*endptr == '\0') condition in patch 3 the same way. Do you accept this argument? :) Yes, I can agree you have 100% code coverage as currently coded. Adding what currently forms redundant cases may avoid future patch-writers from breaking 100% coverage while actually triggering different paths between the cases; but at the same time, we can assume such a future patch-writer would be adding some new feature to the parser, and could expand the testsuite accordingly as part of their efforts. Agreed! So no, I won't insist on a respin :) Thank you very much :) /me bows and scrapes Laszlo
Re: [Qemu-devel] [PATCH v2 repost 8/9] i386: generate pc guest info
On 07/17/13 17:07, Laszlo Ersek wrote: On 07/10/13 15:51, Michael S. Tsirkin wrote: This fills in guest info table with misc information of interest to the guest. Will be used by ACPI table generation code. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- hw/acpi/ich9.c | 7 ++- hw/acpi/piix4.c| 44 +++- hw/i386/Makefile.objs | 2 ++ hw/i386/pc.c | 41 +++-- hw/i386/pc_piix.c | 15 --- hw/i386/pc_q35.c | 10 +++--- hw/isa/lpc_ich9.c | 11 +-- hw/mips/mips_malta.c | 2 +- hw/misc/pvpanic.c | 12 +++- hw/pci-host/q35.c | 1 + include/hw/acpi/ich9.h | 2 +- include/hw/i386/ich9.h | 3 ++- include/hw/i386/pc.h | 37 ++--- 13 files changed, 164 insertions(+), 23 deletions(-) So we won't be fishing in a global pool of information at ACPI table creation time as I had expected / advertized before. Instead any required bits are gradually collected into the guest info structure while creating / configuring the machine. This is likely a better approach; the set of dependencies for all ACPI tables together are tracked explicitly in guest info. Also, we don't collect the bits from the outside, breaching encapsulation of devices; devices publish the bits. Reviewed-by: Laszlo Ersek ler...@redhat.com If I understand correctly, based on the recent comments you got from Gerd and Andreas for this series (in the other, non-repost thread), fishing in the global pool it should be after all, just with a different fishing rod than what I used in my original patch (hw/i386: build ACPI MADT (APIC) for fw_cfg clients). These U-turns in design have proved that I'm not qualified to review this work. So I won't; there's no use in my repeated saying yeah why not to both approaches (which are polar opposites). My apologies. I applaud your perseverance in this matter. Laszlo
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.5.2 Stable released
On 07/25/13 23:44, Michael Roth wrote: The QEMU v1.5.2 stable release is now available at: http://wiki.qemu.org/download/qemu-1.5.2.tar.bz2 This is release is solely to address a security issue (CVE-2013-2231) found in the QEMU Guest Agent on Windows. More details on the nature of the CVE can be found here: http://seclists.org/oss-sec/2013/q3/161 There are 2 minor fixes for qemu-ga for Windows as well, though these are included mainly due to being dependencies of the CVE fix sent upstream. Thanks to Laszlo and the Red Hat security team for identifying/fixing the issue. For identification and analysis Lev Veyde @ RH takes the credit. Thanks, Laszlo
Re: [Qemu-devel] [PATCH 0/8] OptsVisitor: support / flatten integer ranges for repeating options
On 07/22/13 23:07, Laszlo Ersek wrote: rfc-v1: - addressed Paolo's comments for patches 1 and 2, - patches 7 and 8 are new (unit tests), - updated the cover letter to take native lists into account, plus cleaned it up. Will this be considered for 1.7? I'm not sure how Wanlong's NUMA stuff is scheduled, but I think it shouldn't wait for my series' acceptance. (Rebasing -numa range parsing to rely on the new OptsVisitor capability shouldn't be hard -- it's mostly removing client code.) Thanks Laszlo
Re: [Qemu-devel] [PATCH 0/8] OptsVisitor: support / flatten integer ranges for repeating options
On 07/29/13 13:11, Wanlong Gao wrote: On 07/29/2013 07:01 PM, Paolo Bonzini wrote: Il 29/07/2013 11:47, Laszlo Ersek ha scritto: On 07/22/13 23:07, Laszlo Ersek wrote: rfc-v1: - addressed Paolo's comments for patches 1 and 2, - patches 7 and 8 are new (unit tests), - updated the cover letter to take native lists into account, plus cleaned it up. Will this be considered for 1.7? Yes, why not? :) Nice, I'm rebasing my NUMA patch set on this series. Thank you, that's most appreciated. I didn't want to ask for it, but I *have* learned that the best way to sneak in utility stuff is to base new features on them :) (This is how OptsVisitor had gotten in originally BTW; Stefan was working on new network stuff (VLANs vs. hubs IIRC) and he (supposedly) didn't want to wait too long for my series that introduced OptsVisitor and also rebased net option parsing to it (his series was modifying net code too, obviously), so he just applied my stuff :)) Laszlo
[Qemu-devel] [PATCH 0/4] dump-guest-memory: correct the vmcores
(Apologies for the long To: list, I'm including everyone who participated in https://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02607.html). Conceptually, the dump-guest-memory command works as follows: (a) pause the guest, (b) get a snapshot of the guest's physical memory map, as provided by qemu, (c) retrieve the guest's virtual mappings, as seen by the guest (this is where paging=true vs. paging=false makes a difference), (d) filter (c) as requested by the QMP caller, (e) write ELF headers, keying off (b) -- the guest's physmap -- and (d) -- the filtered guest mappings. (f) dump RAM contents, keying off the same (b) and (d), (g) unpause the guest (if necessary). Patch #1 affects step (e); specifically, how (d) is matched against (b), when paging is true, and the guest kernel maps more guest-physical RAM than it actually has. This can be done by non-malicious, clean-state guests (eg. a pristine RHEL-6.4 guest), and may cause libbfd errors due to PT_LOAD entries (coming directly from the guest page tables) exceeding the vmcore file's size. Patches #2 to #4 are independent of the paging option (or, more precisely, affect them equally); they affect (b). Currently input parameter (b), that is, the guest's physical memory map as provided by qemu, is implicitly represented by ram_list.blocks. As a result, steps and outputs dependent on (b) will refer to qemu-internal offsets. Unfortunately, this breaks when the guest-visible physical addresses diverge from the qemu-internal, RAMBlock based representation. This can happen eg. for guests 3.5 GB, due to the 32-bit PCI hole; see patch #4 for a diagram. Patch #2 introduces input parameter (b) explicitly, as a reasonably minimal map of guest-physical address ranges. (Minimality is not a hard requirement here, it just decreases the number of PT_LOAD entries written to the vmcore header.) Patch #3 populates this map. Patch #4 rebases the dump-guest-memory command to it, so that steps (e) and (f) work with guest-phys addresses. As a result, the crash utility can parse vmcores dumped for big x86_64 guests (paging=false). Please refer to Red Hat Bugzilla 981582 https://bugzilla.redhat.com/show_bug.cgi?id=981582. Disclaimer: as you can tell from my progress in the RHBZ, I'm new to the memory API. The way I'm using it might be retarded. Laszlo Ersek (4): dump: clamp guest-provided mapping lengths to ramblock sizes dump: introduce GuestPhysBlockList dump: populate guest_phys_blocks dump: rebase from host-private RAMBlock offsets to guest-physical addresses include/sysemu/dump.h |4 +- include/sysemu/memory_mapping.h | 30 ++- dump.c | 171 +- memory_mapping.c| 174 +-- stubs/dump.c|3 +- target-i386/arch_dump.c | 10 ++- 6 files changed, 300 insertions(+), 92 deletions(-)
[Qemu-devel] [PATCH 3/4] dump: populate guest_phys_blocks
While the machine is paused, in guest_phys_blocks_append() we register a
one-shot MemoryListener, solely for the initial collection of the valid
guest-physical memory ranges that happens at client registration time.
For each range that is reported to guest_phys_blocks_set_memory(), we
attempt to merge the range with adjacent (preceding, subsequent, or both)
ranges. We use two hash tables for this purpose, both indexing the same
ranges, just by different keys (guest-phys-start vs. guest-phys-end).
Ranges can only be joined if they are contiguous in both guest-physical
address space, and contiguous in host virtual address space.
The maximal ranges that remain in the end constitute the guest-physical
memory map that the dump will be based on.
Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=981582
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
include/sysemu/memory_mapping.h |1 +
dump.c |2 +-
memory_mapping.c| 135 +++
3 files changed, 137 insertions(+), 1 deletions(-)
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 53c2cd5..6723dc5 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -66,6 +66,7 @@ void memory_mapping_list_init(MemoryMappingList *list);
void guest_phys_blocks_free(GuestPhysBlockList *list);
void guest_phys_blocks_init(GuestPhysBlockList *list);
+void guest_phys_blocks_append(GuestPhysBlockList *list);
void qemu_get_guest_memory_mapping(MemoryMappingList *list, Error **errp);
diff --git a/dump.c b/dump.c
index 716fb1d..3fa33fc 100644
--- a/dump.c
+++ b/dump.c
@@ -746,7 +746,7 @@ static int dump_init(DumpState *s, int fd, bool paging,
bool has_filter,
s-length = length;
guest_phys_blocks_init(s-guest_phys_blocks);
-/* FILL LIST */
+guest_phys_blocks_append(s-guest_phys_blocks);
s-start = get_start_block(s);
if (s-start == -1) {
diff --git a/memory_mapping.c b/memory_mapping.c
index c70505b..efaabf8 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -11,9 +11,13 @@
*
*/
+#include glib.h
+
#include cpu.h
#include exec/cpu-all.h
#include sysemu/memory_mapping.h
+#include exec/memory.h
+#include exec/address-spaces.h
static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list,
MemoryMapping *mapping)
@@ -182,6 +186,137 @@ void guest_phys_blocks_init(GuestPhysBlockList *list)
QTAILQ_INIT(list-head);
}
+typedef struct GuestPhysListener {
+GHashTable *by_target_start;
+GHashTable *by_target_end;
+MemoryListener listener;
+} GuestPhysListener;
+
+static void guest_phys_blocks_region_add(MemoryListener *listener,
+ MemoryRegionSection *section)
+{
+GuestPhysListener *g;
+uint64_t section_size;
+hwaddr target_start, target_end;
+uint8_t *host_addr;
+GuestPhysBlock *predecessor, *successor, *block;
+bool found;
+
+/* we only care about RAM */
+if (!memory_region_is_ram(section-mr)) {
+return;
+}
+
+g= container_of(listener, GuestPhysListener, listener);
+section_size = int128_get64(section-size);
+target_start = section-offset_within_address_space;
+target_end = target_start + section_size;
+host_addr= memory_region_get_ram_ptr(section-mr) +
+ section-offset_within_region;
+
+/* find continuity in guest physical address space */
+predecessor = g_hash_table_lookup(g-by_target_end, target_start);
+successor = g_hash_table_lookup(g-by_target_start, target_end);
+
+/* we require continuity in host memory too */
+if (predecessor != NULL) {
+hwaddr predecessor_size = predecessor-target_end -
+ predecessor-target_start;
+if (predecessor-host_addr + predecessor_size != host_addr) {
+predecessor = NULL;
+}
+}
+if (successor != NULL
+ host_addr + section_size != successor-host_addr) {
+successor = NULL;
+}
+
+if (predecessor == NULL) {
+if (successor == NULL) {
+/* Isolated mapping, allocate it and add it to both tables. */
+block = g_malloc0(sizeof *block);
+
+block-target_end = target_end;
+g_hash_table_insert(g-by_target_end, block-target_end, block);
+} else {
+/* Mapping has successor only. Merge current into successor by
+ * modifying successor's start. Successor's end doesn't change.
+ */
+block = successor;
+found = g_hash_table_steal(g-by_target_start,
+ block-target_start);
+g_assert(found);
+}
+block-target_start = target_start;
+block-host_addr= host_addr;
+g_hash_table_insert(g-by_target_start, block-target_start
[Qemu-devel] [PATCH 2/4] dump: introduce GuestPhysBlockList
The vmcore must use physical addresses that are visible to the guest, not
addresses that point into linear RAMBlocks. As first step, introduce the
list type into which we'll collect the physical mappings in effect at the
time of the dump.
Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=981582
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
include/sysemu/memory_mapping.h | 22 ++
dump.c | 31 +++
memory_mapping.c| 17 +
3 files changed, 58 insertions(+), 12 deletions(-)
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 6dfb68d..53c2cd5 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -17,6 +17,25 @@
#include qemu/queue.h
#include qemu/typedefs.h
+typedef struct GuestPhysBlock {
+/* visible to guest, reflects PCI hole, etc */
+hwaddr target_start;
+
+/* implies size */
+hwaddr target_end;
+
+/* points into host memory */
+uint8_t *host_addr;
+
+QTAILQ_ENTRY(GuestPhysBlock) next;
+} GuestPhysBlock;
+
+/* point-in-time snapshot of guest-visible physical mappings */
+typedef struct GuestPhysBlockList {
+unsigned num;
+QTAILQ_HEAD(, GuestPhysBlock) head;
+} GuestPhysBlockList;
+
/* The physical and virtual address in the memory mapping are contiguous. */
typedef struct MemoryMapping {
hwaddr phys_addr;
@@ -45,6 +64,9 @@ void memory_mapping_list_free(MemoryMappingList *list);
void memory_mapping_list_init(MemoryMappingList *list);
+void guest_phys_blocks_free(GuestPhysBlockList *list);
+void guest_phys_blocks_init(GuestPhysBlockList *list);
+
void qemu_get_guest_memory_mapping(MemoryMappingList *list, Error **errp);
/* get guest's memory mapping without do paging(virtual address is 0). */
diff --git a/dump.c b/dump.c
index 9a2f939..716fb1d 100644
--- a/dump.c
+++ b/dump.c
@@ -59,6 +59,7 @@ static uint64_t cpu_convert_to_target64(uint64_t val, int
endian)
}
typedef struct DumpState {
+GuestPhysBlockList guest_phys_blocks;
ArchDumpInfo dump_info;
MemoryMappingList list;
uint16_t phdr_num;
@@ -81,6 +82,7 @@ static int dump_cleanup(DumpState *s)
{
int ret = 0;
+guest_phys_blocks_free(s-guest_phys_blocks);
memory_mapping_list_free(s-list);
if (s-fd != -1) {
close(s-fd);
@@ -728,31 +730,34 @@ static int dump_init(DumpState *s, int fd, bool paging,
bool has_filter,
s-resume = false;
}
+/* If we use KVM, we should synchronize the registers before we get dump
+ * info or physmap info.
+ */
+cpu_synchronize_all_states();
+nr_cpus = 0;
+for (cpu = first_cpu; cpu != NULL; cpu = cpu-next_cpu) {
+nr_cpus++;
+}
+
s-errp = errp;
s-fd = fd;
s-has_filter = has_filter;
s-begin = begin;
s-length = length;
+
+guest_phys_blocks_init(s-guest_phys_blocks);
+/* FILL LIST */
+
s-start = get_start_block(s);
if (s-start == -1) {
error_set(errp, QERR_INVALID_PARAMETER, begin);
goto cleanup;
}
-/*
- * get dump info: endian, class and architecture.
+/* get dump info: endian, class and architecture.
* If the target architecture is not supported, cpu_get_dump_info() will
* return -1.
- *
- * If we use KVM, we should synchronize the registers before we get dump
- * info.
*/
-cpu_synchronize_all_states();
-nr_cpus = 0;
-for (cpu = first_cpu; cpu != NULL; cpu = cpu-next_cpu) {
-nr_cpus++;
-}
-
ret = cpu_get_dump_info(s-dump_info);
if (ret 0) {
error_set(errp, QERR_UNSUPPORTED);
@@ -827,6 +832,8 @@ static int dump_init(DumpState *s, int fd, bool paging,
bool has_filter,
return 0;
cleanup:
+guest_phys_blocks_free(s-guest_phys_blocks);
+
if (s-resume) {
vm_start();
}
@@ -874,7 +881,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
bool has_begin,
return;
}
-s = g_malloc(sizeof(DumpState));
+s = g_malloc0(sizeof(DumpState));
ret = dump_init(s, fd, paging, has_begin, begin, length, errp);
if (ret 0) {
diff --git a/memory_mapping.c b/memory_mapping.c
index 515a984..c70505b 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -165,6 +165,23 @@ void memory_mapping_list_init(MemoryMappingList *list)
QTAILQ_INIT(list-head);
}
+void guest_phys_blocks_free(GuestPhysBlockList *list)
+{
+GuestPhysBlock *p, *q;
+
+QTAILQ_FOREACH_SAFE(p, list-head, next, q) {
+QTAILQ_REMOVE(list-head, p, next);
+g_free(p);
+}
+list-num = 0;
+}
+
+void guest_phys_blocks_init(GuestPhysBlockList *list)
+{
+list-num = 0;
+QTAILQ_INIT(list-head);
+}
+
static CPUState *find_paging_enabled_cpu(CPUState *start_cpu)
{
CPUState *cpu;
--
1.7.1
[Qemu-devel] [PATCH 1/4] dump: clamp guest-provided mapping lengths to ramblock sizes
Even a trusted clean-state guest can map more memory than what it was
given. Since the vmcore contains RAMBlocks, mapping sizes should be
clamped to RAMBlock sizes. Otherwise such oversized mappings can exceed
the entire file size, and ELF parsers might refuse even the valid portion
of the PT_LOAD entry.
Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=981582
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
dump.c | 65 +++
1 files changed, 40 insertions(+), 25 deletions(-)
diff --git a/dump.c b/dump.c
index 6a3a72a..9a2f939 100644
--- a/dump.c
+++ b/dump.c
@@ -187,7 +187,8 @@ static int write_elf32_header(DumpState *s)
}
static int write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
-int phdr_index, hwaddr offset)
+int phdr_index, hwaddr offset,
+hwaddr filesz)
{
Elf64_Phdr phdr;
int ret;
@@ -197,15 +198,12 @@ static int write_elf64_load(DumpState *s, MemoryMapping
*memory_mapping,
phdr.p_type = cpu_convert_to_target32(PT_LOAD, endian);
phdr.p_offset = cpu_convert_to_target64(offset, endian);
phdr.p_paddr = cpu_convert_to_target64(memory_mapping-phys_addr, endian);
-if (offset == -1) {
-/* When the memory is not stored into vmcore, offset will be -1 */
-phdr.p_filesz = 0;
-} else {
-phdr.p_filesz = cpu_convert_to_target64(memory_mapping-length,
endian);
-}
+phdr.p_filesz = cpu_convert_to_target64(filesz, endian);
phdr.p_memsz = cpu_convert_to_target64(memory_mapping-length, endian);
phdr.p_vaddr = cpu_convert_to_target64(memory_mapping-virt_addr, endian);
+assert(memory_mapping-length = filesz);
+
ret = fd_write_vmcore(phdr, sizeof(Elf64_Phdr), s);
if (ret 0) {
dump_error(s, dump: failed to write program header table.\n);
@@ -216,7 +214,8 @@ static int write_elf64_load(DumpState *s, MemoryMapping
*memory_mapping,
}
static int write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
-int phdr_index, hwaddr offset)
+int phdr_index, hwaddr offset,
+hwaddr filesz)
{
Elf32_Phdr phdr;
int ret;
@@ -226,15 +225,12 @@ static int write_elf32_load(DumpState *s, MemoryMapping
*memory_mapping,
phdr.p_type = cpu_convert_to_target32(PT_LOAD, endian);
phdr.p_offset = cpu_convert_to_target32(offset, endian);
phdr.p_paddr = cpu_convert_to_target32(memory_mapping-phys_addr, endian);
-if (offset == -1) {
-/* When the memory is not stored into vmcore, offset will be -1 */
-phdr.p_filesz = 0;
-} else {
-phdr.p_filesz = cpu_convert_to_target32(memory_mapping-length,
endian);
-}
+phdr.p_filesz = cpu_convert_to_target32(filesz, endian);
phdr.p_memsz = cpu_convert_to_target32(memory_mapping-length, endian);
phdr.p_vaddr = cpu_convert_to_target32(memory_mapping-virt_addr, endian);
+assert(memory_mapping-length = filesz);
+
ret = fd_write_vmcore(phdr, sizeof(Elf32_Phdr), s);
if (ret 0) {
dump_error(s, dump: failed to write program header table.\n);
@@ -418,17 +414,24 @@ static int write_memory(DumpState *s, RAMBlock *block,
ram_addr_t start,
return 0;
}
-/* get the memory's offset in the vmcore */
-static hwaddr get_offset(hwaddr phys_addr,
- DumpState *s)
+/* get the memory's offset and size in the vmcore */
+static void get_offset_range(hwaddr phys_addr,
+ ram_addr_t mapping_length,
+ DumpState *s,
+ hwaddr *p_offset,
+ hwaddr *p_filesz)
{
RAMBlock *block;
hwaddr offset = s-memory_offset;
int64_t size_in_block, start;
+/* When the memory is not stored into vmcore, offset will be -1 */
+*p_offset = -1;
+*p_filesz = 0;
+
if (s-has_filter) {
if (phys_addr s-begin || phys_addr = s-begin + s-length) {
-return -1;
+return;
}
}
@@ -457,18 +460,26 @@ static hwaddr get_offset(hwaddr phys_addr,
}
if (phys_addr = start phys_addr start + size_in_block) {
-return phys_addr - start + offset;
+*p_offset = phys_addr - start + offset;
+
+/* The offset range mapped from the vmcore file must not spill over
+ * the RAMBlock, clamp it. The rest of the mapping will be
+ * zero-filled in memory at load time; see
+ * http://refspecs.linuxbase.org/elf/gabi4+/ch5.pheader.html.
+ */
+*p_filesz = phys_addr + mapping_length = start + size_in_block ?
+mapping_length :
+size_in_block - (phys_addr - start);
+return;
}
offset += size_in_block
