Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] target/ppc: fmadd check for excp independently

[Programmingkid]

On Mar 3, 2017, at 4:59 AM, qemu-ppc-requ...@nongnu.org wrote:

> 
> Current order of checking does not confirm with the spec
> (ISA 3.0: MultiplyAddDP page-469). Change the order and make them
> independent of each other.
> 
> For example: a = infinity, b = zero, c = SNaN, this should set both
> VXIMZ and VXNAN
> 
> Signed-off-by: Nikunj A Dadhania 
> ---
> target/ppc/fpu_helper.c | 20 
> 1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 0535ad0..a547f58 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -747,17 +747,21 @@ static void float64_maddsub_update_excp(CPUPPCState 
> *env, float64 arg1,
> float64 arg2, float64 arg3,
> unsigned int madd_flags)
> {
> +if (unlikely(float64_is_signaling_nan(arg1, >fp_status) ||
> + float64_is_signaling_nan(arg2, >fp_status) ||
> + float64_is_signaling_nan(arg3, >fp_status))) {
> +/* sNaN operation */
> +float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> +}
> +
> if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
>  (float64_is_zero(arg1) && float64_is_infinity(arg2 {
> /* Multiplication of zero by infinity */
> -arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> -} else if (unlikely(float64_is_signaling_nan(arg1, >fp_status) ||
> -float64_is_signaling_nan(arg2, >fp_status) ||
> -float64_is_signaling_nan(arg3, >fp_status))) {
> -/* sNaN operation */
> -float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> -} else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
> -   float64_is_infinity(arg3)) {
> +float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> +}
> +
> +if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
> +float64_is_infinity(arg3)) {
> uint8_t aSign, bSign, cSign;
> 
> aSign = float64_is_neg(arg1);
> -- 
> 2.7.4


I'm having difficulty applying this patch. I took a look at 
float64_maddsub_update_excp() in fpu_helper.c and found out why. This function 
doesn't exist. Are you missing a patch?

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Jason Wang]

On 2017年03月03日 20:06, Peter Maydell wrote:

On 17 February 2017 at 03:04, Jason Wang  wrote:


On 2017年02月16日 20:29, Dmitry Fleytman wrote:

This series fix a few issues related
to processing of RX packets with VLAN headers.

See commit messages of specific patches
for information regarding affected devices.

Dmitry Fleytman (5):
eth: Extend vlan stripping functions
NetRxPkt: Fix memory corruption on VLAN header stripping
NetRxPkt: Do not try to pull more data than present
NetRxPkt: Account buffer with ETH header in IOV length
NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()

   hw/net/net_rx_pkt.c | 40 +---
   include/net/eth.h   |  4 ++--
   net/eth.c   | 25 ++---
   3 files changed, 37 insertions(+), 32 deletions(-)


Applied. Thanks

Hi Jason -- what's the status of these patches? I don't
see them in master and I don't think you have them in a pull
request yet (though I may have missed it in the deluge of
freeze-deadline pulls...)

thanks
-- PMM



Already queued but haven't sent the pull request.

Plan to send it next Monday.

Thanks

Re: [Qemu-devel] [PATCH 0/7] Introducing libtcg

[Richard Henderson]

On 03/01/2017 04:19 AM, Alessandro Di Federico wrote:

This series of patches is a follow-up to the "Preparing the build system
for libtcg" patch set.

The first six patches' aim is to decouple and factor out some components
so that introducing libtcg can be painless and smooth. The last patch
instead introduces libtcg, along with a set of simple tests.

My main aim is to get the first six patches in mainline as soon as
possible, since rebasing when large amounts of code is moved is quite a
pain. If possible, letting the last patch in would be nice too.

The current implementation of libtcg it's not thread-safe, doesn't allow
to change the CPU, munmap memory pages and the library itself cannot be
unloaded.

Note that if libtcg is enabled in a build directory where it previously
wasn't, a `make clean` is required, since everything has to be built
with `-fPIC -fvisibility=hidden`. Maybe it's possible to have the build
system recognize this automatically (suggestions welcome).

Note also that the factoring process excluded the bsd-user targets,
which would benefit from it (mainly for mmap.c and qemu.h), but are not
in a working state, so I'm leaving them out to avoid making more
damage. Once bsd-user is back in shape in mainline I'll be happy to
include it in the factoring.

For what concerns the test suite, the s390x linux-user target seems to
be broken so I've temporarily excluded it from the test suite. Currently
I'm testing translation only on a subset of architectures (MIPS, ARM and
x86-64). It would be nice to test them all, but manually building all
the toolchains can be a pain. Is there an "official" set of toolchains
ready to use/compile?

For what concerns the PREFIX changes in tcg-common.h, they're not very
elegant. Another approach might be to prefix all the data types, data
structures and enum entries in there with LibTCG/LIBTCG_ even
internally. Opinions?

Thanks.

Alessandro Di Federico (7):
  Factor out linux-user/qemu.h
  Factor out *-user/mmap.c


These two patches I can understand, and seem like a good improvment.


  Move *_cpu_dump_state to translate.c
  *-user targets object files decoupling
  Isolate coprocessor parts from target/arm/helper.c


I have no idea what you're going for here.  How does rearranging anything in 
target/* affect TCG, or creating a library form of TCG?



  Factor out tcg/tcg.h


Most of this looks totally bogus.  Including:

(1) A common TCGReg?  This ought to be totally private between tcg.c and its 
backend.  The only reason this escapes at all right now is the history of 
TCG_AREG0.  Simply adjusting the interface given to target/* would clear that up.


(2) PREFIX(ArgConstraint), PREFIX(TempVal), etc.
Why is these being exported at all?

(3) PREFIX(Opcode), PREFIX3(NB_OPS), PREFIX(Cond), etc.
Why do you think these need to be specialized at all?

Indeed, the entire existence of this patch suggests a fundamental disconnect 
between how you and I interpret the goal and scope of a "libtcg".



  Introduce libtcg infrastructure


And, this confirms it, in the first sentence:


* Extend the build system to build libtcg-$arch.so dynamic libraries.


Why in the world would you want a JIT compiler for something other than the 
host cpu?  There is zero point in building libtcg-mips.so for an x86_64 host.


The only reason to want to pull TCG into a library form is so that another 
project might be able to also generate code on the fly also for the host.



r~

Re: [Qemu-devel] [RFC 0/4] qemu-img: add max-size subcommand

[Nir Soffer]

On Sat, Mar 4, 2017 at 12:15 AM, Nir Soffer  wrote:
> On Sat, Mar 4, 2017 at 12:02 AM, John Snow  wrote:
>>
>>
>> On 03/03/2017 04:38 PM, Nir Soffer wrote:
>>> On Fri, Mar 3, 2017 at 3:51 PM, Stefan Hajnoczi  wrote:

 RFCv1:
  * Publishing patch series with just raw support, no qcow2 yet.  Please 
 review
the command-line interface and let me know if you are happy with this
approach.

 Users and management tools sometimes need to know the size required for a 
 new
 disk image so that an LVM volume, SAN LUN, etc can be allocated ahead of 
 time.
 Image formats like qcow2 have non-trivial metadata that makes it hard to
 estimate the exact size without knowledge of file format internals.

 This patch series introduces a new qemu-img subcommand that calculates the
 required size for both image creation and conversion scenarios.

 The conversion scenario is:

   $ qemu-img max-size -f raw -O qcow2 input.img
   107374184448
>>>
>>> Isn't this the minimal size required to convert input.img?
>>>
>>
>> It's an upper bound for the property being measured, which is current
>> allocation size, not maximum potential size after growth.
>
> From my point of view, this is the minimal size you must allocate if you
> want to convert the image to logical volume.

Thinking more about this, max-size is correct for this use case, the
maximum size of the image, used as the minimal allocation.

>
>>

 Here an existing image file is taken and the output includes the space 
 required
 for data from the input image file.

 The creation scenario is:

   $ qemu-img max-size -O qcow2 --size 5G
   196688
>>>
>>> Again, this is the minimal size.
>>>
>>> So maybe use min-size?
>>>
>>> Or:
>>>
>>> qemu-img measure -f raw -O qcow2 input.img
>>>
>>> Works nicely with other verbs like create, convert, check.
>>>
>>
>> Measure what? This is strictly less descriptive even if "max-size" isn't
>> a verb.
>
> measure-size?
>
>>> Now about the return value, do we want to return both the minimum size
>>> and the maximum size?
>>>
>>> For ovirt use case, we currently calculate the maximum size by multiplying
>>> by 1.1. We use this when doing automatic extending of ovirt thin provisioned
>>> disk. We start with 1G lv, and extend it each time it becomes full, stopping
>>> when we reach virtual size * 1.1. Using more accurate calculation instead
>>> can be nicer.
>>>
>>> So we can retrun:
>>>
>>> {
>>> "min-size": 196688,
>>> "max-size": 5905580032
>>> }
>>>
>>> Anyway thanks for working on this!
>>>
>>
>> It sounds like you want something different from what was intuited by
>> Maor Lipchuck. There are two things to estimate:
>>
>> (A) An estimate of the possible size of an image after conversion to a
>> different format, and
>> (B) An estimate of the possible size after full allocation.
>>
>> I got the sense that Maor was asking for (A), but perhaps I am wrong
>> about that. However, both are "maximums" in different senses.
>
> Both are minimum when you have to allocate the space.
>
> Maor ask about A because he is working on fixing allocation when
> converting existing files, but we also have other use cases like B.
>
> Nir
>
>>
>> --js
>>

 Stefan Hajnoczi (4):
   block: add bdrv_max_size() API
   raw-format: add bdrv_max_size() support
   qemu-img: add max-size subcommand
   iotests: add test 178 for qemu-img max-size

  include/block/block.h  |   2 +
  include/block/block_int.h  |   2 +
  block.c|  37 +
  block/raw-format.c |  16 
  qemu-img.c | 196 
 +
  qemu-img-cmds.hx   |   6 ++
  tests/qemu-iotests/178 |  75 +
  tests/qemu-iotests/178.out |  25 ++
  tests/qemu-iotests/group   |   1 +
  9 files changed, 360 insertions(+)
  create mode 100755 tests/qemu-iotests/178
  create mode 100644 tests/qemu-iotests/178.out

 --
 2.9.3

>>

Re: [Qemu-devel] [PATCH for-2.9 0/6] disas: Fix various coverity nits

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Subject: [Qemu-devel] [PATCH for-2.9 0/6] disas: Fix various coverity nits
Message-id: 1488556233-31246-1-git-send-email-peter.mayd...@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=/var/tmp/patchew-qemu-build
echo -n "Using CC: "
realpath $CC
test -e $BUILD && rm -rf $BUILD
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
90c6513 disas/arm: Avoid unintended sign extension
d1691a2 disas/cris: Avoid unintended sign extension
e2ffe28 disas/microblaze: Avoid unintended sign extension
126e460 disas/m68k: Avoid unintended sign extension in get_field()
a5a7833 disas/i386: Avoid NULL pointer dereference in error case
a161605 disas/hppa: Remove dead code

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=45798
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-90x67e20/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libacl-2.2.52-11.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
cdparanoia-libs-10.2-21.fc24.s390x
ustr-1.0.4-21.fc24.s390x
giflib-4.1.6-15.fc24.s390x
libusb-0.1.5-7.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
readline-devel-6.3-8.fc24.s390x
python-srpm-macros-3-10.fc25.noarch
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
chkconfig-1.8-1.fc25.s390x
libidn-1.33-1.fc25.s390x
file-5.28-4.fc25.s390x
slang-2.3.0-7.fc25.s390x
avahi-libs-0.6.32-4.fc25.s390x
libsemanage-2.5-8.fc25.s390x
perl-Unicode-Normalize-1.25-365.fc25.s390x
perl-libnet-3.10-1.fc25.noarch
perl-Thread-Queue-3.11-1.fc25.noarch
perl-podlators-4.09-1.fc25.noarch
jasper-libs-1.900.13-1.fc25.s390x
graphite2-1.3.6-1.fc25.s390x
libblkid-2.28.2-1.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
dbus-python-1.2.4-2.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
libgnome-keyring-3.12.0-7.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-3.5.2-4.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
python-backports-1.0-8.fc25.s390x
python-magic-5.28-4.fc25.noarch
python-pycparser-2.14-7.fc25.noarch
python-fedora-0.8.0-2.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
plymouth-scripts-0.9.3-0.6.20160620git0e65b86c.fc25.s390x
cronie-1.5.1-2.fc25.s390x
python2-librepo-1.7.18-3.fc25.s390x
wget-1.18-2.fc25.s390x
python3-dnf-plugins-core-0.1.21-4.fc25.noarch
at-spi2-core-2.22.0-1.fc25.s390x
libXv-1.0.11-1.fc25.s390x
dhcp-client-4.3.5-1.fc25.s390x
python2-dnf-plugins-core-0.1.21-4.fc25.noarch
parted-3.2-21.fc25.s390x
python2-ndg_httpsclient-0.4.0-4.fc25.noarch
bash-completion-2.4-1.fc25.noarch
btrfs-progs-4.6.1-1.fc25.s390x
texinfo-6.1-3.fc25.s390x
perl-Filter-1.55-366.fc25.s390x
flex-2.6.0-3.fc25.s390x
libgcc-6.3.1-1.fc25.s390x
glib2-2.50.2-1.fc25.s390x
dbus-libs-1.11.8-1.fc25.s390x
libgomp-6.3.1-1.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
perl-Encode-2.88-5.fc25.s390x
gstreamer1-1.10.2-1.fc25.s390x
cracklib-2.9.6-4.fc25.s390x
rpm-build-libs-4.13.0-6.fc25.s390x
libobjc-6.3.1-1.fc25.s390x
pcre-devel-8.40-1.fc25.s390x
mariadb-config-10.1.20-1.fc25.s390x
gcc-6.3.1-1.fc25.s390x
mesa-libGL-13.0.3-1.fc25.s390x
python3-dnf-plugin-system-upgrade-0.7.1-4.fc25.noarch
bind-libs-9.10.4-4.P5.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
NetworkManager-1.4.4-3.fc25.s390x
audit-2.7.1-1.fc25.s390x
glibc-static-2.24-4.fc25.s390x
perl-Pod-Simple-3.35-1.fc25.noarch
gdb-7.12-36.fc25.s390x
python2-simplejson-3.10.0-1.fc25.s390x
python3-sssdconfig-1.14.2-2.fc25.noarch
texlive-lib-2016-30.20160520.fc25.s390x
boost-random-1.60.0-10.fc25.s390x
brltty-5.4-2.fc25.s390x
libref_array-0.1.5-29.fc25.s390x
librados2-10.2.4-2.fc25.s390x
gnutls-dane-3.5.8-1.fc25.s390x
systemtap-client-3.1-0.20160725git91bfb36.fc25.s390x
libXrender-devel-0.9.10-1.fc25.s390x
libXi-devel-1.7.8-2.fc25.s390x
texlive-pdftex-doc-svn41149-30.fc25.noarch
tcp_wrappers-7.6-83.fc25.s390x
javapackages-tools-4.7.0-6.1.fc25.noarch
texlive-kpathsea-bin-svn40473-30.20160520.fc25.s390x
texlive-url-svn32528.3.4-30.fc25.noarch
texlive-latex-fonts-svn2.0-30.fc25.noarch
texlive-mptopdf-bin-svn18674.0-30.20160520.fc25.noarch
texlive-underscore-svn18261.0-30.fc25.noarch
texlive-subfig-svn15878.1.3-30.fc25.noarch
texlive-dvipdfmx-def-svn40328-30.fc25.noarch
texlive-plain-svn40274-30.fc25.noarch
texlive-texlive-scripts-svn41433-30.fc25.noarch

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Eric Blake]

On 03/03/2017 12:14 PM, Eric Blake wrote:
> On 03/03/2017 11:25 AM, Greg Kurz wrote:
>> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
>> QEMU vulnerable.
>>
>> O_PATH was used as an optimization: the fd returned by openat_dir() is only
>> passed to openat() actually, so we don't really need to reach the underlying
>> filesystem.
>>
>> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
>> return a fd, forcing us to do some other syscall to detect we have a
>> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.
> 
> But the very next use of openat(fd, ) should fail with EBADF if fd is

or ENOTDIR, actually

> not a directory, so you don't need any extra syscalls.  I agree that we
> _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH
> where it works.
> 
> I'm in the middle of writing a test program to probe kernel behavior and
> demonstrate (at least to myself) whether there are scenarios where
> O_PATH makes it possible to open something where omitting it did not,
> while at the same time validating that O_NOFOLLOW doesn't cause problems
> if a symlink-fd is returned instead of a directory fd, based on our
> subsequent use of that fd in a *at call.

My test case is below.  Note that based on my testing, I think you want
a v2 of this patch, which does:

#ifndef O_PATH
#define O_PATH 0
#endif

 static inline int openat_dir(int dirfd, const char *name)
 {
-return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
+return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW |
O_PATH);
 }



#define _GNU_SOURCE 1
#include 
#include 
#include 
#include 
#include 
#include 

int main(int argc, char **argv)
{
int i = 0;
int ret = 1;
int fd;
struct stat st;

if (mkdir("d", 0700) < 0) {
printf("giving up, please try again once 'd' is removed\n");
return ret;
}

/* Create a playground for testing with. */
i = 1;
if (creat("d/file", 0600) < 0)
goto cleanup;
i = 2;
if (mkdir("d/subdir", 0700) < 0)
goto cleanup;
i = 3;
if (creat("d/subdir/subfile", 0600) < 0)
goto cleanup;
i = 4;
if (chmod("d/subdir", 0100) < 0)
goto cleanup;
i = 5;
if (symlink("file", "d/link-file") < 0)
goto cleanup;
i = 6;
if (symlink("subdir", "d/link-subdir") < 0)
goto cleanup;

/* Sanity: We can stat a child file with just search permissions,
 * whether via a directory or symlink-to-directory */
i = 7;
if (stat("d/subdir/subfile", ) < 0)
goto cleanup;
i = 8;
if (stat("d/link-subdir/subfile", ) < 0)
goto cleanup;

/* Since the directory is not readable, we can't get a normal fd */
fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY);
if (fd != -1) {
printf("unexpected success opening non-readable dir\n");
ret = 2;
goto cleanup;
}
/* But we can get at it with O_PATH */
i = 9;
fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH);
if (fd < 0)
goto cleanup;
/* And use it in *at functions */
i = 10;
if (fstatat(fd, "subfile", , 0) < 0)
goto cleanup;
i = 11;
if (close(fd) < 0)
goto cleanup;

/* Note that O_DIRECTORY fails on symlinks with O_PATH... */
i = 12;
fd = open("d/link-subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY |
O_PATH);
if (fd != -1) {
printf("unexpected success on symlink-dir with O_DIRECTORY\n");
ret = 2;
goto cleanup;
}
/* or on symlinks to files regardless of O_PATH... */
i = 13;
fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH);
if (fd != -1) {
printf("unexpected success on symlink-file with
O_DIRECTORY|O_PATH\n");
ret = 2;
goto cleanup;
}
i = 14;
fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY);
if (fd != -1) {
printf("unexpected success on symlink-file with just
O_DIRECTORY\n");
ret = 2;
goto cleanup;
}
/* but O_PATH without O_DIRECTORY opens a symlink fd */
i = 15;
fd = open("d/link-subdir", O_NOFOLLOW | O_RDONLY | O_PATH);
if (fd < 0)
goto cleanup;
/* However, that symlink fd is not usable in *at */
i = 16;
if (fstatat(fd, "subfile", , 0) == 0) {
printf("unexpected success using symlink fd in fstatat\n");
ret = 2;
goto cleanup;
}
if (errno != EBADF && errno != ENOTDIR)
goto cleanup;
i = 17;
if (close(fd) < 0)
goto cleanup;

printf("All tests passed\n");
ret = 0;

 cleanup:
if (ret == 1) {
perror("unexpected failure");
printf("encountered when i=%d\n", i);
}
system("chmod -R u+rwx d; rm -rf d");
return ret;
}



-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital 

[Qemu-devel] [PATCH v2] spapr_pci: allow control of BAR alignment through SLOF

[Michael Roth]

In certain cases, such as PCI-passthrough with VFIO, we cannot offload
MMIO accesses to KVM unless the BAR alignment matches the host. This
patch, in conjunction with a separately submitted patch for SLOF
which allows for control of this via the device-tree, allows us to
set this alignment via QEMU.

Cc: qemu-...@nongnu.org
Cc: Nikunj A Dadhania 
Cc: David Gibson 
Cc: Alexey Kardashevskiy 
Signed-off-by: Michael Roth 
---
v2:
  * Keep natural alignment as the default in SLOF, only set DT prop if
different alignment is specified by QEMU (David)
---
 hw/ppc/spapr.c  |  7 ++-
 hw/ppc/spapr_pci.c  | 10 ++
 include/hw/pci-host/spapr.h |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 14192ac..ef8df35 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3166,7 +3166,12 @@ DEFINE_SPAPR_MACHINE(2_9, "2.9", true);
  * pseries-2.8
  */
 #define SPAPR_COMPAT_2_8\
-HW_COMPAT_2_8
+HW_COMPAT_2_8   \
+{   \
+.driver   = TYPE_SPAPR_PCI_HOST_BRIDGE, \
+.property = "mem_bar_min_align",\
+.value= "0",\
+},  \
 
 static void spapr_machine_2_8_instance_options(MachineState *machine)
 {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 919d3c2..485d32d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1664,6 +1664,10 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+if (sphb->mem_bar_min_align == (uint64_t)-1) {
+sphb->mem_bar_min_align = qemu_real_host_page_size;
+}
+
 sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
 
 namebuf = alloca(strlen(sphb->dtbusname) + 32);
@@ -1858,6 +1862,8 @@ static Property spapr_phb_properties[] = {
 DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
 DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
  pre_2_8_migration, false),
+DEFINE_PROP_UINT64("mem_bar_min_align", sPAPRPHBState, mem_bar_min_align,
+   -1),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2228,6 +2234,10 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
 if (ret) {
 return ret;
 }
+if (phb->mem_bar_min_align) {
+_FDT(fdt_setprop_cell(fdt, bus_off, "qemu,mem-bar-min-align",
+  phb->mem_bar_min_align));
+}
 
 return 0;
 }
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index dfa7614..fa33346 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -79,6 +79,7 @@ struct sPAPRPHBState {
 uint64_t dma64_win_addr;
 
 uint32_t numa_node;
+uint64_t mem_bar_min_align;
 
 /* Fields for migration compatibility hacks */
 bool pre_2_8_migration;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[John Snow]

On 03/02/2017 05:08 AM, Li Qiang wrote:
> As the pci ahci can be hotplug and unplug, in the ahci unrealize
> function it should free all the resource once allocated in the
> realized function. This patchset first add cleanup function in
> core layer and then call it in the ahci unit.
> 
> Li Qiang (3):
>   ide: qdev: register ide bus unrealize function
>   ide: core: add cleanup function
>   ide: ahci: call cleanup function in ahci unit
> 
>  hw/ide/ahci.c | 12 
>  hw/ide/core.c |  8 
>  hw/ide/qdev.c | 12 ++--
>  include/hw/ide/internal.h |  1 +
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [RFC 0/4] qemu-img: add max-size subcommand

[Nir Soffer]

On Sat, Mar 4, 2017 at 12:02 AM, John Snow  wrote:
>
>
> On 03/03/2017 04:38 PM, Nir Soffer wrote:
>> On Fri, Mar 3, 2017 at 3:51 PM, Stefan Hajnoczi  wrote:
>>>
>>> RFCv1:
>>>  * Publishing patch series with just raw support, no qcow2 yet.  Please 
>>> review
>>>the command-line interface and let me know if you are happy with this
>>>approach.
>>>
>>> Users and management tools sometimes need to know the size required for a 
>>> new
>>> disk image so that an LVM volume, SAN LUN, etc can be allocated ahead of 
>>> time.
>>> Image formats like qcow2 have non-trivial metadata that makes it hard to
>>> estimate the exact size without knowledge of file format internals.
>>>
>>> This patch series introduces a new qemu-img subcommand that calculates the
>>> required size for both image creation and conversion scenarios.
>>>
>>> The conversion scenario is:
>>>
>>>   $ qemu-img max-size -f raw -O qcow2 input.img
>>>   107374184448
>>
>> Isn't this the minimal size required to convert input.img?
>>
>
> It's an upper bound for the property being measured, which is current
> allocation size, not maximum potential size after growth.

>From my point of view, this is the minimal size you must allocate if you
want to convert the image to logical volume.

>
>>>
>>> Here an existing image file is taken and the output includes the space 
>>> required
>>> for data from the input image file.
>>>
>>> The creation scenario is:
>>>
>>>   $ qemu-img max-size -O qcow2 --size 5G
>>>   196688
>>
>> Again, this is the minimal size.
>>
>> So maybe use min-size?
>>
>> Or:
>>
>> qemu-img measure -f raw -O qcow2 input.img
>>
>> Works nicely with other verbs like create, convert, check.
>>
>
> Measure what? This is strictly less descriptive even if "max-size" isn't
> a verb.

measure-size?

>> Now about the return value, do we want to return both the minimum size
>> and the maximum size?
>>
>> For ovirt use case, we currently calculate the maximum size by multiplying
>> by 1.1. We use this when doing automatic extending of ovirt thin provisioned
>> disk. We start with 1G lv, and extend it each time it becomes full, stopping
>> when we reach virtual size * 1.1. Using more accurate calculation instead
>> can be nicer.
>>
>> So we can retrun:
>>
>> {
>> "min-size": 196688,
>> "max-size": 5905580032
>> }
>>
>> Anyway thanks for working on this!
>>
>
> It sounds like you want something different from what was intuited by
> Maor Lipchuck. There are two things to estimate:
>
> (A) An estimate of the possible size of an image after conversion to a
> different format, and
> (B) An estimate of the possible size after full allocation.
>
> I got the sense that Maor was asking for (A), but perhaps I am wrong
> about that. However, both are "maximums" in different senses.

Both are minimum when you have to allocate the space.

Maor ask about A because he is working on fixing allocation when
converting existing files, but we also have other use cases like B.

Nir

>
> --js
>
>>>
>>> Stefan Hajnoczi (4):
>>>   block: add bdrv_max_size() API
>>>   raw-format: add bdrv_max_size() support
>>>   qemu-img: add max-size subcommand
>>>   iotests: add test 178 for qemu-img max-size
>>>
>>>  include/block/block.h  |   2 +
>>>  include/block/block_int.h  |   2 +
>>>  block.c|  37 +
>>>  block/raw-format.c |  16 
>>>  qemu-img.c | 196 
>>> +
>>>  qemu-img-cmds.hx   |   6 ++
>>>  tests/qemu-iotests/178 |  75 +
>>>  tests/qemu-iotests/178.out |  25 ++
>>>  tests/qemu-iotests/group   |   1 +
>>>  9 files changed, 360 insertions(+)
>>>  create mode 100755 tests/qemu-iotests/178
>>>  create mode 100644 tests/qemu-iotests/178.out
>>>
>>> --
>>> 2.9.3
>>>
>

Re: [Qemu-devel] [RFC 0/4] qemu-img: add max-size subcommand

[John Snow]

On 03/03/2017 04:38 PM, Nir Soffer wrote:
> On Fri, Mar 3, 2017 at 3:51 PM, Stefan Hajnoczi  wrote:
>>
>> RFCv1:
>>  * Publishing patch series with just raw support, no qcow2 yet.  Please 
>> review
>>the command-line interface and let me know if you are happy with this
>>approach.
>>
>> Users and management tools sometimes need to know the size required for a new
>> disk image so that an LVM volume, SAN LUN, etc can be allocated ahead of 
>> time.
>> Image formats like qcow2 have non-trivial metadata that makes it hard to
>> estimate the exact size without knowledge of file format internals.
>>
>> This patch series introduces a new qemu-img subcommand that calculates the
>> required size for both image creation and conversion scenarios.
>>
>> The conversion scenario is:
>>
>>   $ qemu-img max-size -f raw -O qcow2 input.img
>>   107374184448
> 
> Isn't this the minimal size required to convert input.img?
> 

It's an upper bound for the property being measured, which is current
allocation size, not maximum potential size after growth.

>>
>> Here an existing image file is taken and the output includes the space 
>> required
>> for data from the input image file.
>>
>> The creation scenario is:
>>
>>   $ qemu-img max-size -O qcow2 --size 5G
>>   196688
> 
> Again, this is the minimal size.
> 
> So maybe use min-size?
> 
> Or:
> 
> qemu-img measure -f raw -O qcow2 input.img
> 
> Works nicely with other verbs like create, convert, check.
> 

Measure what? This is strictly less descriptive even if "max-size" isn't
a verb.

> Now about the return value, do we want to return both the minimum size
> and the maximum size?
> 
> For ovirt use case, we currently calculate the maximum size by multiplying
> by 1.1. We use this when doing automatic extending of ovirt thin provisioned
> disk. We start with 1G lv, and extend it each time it becomes full, stopping
> when we reach virtual size * 1.1. Using more accurate calculation instead
> can be nicer.
> 
> So we can retrun:
> 
> {
> "min-size": 196688,
> "max-size": 5905580032
> }
> 
> Anyway thanks for working on this!
> 

It sounds like you want something different from what was intuited by
Maor Lipchuck. There are two things to estimate:

(A) An estimate of the possible size of an image after conversion to a
different format, and
(B) An estimate of the possible size after full allocation.

I got the sense that Maor was asking for (A), but perhaps I am wrong
about that. However, both are "maximums" in different senses.

--js

>>
>> Stefan Hajnoczi (4):
>>   block: add bdrv_max_size() API
>>   raw-format: add bdrv_max_size() support
>>   qemu-img: add max-size subcommand
>>   iotests: add test 178 for qemu-img max-size
>>
>>  include/block/block.h  |   2 +
>>  include/block/block_int.h  |   2 +
>>  block.c|  37 +
>>  block/raw-format.c |  16 
>>  qemu-img.c | 196 
>> +
>>  qemu-img-cmds.hx   |   6 ++
>>  tests/qemu-iotests/178 |  75 +
>>  tests/qemu-iotests/178.out |  25 ++
>>  tests/qemu-iotests/group   |   1 +
>>  9 files changed, 360 insertions(+)
>>  create mode 100755 tests/qemu-iotests/178
>>  create mode 100644 tests/qemu-iotests/178.out
>>
>> --
>> 2.9.3
>>

Re: [Qemu-devel] [RFC 3/4] qemu-img: add max-size subcommand

[Nir Soffer]

On Fri, Mar 3, 2017 at 3:51 PM, Stefan Hajnoczi  wrote:
> The max-size subcommand calculates the maximum size required by a new
> image file.  This can be used by users or management tools that need to
> allocate space on an LVM volume, SAN LUN, etc before creating or
> converting an image file.
>
> Suggested-by: Maor Lipchuk 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  qemu-img.c   | 196 
> +++
>  qemu-img-cmds.hx |   6 ++
>  2 files changed, 202 insertions(+)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 98b836b..f0a5a85 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -59,6 +59,7 @@ enum {
>  OPTION_PATTERN = 260,
>  OPTION_FLUSH_INTERVAL = 261,
>  OPTION_NO_DRAIN = 262,
> +OPTION_SIZE = 263,
>  };
>
>  typedef enum OutputFormat {
> @@ -4287,6 +4288,201 @@ out:
>  return 0;
>  }
>
> +static int img_max_size(int argc, char **argv)
> +{
> +static const struct option long_options[] = {
> +{"help", no_argument, 0, 'h'},
> +{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> +{"object", required_argument, 0, OPTION_OBJECT},
> +{"output", required_argument, 0, OPTION_OUTPUT},
> +{"size", required_argument, 0, OPTION_SIZE},
> +{0, 0, 0, 0}
> +};
> +OutputFormat output_format = OFORMAT_HUMAN;
> +BlockBackend *in_blk = NULL;
> +BlockDriver *drv;
> +const char *filename = NULL;
> +const char *fmt = NULL;
> +const char *out_fmt = "raw";
> +char *options = NULL;
> +char *snapshot_name = NULL;
> +QemuOpts *opts = NULL;
> +QemuOpts *object_opts = NULL;
> +QemuOpts *sn_opts = NULL;
> +QemuOptsList *create_opts = NULL;
> +bool image_opts = false;
> +uint64_t img_size = ~0ULL;
> +uint64_t ret_size;
> +Error *local_err = NULL;
> +int ret = 1;
> +int c;
> +
> +while ((c = getopt_long(argc, argv, "hf:O:o:l:",
> +long_options, NULL)) != -1) {
> +switch (c) {
> +case '?':
> +case 'h':
> +help();
> +break;
> +case 'f':
> +fmt = optarg;
> +break;
> +case 'O':
> +out_fmt = optarg;
> +break;
> +case 'o':
> +if (!is_valid_option_list(optarg)) {
> +error_report("Invalid option list: %s", optarg);
> +goto fail;
> +}
> +if (!options) {
> +options = g_strdup(optarg);
> +} else {
> +char *old_options = options;
> +options = g_strdup_printf("%s,%s", options, optarg);
> +g_free(old_options);
> +}
> +break;
> +case 'l':
> +if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
> +sn_opts = qemu_opts_parse_noisily(_snapshot_opts,
> +  optarg, false);
> +if (!sn_opts) {
> +error_report("Failed in parsing snapshot param '%s'",
> + optarg);
> +goto fail;
> +}
> +} else {
> +snapshot_name = optarg;
> +}
> +break;
> +case OPTION_OBJECT:
> +object_opts = qemu_opts_parse_noisily(_object_opts,
> +  optarg, true);
> +if (!object_opts) {
> +goto fail;
> +}
> +break;
> +case OPTION_IMAGE_OPTS:
> +image_opts = true;
> +break;
> +case OPTION_OUTPUT:
> +if (!strcmp(optarg, "json")) {
> +output_format = OFORMAT_JSON;
> +} else if (!strcmp(optarg, "human")) {
> +output_format = OFORMAT_HUMAN;
> +} else {
> +error_report("--output must be used with human or json "
> + "as argument.");
> +goto fail;
> +}
> +break;
> +case OPTION_SIZE:
> +{
> +int64_t sval;
> +
> +sval = cvtnum(optarg);
> +if (sval < 0) {
> +if (sval == -ERANGE) {
> +error_report("Image size must be less than 8 EiB!");
> +} else {
> +error_report("Invalid image size specified! You may use "
> + "k, M, G, T, P or E suffixes for ");
> +error_report("kilobytes, megabytes, gigabytes, 
> terabytes, "
> + "petabytes and exabytes.");
> +}
> +goto fail;
> +}
> +img_size = (uint64_t)sval;
> +}
> +break;
> +}
> +}
> +
> +if (qemu_opts_foreach(_object_opts,
> +  

Re: [Qemu-devel] -rtc clock=vm with -icount 1, sleep=off introduces unexpected delays in device interactions

[Frédéric Konrad]

Hi Jim,

I think Alex and Paolo worked on an mttcg/icount issue.
Not sure if its related or not.

Fred

On 03/03/2017 08:38 PM, Nutaro, James J. wrote:
> My original problem seems to stem from something that changed in the way that 
> device emulation and instruction execution interact (I'm guessing). To 
> reproduce the issue, I started a linux image with
> 
> qemu-system-i386 -rtc clock=vm -monitor none -icount 1,sleep=off jack.img
> 
> After booting, I run
> 
> ping localhost
> 
> The first round trip time reports look reasonable, being in the millisecond 
> to sub-millisecond range. These occur while the emulator is running slower 
> than real time. 
> 
> After a bit, the emulator speeds up (skipping over idle periods during I/O?) 
> and the round trip times jump to hundreds of milliseconds, which I had not 
> expected.
> 
> If you want to try this experiment yourself, you can get the disk image that 
> I used from here:
> 
> http://www.ornl.gov/~1qn/qemu-images/jack.img
> 
> 
> Jim
> 

Re: [Qemu-devel] [RFC 0/4] qemu-img: add max-size subcommand

[Nir Soffer]

On Fri, Mar 3, 2017 at 3:51 PM, Stefan Hajnoczi  wrote:
>
> RFCv1:
>  * Publishing patch series with just raw support, no qcow2 yet.  Please review
>the command-line interface and let me know if you are happy with this
>approach.
>
> Users and management tools sometimes need to know the size required for a new
> disk image so that an LVM volume, SAN LUN, etc can be allocated ahead of time.
> Image formats like qcow2 have non-trivial metadata that makes it hard to
> estimate the exact size without knowledge of file format internals.
>
> This patch series introduces a new qemu-img subcommand that calculates the
> required size for both image creation and conversion scenarios.
>
> The conversion scenario is:
>
>   $ qemu-img max-size -f raw -O qcow2 input.img
>   107374184448

Isn't this the minimal size required to convert input.img?

>
> Here an existing image file is taken and the output includes the space 
> required
> for data from the input image file.
>
> The creation scenario is:
>
>   $ qemu-img max-size -O qcow2 --size 5G
>   196688

Again, this is the minimal size.

So maybe use min-size?

Or:

qemu-img measure -f raw -O qcow2 input.img

Works nicely with other verbs like create, convert, check.

Now about the return value, do we want to return both the minimum size
and the maximum size?

For ovirt use case, we currently calculate the maximum size by multiplying
by 1.1. We use this when doing automatic extending of ovirt thin provisioned
disk. We start with 1G lv, and extend it each time it becomes full, stopping
when we reach virtual size * 1.1. Using more accurate calculation instead
can be nicer.

So we can retrun:

{
"min-size": 196688,
"max-size": 5905580032
}

Anyway thanks for working on this!

>
> Stefan Hajnoczi (4):
>   block: add bdrv_max_size() API
>   raw-format: add bdrv_max_size() support
>   qemu-img: add max-size subcommand
>   iotests: add test 178 for qemu-img max-size
>
>  include/block/block.h  |   2 +
>  include/block/block_int.h  |   2 +
>  block.c|  37 +
>  block/raw-format.c |  16 
>  qemu-img.c | 196 
> +
>  qemu-img-cmds.hx   |   6 ++
>  tests/qemu-iotests/178 |  75 +
>  tests/qemu-iotests/178.out |  25 ++
>  tests/qemu-iotests/group   |   1 +
>  9 files changed, 360 insertions(+)
>  create mode 100755 tests/qemu-iotests/178
>  create mode 100644 tests/qemu-iotests/178.out
>
> --
> 2.9.3
>

Re: [Qemu-devel] [RFC PATCH] configure: remove --enable-replication/--disable-replication

[Paolo Bonzini]

> > I would actually prefer to remove many of the latter
> > (‑‑enable‑vhost‑net, ‑‑enable‑vhost‑scsi, ‑‑enable‑vhost‑socket) and
> > just use default‑configs.  We are already doing it for ivshmem for
> > example:
> 
> Was there ever a conclusion here? The reason I ask is that I see that
> currently
> using --disable-replication fails for me as follows:

No conclusion.  I suppose if people are interested in --disable-replication
they can submit a patch to fix the bitrot.  If 2.9 ships with the option
broken, I'll resend the patch for inclusion in 2.10.

This should give about one month to fix the option, which should be enough.

Paolo

> # ./configure --disable-replication
> ...
> # make
> ...
> make  all-recursive
> Making all in pixman
> make[3]: Nothing to be done for 'all'.
> Making all in demos
> make[3]: Nothing to be done for 'all'.
> Making all in test
> make[3]: Nothing to be done for 'all'.
>   CHK version_gen.h
>   LINKaarch64-softmmu/qemu-system-aarch64
> ../migration/colo.o: In function `qmp_query_xen_replication_status':
> /home/brogers/osr/git/qemu/migration/colo.c:181: undefined reference to
> `replication_get_error_all'
> ../migration/colo.o: In function `qmp_xen_set_replication':
> /home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to
> `replication_stop_all'
> /home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to
> `replication_stop_all'
> /home/brogers/osr/git/qemu/migration/colo.c:167: undefined reference to
> `replication_start_all'
> ../migration/colo.o: In function `qmp_xen_colo_do_checkpoint':
> /home/brogers/osr/git/qemu/migration/colo.c:196: undefined reference to
> `replication_do_checkpoint_all'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Makefile:208: qemu-system-aarch64] Error 1
> make: *** [Makefile:322: subdir-aarch64-softmmu] Error 2
> 
> --
> Bruce
>  
> 
> 

Re: [Qemu-devel] [PULL 08/24] tcg: drop global lock during TCG code execution

[Alex Bennée]

Aaron Lindsay  writes:

> On Feb 27 14:39, Alex Bennée wrote:
>>
>> Laurent Desnogues  writes:
>>
>> > Hello,
>> >
>> > On Fri, Feb 24, 2017 at 12:20 PM, Alex Bennée  
>> > wrote:
>> >> From: Jan Kiszka 
>> >>
>> >> This finally allows TCG to benefit from the iothread introduction: Drop
>> >> the global mutex while running pure TCG CPU code. Reacquire the lock
>> >> when entering MMIO or PIO emulation, or when leaving the TCG loop.
>> >>
>> >> We have to revert a few optimization for the current TCG threading
>> >> model, namely kicking the TCG thread in qemu_mutex_lock_iothread and not
>> >> kicking it in qemu_cpu_kick. We also need to disable RAM block
>> >> reordering until we have a more efficient locking mechanism at hand.
>> >>
>> >> Still, a Linux x86 UP guest and my Musicpal ARM model boot fine here.
>> >> These numbers demonstrate where we gain something:
>> >>
>> >> 20338 jan   20   0  331m  75m 6904 R   99  0.9   0:50.95 
>> >> qemu-system-arm
>> >> 20337 jan   20   0  331m  75m 6904 S   20  0.9   0:26.50 
>> >> qemu-system-arm
>> >>
>> >> The guest CPU was fully loaded, but the iothread could still run mostly
>> >> independent on a second core. Without the patch we don't get beyond
>> >>
>> >> 32206 jan   20   0  330m  73m 7036 R   82  0.9   1:06.00 
>> >> qemu-system-arm
>> >> 32204 jan   20   0  330m  73m 7036 S   21  0.9   0:17.03 
>> >> qemu-system-arm
>> >>
>> >> We don't benefit significantly, though, when the guest is not fully
>> >> loading a host CPU.
>> >
>> > I tried this patch (8d04fb55 in the repository) with the following image:
>> >
>> >http://wiki.qemu.org/download/arm-test-0.2.tar.gz
>> >
>> > Running the image with no option works fine.  But specifying '-icount
>> > 1' results in a (guest) deadlock. Enabling some heavy logging (-d
>> > in_asm,exec) sometimes results in a 'Bad ram offset'.
>> >
>> > Is it expected that this patch breaks -icount?
>>
>> Not really. Using icount will disable MTTCG and run single threaded as
>> before. Paolo reported another icount failure so they may be related. I
>> shall have a look at it.
>>
>> Thanks for the report.
>
> I have not experienced a guest deadlock, but for me this patch makes
> booting a simple Linux distribution take about an order of magnitude
> longer when using '-icount 0' (from about 1.6 seconds to 17.9). It was
> slow enough to get to the printk the first time after recompiling that I
> killed it thinking it *had* deadlocked.
>
> `perf report` from before this patch (snipped to >1%):
>  23.81%  qemu-system-aar  perf-9267.map[.] 0x41a5cc9e
>   7.15%  qemu-system-aar  [kernel.kallsyms][k] 0x8172bc82
>   6.29%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
>   4.99%  qemu-system-aar  qemu-system-aarch64  [.] tcg_gen_code
>   4.71%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
>   4.39%  qemu-system-aar  qemu-system-aarch64  [.] tcg_optimize
>   3.28%  qemu-system-aar  qemu-system-aarch64  [.] helper_dc_zva
>   2.66%  qemu-system-aar  qemu-system-aarch64  [.] liveness_pass_1
>   1.98%  qemu-system-aar  qemu-system-aarch64  [.] qht_lookup
>   1.93%  qemu-system-aar  qemu-system-aarch64  [.] tcg_out_opc
>   1.81%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
>   1.71%  qemu-system-aar  qemu-system-aarch64  [.] 
> object_class_dynamic_cast_assert
>   1.38%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1
>   1.10%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0
>
> and after this patch:
>  20.10%  qemu-system-aar  perf-3285.map[.] 0x40a3b690
> *18.08%  qemu-system-aar  [kernel.kallsyms][k] 0x81371865
>   7.87%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
>   4.70%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
> * 2.64%  qemu-system-aar  qemu-system-aarch64  [.] g_mutex_get_impl
>   2.39%  qemu-system-aar  qemu-system-aarch64  [.] gic_update
> * 1.89%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_unlock
>   1.61%  qemu-system-aar  qemu-system-aarch64  [.] 
> object_class_dynamic_cast_assert
> * 1.55%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_lock
>   1.31%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
>   1.21%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0
>   1.13%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1
>
> I've put asterisks by a few suspicious mutex-related functions, though I 
> wonder
> if the slowdowns are also partially inlined into some of the other functions.
> The kernel also jumps up, presumably from handling more mutexes?
>
> I confess I'm not familiar enough with this code to suggest optimizations, but
> I'll be glad to test any.

Please see the series Paolo posted:

  Subject: [PATCH 0/6] tcg: fix icount super slowdown
  Date: Fri,  3 Mar 2017 14:11:08 +0100
  Message-Id: 

Re: [Qemu-devel] [PATCH v2]util:Removed header qemu-common.h from path.c

[Eric Blake]

On 03/03/2017 02:22 PM, Suramya Shah wrote:
> Signed-off-by: Suramya Shah 

Technically, this is v3 now (and your next submission will be v4).
Also, if you use 'git send-email -v4' to send your patch, it would
automatically put a space after the closing ] of the subject prefix (I
think 'git am' does the right thing even without the space, but when
most submissions look consistent in the archives, the ones that are
different stand out)

In the subject, put a space after the colon.

It's also fine that your subject says 'what' was done, but there is no
'why'; that's what the commit body is good for.  It's also good to write
patches in the imperative sense (present tense command form) rather than
past tense.  Visualize it as "this patch does something" (or even "apply
this patch for these results"), not "I wrote this patch to do something"
or "this patch did something".

Another tip: if you can easily identify a patch where a particular
situation started that you are now fixing, calling it out may help
reviewers.  In this case, looking at 'git log util/path.c', it looks
like commit f348b6d was the one that got rid of the need for using
qemu-common.h, when it moved declarations into other headers.  Showing
that you've researched the history gives more weight to your patch being
valid.

> --- fix of typo in v1 that broke compilation

'git am' requires '---' to be on a line of its own.  You want the
changelog description to occur on its own line, after the --- separator.

Also, since I gave a Reviewed-by: on your last mail, you can add it here
to mark that the patch body is unchanged since it was last reviewed.

Since this appears to be your first contribution, the maintainer that
incorporates your patch can probably clean up these (minor) mistakes, so
you may not need to resubmit (on the other hand, resubmitting makes it
less work for the maintainer, so feel free to do it if you'd like). If
you'd like more tips for successful submissions, there's a lot of
resources at http://wiki.qemu-project.org/Contribute/SubmitAPatch

Putting this all together, I suggest the following header if you want to
resubmit as a v4:


util: Remove unneeded header from path.c

qemu-common.h is no longer necessary, as of the refactoring done in
commit f348b6d.

Signed-off-by:...
Reviewed-by: Eric Blake 

---
v4: rewrite commit log based on Eric's hints

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 08/24] tcg: drop global lock during TCG code execution

[Aaron Lindsay]

On Feb 27 14:39, Alex Bennée wrote:
> 
> Laurent Desnogues  writes:
> 
> > Hello,
> >
> > On Fri, Feb 24, 2017 at 12:20 PM, Alex Bennée  
> > wrote:
> >> From: Jan Kiszka 
> >>
> >> This finally allows TCG to benefit from the iothread introduction: Drop
> >> the global mutex while running pure TCG CPU code. Reacquire the lock
> >> when entering MMIO or PIO emulation, or when leaving the TCG loop.
> >>
> >> We have to revert a few optimization for the current TCG threading
> >> model, namely kicking the TCG thread in qemu_mutex_lock_iothread and not
> >> kicking it in qemu_cpu_kick. We also need to disable RAM block
> >> reordering until we have a more efficient locking mechanism at hand.
> >>
> >> Still, a Linux x86 UP guest and my Musicpal ARM model boot fine here.
> >> These numbers demonstrate where we gain something:
> >>
> >> 20338 jan   20   0  331m  75m 6904 R   99  0.9   0:50.95 
> >> qemu-system-arm
> >> 20337 jan   20   0  331m  75m 6904 S   20  0.9   0:26.50 
> >> qemu-system-arm
> >>
> >> The guest CPU was fully loaded, but the iothread could still run mostly
> >> independent on a second core. Without the patch we don't get beyond
> >>
> >> 32206 jan   20   0  330m  73m 7036 R   82  0.9   1:06.00 
> >> qemu-system-arm
> >> 32204 jan   20   0  330m  73m 7036 S   21  0.9   0:17.03 
> >> qemu-system-arm
> >>
> >> We don't benefit significantly, though, when the guest is not fully
> >> loading a host CPU.
> >
> > I tried this patch (8d04fb55 in the repository) with the following image:
> >
> >http://wiki.qemu.org/download/arm-test-0.2.tar.gz
> >
> > Running the image with no option works fine.  But specifying '-icount
> > 1' results in a (guest) deadlock. Enabling some heavy logging (-d
> > in_asm,exec) sometimes results in a 'Bad ram offset'.
> >
> > Is it expected that this patch breaks -icount?
> 
> Not really. Using icount will disable MTTCG and run single threaded as
> before. Paolo reported another icount failure so they may be related. I
> shall have a look at it.
> 
> Thanks for the report.

I have not experienced a guest deadlock, but for me this patch makes
booting a simple Linux distribution take about an order of magnitude
longer when using '-icount 0' (from about 1.6 seconds to 17.9). It was
slow enough to get to the printk the first time after recompiling that I
killed it thinking it *had* deadlocked.

`perf report` from before this patch (snipped to >1%):
 23.81%  qemu-system-aar  perf-9267.map[.] 0x41a5cc9e
  7.15%  qemu-system-aar  [kernel.kallsyms][k] 0x8172bc82
  6.29%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
  4.99%  qemu-system-aar  qemu-system-aarch64  [.] tcg_gen_code
  4.71%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
  4.39%  qemu-system-aar  qemu-system-aarch64  [.] tcg_optimize
  3.28%  qemu-system-aar  qemu-system-aarch64  [.] helper_dc_zva
  2.66%  qemu-system-aar  qemu-system-aarch64  [.] liveness_pass_1
  1.98%  qemu-system-aar  qemu-system-aarch64  [.] qht_lookup
  1.93%  qemu-system-aar  qemu-system-aarch64  [.] tcg_out_opc
  1.81%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
  1.71%  qemu-system-aar  qemu-system-aarch64  [.] 
object_class_dynamic_cast_assert
  1.38%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1
  1.10%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0

and after this patch:
 20.10%  qemu-system-aar  perf-3285.map[.] 0x40a3b690
*18.08%  qemu-system-aar  [kernel.kallsyms][k] 0x81371865
  7.87%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
  4.70%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
* 2.64%  qemu-system-aar  qemu-system-aarch64  [.] g_mutex_get_impl
  2.39%  qemu-system-aar  qemu-system-aarch64  [.] gic_update
* 1.89%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_unlock
  1.61%  qemu-system-aar  qemu-system-aarch64  [.] 
object_class_dynamic_cast_assert
* 1.55%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_lock
  1.31%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
  1.21%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0
  1.13%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1

I've put asterisks by a few suspicious mutex-related functions, though I wonder
if the slowdowns are also partially inlined into some of the other functions.
The kernel also jumps up, presumably from handling more mutexes?

I confess I'm not familiar enough with this code to suggest optimizations, but
I'll be glad to test any.

-Aaron

> 
> >
> > Thanks,
> >
> > Laurent
> >
> > PS - To clarify 791158d9 works.
> >
> >> Signed-off-by: Jan Kiszka 
> >> Message-Id: <1439220437-23957-10-git-send-email-fred.kon...@greensocs.com>
> >> [FK: Rebase, fix qemu_devices_reset deadlock, rm address_space_* mutex]
> >> Signed-off-by: KONRAD Frederic 

Re: [Qemu-devel] [PATCH v3 04/13] sm501: QOMify

[BALATON Zoltan]

On Fri, 3 Mar 2017, Peter Maydell wrote:

On 3 March 2017 at 01:03, BALATON Zoltan  wrote:

Adding vmstate saving is not in this patch because the state structure
will be changed in further patches, then another patch will add
vmstate descriptor after those changes.

Signed-off-by: BALATON Zoltan 



+static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base,
+   uint32_t local_mem_bytes)
+{
+s->base = base;
+s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
+SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
+  s->local_mem_size_index);
+if (get_local_mem_size(s) != local_mem_bytes) {
+error_report("Warning: sm501 VRAM size adjusted to %" PRIu32,
+ get_local_mem_size(s));
+}


Just noticed this. I think reporting the error upwards by
failing device realize is better than adjusting the value.
It's what we tend to do for other devices. Management tools
like libvirt prefer to get hard errors if there's a config
file error that means they misconfigure a device.


Making this change would need rebasing the whole series again an fixing 
conflicts. Do you insist on this or can we live with this warning which is 
closer to the original behaviour of this device?

Re: [Qemu-devel] [RFC 0/4] qemu-img: add max-size subcommand

[John Snow]

On 03/03/2017 08:51 AM, Stefan Hajnoczi wrote:
> RFCv1:
>  * Publishing patch series with just raw support, no qcow2 yet.  Please review
>the command-line interface and let me know if you are happy with this
>approach.
> 
> Users and management tools sometimes need to know the size required for a new
> disk image so that an LVM volume, SAN LUN, etc can be allocated ahead of time.
> Image formats like qcow2 have non-trivial metadata that makes it hard to
> estimate the exact size without knowledge of file format internals.
> 
> This patch series introduces a new qemu-img subcommand that calculates the
> required size for both image creation and conversion scenarios.
> 
> The conversion scenario is:
> 
>   $ qemu-img max-size -f raw -O qcow2 input.img
>   107374184448
> 
> Here an existing image file is taken and the output includes the space 
> required
> for data from the input image file.
> 
> The creation scenario is:
> 
>   $ qemu-img max-size -O qcow2 --size 5G
>   196688
> 

It looks sane to me, implementation looks clean enough.
Thanks!

> Stefan Hajnoczi (4):
>   block: add bdrv_max_size() API
>   raw-format: add bdrv_max_size() support
>   qemu-img: add max-size subcommand
>   iotests: add test 178 for qemu-img max-size
> 
>  include/block/block.h  |   2 +
>  include/block/block_int.h  |   2 +
>  block.c|  37 +
>  block/raw-format.c |  16 
>  qemu-img.c | 196 
> +
>  qemu-img-cmds.hx   |   6 ++
>  tests/qemu-iotests/178 |  75 +
>  tests/qemu-iotests/178.out |  25 ++
>  tests/qemu-iotests/group   |   1 +
>  9 files changed, 360 insertions(+)
>  create mode 100755 tests/qemu-iotests/178
>  create mode 100644 tests/qemu-iotests/178.out
> 

Re: [Qemu-devel] [PATCH v2 5/5] xen: use libxendevicemodel when available

[Stefano Stabellini]

On Fri, 3 Mar 2017, Stefano Stabellini wrote:
> On Fri, 3 Mar 2017, Paul Durrant wrote:
> > > -Original Message-
> > > From: Stefano Stabellini [mailto:sstabell...@kernel.org]
> > > Sent: 02 March 2017 22:50
> > > To: Paul Durrant 
> > > Cc: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org; Stefano
> > > Stabellini ; Anthony Perard
> > > 
> > > Subject: Re: [PATCH v2 5/5] xen: use libxendevicemodel when available
> > > 
> > > On Thu, 2 Mar 2017, Paul Durrant wrote:
> > > > This patch modifies the wrapper functions in xen_common.h to use the
> > > > new xendevicemodel interface if it is available along with compatibility
> > > > code to use the old libxenctrl interface if it is not.
> > > >
> > > > Signed-off-by: Paul Durrant 
> > > > ---
> > > > Cc: Stefano Stabellini 
> > > > Cc: Anthony Perard 
> > > >
> > > > v2:
> > > > - Add a compat define for xenforeignmemory_close() since this is now
> > > >   used.
> > > > ---
> > > >  include/hw/xen/xen_common.h | 115
> > > +++-
> > > >  xen-common.c|   8 +++
> > > >  2 files changed, 90 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/include/hw/xen/xen_common.h
> > > b/include/hw/xen/xen_common.h
> > > > index 31cf25f..48444e5 100644
> > > > --- a/include/hw/xen/xen_common.h
> > > > +++ b/include/hw/xen/xen_common.h
> > > > @@ -9,6 +9,7 @@
> > > >  #undef XC_WANT_COMPAT_EVTCHN_API
> > > >  #undef XC_WANT_COMPAT_GNTTAB_API
> > > >  #undef XC_WANT_COMPAT_MAP_FOREIGN_API
> > > > +#undef XC_WANT_COMPAT_DEVICEMODEL_API
> > > >
> > > >  #include 
> > > >  #include 
> > > > @@ -26,48 +27,95 @@ extern xc_interface *xen_xc;
> > > >   * We don't support Xen prior to 4.2.0.
> > > >   */
> > > >
> > > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 490
> > > > +
> > > > +typedef xc_interface xendevicemodel_handle;
> > > > +
> > > > +#define xendevicemodel_open(l, f) xen_xc
> > > > +
> > > > +#define xendevicemodel_map_io_range_to_ioreq_server \
> > > > +xc_hvm_map_io_range_to_ioreq_server
> > > > +#define xendevicemodel_unmap_io_range_from_ioreq_server \
> > > > +xc_hvm_unmap_io_range_from_ioreq_server
> > > > +#define xendevicemodel_map_pcidev_to_ioreq_server \
> > > > +xc_hvm_map_pcidev_to_ioreq_server
> > > > +#define xendevicemodel_unmap_pcidev_from_ioreq_server \
> > > > +xc_hvm_unmap_pcidev_from_ioreq_server
> > > > +#define xendevicemodel_create_ioreq_server \
> > > > +xc_hvm_create_ioreq_server
> > > > +#define xendevicemodel_destroy_ioreq_server \
> > > > +xc_hvm_destroy_ioreq_server
> > > > +#define xendevicemodel_get_ioreq_server_info \
> > > > +xc_hvm_get_ioreq_server_info
> > > > +#define xendevicemodel_set_ioreq_server_state \
> > > > +xc_hvm_set_ioreq_server_state
> > > > +#define xendevicemodel_set_pci_intx_level \
> > > > +xc_hvm_set_pci_intx_level
> > > > +#define xendevicemodel_set_pci_link_route \
> > > > +xc_hvm_set_pci_link_route
> > > > +#define xendevicemodel_set_isa_irq_level \
> > > > +xc_hvm_set_isa_irq_level
> > > > +#define xendevicemodel_inject_msi \
> > > > +xc_hvm_inject_msi
> > > > +#define xendevicemodel_set_mem_type \
> > > > +xc_hvm_set_mem_type
> > > > +#define xendevicemodel_track_dirty_vram \
> > > > +xc_hvm_track_dirty_vram
> > > > +#define xendevicemodel_modified_memory \
> > > > +xc_hvm_modified_memory
> > > 
> > > It does build correctly now for Xen < 4.9, however it breaks against
> > > xen-unstable:
> > > 
> > >   ERROR: configure test passed without -Werror but failed with -Werror.
> > >  This is probably a bug in the configure script. The failing 
> > > command
> > >  will be at the bottom of config.log.
> > >  You can run configure with --disable-werror to bypass this check.
> > > 
> > > and config.log says:
> > > 
> > >   config-temp/qemu-conf.c: In function 'main':
> > >   config-temp/qemu-conf.c:32:3: error: implicit declaration of function
> > > 'xc_hvm_set_mem_type' [-Werror=implicit-function-declaration]
> > >   config-temp/qemu-conf.c:32:3: error: nested extern declaration of
> > > 'xc_hvm_set_mem_type' [-Werror=nested-externs]
> > >   config-temp/qemu-conf.c:34:3: error: implicit declaration of function
> > > 'xc_hvm_inject_msi' [-Werror=implicit-function-declaration]
> > >   config-temp/qemu-conf.c:34:3: error: nested extern declaration of
> > > 'xc_hvm_inject_msi' [-Werror=nested-externs]
> > >   config-temp/qemu-conf.c:35:3: error: implicit declaration of function
> > > 'xc_hvm_create_ioreq_server' [-Werror=implicit-function-declaration]
> > >   config-temp/qemu-conf.c:35:3: error: nested extern declaration of
> > > 'xc_hvm_create_ioreq_server' [-Werror=nested-externs]
> > > 
> > > 
> > > With -DXC_WANT_COMPAT_DEVICEMODEL_API=1:
> > > 
> > >   In file included from /local/qemu-
> > > 

Re: [Qemu-devel] [PATCH v2 5/5] xen: use libxendevicemodel when available

[Stefano Stabellini]

On Fri, 3 Mar 2017, Paul Durrant wrote:
> > -Original Message-
> > From: Stefano Stabellini [mailto:sstabell...@kernel.org]
> > Sent: 02 March 2017 22:50
> > To: Paul Durrant 
> > Cc: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org; Stefano
> > Stabellini ; Anthony Perard
> > 
> > Subject: Re: [PATCH v2 5/5] xen: use libxendevicemodel when available
> > 
> > On Thu, 2 Mar 2017, Paul Durrant wrote:
> > > This patch modifies the wrapper functions in xen_common.h to use the
> > > new xendevicemodel interface if it is available along with compatibility
> > > code to use the old libxenctrl interface if it is not.
> > >
> > > Signed-off-by: Paul Durrant 
> > > ---
> > > Cc: Stefano Stabellini 
> > > Cc: Anthony Perard 
> > >
> > > v2:
> > > - Add a compat define for xenforeignmemory_close() since this is now
> > >   used.
> > > ---
> > >  include/hw/xen/xen_common.h | 115
> > +++-
> > >  xen-common.c|   8 +++
> > >  2 files changed, 90 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/include/hw/xen/xen_common.h
> > b/include/hw/xen/xen_common.h
> > > index 31cf25f..48444e5 100644
> > > --- a/include/hw/xen/xen_common.h
> > > +++ b/include/hw/xen/xen_common.h
> > > @@ -9,6 +9,7 @@
> > >  #undef XC_WANT_COMPAT_EVTCHN_API
> > >  #undef XC_WANT_COMPAT_GNTTAB_API
> > >  #undef XC_WANT_COMPAT_MAP_FOREIGN_API
> > > +#undef XC_WANT_COMPAT_DEVICEMODEL_API
> > >
> > >  #include 
> > >  #include 
> > > @@ -26,48 +27,95 @@ extern xc_interface *xen_xc;
> > >   * We don't support Xen prior to 4.2.0.
> > >   */
> > >
> > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 490
> > > +
> > > +typedef xc_interface xendevicemodel_handle;
> > > +
> > > +#define xendevicemodel_open(l, f) xen_xc
> > > +
> > > +#define xendevicemodel_map_io_range_to_ioreq_server \
> > > +xc_hvm_map_io_range_to_ioreq_server
> > > +#define xendevicemodel_unmap_io_range_from_ioreq_server \
> > > +xc_hvm_unmap_io_range_from_ioreq_server
> > > +#define xendevicemodel_map_pcidev_to_ioreq_server \
> > > +xc_hvm_map_pcidev_to_ioreq_server
> > > +#define xendevicemodel_unmap_pcidev_from_ioreq_server \
> > > +xc_hvm_unmap_pcidev_from_ioreq_server
> > > +#define xendevicemodel_create_ioreq_server \
> > > +xc_hvm_create_ioreq_server
> > > +#define xendevicemodel_destroy_ioreq_server \
> > > +xc_hvm_destroy_ioreq_server
> > > +#define xendevicemodel_get_ioreq_server_info \
> > > +xc_hvm_get_ioreq_server_info
> > > +#define xendevicemodel_set_ioreq_server_state \
> > > +xc_hvm_set_ioreq_server_state
> > > +#define xendevicemodel_set_pci_intx_level \
> > > +xc_hvm_set_pci_intx_level
> > > +#define xendevicemodel_set_pci_link_route \
> > > +xc_hvm_set_pci_link_route
> > > +#define xendevicemodel_set_isa_irq_level \
> > > +xc_hvm_set_isa_irq_level
> > > +#define xendevicemodel_inject_msi \
> > > +xc_hvm_inject_msi
> > > +#define xendevicemodel_set_mem_type \
> > > +xc_hvm_set_mem_type
> > > +#define xendevicemodel_track_dirty_vram \
> > > +xc_hvm_track_dirty_vram
> > > +#define xendevicemodel_modified_memory \
> > > +xc_hvm_modified_memory
> > 
> > It does build correctly now for Xen < 4.9, however it breaks against
> > xen-unstable:
> > 
> >   ERROR: configure test passed without -Werror but failed with -Werror.
> >  This is probably a bug in the configure script. The failing command
> >  will be at the bottom of config.log.
> >  You can run configure with --disable-werror to bypass this check.
> > 
> > and config.log says:
> > 
> >   config-temp/qemu-conf.c: In function 'main':
> >   config-temp/qemu-conf.c:32:3: error: implicit declaration of function
> > 'xc_hvm_set_mem_type' [-Werror=implicit-function-declaration]
> >   config-temp/qemu-conf.c:32:3: error: nested extern declaration of
> > 'xc_hvm_set_mem_type' [-Werror=nested-externs]
> >   config-temp/qemu-conf.c:34:3: error: implicit declaration of function
> > 'xc_hvm_inject_msi' [-Werror=implicit-function-declaration]
> >   config-temp/qemu-conf.c:34:3: error: nested extern declaration of
> > 'xc_hvm_inject_msi' [-Werror=nested-externs]
> >   config-temp/qemu-conf.c:35:3: error: implicit declaration of function
> > 'xc_hvm_create_ioreq_server' [-Werror=implicit-function-declaration]
> >   config-temp/qemu-conf.c:35:3: error: nested extern declaration of
> > 'xc_hvm_create_ioreq_server' [-Werror=nested-externs]
> > 
> > 
> > With -DXC_WANT_COMPAT_DEVICEMODEL_API=1:
> > 
> >   In file included from /local/qemu-
> > upstream/include/hw/xen/xen_backend.h:4:0,
> >from hw/block/xen_disk.c:27:
> >   /local/qemu-upstream/include/hw/xen/xen_common.h: In function
> > 'xen_set_mem_type':
> >   /local/qemu-upstream/include/hw/xen/xen_common.h:78:5: error: implicit
> > declaration of function 

Re: [Qemu-devel] [RFC PATCH] configure: remove --enable-replication/--disable-replication

[Bruce Rogers]

>>> On 2/6/2017 at 4:57 AM,  wrote:
> * Paolo Bonzini (pbonz...@redhat.com) wrote:
>> 
>> 
>> On 03/02/2017 07:00, Stefan Hajnoczi wrote:
>> > On Thu, Feb 02, 2017 at 07:05:30AM ‑0800, Paolo Bonzini wrote:
>> >> The replication feature is a small amount of code, does not
>> >> require any external library and unless used does not add
>> >> anything to the guest's attack surface.  Since any extra
>> >> configure option affects maintainability on the other hand
>> >> and is subject to bit rot, I think there is no need to
>> >> make it configurable.
>> > 
>> > I think the current state is good: replication is enabled by default but
>> > can be compiled out if desired.
>> > 
>> > Downstreams may not be comfortable supporting this feature yet since
>> > it's incomplete.  It's fair to offer an option to disable it, otherwise
>> > downstreams will have to patch this themselves.
>> 
>> I understand‑‑‑I just am not sure where to draw the line because there's
>> plenty of other incomplete features, hence the RFC.  For example,
>> record/replay cannot be enabled or disabled on the configure command
>> line.  That was the case even in the beginning, where it didn't support
>> either block or character device replay.
> 
> The line is certainly fuzzy, but I think it's worth making the following
> type of things configurable:
>Features that have a large chunk of code
>  ‑ dont lets try and configure tiny things on and off
>That can be trivially configured
>  ‑ lets not put big chunks of code around making them configurable
> and   that are incomplete
>or are unused by large chunks of the users
> 
> Dave
> 
>> ‑‑enable‑coroutine‑pool is a relic of when Windows builds needed it, but
>> all other ‑‑enable‑* options require an external library or at least a
>> specific operating system.  See for example this patch:
>> 
>> commit 52b53c04faab9f7a9879c8dc014930649a3e698d
>> Author: Fam Zheng 
>> Date:   Wed Sep 10 14:17:51 2014 +0800
>> 
>> block: Always compile virtio‑blk dataplane
>> 
>> Dataplane doesn't depend on linux‑aio any more, so we don't need the
>> compiling condition now.
>> 
>> Configure options are kept but just print a message.
>> 
>> Signed‑off‑by: Fam Zheng 
>> Reviewed‑by: Paolo Bonzini 
>> Message‑id: 1410329871‑28885‑4‑git‑send‑email‑f...@redhat.com 
>> Signed‑off‑by: Stefan Hajnoczi 
>> 
>> 
>> I would actually prefer to remove many of the latter
>> (‑‑enable‑vhost‑net, ‑‑enable‑vhost‑scsi, ‑‑enable‑vhost‑socket) and
>> just use default‑configs.  We are already doing it for ivshmem for example:
>> 
>> CONFIG_IVSHMEM=$(CONFIG_EVENTFD)
>> 
>> Paolo
> ‑‑
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Was there ever a conclusion here? The reason I ask is that I see that currently
using --disable-replication fails for me as follows:

# ./configure --disable-replication
...
# make
...
make  all-recursive
Making all in pixman
make[3]: Nothing to be done for 'all'.
Making all in demos
make[3]: Nothing to be done for 'all'.
Making all in test
make[3]: Nothing to be done for 'all'.
CHK version_gen.h
  LINKaarch64-softmmu/qemu-system-aarch64
../migration/colo.o: In function `qmp_query_xen_replication_status':
/home/brogers/osr/git/qemu/migration/colo.c:181: undefined reference to 
`replication_get_error_all'
../migration/colo.o: In function `qmp_xen_set_replication':
/home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to 
`replication_stop_all'
/home/brogers/osr/git/qemu/migration/colo.c:172: undefined reference to 
`replication_stop_all'
/home/brogers/osr/git/qemu/migration/colo.c:167: undefined reference to 
`replication_start_all'
../migration/colo.o: In function `qmp_xen_colo_do_checkpoint':
/ho
me/brogers/osr/git/qemu/migration/colo.c:196: undefined reference to 
`replication_do_checkpoint_all'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:208: qemu-system-aarch64] Error 1
make: *** [Makefile:322: subdir-aarch64-softmmu] Error 2

-- 
Bruce
 

[Qemu-devel] [PATCH v2]util:Removed header qemu-common.h from path.c

[Suramya Shah]

Signed-off-by: Suramya Shah 
--- fix of typo in v1 that broke compilation
 util/path.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/path.c b/util/path.c
index 5479f76..7f9fc27 100644
--- a/util/path.c
+++ b/util/path.c
@@ -6,7 +6,6 @@
 #include "qemu/osdep.h"
 #include 
 #include 
-#include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "qemu/path.h"
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH 09/15] sheepdog: Implement bdrv_parse_filename()

[Eric Blake]

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> This permits configuration with driver-specific options in addition to
> pseudo-filename parsed as URI.  For instance,
> 
> --drive driver=sheepdog,host=fido,vdi=dolly
> 
> instead of
> 
> --drive driver=sheepdog,file=sheepdog://fido/dolly
> 
> It's also a first step towards supporting blockdev-add.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  block/sheepdog.c | 233 
> +--
>  1 file changed, 176 insertions(+), 57 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 07/14] sm501: Fix device endianness

[BALATON Zoltan]

On Fri, 3 Mar 2017, Peter Maydell wrote:

On 3 March 2017 at 02:15, BALATON Zoltan  wrote:

Maybe it's not correct but works for everything I could test better than the
original code (which was broken even for the SH images one can find) so I
think we could just go with this until someone complains and provides a test
case. I've given up on trying to understand it because I don't really know
this device and SH so I could only go by the images I could find and they
seem to like it this way.


So what cases have you tested? The Linux kernel seems to support:
* sh embedded device, little endian
* PCI card, little endian host
* PCI card, big endian host
and there are also
* 16 bit pixel format
* 32 bit pixel format

which makes 6 different combinations.
(It's a shame there's no sh4 BE code to run on this.)


As mentioned before I've used this image to test SH:

https://people.debian.org/~aurel32/qemu/sh4/

with video=800x600-16 kernel parameter where changing -16 to different bit 
depths (8, 32) reproduces the problem. On ppc I've tested with MorphOS 
which uses 16 bpp, Linux and U-boot which I think uses 8 bit. These run on 
real hardware so I think if they work the emulation should be OK.


You've said before that PCI is likely always little endian and SH embedded 
is LE too so unless something uses the device in BE mode (which we don't 
emulate) setting it to little endian should be correct. For frame buffer 
endianness I can only go with what I've seen clients doing and my patch 
does what worked for the above on SH and PPC. I can't prove it's correct 
but works for the systems I've targeted and also fixes the SH case which 
seemed to be broken.



Looking at how the SWAP_FB_ENDIAN flag gets set:
* for the r2d board it is set always
* for PCI devices, it is set only if big-endian

I suspect that what we actually have here is something
like:
* for the PCI device it's always little endian (and the
  code ought not to need to depend on TARGET_WORDS_BIGENDIAN)
* sysbus device may be more complicated

Also I note there's an SM501_ENDIAN_CONTROL register on the
device, which presumably if written changes the behaviour.



I've seen this but it's not emulated so it can be ignored for now. The spec
also says that default is little endian so for regs at least
DEVICE_LITTLE_ENDIAN should be OK.


Yes, that makes sense. So we should be modelling this as an "always
little endian device".

The changes you have to the memory region ops achieve this for
the registers implemented in this device itself. It looks like
SYSBUS_OHCI is already always little endian. You also need to
change the argument to the serial_mm_init() call to pass
DEVICE_LITTLE_ENDIAN.


OK, will do that.

Re: [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

Eric Blake  writes:

> On 03/02/2017 03:44 PM, Markus Armbruster wrote:
>> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
>> the discriminator value for variant InetSocketAddress is 'tcp' instead
>> of 'inet'.  Rename.
>> 
>> The type is far only used by the Gluster block drivers.  Take care to
>> keep 'tcp' working there.
>
> The old name was visible in QMP in 2.8, but only by blockdev-add, which
> we've already argued was not stable (and where we've already made other
> non-back-compat changes to it).  But that means this HAS to go into 2.9,
> if we're declaring blockdev-add stable for 2.9.

Yes.

Note that the command line pseudo-filename's URI syntax stays the same
(file=gluster+tcp://), and the command line's dotted key syntax keeps
accepting tcp for compatiblity (file.server.0.type=tcp works in addition
to =inet).

> It wouldn't hurt to mention that additional information in the commit
> message.

I'll cook something up.

>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  block/gluster.c  | 59 
>> +---
>>  qapi-schema.json |  8 
>>  2 files changed, 35 insertions(+), 32 deletions(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 77ce45b..0155188 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
[...]
>> @@ -536,21 +536,24 @@ static int 
>> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>  
>>  }
>>  gsconf = g_new0(SocketAddressFlat, 1);
>> +if (!strcmp(ptr, "tcp")) {
>> +ptr = "inet";   /* accept legacy "tcp" */
>> +}
>>  gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
>> SOCKET_ADDRESS_FLAT_TYPE__MAX, 0,
>> _err);

This is where I keep file.server.N.type=tcp working.

[...]
>> +++ b/qapi-schema.json
>> @@ -4105,14 +4105,14 @@
>>  #
>>  # Available SocketAddressFlat types
>>  #
>> -# @tcp:   Internet address
>> +# @inet:   Internet address
>>  #
>>  # @unix:  Unix domain socket
>
> Nit: Spacing is now inconsistent.

Will fix.

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails

[Eric Blake]

On 03/03/2017 01:50 PM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>> Fix the design flaw demonstrated in the previous commit: new method
>>> check_list() lets input visitors report that unvisited input remains
>>> for a list, exactly like check_struct() lets them report that
>>> unvisited input remains for a struct or union.
>>>
>>> Implement the method for the qobject input visitor (straightforward),
>>> and the string input visitor (less so, due to the magic list syntax
>>> there).  The opts visitor's list magic is even more impenetrable, and
>>> all I can do there today is a stub with a FIXME comment.  No worse
>>> than before.
>>>
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>
>> Didn't I already review this one?
>>
>> Ah, there's my R-b:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
> 

>>> 
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>>  return container_of(v, QObjectInputVisitor, visitor);
>>>  }
>>>  
>>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char 
>>> *name,
>>> + int n)
>>>  {

No function comment, so the _nth and int n are guesses on their meaning...


>> If I'm reading this right, your use of n-- in the loop followed by the
>> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
>> lets see what callers pass for n:
> 
> At least @n times.

Ah, as in 'use first available result' or 'iterate at least once', based
on our callers, but could also mean 'iterate at least twice' for a
caller that passes 2.


>> the other passes 1.  No other calls.  Did we really need an integer,
>> where we use n--, or would a bool have done as well?
> 
> Since I actually use only 0 and 1, a bool would do, but would it make
> the code simpler?

I don't know that a bool would be any simpler,

> 
>> At any rate, since I've already reviewed it once, you can add R-b, but
>> we may want a followup to make it less confusing.
> 
> Would renaming the function to full_name_but_n() help?

Or even keep the name unchanged, but add function comments describing
what 'n' means.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: fix fd leak in local_opendir()

[Philippe Mathieu-Daudé]

On 03/03/2017 02:54 PM, Daniel P. Berrange wrote:

On Fri, Mar 03, 2017 at 06:52:42PM +0100, Greg Kurz wrote:

Coverity issue CID1371731

Signed-off-by: Greg Kurz 
---
 hw/9pfs/9p-local.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index e31309a29c58..fe930300445a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -435,6 +435,7 @@ static int local_opendir(FsContext *ctx,

 stream = fdopendir(dirfd);
 if (!stream) {
+close(dirfd);
 return -1;
 }
 fs->dir.stream = stream;


Reviewed-by: Daniel P. Berrange 

Regards,
Daniel



Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-devel] [PATCH] 9pfs: fail local_statfs() earlier

[Philippe Mathieu-Daudé]

On 03/03/2017 03:03 PM, Greg Kurz wrote:

If we cannot open the given path, we can return right away instead of
passing -1 to fstatfs() and close(). This will make Coverity happy.

(Coverity issue CID1371729)

Signed-off-by: Greg Kurz 


Reviewed-by: Philippe Mathieu-Daudé 


---
 hw/9pfs/9p-local.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index fe930300445a..ea9a1ced0394 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1053,6 +1053,9 @@ static int local_statfs(FsContext *s, V9fsPath *fs_path, 
struct statfs *stbuf)
 int fd, ret;

 fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0);
+if (fd == -1) {
+return -1;
+}
 ret = fstatfs(fd, stbuf);
 close_preserve_errno(fd);
 return ret;

Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation

[Eric Blake]

On 03/03/2017 01:45 PM, Markus Armbruster wrote:

>>> -rsp = qmp_dispatch(_commands, req);
>>> +qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
>>> +if (qdict) {
>>> +if (mon->qmp.commands == _cap_negotiation_commands
>>> +&& !g_strcmp0(qdict_get_try_str(qdict, "class"),
>>> +QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) 
>>> {
>>
>> Could join these two 'if' into one, for less {}, but that's cosmetic.
> 
> Or maybe get reshuffle so that qdict_get_qdict() is called only when
> needed:
> 
> if (mon->qmp.commands == _cap_negotiation_commands) {
> qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
> if (qdict
> && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
> /* Provide a more useful error message */

Yes, that's even nicer (it's probably in the noise, but
micro-optimizations are fun!)


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably

[Philippe Mathieu-Daudé]

On 03/03/2017 09:32 AM, Markus Armbruster wrote:

qobject_input_start_struct() sets *list, except when it fails because
qobject_input_get_object() fails, i.e. the input object doesn't exist.

All the other input visitor start_struct(), start_list(),
start_alternate() always set *obj / *list.

Change qobject_input_start_struct() to match.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---
 qapi/qobject-input-visitor.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 2c2f883..d58696c 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -196,25 +196,21 @@ static void qobject_input_start_list(Visitor *v, const 
char *name,
 QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
 const QListEntry *entry;

+if (list) {
+*list = NULL;
+}
 if (!qobj) {
 return;
 }
 if (qobject_type(qobj) != QTYPE_QLIST) {
-if (list) {
-*list = NULL;
-}
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"list");
 return;
 }

 entry = qobject_input_push(qiv, qobj, list);
-if (list) {
-if (entry) {
-*list = g_malloc0(size);
-} else {
-*list = NULL;
-}
+if (entry && list) {
+*list = g_malloc0(size);
 }
 }

Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages

[Philippe Mathieu-Daudé]

Hi Markus,

On 03/03/2017 09:32 AM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 qapi/qmp-dispatch.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 23b0528..578c6d8 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)

 dict = qobject_to_qdict(request);
 if (!dict) {
-error_setg(errp, "Expected '%s' in QMP input", "object");
+error_setg(errp, "QMP input must be a JSON object");
 return NULL;
 }

@@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject 
*request, Error **errp)

 if (!strcmp(arg_name, "execute")) {
 if (qobject_type(arg_obj) != QTYPE_QSTRING) {
-error_setg(errp, "QMP input object member '%s' expects '%s'",
-   "execute", "string");
+error_setg(errp,
+   "QMP input object member '%s' must be %s",
+   "execute", "a string");


let's avoid formatting like the rest of this patch.


 return NULL;
 }
 has_exec_key = true;
 } else if (!strcmp(arg_name, "arguments")) {
 if (qobject_type(arg_obj) != QTYPE_QDICT) {
-error_setg(errp, "QMP input object member '%s' expects '%s'",
-   "arguments", "object");
+error_setg(errp,
+   "QMP input object member '%s' must be %s",
+   "arguments", "an object");


same.


 return NULL;
 }
 } else {
@@ -60,7 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 }

 if (!has_exec_key) {
-error_setg(errp, "Expected '%s' in QMP input", "execute");
+error_setg(errp, "QMP input object lacks key 'execute'");
 return NULL;
 }

Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program

[Eric Blake]

On 03/03/2017 01:37 PM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>> The command registry encapsulates a single command list.  Give the
>>> functions using it a parameter instead.  Define suitable command lists
>>> in monitor, guest agent and test-qmp-commands.
>>>

>> ...or is this leftovers from your debugging?
> 
> Corret.  I'll drop it.
> 
>> The rest of the patch looks fine; it is converting a global variable
>> into a per-instance variable.
> 
> Squashing in the obvious garbage collection.  May I add your R-by?
> 

Yes, with that squashed in,
Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Richard Henderson]

On 03/03/2017 09:03 PM, Alex Bennée wrote:

We *should* have retaddr == 0 for this case, which indicates that we
should not attempt to restore state.  Are you seeing a non-zero value?


Actually looking at xtensa I see:

  Attempt to resolve CPU state @ 0x0 while translating

So maybe I should check just that - but I don't see where we ensure we
always pass zero.


cpu_ld*_cmmu, in include/exec/cpu_ldst_template.h, has

return glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);

so there's your zero.


#0  0x555e3712 in cpu_restore_state (cpu=cpu@entry=0x56032600, 
retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
#1  0x5564cb38 in tlb_fill (cs=cs@entry=0x56032600, 
vaddr=vaddr@entry=537034752, access_type=access_type@entry=MMU_INST_FETCH, 
mmu_idx=mmu_idx@entry=1, retaddr=retaddr@entry=0) at 
/home/alex/lsrc/qemu/qemu.git/target/xtensa/op_helper.c:73
#2  0x5562d604 in helper_ret_ldb_cmmu (env=env@entry=0x5603a890, 
addr=537034752, oi=, retaddr=retaddr@entry=0) at 
/home/alex/lsrc/qemu/qemu.git/softmmu_template.h:127


Confirmed.  This is a simple bug in xtensa -- failure to check retaddr == 0 
before calling cpu_restore_state.


That said, I do wonder if it wouldn't be better to move that check inside 
cpu_restore_state instead.  Put the check there now, but leave the follow-on 
cleanup for the next devel cycle.


It would also save auditing the other usages of cpu_restore_state in the tree.


Is this in fact linux-user, not softmmu, as you imply from tlb_fill?
Because handle_cpu_signal will in fact pass a genuine non-zero retaddr
for the SIGSEGV resulting from a cpu_ld*_code from a non-mapped
address.


I think that is another call chain that might trip us up. Peter
mentioned he'd hit it. This one is definitely softmmu.


This is really easy to reproduce on any guest with a call to a null function 
pointer.



r~

Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails

[Markus Armbruster]

Eric Blake  writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> Fix the design flaw demonstrated in the previous commit: new method
>> check_list() lets input visitors report that unvisited input remains
>> for a list, exactly like check_struct() lets them report that
>> unvisited input remains for a struct or union.
>> 
>> Implement the method for the qobject input visitor (straightforward),
>> and the string input visitor (less so, due to the magic list syntax
>> there).  The opts visitor's list magic is even more impenetrable, and
>> all I can do there today is a stub with a FIXME comment.  No worse
>> than before.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>
> Didn't I already review this one?
>
> Ah, there's my R-b:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html

Oops!  My apologies...

> But since it disappeared again, I had another look.
>
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>  return container_of(v, QObjectInputVisitor, visitor);
>>  }
>>  
>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>> + int n)
>>  {
>>  StackObject *so;
>>  char buf[32];
>> @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, 
>> const char *name)
>>  }
>>  
>>  QSLIST_FOREACH(so , >stack, node) {
>> -if (qobject_type(so->obj) == QTYPE_QDICT) {
>> -g_string_prepend(qiv->errname, name);
>> +if (n) {
>> +n--;
>> +} else if (qobject_type(so->obj) == QTYPE_QDICT) {
>> +g_string_prepend(qiv->errname, name ?: "");
>>  g_string_prepend_c(qiv->errname, '.');
>>  } else {
>>  snprintf(buf, sizeof(buf), "[%u]", so->index);
>> @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, 
>> const char *name)
>>  }
>>  name = so->name;
>>  }
>> +assert(!n);
>
> If I'm reading this right, your use of n-- in the loop followed by the
> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
> lets see what callers pass for n:

At least @n times.

>>  
>>  if (name) {
>>  g_string_prepend(qiv->errname, name);
>>  } else if (qiv->errname->str[0] == '.') {
>>  g_string_erase(qiv->errname, 0, 1);
>> -} else {
>> +} else if (!qiv->errname->str[0]) {
>>  return "";
>>  }
>>  
>>  return qiv->errname->str;
>>  }
>>  
>> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +{
>> +return full_name_nth(qiv, name, 0);
>> +}
>
> One caller passes 0,
>
>
>> +static void qobject_input_check_list(Visitor *v, Error **errp)
>> +{
>> +QObjectInputVisitor *qiv = to_qiv(v);
>> +StackObject *tos = QSLIST_FIRST(>stack);
>> +
>> +assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
>> +
>> +if (tos->entry) {
>> +error_setg(errp, "Only %u list elements expected in %s",
>> +   tos->index + 1, full_name_nth(qiv, NULL, 1));
>
> the other passes 1.  No other calls.  Did we really need an integer,
> where we use n--, or would a bool have done as well?

Since I actually use only 0 and 1, a bool would do, but would it make
the code simpler?

> At any rate, since I've already reviewed it once, you can add R-b, but
> we may want a followup to make it less confusing.

Would renaming the function to full_name_but_n() help?

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Eric Blake]

On 03/03/2017 01:47 PM, Eric Blake wrote:
> On 03/03/2017 01:35 PM, Richard Henderson wrote:
>>
>> Which is exactly the point when you have a condition like (X > 0);
>> letting the compiler have the same information for the production build
>> that it would have gleaned from the debug build.
>>
>> But that's not the same as dropping the assert, which is what you wanted.
>>
>> On the other hand, isn't "assert" instead of "g_assert" exactly what you
>> wanted?  Don't we define NDEBUG for production builds?
> 
> No - no one in their right mind defines NDEBUG for qemu.  For better or
> worse, there are too many places where we liberally use assert(), and
> where the code WILL crash and burn when it falls through to subsequent
> code if a failed assert() is not equivalent to a fatal exit.  (I still
> try to make sure we avoid any new side-effects in assert in my code
> reviews, but there's no way you'll convince me to audit the code base
> for NDEBUG-safety violations).

A quick git grep shows, among others:

hw/scsi/mptsas.c: * When we do, we might be able to re-enable NDEBUG
below.
hw/scsi/mptsas.c:#ifdef NDEBUG
hw/scsi/mptsas.c:#error building with NDEBUG is not supported
hw/virtio/virtio.c: * When we do, we might be able to re-enable
NDEBUG below.
hw/virtio/virtio.c:#ifdef NDEBUG
hw/virtio/virtio.c:#error building with NDEBUG is not supported


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Eric Blake]

On 03/03/2017 01:35 PM, Richard Henderson wrote:
> 
> Which is exactly the point when you have a condition like (X > 0);
> letting the compiler have the same information for the production build
> that it would have gleaned from the debug build.
> 
> But that's not the same as dropping the assert, which is what you wanted.
> 
> On the other hand, isn't "assert" instead of "g_assert" exactly what you
> wanted?  Don't we define NDEBUG for production builds?

No - no one in their right mind defines NDEBUG for qemu.  For better or
worse, there are too many places where we liberally use assert(), and
where the code WILL crash and burn when it falls through to subsequent
code if a failed assert() is not equivalent to a fatal exit.  (I still
try to make sure we avoid any new side-effects in assert in my code
reviews, but there's no way you'll convince me to audit the code base
for NDEBUG-safety violations).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 0/2] submodule-update queue 20170303

[James Hanley]

I'm trying to clone clean and I'm getting the following when initing the
submodules updated:

jim@jim-VirtualBox:~/project/lg_4k_test/qemu_pristine$ ( git submodule
sync; git submodule update --init --recursive )
Submodule 'dtc' (http://git.qemu-project.org/git/dtc.git) registered for
path 'dtc'
Submodule 'pixman' (https://anongit.freedesktop.org/git/pixman.git)
registered for path 'pixman'
Submodule 'roms/SLOF' (http://git.qemu-project.org/git/SLOF.git) registered
for path 'roms/SLOF'
Submodule 'roms/ipxe' (http://git.qemu-project.org/git/ipxe.git) registered
for path 'roms/ipxe'
Submodule 'roms/openbios' (http://git.qemu-project.org/git/openbios.git)
registered for path 'roms/openbios'
Submodule 'roms/openhackware' (
http://git.qemu-project.org/git/openhackware.git) registered for path
'roms/openhackware'
Submodule 'roms/qemu-palcode' (https://github.com/rth7680/qemu-palcode.git)
registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (http://git.qemu-project.org/git/seabios.git)
registered for path 'roms/seabios'
Submodule 'roms/sgabios' (http://git.qemu-project.org/git/sgabios.git)
registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (http://git.qemu-project.org/git/skiboot.git)
registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (http://git.qemu-project.org/git/u-boot.git)
registered for path 'roms/u-boot'
Submodule 'roms/vgabios' (http://git.qemu-project.org/git/vgabios.git)
registered for path 'roms/vgabios'
Cloning into 'dtc'...
Checking connectivity... done.
fatal: reference is not a tree: fa8bc7f928ac25f23532afc8beb2073efc8fb063
Cloning into 'pixman'...
remote: Counting objects: 11541, done.
remote: Compressing objects: 100% (2702/2702), done.
remote: Total 11541 (delta 8882), reused 11467 (delta 8837)
Receiving objects: 100% (11541/11541), 2.64 MiB | 1.77 MiB/s, done.
Resolving deltas: 100% (8882/8882), done.
Checking connectivity... done.
Submodule path 'pixman': checked out
'87eea99e443b389c978cf37efc52788bf03a0ee0'
Cloning into 'roms/SLOF'...
Checking connectivity... done.
fatal: reference is not a tree: 66d250ef0fd06bb88b7399b9563b5008201f2d63
Cloning into 'roms/ipxe'...
Checking connectivity... done.
Submodule path 'roms/ipxe': checked out
'b991c67c1d91574ef22336cc3a5944d1e63230c9'
Cloning into 'roms/openbios'...
Checking connectivity... done.
Submodule path 'roms/openbios': checked out
'0cd97cc904e71fbb461112f6756934ec6af890b8'
Cloning into 'roms/openhackware'...
Checking connectivity... done.
Submodule path 'roms/openhackware': checked out
'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/qemu-palcode'...
remote: Counting objects: 279, done.
remote: Total 279 (delta 0), reused 0 (delta 0), pack-reused 279
Receiving objects: 100% (279/279), 143.11 KiB | 0 bytes/s, done.
Resolving deltas: 100% (171/171), done.
Checking connectivity... done.
Submodule path 'roms/qemu-palcode': checked out
'f3c7e44c70254975df2a00af39701eafbac4d471'
Cloning into 'roms/seabios'...
Checking connectivity... done.
fatal: reference is not a tree: 5f4c7b13cdf9c450eb55645f4362ea58fa61b79b
Cloning into 'roms/sgabios'...
Checking connectivity... done.
Submodule path 'roms/sgabios': checked out
'23d474943dcd55d0550a3d20b3d30e9040a4f15b'
Cloning into 'roms/skiboot'...
Checking connectivity... done.
Submodule path 'roms/skiboot': checked out
'762d0082f18e4fb921a2d44a1051b02d8b0f6381'
Cloning into 'roms/u-boot'...
Checking connectivity... done.
Submodule path 'roms/u-boot': checked out
'2072e7262965bb48d7fffb1e283101e6ed8b21a8'
Cloning into 'roms/vgabios'...
Checking connectivity... done.
Submodule path 'roms/vgabios': checked out
'19ea12c230ded95928ecaef0db47a82231c2e485'
Unable to checkout 'fa8bc7f928ac25f23532afc8beb2073efc8fb063' in submodule
path 'dtc'
Unable to checkout '66d250ef0fd06bb88b7399b9563b5008201f2d63' in submodule
path 'roms/SLOF'
Unable to checkout '5f4c7b13cdf9c450eb55645f4362ea58fa61b79b' in submodule
path 'roms/seabios'
jim@jim-VirtualBox:~/project/lg_4k_test/qemu_pristine$


It's unclear to me what should be done for a pristine clone - is there a
step missing from pulling those submodules?

-Jim

Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation

[Markus Armbruster]

Eric Blake  writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> To enforce capability negotiation before normal operation,
>> handle_qmp_command() inspects every command before it's handed off to
>> qmp_dispatch().  This is a bit of a layering violation, and results in
>> duplicated code.
>> 
>> Before capability negotiation (!cur_mon->in_command_mode), we fail
>> commands other than "qmp_capabilities".  This is what enforces
>> capability negotiation.
>> 
>> Afterwards, we fail command "qmp_capabilities".
>> 
>> Clean this up as follows.
>> 
>> The obvious place to fail a command is the command itself, so move the
>> "afterwards" check to qmp_qmp_capabilities().
>> 
>> We do the "before" check in every other command, but that would be
>> bothersome.  Instead, start with an alternate list of commant that
>
> s/commant/commands/

Fixed.

>> contains only "qmp_capabilities".  Switch to the full list in
>> qmp_qmp_capabilities().
>> 
>> Additionally, replace the generic human-readable error message for
>> CommandNotFound by one that reminds the user to run qmp_capabilities.
>> Without that, we'd regress commit 2d5a834.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  monitor.c | 76 
>> +++
>>  1 file changed, 42 insertions(+), 34 deletions(-)
>> 
>
>> @@ -3786,11 +3785,20 @@ static void handle_qmp_command(JSONMessageParser 
>> *parser, GQueue *tokens)
>>  cmd_name = qdict_get_str(qdict, "execute");
>>  trace_handle_qmp_command(mon, cmd_name);
>>  
>> -if (invalid_qmp_mode(mon, cmd_name, )) {
>> -goto err_out;
>> -}
>> +rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>>  
>> -rsp = qmp_dispatch(_commands, req);
>> +qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
>> +if (qdict) {
>> +if (mon->qmp.commands == _cap_negotiation_commands
>> +&& !g_strcmp0(qdict_get_try_str(qdict, "class"),
>> +QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
>
> Could join these two 'if' into one, for less {}, but that's cosmetic.

Or maybe get reshuffle so that qdict_get_qdict() is called only when
needed:

if (mon->qmp.commands == _cap_negotiation_commands) {
qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
if (qdict
&& !g_strcmp0(qdict_get_try_str(qdict, "class"),
QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
/* Provide a more useful error message */

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH 1/1] virtio-blk: fix race on guest notifiers

[Halil Pasic]

On 03/02/2017 05:21 PM, Paolo Bonzini wrote:
> 
> 
> On 02/03/2017 16:55, Halil Pasic wrote:
>  blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
>> I'm wondering if synchronization is needed for batch_notify_vqs. I think
>> the set_bit can be from the iothread, but the notify_guest_bh below is main
>> event loop. Is it OK like this (could we miss bits set in other thread)?
>> Does blk_set_aio_context include a barrier?
> 
> Another good question---yes, it does (via bdrv_set_aio_context's call to
> aio_context_acquire).
> 
> But it's something to remember for when I get round to removing
> aio_context_acquire!

Uh, this is complicated. I'm not out of questions, but I fear taking to
much of your precious time. I will ask again nevertheless, but please
just cut the conversation with -EBUSY if it gets to expensive.

What I understand is the batch_notify_vqs is effectively protected
by blk_get_aio_context(s->conf.conf.blk)->lock which might or might not
be the same as qemu_get_aio_context()->lock. If it ain't the same
that means we are running with iothread. Now let us have a look
at the drain. IFAIU in

#define BDRV_POLL_WHILE(bs, cond) ({   \
bool waited_ = false;  \
BlockDriverState *bs_ = (bs);  \
AioContext *ctx_ = bdrv_get_aio_context(bs_);  \
if (aio_context_in_iothread(ctx_)) {   \
while ((cond)) {   \
aio_poll(ctx_, true);  \
waited_ = true;\
}  \
} else {   \
assert(qemu_get_current_aio_context() ==   \
   qemu_get_aio_context());\
/* Ask bdrv_dec_in_flight to wake up the main  \
 * QEMU AioContext.  Extra I/O threads never take  \
 * other I/O threads' AioContexts (see for example \
 * block_job_defer_to_main_loop for how to do it). \
 */\
assert(!bs_->wakeup);  \
bs_->wakeup = true;\
while ((cond)) {   \
aio_context_release(ctx_); \
aio_poll(qemu_get_aio_context(), true);\
aio_context_acquire(ctx_); \
waited_ = true;\
}  \
bs_->wakeup = false;   \
}  \
waited_; })

where it's interesting (me running with 2 iothreads one assigned to my
block device) we are gonna take  the "else" branch , a will end up
releasing the ctx belonging to the iothread and then acquire it again,
and basically wait till the requests are done.

Since is virtio_blk_rw_complete acquiring-releasing the same ctx this
makes a lot of sense. If we would not release the ctx we would end up
with a deadlock.


What I do not understand entirely are the atomic_read/write on
bs->in_flight which tells us when are we done waiting. In

static void blk_aio_complete(BlkAioEmAIOCB *acb)
{
if (acb->has_returned) {
bdrv_dec_in_flight(acb->common.bs);
acb->common.cb(acb->common.opaque, acb->rwco.ret);
qemu_aio_unref(acb);
}
}

the atomic decrements is before the callback, that is
virtio_blk_rw_complete called. My guess here is, that this does not
matter, because we are holding the ctx anyway, so it is ensured that we
will not observe 0 until the last virtio_blk_rw_complete completed (mutex
is recursive). But then I do not understand what is the point acquiring
the mutex in virtio_blk_rw_complete?

TL;DR

Is patch b9e413dd required for the correctness of this patch?

What is the role of the aio_context_acquire/release introduced by
b9e413dd in virtio_blk_rw_complete?

Regards,
Halil

Re: [Qemu-devel] [PATCH for-2.9 5/6] disas/cris: Avoid unintended sign extension

[Philippe Mathieu-Daudé]

On 03/03/2017 12:58 PM, Edgar E. Iglesias wrote:

On Fri, Mar 03, 2017 at 03:50:32PM +, Peter Maydell wrote:

In the cris disassembler we were using 'unsigned long' to calculate
addresses which are supposed to be 32 bits.  This meant that we might
accidentally sign extend or calculate a value that was outside the 32
bit range of the guest CPU.  Use 'uint32_t' instead so we give the
right answers on 64-bit hosts.

(Spotted by Coverity, CID 1005402, 1005403.)


Reviewed-by: Edgar E. Iglesias 



Reviewed-by: Philippe Mathieu-Daudé 





Signed-off-by: Peter Maydell 
---
 disas/cris.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/disas/cris.c b/disas/cris.c
index 8a1daf9..30217f1 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -2009,7 +2009,7 @@ print_with_operands (const struct cris_opcode *opcodep,
   case 'n':
{
  /* Like N but pc-relative to the start of the insn.  */
- unsigned long number
+ uint32_t number
= (buffer[2] + buffer[3] * 256 + buffer[4] * 65536
   + buffer[5] * 0x100 + addr);

@@ -2201,7 +2201,7 @@ print_with_operands (const struct cris_opcode *opcodep,
  {
/* It's [pc+].  This cannot possibly be anything
   but an address.  */
-   unsigned long number
+   uint32_t number
  = prefix_buffer[2] + prefix_buffer[3] * 256
  + prefix_buffer[4] * 65536
  + prefix_buffer[5] * 0x100;
--
2.7.4

Re: [Qemu-devel] [PATCH RESEND] qdev: Make "hotplugged" property read-only

[Eduardo Habkost]

On Mon, Feb 27, 2017 at 07:05:24PM +0100, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > The "hotplugged" property is user visible, but it was never meant
> > to be set by the user. There are probably multiple ways to break
> > or crash device code by overriding the property. For example, we
> > recently fixed a crash in rtc_set_memory() related to the
> > property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d).
> >
> > There has been some discussion about making management software
> > use "hotplugged=on" on migration, to indicate devices that were
> > hotplugged in the migration source. There were other suggestions
> > to address this, like including the "hotplugged" field in the
> > migration stream instead of requiring it to be set propertly.
> >
> > Whatever solution we choose in the future, this patch disables
> > setting "hotplugged" explicitly in the command-line by now,
> > because the ability to set the property is unused, untested, and
> > undocumented.
> >
> > Signed-off-by: Eduardo Habkost 
> 
> I figure this is the safe thing to do for 2.9.
> 
> Reviewed-by: Markus Armbruster 

Thanks. I have merged it to my "machine" branch, and I plan to
include it in a pull request next week.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C

[Markus Armbruster]

Eric Blake  writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  scripts/qapi-introspect.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Eric Blake 
>
> I'm guessing we haven't seen a use of a prefix that matters yet, but
> that an upcoming patch triggered a compilation failure without this fix.
>  Mentioning that in the commit message wouldn't hurt.

Done.  Thanks!


[...]

Re: [Qemu-devel] [PATCH for-2.9 1/6] disas/hppa: Remove dead code

[Philippe Mathieu-Daudé]

On 03/03/2017 12:50 PM, Peter Maydell wrote:

Coverity complains (CID 1302705) that the "fr0" part of the ?: in
fput_fp_reg_r() is dead.  This looks like cut-n-paste error from
fput_fp_reg(); delete the dead code.

Signed-off-by: Peter Maydell 


Reviewed-by: Philippe Mathieu-Daudé 


---
 disas/hppa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/disas/hppa.c b/disas/hppa.c
index 43facdc..a2d371f 100644
--- a/disas/hppa.c
+++ b/disas/hppa.c
@@ -1788,8 +1788,7 @@ fput_fp_reg_r (unsigned reg, disassemble_info *info)
   if (reg < 4)
 (*info->fprintf_func) (info->stream, "fpe%d", reg * 2 + 1);
   else
-(*info->fprintf_func) (info->stream, "%sR",
-  reg ? fp_reg_names[reg] : "fr0");
+(*info->fprintf_func) (info->stream, "%sR", fp_reg_names[reg]);
 }

 static void

Re: [Qemu-devel] -rtc clock=vm with -icount 1, sleep=off introduces unexpected delays in device interactions

[Nutaro, James J.]

My original problem seems to stem from something that changed in the way that 
device emulation and instruction execution interact (I'm guessing). To 
reproduce the issue, I started a linux image with

qemu-system-i386 -rtc clock=vm -monitor none -icount 1,sleep=off jack.img

After booting, I run

ping localhost

The first round trip time reports look reasonable, being in the millisecond to 
sub-millisecond range. These occur while the emulator is running slower than 
real time. 

After a bit, the emulator speeds up (skipping over idle periods during I/O?) 
and the round trip times jump to hundreds of milliseconds, which I had not 
expected.

If you want to try this experiment yourself, you can get the disk image that I 
used from here:

http://www.ornl.gov/~1qn/qemu-images/jack.img


Jim

Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program

[Markus Armbruster]

Eric Blake  writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> The command registry encapsulates a single command list.  Give the
>> functions using it a parameter instead.  Define suitable command lists
>> in monitor, guest agent and test-qmp-commands.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  include/qapi/qmp/dispatch.h | 22 ++
>>  monitor.c   | 31 +--
>>  qapi/qmp-dispatch.c | 17 +
>>  qapi/qmp-registry.c | 37 ++---
>>  qga/commands.c  |  2 +-
>>  qga/guest-agent-core.h  |  2 ++
>>  qga/main.c  | 19 ++-
>>  scripts/qapi-commands.py| 16 ++--
>>  tests/test-qmp-commands.c   | 12 +++-
>>  9 files changed, 92 insertions(+), 66 deletions(-)
>> 
>
>> +++ b/qapi/qmp-dispatch.c
>> @@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject 
>> *request, Error **errp)
>>  return dict;
>>  }
>>  
>> -static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>> +volatile QmpCommand *save_cmd;
>
> A comment why volatile is necessary would be nice...

Uh...

>> +QmpCommand cmd2;
>> +
>> +static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
>> +Error **errp)
>>  {
>>  Error *local_err = NULL;
>>  const char *command;
>> @@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
>> **errp)
>>  }
>>  
>>  command = qdict_get_str(dict, "execute");
>> -cmd = qmp_find_command(command);
>> +cmd = qmp_find_command(cmds, command);
>>  if (cmd == NULL) {
>>  error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>>"The command %s has not been found", command);
>> @@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
>> **errp)
>>  return NULL;
>>  }
>>  
>> +assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> +save_cmd = cmd;
>> +cmd2 = *cmd;
>>  if (!qdict_haskey(dict, "arguments")) {
>>  args = qdict_new();
>>  } else {
>> @@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
>> **errp)
>>  
>>  QDECREF(args);
>>  
>> +assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> +assert(ret || local_err);
>
> ...or is this leftovers from your debugging?

Corret.  I'll drop it.

> The rest of the patch looks fine; it is converting a global variable
> into a per-instance variable.

Squashing in the obvious garbage collection.  May I add your R-by?



diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 95a0f48..72827a3 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -67,9 +67,6 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 return dict;
 }
 
-volatile QmpCommand *save_cmd;
-QmpCommand cmd2;
-
 static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
 Error **errp)
 {
@@ -97,9 +94,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject 
*request,
 return NULL;
 }
 
-assert(!cmd->options & QCO_NO_SUCCESS_RESP);
-save_cmd = cmd;
-cmd2 = *cmd;
 if (!qdict_haskey(dict, "arguments")) {
 args = qdict_new();
 } else {
@@ -118,8 +112,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, 
QObject *request,
 
 QDECREF(args);
 
-assert(!cmd->options & QCO_NO_SUCCESS_RESP);
-assert(ret || local_err);
 return ret;
 }
 

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Richard Henderson]

On 03/03/2017 10:19 PM, Peter Maydell wrote:

On 3 March 2017 at 11:05, Alex Bennée  wrote:

According to the commit that added it
(c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to
the compiler. Reading the GCC notes however seems to contradict that.

FWIW I did test it in both builds and we do used tese for a bunch of
other asserts and they haven't blown up.


If what we want is "don't actually check this condition in
the non-tcg-debug config" then we should do something
that means we don't actually check the condition...


Hmm:

28  intptr_t qemu_real_host_page_mask;
29
30  #ifndef CONFIG_USER_ONLY
31  /* mask must never be zero, except for A20 change call */
32  static void tcg_handle_interrupt(CPUState *cpu, int mask)
33  {
34  int old_mask;
35  tcg_debug_assert(qemu_mutex_iothread_locked());
36
37  old_mask = cpu->interrupt_request;
Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a 
 but contains no code.
Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a 
 and ends at 0x24db0f .
Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f 
 but contains no code.
Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f 
 and ends at 0x24db15 .
   0x24db0a :  callq  0x27a570 

   0x24db0f :  mov0xa8(%rbx),%ebp
   0x24db15 :  mov%r12d,%eax
   0x24db18 :  mov%rbx,%rdi
   0x24db1b :  or %ebp,%eax
   0x24db1d :  mov%eax,0xa8(%rbx)
   0x24db23 :  callq  0x27a530 

It certainly looks as though it makes the call but ignores the result?


That is one permitted implementation of the undefined behaviour,
certainly. Not the only one, though. In particular you're telling
the compiler's optimization passes "this can never be reached"
which can result in the optimizer making significantly different
decisions (especially if it manages to inline things or it can
look "inside" the condition being asserted here, etc etc).


Yep, Peter's right.

Which is exactly the point when you have a condition like (X > 0); letting the 
compiler have the same information for the production build that it would have 
gleaned from the debug build.


But that's not the same as dropping the assert, which is what you wanted.

On the other hand, isn't "assert" instead of "g_assert" exactly what you 
wanted?  Don't we define NDEBUG for production builds?



r~

Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history

[John Snow]

On 03/03/2017 02:26 PM, Nir Soffer wrote:
> On Fri, Mar 3, 2017 at 8:54 PM, John Snow  wrote:
>> Use the existing readline history function we are utilizing
>> to provide persistent command history across instances of qmp-shell.
>>
>> This assists entering debug commands across sessions that may be
>> interrupted by QEMU sessions terminating, where the qmp-shell has
>> to be relaunched.
>>
>> Signed-off-by: John Snow 
>> ---
>>
>> v2: Adjusted the errors to whine about non-ENOENT errors, but still
>> intercept all errors as non-fatal.
>> Save history atexit() to match bash standard behavior
>>
>>  scripts/qmp/qmp-shell | 19 +++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>> index 0373b24..55a8285 100755
>> --- a/scripts/qmp/qmp-shell
>> +++ b/scripts/qmp/qmp-shell
>> @@ -70,6 +70,9 @@ import json
>>  import ast
>>  import readline
>>  import sys
>> +import os
>> +import errno
>> +import atexit
>>
>>  class QMPCompleter(list):
>>  def complete(self, text, state):
>> @@ -109,6 +112,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>  self._pretty = pretty
>>  self._transmode = False
>>  self._actions = list()
>> +self._histfile = os.path.join(os.path.expanduser('~'), 
>> '.qmp_history')
>>
>>  def __get_address(self, arg):
>>  """
>> @@ -137,6 +141,21 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>  # XXX: default delimiters conflict with some command names (eg. 
>> query-),
>>  # clearing everything as it doesn't seem to matter
>>  readline.set_completer_delims('')
>> +try:
>> +readline.read_history_file(self._histfile)
>> +except Exception as e:
>> +if isinstance(e, IOError) and e.errno == errno.ENOENT:
>> +# File not found. No problem.
>> +pass
>> +else:
>> +print "Failed to read history '%s'; %s" % (self._histfile, 
>> e)
> 
> I would handle only IOError, since any other error means a bug in this code
> or in the underlying readline library, and the best way to handle this is to
> let it fail loudly.
> 

Disagree. No reason to stop the shell from working because a QOL feature
didn't initialize correctly.

The warning will be enough to solicit reports and fixes if necessary.

>> +atexit.register(self.__save_history)
>> +
>> +def __save_history(self):
>> +try:
>> +readline.write_history_file(self._histfile)
>> +except Exception as e:
>> +print "Failed to save history file '%s'; %s" % (self._histfile, 
>> e)
>>
>>  def __parse_value(self, val):
>>  try:
> 
> But I think this is good enough and useful as is.
> 
> Reviewed-by: Nir Soffer 
> 

Re: [Qemu-devel] [PATCH v2 02/11] target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO

[Eduardo Habkost]

On Thu, Mar 02, 2017 at 07:53:28PM +, Alex Bennée wrote:
> This suppresses the incorrect warning when forcing MTTCG for x86
> guests on x86 hosts. A future patch will still warn when
> TARGET_SUPPORT_MTTCG hasn't been defined for the guest (which is still
> pending for x86).
> 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Richard Henderson 

Reviewed-by: Eduardo Habkost 

I assume the whole series is going to be merged through the same
tree, so:

Acked-by: Eduardo Habkost 

> ---
>  target/i386/cpu.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 12a39d590f..ecdd3bbc2a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -30,6 +30,9 @@
>  #define TARGET_LONG_BITS 32
>  #endif
>  
> +/* The x86 has a strong memory model with some store-after-load re-ordering 
> */
> +#define TCG_GUEST_DEFAULT_MO  (TCG_MO_ALL & ~TCG_MO_ST_LD)
> +
>  /* Maximum instruction code size */
>  #define TARGET_MAX_INSN_SIZE 16
>  
> -- 
> 2.11.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history

[Nir Soffer]

On Fri, Mar 3, 2017 at 8:54 PM, John Snow  wrote:
> Use the existing readline history function we are utilizing
> to provide persistent command history across instances of qmp-shell.
>
> This assists entering debug commands across sessions that may be
> interrupted by QEMU sessions terminating, where the qmp-shell has
> to be relaunched.
>
> Signed-off-by: John Snow 
> ---
>
> v2: Adjusted the errors to whine about non-ENOENT errors, but still
> intercept all errors as non-fatal.
> Save history atexit() to match bash standard behavior
>
>  scripts/qmp/qmp-shell | 19 +++
>  1 file changed, 19 insertions(+)
>
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index 0373b24..55a8285 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -70,6 +70,9 @@ import json
>  import ast
>  import readline
>  import sys
> +import os
> +import errno
> +import atexit
>
>  class QMPCompleter(list):
>  def complete(self, text, state):
> @@ -109,6 +112,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>  self._pretty = pretty
>  self._transmode = False
>  self._actions = list()
> +self._histfile = os.path.join(os.path.expanduser('~'), 
> '.qmp_history')
>
>  def __get_address(self, arg):
>  """
> @@ -137,6 +141,21 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>  # XXX: default delimiters conflict with some command names (eg. 
> query-),
>  # clearing everything as it doesn't seem to matter
>  readline.set_completer_delims('')
> +try:
> +readline.read_history_file(self._histfile)
> +except Exception as e:
> +if isinstance(e, IOError) and e.errno == errno.ENOENT:
> +# File not found. No problem.
> +pass
> +else:
> +print "Failed to read history '%s'; %s" % (self._histfile, e)

I would handle only IOError, since any other error means a bug in this code
or in the underlying readline library, and the best way to handle this is to
let it fail loudly.

> +atexit.register(self.__save_history)
> +
> +def __save_history(self):
> +try:
> +readline.write_history_file(self._histfile)
> +except Exception as e:
> +print "Failed to save history file '%s'; %s" % (self._histfile, 
> e)
>
>  def __parse_value(self, val):
>  try:

But I think this is good enough and useful as is.

Reviewed-by: Nir Soffer 

Re: [Qemu-devel] [PATCH] Removed header qemu-common.h from path.c

[Eric Blake]

[adding qemu-trivial in cc]

On 03/03/2017 01:18 PM, Suramya Shah wrote:
> Signed-off-by: Suramya Shah 
> ---

It would have been nice to mention in the subject that this is v2 (v1
was here
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00110.html),
and then mention here after the --- that the difference was a fix of a
typo that broke compilation.

Also, the subject line doesn't follow the usual conventions of "topic:
Description", a topic of 'util: ' may be appropriate.

>  util/path.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Reviewed-by: Eric Blake 

> diff --git a/util/path.c b/util/path.c
> index 5479f76..7f9fc27 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -6,7 +6,6 @@
>  #include "qemu/osdep.h"
>  #include 
>  #include 
> -#include "qemu-common.h"
>  #include "qemu/cutils.h"
>  #include "qemu/path.h"
>  
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] hppa: avoid anonymous unions in designated initializers.

[Richard Henderson]

On 03/04/2017 03:28 AM, Paolo Bonzini wrote:

These cause compilation failures on CentOS 6 or other operating
systems with older GCCs.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Paolo Bonzini 
---
Peter, please consider applying this as a build fix, because
the centos6 docker target is broken.


Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH] Removed header qemu-common.h from path.c

[Suramya Shah]

Signed-off-by: Suramya Shah 
---
 util/path.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/path.c b/util/path.c
index 5479f76..7f9fc27 100644
--- a/util/path.c
+++ b/util/path.c
@@ -6,7 +6,6 @@
 #include "qemu/osdep.h"
 #include 
 #include 
-#include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "qemu/path.h"
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/3] target/ppc: fmadd: add macro for updating flags

[Richard Henderson]

On 03/03/2017 05:58 PM, Nikunj A Dadhania wrote:

+#define FPU_MADDSUB_UPDATE(name, tp)\
+static void name(CPUPPCState *env, float64 arg1,\
+ float64 arg2, float64 arg3,\
+ unsigned int madd_flags)   \


Use ALL CAPS for macro arguments which you paste.
You forgot to use TP for arg{1,2,3}.


r~

Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails

[Eric Blake]

On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Fix the design flaw demonstrated in the previous commit: new method
> check_list() lets input visitors report that unvisited input remains
> for a list, exactly like check_struct() lets them report that
> unvisited input remains for a struct or union.
> 
> Implement the method for the qobject input visitor (straightforward),
> and the string input visitor (less so, due to the magic list syntax
> there).  The opts visitor's list magic is even more impenetrable, and
> all I can do there today is a stub with a FIXME comment.  No worse
> than before.
> 
> Signed-off-by: Markus Armbruster 
> ---

Didn't I already review this one?

Ah, there's my R-b:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html

But since it disappeared again, I had another look.

> +++ b/qapi/qobject-input-visitor.c
> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>  return container_of(v, QObjectInputVisitor, visitor);
>  }
>  
> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
> + int n)
>  {
>  StackObject *so;
>  char buf[32];
> @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, 
> const char *name)
>  }
>  
>  QSLIST_FOREACH(so , >stack, node) {
> -if (qobject_type(so->obj) == QTYPE_QDICT) {
> -g_string_prepend(qiv->errname, name);
> +if (n) {
> +n--;
> +} else if (qobject_type(so->obj) == QTYPE_QDICT) {
> +g_string_prepend(qiv->errname, name ?: "");
>  g_string_prepend_c(qiv->errname, '.');
>  } else {
>  snprintf(buf, sizeof(buf), "[%u]", so->index);
> @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, 
> const char *name)
>  }
>  name = so->name;
>  }
> +assert(!n);

If I'm reading this right, your use of n-- in the loop followed by the
post-condition is to assert that QSLIST_FOREACH() iterated n times, but
lets see what callers pass for n:

>  
>  if (name) {
>  g_string_prepend(qiv->errname, name);
>  } else if (qiv->errname->str[0] == '.') {
>  g_string_erase(qiv->errname, 0, 1);
> -} else {
> +} else if (!qiv->errname->str[0]) {
>  return "";
>  }
>  
>  return qiv->errname->str;
>  }
>  
> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> +{
> +return full_name_nth(qiv, name, 0);
> +}

One caller passes 0,


> +static void qobject_input_check_list(Visitor *v, Error **errp)
> +{
> +QObjectInputVisitor *qiv = to_qiv(v);
> +StackObject *tos = QSLIST_FIRST(>stack);
> +
> +assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
> +
> +if (tos->entry) {
> +error_setg(errp, "Only %u list elements expected in %s",
> +   tos->index + 1, full_name_nth(qiv, NULL, 1));

the other passes 1.  No other calls.  Did we really need an integer,
where we use n--, or would a bool have done as well?

At any rate, since I've already reviewed it once, you can add R-b, but
we may want a followup to make it less confusing.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/3] target/ppc: fmadd check for excp independently

[Richard Henderson]

On 03/03/2017 05:58 PM, Nikunj A Dadhania wrote:

Current order of checking does not confirm with the spec
(ISA 3.0: MultiplyAddDP page-469). Change the order and make them
independent of each other.

For example: a = infinity, b = zero, c = SNaN, this should set both
VXIMZ and VXNAN

Signed-off-by: Nikunj A Dadhania 
---
 target/ppc/fpu_helper.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0535ad0..a547f58 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -747,17 +747,21 @@ static void float64_maddsub_update_excp(CPUPPCState *env, 
float64 arg1,
 float64 arg2, float64 arg3,
 unsigned int madd_flags)
 {
+if (unlikely(float64_is_signaling_nan(arg1, >fp_status) ||
+ float64_is_signaling_nan(arg2, >fp_status) ||
+ float64_is_signaling_nan(arg3, >fp_status))) {
+/* sNaN operation */
+float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+}
+
 if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
  (float64_is_zero(arg1) && float64_is_infinity(arg2 {
 /* Multiplication of zero by infinity */
-arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-} else if (unlikely(float64_is_signaling_nan(arg1, >fp_status) ||
-float64_is_signaling_nan(arg2, >fp_status) ||
-float64_is_signaling_nan(arg3, >fp_status))) {
-/* sNaN operation */
-float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-} else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
-   float64_is_infinity(arg3)) {
+float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
+}
+


While these two bits should be both be set if the inputs demand, it won't 
happen if the exception enable bit is set, since the first call will longjmp. 
I'm not sure what to do about that; perhaps just ignore it for now.


More importantly, it will longjmp with the wrong source location.  Note that 
float_invalid_op_excp is attempting to use __always_inline__ to grab a correct 
value for GETPC.  Which, as one can predict, is doomed to failure.  As here we 
are, in another function which is not __always_inline__, producing the wrong value.


We need to drop all of the __always_inline__ nonsense, and properly pass down a 
value for GETPC from the top-level helper, so that we always think about it.



r~

Re: [Qemu-devel] [PULL 00/15] virtio, pc: fixes, features

[Michael S. Tsirkin]

On Fri, Mar 03, 2017 at 12:44:59PM +, Peter Maydell wrote:
> On 2 March 2017 at 06:20, Michael S. Tsirkin  wrote:
> >  dtc  |   2 +-
> 
> I just noticed this erroneous submodule update in here, unfortunately
> after I pushed it to master :-(
> 
> I'll fix up the tree...
> 
> thanks
> -- PMM

Ouch. submodules are a pain :(
Let me know if you need me to rebase. Sorry.

-- 
MST

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

On Fri, 3 Mar 2017 12:14:10 -0600
Eric Blake  wrote:

> On 03/03/2017 11:25 AM, Greg Kurz wrote:
> > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
> > QEMU vulnerable.
> > 
> > O_PATH was used as an optimization: the fd returned by openat_dir() is only
> > passed to openat() actually, so we don't really need to reach the underlying
> > filesystem.
> > 
> > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
> > return a fd, forcing us to do some other syscall to detect we have a
> > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.  
> 
> But the very next use of openat(fd, ) should fail with EBADF if fd is
> not a directory, so you don't need any extra syscalls.  I agree that we
> _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH
> where it works.
> 

You may have a point indeed.

> I'm in the middle of writing a test program to probe kernel behavior and
> demonstrate (at least to myself) whether there are scenarios where
> O_PATH makes it possible to open something where omitting it did not,
> while at the same time validating that O_NOFOLLOW doesn't cause problems
> if a symlink-fd is returned instead of a directory fd, based on our
> subsequent use of that fd in a *at call.
> 

I must leave right now, but please share the result of your experiment.

Thanks for your support on helping to fix 9p, Eric :)

Cheers.

--
Greg


pgpA175fXhata.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

On Fri, 3 Mar 2017 17:54:35 +
Mark Cave-Ayland  wrote:

> On 03/03/17 17:25, Greg Kurz wrote:
> 
> > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
> > QEMU vulnerable.
> > 
> > O_PATH was used as an optimization: the fd returned by openat_dir() is only
> > passed to openat() actually, so we don't really need to reach the underlying
> > filesystem.
> > 
> > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
> > return a fd, forcing us to do some other syscall to detect we have a
> > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.
> > 
> > The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW.
> > 
> > While here, we also fix local_unlinkat_common() to use openat_dir() for
> > the same reasons (it was a leftover in the original patchset actually).
> > 
> > This fixes CVE-2016-9602.
> > 
> > Signed-off-by: Greg Kurz 
> > ---
> >  hw/9pfs/9p-local.c |2 +-
> >  hw/9pfs/9p-util.h  |2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 5db7104334d6..e31309a29c58 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int 
> > dirfd, const char *name,
> >  if (flags == AT_REMOVEDIR) {
> >  int fd;
> >  
> > -fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH);
> > +fd = openat_dir(dirfd, name);
> >  if (fd == -1) {
> >  goto err_out;
> >  }
> > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > index 091f3ce88e15..4001d1fd8398 100644
> > --- a/hw/9pfs/9p-util.h
> > +++ b/hw/9pfs/9p-util.h
> > @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd)
> >  
> >  static inline int openat_dir(int dirfd, const char *name)
> >  {
> > -return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
> > +return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW);
> >  }
> >  
> >  static inline int openat_file(int dirfd, const char *name, int flags,  
> 
> Thanks Greg.
> 
> This patch got me slightly further, but I now get another build failure:
> 
> 
> cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs
> -I/home/build/src/qemu/git/qemu/tcg
> -I/home/build/src/qemu/git/qemu/tcg/i386
> -I/home/build/src/qemu/git/qemu/linux-headers
> -I/home/build/src/qemu/git/qemu/linux-headers -I.
> -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include
> -I/usr/include/pixman-1   -I/home/build/src/qemu/git/qemu/dtc/libfdt
> -Werror -pthread -I/usr/include/glib-2.0
> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -m64 -mcx16 -D_GNU_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
> -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
> -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels
> -Wno-missing-include-dirs -Wempty-body -Wnested-externs
> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
> -Wold-style-declaration -Wold-style-definition -Wtype-limits
> -fstack-protector-all -I/usr/include/p11-kit-1
> -I/usr/include/libpng12   -I/home/build/src/qemu/git/qemu/tests -MMD -MP
> -MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE
> -D_FORTIFY_SOURCE=2 -g   -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c
> hw/9pfs/9p-local.c: In function ‘local_set_cred_passthrough’:
> hw/9pfs/9p-local.c:352:40: error: ‘AT_EMPTY_PATH’ undeclared (first use
> in this function)
> hw/9pfs/9p-local.c:352:40: note: each undeclared identifier is reported
> only once for each function it appears in
> make: *** [hw/9pfs/9p-local.o] Error 1
> 

Oops, my bad. In a previous version of the patchset, name could be an
empty string on purpose... This isn't the case in the latest version
but I forgot to drop AT_EMPTY_PATH. I'll send a patch, but you can
simply remove it in the meantime to fix your build.

> 
> A quick search suggested that I should be able to get around this by
> adding a #include in hw/9pfs/9p-local.c for "linux/fcntl.h" however
> adding that gives me a couple of redefinition errors:
> 
> 
> cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs
> -I/home/build/src/qemu/git/qemu/tcg
> -I/home/build/src/qemu/git/qemu/tcg/i386
> -I/home/build/src/qemu/git/qemu/linux-headers
> -I/home/build/src/qemu/git/qemu/linux-headers -I.
> -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include
> -I/usr/include/pixman-1   -I/home/build/src/qemu/git/qemu/dtc/libfdt
> -Werror -pthread -I/usr/include/glib-2.0
> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -m64 -mcx16 -D_GNU_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
> -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
> -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels
> -Wno-missing-include-dirs -Wempty-body -Wnested-externs
> -Wformat-security 

Re: [Qemu-devel] [PATCH for-2.9 3/6] disas/m68k: Avoid unintended sign extension in get_field()

[Laurent Vivier]

Le 03/03/2017 à 16:50, Peter Maydell a écrit :
> In get_field(), we take an 'unsigned char' value and shift it left,
> which implicitly promotes it to 'signed int', before ORing it into an
> 'unsigned long' type.  If 'unsigned long' is 64 bits then this will
> result in a sign extension and the top 32 bits of the result will be
> 1s.  Add explicit casts to unsigned long before shifting to prevent
> this.
> 
> (Spotted by Coverity, CID 715697.)
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Laurent Vivier 

> ---
>  disas/m68k.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/disas/m68k.c b/disas/m68k.c
> index 073abb9..61b689e 100644
> --- a/disas/m68k.c
> +++ b/disas/m68k.c
> @@ -4685,10 +4685,11 @@ get_field (const unsigned char *data, enum 
> floatformat_byteorders order,
>   /* This is the last byte; zero out the bits which are not part of
>  this field.  */
>   result |=
> -   (*(data + cur_byte) & ((1 << (len - cur_bitshift)) - 1))
> +   (unsigned long)(*(data + cur_byte)
> +   & ((1 << (len - cur_bitshift)) - 1))
>   << cur_bitshift;
>else
> - result |= *(data + cur_byte) << cur_bitshift;
> + result |= (unsigned long)*(data + cur_byte) << cur_bitshift;
>cur_bitshift += FLOATFORMAT_CHAR_BIT;
>if (order == floatformat_little)
>   ++cur_byte;
> 

[Qemu-devel] [PATCH v2] qmp-shell: add persistent command history

[John Snow]

Use the existing readline history function we are utilizing
to provide persistent command history across instances of qmp-shell.

This assists entering debug commands across sessions that may be
interrupted by QEMU sessions terminating, where the qmp-shell has
to be relaunched.

Signed-off-by: John Snow 
---

v2: Adjusted the errors to whine about non-ENOENT errors, but still
intercept all errors as non-fatal.
Save history atexit() to match bash standard behavior

 scripts/qmp/qmp-shell | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 0373b24..55a8285 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -70,6 +70,9 @@ import json
 import ast
 import readline
 import sys
+import os
+import errno
+import atexit
 
 class QMPCompleter(list):
 def complete(self, text, state):
@@ -109,6 +112,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 self._pretty = pretty
 self._transmode = False
 self._actions = list()
+self._histfile = os.path.join(os.path.expanduser('~'), '.qmp_history')
 
 def __get_address(self, arg):
 """
@@ -137,6 +141,21 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 # XXX: default delimiters conflict with some command names (eg. 
query-),
 # clearing everything as it doesn't seem to matter
 readline.set_completer_delims('')
+try:
+readline.read_history_file(self._histfile)
+except Exception as e:
+if isinstance(e, IOError) and e.errno == errno.ENOENT:
+# File not found. No problem.
+pass
+else:
+print "Failed to read history '%s'; %s" % (self._histfile, e)
+atexit.register(self.__save_history)
+
+def __save_history(self):
+try:
+readline.write_history_file(self._histfile)
+except Exception as e:
+print "Failed to save history file '%s'; %s" % (self._histfile, e)
 
 def __parse_value(self, val):
 try:
-- 
2.9.3

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Peter Maydell]

On 3 March 2017 at 18:37, Markus Armbruster  wrote:
> Peter Maydell  writes:
>> Did you find these by looking at our Coverity results, or
>> cross-reference them against Coverity? (Coverity definitely
>> found the gluster leaks, for instance.)
>
> Neither.  I merely hacked myself a path through an unfamliar jungle :)

OK; I guess we'll see how many of the coverity nits get fixed :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 07/14] sm501: Fix device endianness

[Peter Maydell]

On 3 March 2017 at 02:15, BALATON Zoltan  wrote:
> Maybe it's not correct but works for everything I could test better than the
> original code (which was broken even for the SH images one can find) so I
> think we could just go with this until someone complains and provides a test
> case. I've given up on trying to understand it because I don't really know
> this device and SH so I could only go by the images I could find and they
> seem to like it this way.

So what cases have you tested? The Linux kernel seems to support:
 * sh embedded device, little endian
 * PCI card, little endian host
 * PCI card, big endian host
and there are also
 * 16 bit pixel format
 * 32 bit pixel format

which makes 6 different combinations.
(It's a shame there's no sh4 BE code to run on this.)

>> Looking at how the SWAP_FB_ENDIAN flag gets set:
>> * for the r2d board it is set always
>> * for PCI devices, it is set only if big-endian
>>
>> I suspect that what we actually have here is something
>> like:
>> * for the PCI device it's always little endian (and the
>>   code ought not to need to depend on TARGET_WORDS_BIGENDIAN)
>> * sysbus device may be more complicated
>>
>> Also I note there's an SM501_ENDIAN_CONTROL register on the
>> device, which presumably if written changes the behaviour.

> I've seen this but it's not emulated so it can be ignored for now. The spec
> also says that default is little endian so for regs at least
> DEVICE_LITTLE_ENDIAN should be OK.

Yes, that makes sense. So we should be modelling this as an "always
little endian device".

The changes you have to the memory region ops achieve this for
the registers implemented in this device itself. It looks like
SYSBUS_OHCI is already always little endian. You also need to
change the argument to the serial_mm_init() call to pass
DEVICE_LITTLE_ENDIAN.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member

[Eric Blake]

On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  tests/test-qobject-input-visitor.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 15/15] sheepdog: Support blockdev-add

[Eric Blake]

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-core.json | 27 ---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation

[Eric Blake]

On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> To enforce capability negotiation before normal operation,
> handle_qmp_command() inspects every command before it's handed off to
> qmp_dispatch().  This is a bit of a layering violation, and results in
> duplicated code.
> 
> Before capability negotiation (!cur_mon->in_command_mode), we fail
> commands other than "qmp_capabilities".  This is what enforces
> capability negotiation.
> 
> Afterwards, we fail command "qmp_capabilities".
> 
> Clean this up as follows.
> 
> The obvious place to fail a command is the command itself, so move the
> "afterwards" check to qmp_qmp_capabilities().
> 
> We do the "before" check in every other command, but that would be
> bothersome.  Instead, start with an alternate list of commant that

s/commant/commands/

> contains only "qmp_capabilities".  Switch to the full list in
> qmp_qmp_capabilities().
> 
> Additionally, replace the generic human-readable error message for
> CommandNotFound by one that reminds the user to run qmp_capabilities.
> Without that, we'd regress commit 2d5a834.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  monitor.c | 76 
> +++
>  1 file changed, 42 insertions(+), 34 deletions(-)
> 

> @@ -3786,11 +3785,20 @@ static void handle_qmp_command(JSONMessageParser 
> *parser, GQueue *tokens)
>  cmd_name = qdict_get_str(qdict, "execute");
>  trace_handle_qmp_command(mon, cmd_name);
>  
> -if (invalid_qmp_mode(mon, cmd_name, )) {
> -goto err_out;
> -}
> +rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>  
> -rsp = qmp_dispatch(_commands, req);
> +qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
> +if (qdict) {
> +if (mon->qmp.commands == _cap_negotiation_commands
> +&& !g_strcmp0(qdict_get_try_str(qdict, "class"),
> +QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {

Could join these two 'if' into one, for less {}, but that's cosmetic.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 04/13] sm501: QOMify

[Peter Maydell]

On 3 March 2017 at 01:03, BALATON Zoltan  wrote:
> Adding vmstate saving is not in this patch because the state structure
> will be changed in further patches, then another patch will add
> vmstate descriptor after those changes.
>
> Signed-off-by: BALATON Zoltan 

> +static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base,
> +   uint32_t local_mem_bytes)
> +{
> +s->base = base;
> +s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
> +SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", 
> get_local_mem_size(s),
> +  s->local_mem_size_index);
> +if (get_local_mem_size(s) != local_mem_bytes) {
> +error_report("Warning: sm501 VRAM size adjusted to %" PRIu32,
> + get_local_mem_size(s));
> +}

Just noticed this. I think reporting the error upwards by
failing device realize is better than adjusting the value.
It's what we tend to do for other devices. Management tools
like libvirt prefer to get hard errors if there's a config
file error that means they misconfigure a device.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Eric Blake]

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
> the discriminator value for variant InetSocketAddress is 'tcp' instead
> of 'inet'.  Rename.
> 
> The type is far only used by the Gluster block drivers.  Take care to
> keep 'tcp' working there.

The old name was visible in QMP in 2.8, but only by blockdev-add, which
we've already argued was not stable (and where we've already made other
non-back-compat changes to it).  But that means this HAS to go into 2.9,
if we're declaring blockdev-add stable for 2.9.

It wouldn't hurt to mention that additional information in the commit
message.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  block/gluster.c  | 59 
> +---
>  qapi-schema.json |  8 
>  2 files changed, 35 insertions(+), 32 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -4105,14 +4105,14 @@
>  #
>  # Available SocketAddressFlat types
>  #
> -# @tcp:   Internet address
> +# @inet:   Internet address
>  #
>  # @unix:  Unix domain socket

Nit: Spacing is now inconsistent.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 13/13] ppc: Add SM501 device in config for ppc and ppcemb targets

[Peter Maydell]

On 13 December 2016 at 21:00, BALATON Zoltan  wrote:
> This is not used by default on any emulated machine yet but it is
> still useful to have it compiled so it can be added from the command
> line for clients that can use it (e.g. MorphOS has no driver for any
> other emulated video cards but can output via SM501)
>
> Signed-off-by: BALATON Zoltan 
> ---
>  default-configs/ppc-softmmu.mak| 1 +
>  default-configs/ppcemb-softmmu.mak | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 09c1d45..1f1cd85 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  CONFIG_PLATFORM_BUS=y
>  CONFIG_ETSEC=y
>  CONFIG_LIBDECNUMBER=y
> +CONFIG_SM501=y
>  # For PReP
>  CONFIG_SERIAL_ISA=y
>  CONFIG_MC146818RTC=y
> diff --git a/default-configs/ppcemb-softmmu.mak 
> b/default-configs/ppcemb-softmmu.mak
> index 7f56004..94340de 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -15,3 +15,4 @@ CONFIG_I8259=y
>  CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_LIBDECNUMBER=y
> +CONFIG_SM501=y
> --
> 2.7.4

This looks fine but I'll leave official review of it to a ppc person.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Markus Armbruster]

Peter Maydell  writes:

> On 2 March 2017 at 21:43, Markus Armbruster  wrote:
>> Bad error handling, memory leaks, and lack of blockdev-add support.
>>
>> Markus Armbruster (15):
>>   sheepdog: Defuse time bomb in sd_open() error handling
>>   sheepdog: Fix error handling in sd_snapshot_delete()
>>   sheepdog: Fix error handling sd_create()
>>   sheepdog: Mark sd_snapshot_delete() lossage FIXME
>>   sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
>>   sheepdog: Don't truncate long VDI name in _open(), _create()
>>   sheepdog: Report errors in pseudo-filename more usefully
>>   sheepdog: Use SocketAddress and socket_connect()
>>   sheepdog: Implement bdrv_parse_filename()
>>   gluster: Drop assumptions on SocketTransport names
>>   gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
>>   gluster: Plug memory leaks in qemu_gluster_parse_json()
>>   qapi-schema: Rename GlusterServer to SocketAddressFlat
>>   qapi-schema: Rename SocketAddressFlat's variant tcp to inet
>>   sheepdog: Support blockdev-add
>>
>>  block/gluster.c  | 127 +++
>>  block/sheepdog.c | 436 
>> +--
>>  qapi-schema.json |  38 +
>>  qapi/block-core.json |  73 +++--
>>  4 files changed, 443 insertions(+), 231 deletions(-)
>
> Did you find these by looking at our Coverity results, or
> cross-reference them against Coverity? (Coverity definitely
> found the gluster leaks, for instance.)

Neither.  I merely hacked myself a path through an unfamliar jungle :)

Re: [Qemu-devel] [PATCH] hw/core/null-machine: Print error message when using the -kernel parameter

[Eduardo Habkost]

On Tue, Feb 28, 2017 at 09:52:51AM +0100, Thomas Huth wrote:
> If the user currently tries to use the -kernel parameter, simply nothing
> happens, and the user might get confused that there is nothing loaded
> to memory, but also no error message has been issued. Since there is no
> real generic way to load a kernel on all CPU types (but on some targets,
> the generic loader can be used instead), issue an appropriate error
> message here now to avoid the possible confusion.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Eduardo Habkost 

Applied to my machine-next branch, but I'm unsure if we should
break soft freeze and include this on 2.9. I will probably hold
it for 2.10.

> ---
>  hw/core/null-machine.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index 27c8369..864832d 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -40,6 +40,12 @@ static void machine_none_init(MachineState *mch)
>  memory_region_allocate_system_memory(ram, NULL, "ram", 
> mch->ram_size);
>  memory_region_add_subregion(get_system_memory(), 0, ram);
>  }
> +
> +if (mch->kernel_filename) {
> +error_report("The -kernel parameter is not supported "
> + "(use the generic 'loader' device instead).");
> +exit(1);
> +}
>  }
>  
>  static void machine_none_machine_init(MachineClass *mc)
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 12/13] sm501: Add vmstate descriptor

[Peter Maydell]

On 25 February 2017 at 23:53, BALATON Zoltan  wrote:
> Signed-off-by: BALATON Zoltan 
> ---
>
> v3: Added local_mem_size_index to vmstate, add vmstate for sysbus version too
>
>  hw/display/sm501.c | 100 
> -
>  1 file changed, 99 insertions(+), 1 deletion(-)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 11/13] sm501: Add some more missing registers

[Peter Maydell]

On 10 December 2016 at 02:05, BALATON Zoltan  wrote:
> This is to allow clients to initialise these without failing as long
> as no 2D engine function is called that would use the written value.
> Saved values are not used yet (may get used when more of 2D engine is
> added sometimes) and clients normally only write to most of these
> registers, nothing is known to ever read them but they are documented
> as read/write so also implement read for these.
>
> Signed-off-by: BALATON Zoltan 
> ---
>
> v2: Fixed mask of video_control register for a read only bit
> Changed IRQ status register to write ignored as IRQ is not implemented
> v3: Squashed read implementation into this patch
>
>  hw/display/sm501.c | 107 
> -
>  1 file changed, 106 insertions(+), 1 deletion(-)
>

> @@ -1454,6 +1558,7 @@ static void sm501_reset(SM501State *s)
>  s->misc_timing = 0;
>  s->power_mode_control = 0;
>  s->dc_panel_control = 0x0001; /* FIFO level 3 */
> +s->dc_video_control = 0;
>  s->dc_crt_control = 0x0001;
>  s->twoD_control = 0;
>  }

You want to reset all these new fields, not just one of them.
If you do that then you can add
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Eric Blake]

On 03/03/2017 11:05 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 

>>> +# @SocketAddressFlatType:
>>> +#
>>> +# Available SocketAddressFlat types
>>> +#
>>> +# @tcp:   Internet address
>>> +#
>>> +# @unix:  Unix domain socket
>>> +#
>>> +# Since: 2.9
>>
>> I probably would have listed 'since: 2.7', since the type is unchanged
>> from its pre-move location...
> 
> I'd have to bump it right in the next patch :)

Too true (now that I've read the next patch)!  No need to tweak it, then.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C

[Eric Blake]

On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi-introspect.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

I'm guessing we haven't seen a use of a prefix that matters yet, but
that an upcoming patch triggered a compilation failure without this fix.
 Mentioning that in the commit message wouldn't hurt.

> 
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 541644e..fb72c61 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -64,7 +64,7 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>  # generate C
>  # TODO can generate awfully long lines
>  jsons.extend(self._jsons)
> -name = prefix + 'qmp_schema_json'
> +name = c_name(prefix, protect=False) + 'qmp_schema_json'
>  self.decl = mcgen('''
>  extern const char %(c_name)s[];
>  ''',
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] git master build failure in 9pfs

[Eric Blake]

On 03/03/2017 12:15 PM, Greg Kurz wrote:

> 
> O_PATH | O_NOFOLLOW is a special case as described in the last paragraph
> of O_PATH in the man page:
> 
>   If  pathname  is a symbolic link and the O_NOFOLLOW flag is also
>   specified, then the call returns a file descriptor referring  to
>   the  symbolic  link.   This  file  descriptor can be used as the
>   dirfd argument in calls to fchownat(2),  fstatat(2),  linkat(2),
>   and readlinkat(2) with an empty pathname to have the calls oper‐
>   ate on the symbolic link.

Only when coupled with AT_EMPTY_PATHNAME.  Without that additional flag,
then it must be a directory.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 08/13] sm501: Fix hardware cursor

[Peter Maydell]

On 4 December 2016 at 18:01, BALATON Zoltan  wrote:
> Signed-off-by: BALATON Zoltan 
> ---
>
> v3: simplify return expression in get_bpp

In my review on v2 I asked for the commit message to
say clearly what the bugs being fixed here are. It's
a lot easier to review a patch if I know what it's supposed
to be doing rather than having to figure it out from scratch.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 06/13] sm501: Add emulation of chip connected via PCI

[Peter Maydell]

On 25 February 2017 at 18:31, BALATON Zoltan  wrote:
> Only the display controller part is created automatically on PCI
>
> Signed-off-by: BALATON Zoltan 
> ---
>
> v2: Split off removing dependency on base address to separate patch
> v3: Added reset function and PCI ID constant definitions in pci_ids.h
>
>  hw/display/sm501.c   | 58 
> 
>  include/hw/pci/pci_ids.h |  3 +++
>  2 files changed, 61 insertions(+)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program

[Eric Blake]

On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> The command registry encapsulates a single command list.  Give the
> functions using it a parameter instead.  Define suitable command lists
> in monitor, guest agent and test-qmp-commands.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/dispatch.h | 22 ++
>  monitor.c   | 31 +--
>  qapi/qmp-dispatch.c | 17 +
>  qapi/qmp-registry.c | 37 ++---
>  qga/commands.c  |  2 +-
>  qga/guest-agent-core.h  |  2 ++
>  qga/main.c  | 19 ++-
>  scripts/qapi-commands.py| 16 ++--
>  tests/test-qmp-commands.c   | 12 +++-
>  9 files changed, 92 insertions(+), 66 deletions(-)
> 

> +++ b/qapi/qmp-dispatch.c
> @@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject 
> *request, Error **errp)
>  return dict;
>  }
>  
> -static QObject *do_qmp_dispatch(QObject *request, Error **errp)
> +volatile QmpCommand *save_cmd;

A comment why volatile is necessary would be nice...

> +QmpCommand cmd2;
> +
> +static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
> +Error **errp)
>  {
>  Error *local_err = NULL;
>  const char *command;
> @@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
> **errp)
>  }
>  
>  command = qdict_get_str(dict, "execute");
> -cmd = qmp_find_command(command);
> +cmd = qmp_find_command(cmds, command);
>  if (cmd == NULL) {
>  error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>"The command %s has not been found", command);
> @@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
> **errp)
>  return NULL;
>  }
>  
> +assert(!cmd->options & QCO_NO_SUCCESS_RESP);
> +save_cmd = cmd;
> +cmd2 = *cmd;
>  if (!qdict_haskey(dict, "arguments")) {
>  args = qdict_new();
>  } else {
> @@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
> **errp)
>  
>  QDECREF(args);
>  
> +assert(!cmd->options & QCO_NO_SUCCESS_RESP);
> +assert(ret || local_err);

...or is this leftovers from your debugging?

The rest of the patch looks fine; it is converting a global variable
into a per-instance variable.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 04/13] sm501: QOMify

[Peter Maydell]

On 3 March 2017 at 01:03, BALATON Zoltan  wrote:
> Adding vmstate saving is not in this patch because the state structure
> will be changed in further patches, then another patch will add
> vmstate descriptor after those changes.
>
> Signed-off-by: BALATON Zoltan 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 03/13] sm501: Add missing arbitration control register

[Peter Maydell]

On 3 December 2016 at 15:32, BALATON Zoltan  wrote:
> Signed-off-by: BALATON Zoltan 
> ---
>  hw/display/sm501.c | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 02/13] sm501: Use defined constants instead of literal values where available

[Peter Maydell]

On 3 March 2017 at 00:21, BALATON Zoltan  wrote:
> Signed-off-by: BALATON Zoltan 
> ---
>
> v3: Fix initial value of misc_control register as Peter Maydell suggested
> Also use M_BYTE constant from cutils.h
>

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH] 9pfs: fail local_statfs() earlier

[Eric Blake]

On 03/03/2017 12:03 PM, Greg Kurz wrote:
> If we cannot open the given path, we can return right away instead of
> passing -1 to fstatfs() and close(). This will make Coverity happy.
> 
> (Coverity issue CID1371729)
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/9pfs/9p-local.c |3 +++
>  1 file changed, 3 insertions(+)

No difference in end-user observable behavior, but arguably nicer (and
always more efficient to avoid unneeded syscalls).

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] git master build failure in 9pfs

[Greg Kurz]

On Fri, 3 Mar 2017 12:11:36 -0600
Eric Blake  wrote:

> On 03/03/2017 10:43 AM, Greg Kurz wrote:
> 
> >>> +#ifndef O_PATH
> >>> +#define O_PATH 0
> >>> +#endif
> >>
> >> Isn't the use of O_PATH required in order to fix the recent
> >> security vulnerability in 9p ?  If so, then defining it to
> >> 0 means the QEMU is silently becoming vulnerable once again
> >> which I don't think is a good idea.
> >>  
> > 
> > O_PATH was supposed to be used as an optimization here, since fds returned 
> > by
> > this function are only passed to openat()... but your comment makes me 
> > realize
> > I inadvertently dropped O_NOFOLLOW between v1 and v2 of the patchset. And 
> > this
> > IS an actual vulnerability issue :) And reading the openat() manpage, I see
> > that O_PATH | O_NOFOLLOW doesn't cause openat() to fail, but to return a fd
> > pointing to the symlink which is certainly not what I want :)  
> 
> Why not? It works, since openat(fd, ...) fails with EBADF if fd is a
> symlink rather than a directory.  (Well, it SHOULD fail like that,
> according to the man page; I need to write a test program and find out
> for sure).  So you don't have to do any additional syscalls, as your
> very next *at call will tell you if you actually got a directory or a
> symlink.
> 

O_PATH | O_NOFOLLOW is a special case as described in the last paragraph
of O_PATH in the man page:

  If  pathname  is a symbolic link and the O_NOFOLLOW flag is also
  specified, then the call returns a file descriptor referring  to
  the  symbolic  link.   This  file  descriptor can be used as the
  dirfd argument in calls to fchownat(2),  fstatat(2),  linkat(2),
  and readlinkat(2) with an empty pathname to have the calls oper‐
  ate on the symbolic link.

Cheers.

--
Greg


pgpKcfxukjmRF.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Eric Blake]

On 03/03/2017 11:25 AM, Greg Kurz wrote:
> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
> QEMU vulnerable.
> 
> O_PATH was used as an optimization: the fd returned by openat_dir() is only
> passed to openat() actually, so we don't really need to reach the underlying
> filesystem.
> 
> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
> return a fd, forcing us to do some other syscall to detect we have a
> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.

But the very next use of openat(fd, ) should fail with EBADF if fd is
not a directory, so you don't need any extra syscalls.  I agree that we
_need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH
where it works.

I'm in the middle of writing a test program to probe kernel behavior and
demonstrate (at least to myself) whether there are scenarios where
O_PATH makes it possible to open something where omitting it did not,
while at the same time validating that O_NOFOLLOW doesn't cause problems
if a symlink-fd is returned instead of a directory fd, based on our
subsequent use of that fd in a *at call.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 9pfs: fail local_statfs() earlier

[Daniel P. Berrange]

On Fri, Mar 03, 2017 at 07:03:47PM +0100, Greg Kurz wrote:
> If we cannot open the given path, we can return right away instead of
> passing -1 to fstatfs() and close(). This will make Coverity happy.
> 
> (Coverity issue CID1371729)
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/9pfs/9p-local.c |3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index fe930300445a..ea9a1ced0394 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1053,6 +1053,9 @@ static int local_statfs(FsContext *s, V9fsPath 
> *fs_path, struct statfs *stbuf)
>  int fd, ret;
>  
>  fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0);
> +if (fd == -1) {
> +return -1;
> +}
>  ret = fstatfs(fd, stbuf);
>  close_preserve_errno(fd);
>  return ret;

Reviewed-by: Daniel P. berrange 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] git master build failure in 9pfs

[Eric Blake]

On 03/03/2017 10:43 AM, Greg Kurz wrote:

>>> +#ifndef O_PATH
>>> +#define O_PATH 0
>>> +#endif  
>>
>> Isn't the use of O_PATH required in order to fix the recent
>> security vulnerability in 9p ?  If so, then defining it to
>> 0 means the QEMU is silently becoming vulnerable once again
>> which I don't think is a good idea.
>>
> 
> O_PATH was supposed to be used as an optimization here, since fds returned by
> this function are only passed to openat()... but your comment makes me realize
> I inadvertently dropped O_NOFOLLOW between v1 and v2 of the patchset. And this
> IS an actual vulnerability issue :) And reading the openat() manpage, I see
> that O_PATH | O_NOFOLLOW doesn't cause openat() to fail, but to return a fd
> pointing to the symlink which is certainly not what I want :)

Why not? It works, since openat(fd, ...) fails with EBADF if fd is a
symlink rather than a directory.  (Well, it SHOULD fail like that,
according to the man page; I need to write a test program and find out
for sure).  So you don't have to do any additional syscalls, as your
very next *at call will tell you if you actually got a directory or a
symlink.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 10/11] target/arm/helper: make it clear the EC field is also in hex

[Peter Maydell]

On 2 March 2017 at 19:53, Alex Bennée  wrote:
> ..just like the rest of the displayed ESR register. Otherwise people
> might scratch their heads if a not obviously hex number is displayed
> for the EC field.
>
> Signed-off-by: Alex Bennée 
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3f4211b572..76b608f0ba 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6857,7 +6857,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>new_el);
>  if (qemu_loglevel_mask(CPU_LOG_INT)
>  && !excp_is_internal(cs->exception_index)) {
> -qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%x/0x%" PRIx32 "\n",
>env->exception.syndrome >> ARM_EL_EC_SHIFT,
>env->exception.syndrome);
>  }
> --

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 11/11] hw/intc/arm_gic: modernise the DPRINTF

[Peter Maydell]

On 2 March 2017 at 19:53, Alex Bennée  wrote:
> While I was debugging the icount issues I realised a bunch of the
> messages look quite similar. I've fixed this by including __func__ in
> the debug print. At the same time I move the a modern if (GATE) style
> printf which ensures the compiler can check for format string errors
> even if the code gets optimised away in the non-DEBUG_GIC case.
>
> Signed-off-by: Alex Bennée 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH] [RFT] target/arm/arm-powerctl: Fix psci info return values

[Peter Maydell]

On 3 March 2017 at 12:32, Andrew Jones  wrote:
> The power state spec section 5.1.5 AFFINITY_INFO defines the
> affinity info return values as
>
>   0 ON
>   1 OFF
>   2 ON_PENDING
>
> I grepped QEMU for power_state to ensure that no assumptions
> of OFF=0 were being made.
>
> Signed-off-by: Andrew Jones 
>
> ---
>
> I found this with the yet to be committed kvm-unit-tests test
> https://lists.cs.columbia.edu/pipermail/kvmarm/2017-February/023820.html
>
> I've added the RFT because I didn't bother to confirm Linux is still
> happy (although I can't see why it wouldn't be happier).

Nice catch. Without this patch:

root@genericarmv8:~# echo 0 > /sys/devices/system/cpu/cpu1/online
[  112.797621] CPU1: shutdown
[  112.822447] psci: Retrying again to check for CPU kill
[  112.841306] psci: Retrying again to check for CPU kill
[  112.861303] psci: Retrying again to check for CPU kill
[  112.881346] psci: Retrying again to check for CPU kill
[  112.901302] psci: Retrying again to check for CPU kill
[  112.921301] psci: Retrying again to check for CPU kill
[  112.941303] psci: Retrying again to check for CPU kill
[  112.961302] psci: Retrying again to check for CPU kill
[  112.981350] psci: Retrying again to check for CPU kill
[  113.001368] psci: Retrying again to check for CPU kill
[  113.001789] psci: CPU1 may not have shut down cleanly
(AFFINITY_INFO reports 0)
[  113.002429] CPU1 may not have shut down cleanly: -110

With the patch:
root@genericarmv8:~# echo 0 > /sys/devices/system/cpu/cpu1/online
[   83.303563] CPU1: shutdown
[   83.308003] psci: CPU1 killed.

Applied to target-arm.next for 2.9.

thanks
-- PMM

[Qemu-devel] [PATCH] 9pfs: fail local_statfs() earlier

[Greg Kurz]

If we cannot open the given path, we can return right away instead of
passing -1 to fstatfs() and close(). This will make Coverity happy.

(Coverity issue CID1371729)

Signed-off-by: Greg Kurz 
---
 hw/9pfs/9p-local.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index fe930300445a..ea9a1ced0394 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1053,6 +1053,9 @@ static int local_statfs(FsContext *s, V9fsPath *fs_path, 
struct statfs *stbuf)
 int fd, ret;
 
 fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0);
+if (fd == -1) {
+return -1;
+}
 ret = fstatfs(fd, stbuf);
 close_preserve_errno(fd);
 return ret;

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Mark Cave-Ayland]

On 03/03/17 17:25, Greg Kurz wrote:

> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
> QEMU vulnerable.
> 
> O_PATH was used as an optimization: the fd returned by openat_dir() is only
> passed to openat() actually, so we don't really need to reach the underlying
> filesystem.
> 
> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
> return a fd, forcing us to do some other syscall to detect we have a
> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.
> 
> The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW.
> 
> While here, we also fix local_unlinkat_common() to use openat_dir() for
> the same reasons (it was a leftover in the original patchset actually).
> 
> This fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/9pfs/9p-local.c |2 +-
>  hw/9pfs/9p-util.h  |2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 5db7104334d6..e31309a29c58 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int 
> dirfd, const char *name,
>  if (flags == AT_REMOVEDIR) {
>  int fd;
>  
> -fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH);
> +fd = openat_dir(dirfd, name);
>  if (fd == -1) {
>  goto err_out;
>  }
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 091f3ce88e15..4001d1fd8398 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd)
>  
>  static inline int openat_dir(int dirfd, const char *name)
>  {
> -return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
> +return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW);
>  }
>  
>  static inline int openat_file(int dirfd, const char *name, int flags,

Thanks Greg.

This patch got me slightly further, but I now get another build failure:


cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs
-I/home/build/src/qemu/git/qemu/tcg
-I/home/build/src/qemu/git/qemu/tcg/i386
-I/home/build/src/qemu/git/qemu/linux-headers
-I/home/build/src/qemu/git/qemu/linux-headers -I.
-I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include
-I/usr/include/pixman-1   -I/home/build/src/qemu/git/qemu/dtc/libfdt
-Werror -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -m64 -mcx16 -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels
-Wno-missing-include-dirs -Wempty-body -Wnested-externs
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-fstack-protector-all -I/usr/include/p11-kit-1
-I/usr/include/libpng12   -I/home/build/src/qemu/git/qemu/tests -MMD -MP
-MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -g   -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c
hw/9pfs/9p-local.c: In function ‘local_set_cred_passthrough’:
hw/9pfs/9p-local.c:352:40: error: ‘AT_EMPTY_PATH’ undeclared (first use
in this function)
hw/9pfs/9p-local.c:352:40: note: each undeclared identifier is reported
only once for each function it appears in
make: *** [hw/9pfs/9p-local.o] Error 1


A quick search suggested that I should be able to get around this by
adding a #include in hw/9pfs/9p-local.c for "linux/fcntl.h" however
adding that gives me a couple of redefinition errors:


cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs
-I/home/build/src/qemu/git/qemu/tcg
-I/home/build/src/qemu/git/qemu/tcg/i386
-I/home/build/src/qemu/git/qemu/linux-headers
-I/home/build/src/qemu/git/qemu/linux-headers -I.
-I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include
-I/usr/include/pixman-1   -I/home/build/src/qemu/git/qemu/dtc/libfdt
-Werror -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -m64 -mcx16 -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels
-Wno-missing-include-dirs -Wempty-body -Wnested-externs
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-fstack-protector-all -I/usr/include/p11-kit-1
-I/usr/include/libpng12   -I/home/build/src/qemu/git/qemu/tests -MMD -MP
-MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -g   -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c
In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0,
 from /usr/include/linux/fcntl.h:4,
 from hw/9pfs/9p-local.c:30:
/usr/include/asm-generic/fcntl.h:127:8: 

Re: [Qemu-devel] [PATCH] 9pfs: fix fd leak in local_opendir()

[Daniel P. Berrange]

On Fri, Mar 03, 2017 at 06:52:42PM +0100, Greg Kurz wrote:
> Coverity issue CID1371731
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/9pfs/9p-local.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index e31309a29c58..fe930300445a 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -435,6 +435,7 @@ static int local_opendir(FsContext *ctx,
>  
>  stream = fdopendir(dirfd);
>  if (!stream) {
> +close(dirfd);
>  return -1;
>  }
>  fs->dir.stream = stream;

Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH] 9pfs: fix fd leak in local_opendir()

[Greg Kurz]

Coverity issue CID1371731

Signed-off-by: Greg Kurz 
---
 hw/9pfs/9p-local.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index e31309a29c58..fe930300445a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -435,6 +435,7 @@ static int local_opendir(FsContext *ctx,
 
 stream = fdopendir(dirfd);
 if (!stream) {
+close(dirfd);
 return -1;
 }
 fs->dir.stream = stream;

Re: [Qemu-devel] [PATCH] spapr: ensure that all threads within core are on the same NUMA node

[Igor Mammedov]

On Fri, 24 Feb 2017 10:26:56 +0100
Igor Mammedov  wrote:

> Threads within a core shouldn't be on different
> NUMA nodes, so if user has misconfgured command
> line, fail QEMU at start up to force user fix it.
> 
> For now use the first thread on the core as source
> of core's node-id. Later when cpu-numa refactoring
> lands  it will be switched to core's node-id from
> possible_cpus[].
> 
> Signed-off-by: Igor Mammedov 
ping,

David could you review/merge maybe adding to commit
message:

"
patch prevents the same issues as commit 20bb648d
fixes, only instead of default mapping it adds
check for manually configured NUMA node mapping
"

> ---
> Enforcement is necessary to ensure that it would be
> possible to map legacy CPU index based CLI mapping
> to core based one and replace numa_info[XXX].node_cpu
> with possible_cpus[] as storage for mapping info.
> 
> CC: qemu-...@nongnu.org
> CC: lviv...@redhat.com
> CC: David Gibson 
> CC: th...@redhat.com
> CC: mdr...@linux.vnet.ibm.com
> CC: ag...@suse.de
> ---
>  hw/ppc/spapr_cpu_core.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 55cd045..1499a8b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -50,8 +50,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, 
> PowerPCCPU *cpu,
> Error **errp)
>  {
>  CPUPPCState *env = >env;
> -CPUState *cs = CPU(cpu);
> -int i;
>  
>  /* Set time-base frequency to 512 MHz */
>  cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> @@ -70,12 +68,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, 
> PowerPCCPU *cpu,
>  }
>  }
>  
> -/* Set NUMA node for the added CPUs  */
> -i = numa_get_node_for_cpu(cs->cpu_index);
> -if (i < nb_numa_nodes) {
> -cs->numa_node = i;
> -}
> -
>  xics_cpu_setup(spapr->xics, cpu);
>  
>  qemu_register_reset(spapr_cpu_reset, cpu);
> @@ -159,11 +151,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> Error **errp)
>  const char *typename = object_class_get_name(scc->cpu_class);
>  size_t size = object_type_get_instance_size(typename);
>  Error *local_err = NULL;
> +int core_node_id = numa_get_node_for_cpu(cc->core_id);;
>  void *obj;
>  int i, j;
>  
>  sc->threads = g_malloc0(size * cc->nr_threads);
>  for (i = 0; i < cc->nr_threads; i++) {
> +int node_id;
>  char id[32];
>  CPUState *cs;
>  
> @@ -172,6 +166,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> Error **errp)
>  object_initialize(obj, size, typename);
>  cs = CPU(obj);
>  cs->cpu_index = cc->core_id + i;
> +
> +/* Set NUMA node for the added CPUs  */
> +node_id = numa_get_node_for_cpu(cs->cpu_index);
> +if (node_id != core_node_id) {
> +error_setg(_err, "Invalid node-id=%d of thread[cpu-index: 
> %d]"
> +" on CPU[core-id: %d, node-id: %d], node-id must be the 
> same",
> + node_id, cs->cpu_index, cc->core_id, core_node_id);
> +goto err;
> +}
> +if (node_id < nb_numa_nodes) {
> +cs->numa_node = node_id;
> +}
> +
>  snprintf(id, sizeof(id), "thread[%d]", i);
>  object_property_add_child(OBJECT(sc), id, obj, _err);
>  if (local_err) {

Re: [Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Frederic Konrad]

Hi All,

I've a strangeness with the "drop iolock" patch.
It seems it has an impact on the MMIO execution patch-set I'm working
on:

Basically modifying the memory tree is causing a Bad Ram Address error.
I wonder if this can't happen with hotplug/unplug model as well.

I'll look into this.
Shoot if you have any evidence of why that doesn't work :).

Thanks,
Fred

On 03/02/2017 08:53 PM, Alex Bennée wrote:
> Hi Peter,
> 
> So perhaps predictably the merging of MTTCG has thrown up some issues
> during soft-freeze. The following patches fix up some of those:
> 
> The following where in v1 and just have r-b tags:
> 
>   vl/cpus: be smarter with icount and MTTCG
>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
> 
> The next change downgrades the asserts to tcg_debug_asserts. This will
> reduce the instances of production builds failing on non-MTTCG enabled
> guests. The races still need to be fixed but you now have to
> --enable-debug-tcg to go hunting for them:
> 
>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
> 
> This never came up in my testing but it seems some guests can
> translate while doing a tlb_fill. The call to cpu_restore_state never
> worked before (as we are translating the block you are looking for)
> but by bugging out we avoid the double tb_lock().
> 
>   translate-all: exit cpu_restore_state early if translating
> 
> Then we have a bunch of fixes from the reports on the list. They are
> safe to merge but I'll leave that up to the maintainers. There may be
> an argument for holding these patches back until the guest is
> converted to be MTTCG aware and a full audit of the CPU<->HW emulation
> boundaries is done for each one:
> 
>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>   s390x/misc_helper.c: wrap IO instructions in BQL
>   target/xtensa: hold BQL for interrupt processing
>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
> 
> Finally these are just tiny debug fixes while investigating the icount
> regression:
> 
>   target/arm/helper: make it clear the EC field is also in hex
>   hw/intc/arm_gic: modernise the DPRINTF
> 
> Going forward I think 1-5 are ready to be merged now. For 6-9 we
> should await decisions from the target maintainers. The last two are
> entirely up to you.
> 
> It looks like the -icount regression is a poor interaction between
> icount timers and the BQL but this is going to require discussion with
> Paolo in the morning.
> 
> Cheers,
> 
> 
> Alex Bennée (11):
>   vl/cpus: be smarter with icount and MTTCG
>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>   translate-all: exit cpu_restore_state early if translating
>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>   s390x/misc_helper.c: wrap IO instructions in BQL
>   target/xtensa: hold BQL for interrupt processing
>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>   target/arm/helper: make it clear the EC field is also in hex
>   hw/intc/arm_gic: modernise the DPRINTF
> 
>  cpus.c  | 11 +++
>  hw/intc/arm_gic.c   | 13 +
>  hw/sparc/sun4m.c|  3 +++
>  hw/sparc64/sparc64.c|  3 +++
>  target/arm/helper.c |  2 +-
>  target/i386/cpu.h   |  3 +++
>  target/mips/op_helper.c | 17 ++---
>  target/s390x/misc_helper.c  | 21 +
>  target/sparc/int64_helper.c |  3 +++
>  target/sparc/win_helper.c   | 11 +++
>  target/xtensa/helper.c  |  1 +
>  target/xtensa/op_helper.c   |  7 +++
>  translate-all.c |  9 -
>  translate-common.c  |  3 ++-
>  vl.c|  7 ++-
>  15 files changed, 95 insertions(+), 19 deletions(-)
> 

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Daniel P. Berrange]

On Fri, Mar 03, 2017 at 06:25:30PM +0100, Greg Kurz wrote:
> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
> QEMU vulnerable.
> 
> O_PATH was used as an optimization: the fd returned by openat_dir() is only
> passed to openat() actually, so we don't really need to reach the underlying
> filesystem.
> 
> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
> return a fd, forcing us to do some other syscall to detect we have a
> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.
> 
> The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW.
> 
> While here, we also fix local_unlinkat_common() to use openat_dir() for
> the same reasons (it was a leftover in the original patchset actually).
> 
> This fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/9pfs/9p-local.c |2 +-
>  hw/9pfs/9p-util.h  |2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 5db7104334d6..e31309a29c58 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int 
> dirfd, const char *name,
>  if (flags == AT_REMOVEDIR) {
>  int fd;
>  
> -fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH);
> +fd = openat_dir(dirfd, name);
>  if (fd == -1) {
>  goto err_out;
>  }
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 091f3ce88e15..4001d1fd8398 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd)
>  
>  static inline int openat_dir(int dirfd, const char *name)
>  {
> -return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
> +return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW);
>  }
>  
>  static inline int openat_file(int dirfd, const char *name, int flags,

Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make
QEMU vulnerable.

O_PATH was used as an optimization: the fd returned by openat_dir() is only
passed to openat() actually, so we don't really need to reach the underlying
filesystem.

O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will
return a fd, forcing us to do some other syscall to detect we have a
symlink. Also, O_PATH doesn't exist in glibc 2.13 and older.

The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW.

While here, we also fix local_unlinkat_common() to use openat_dir() for
the same reasons (it was a leftover in the original patchset actually).

This fixes CVE-2016-9602.

Signed-off-by: Greg Kurz 
---
 hw/9pfs/9p-local.c |2 +-
 hw/9pfs/9p-util.h  |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5db7104334d6..e31309a29c58 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, 
const char *name,
 if (flags == AT_REMOVEDIR) {
 int fd;
 
-fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH);
+fd = openat_dir(dirfd, name);
 if (fd == -1) {
 goto err_out;
 }
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 091f3ce88e15..4001d1fd8398 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd)
 
 static inline int openat_dir(int dirfd, const char *name)
 {
-return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
+return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW);
 }
 
 static inline int openat_file(int dirfd, const char *name, int flags,

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Cédric Le Goater]

On 03/03/2017 05:54 PM, Thomas Huth wrote:
> On 03.03.2017 15:49, Cédric Le Goater wrote:
>> On 03/03/2017 03:13 PM, Thomas Huth wrote:
>>> On 03.03.2017 14:37, Cédric Le Goater wrote:
 Also use an 'Object *' under the sPAPR machine to hold the RTC
 object.
>>>
>>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
>>> idea! But what's the advantage of using Object* instead of DeviceState*
>>> in sPAPRMachineState ?
>>
>> it makes spapr_rtc_create() a little simpler.  
>>
>> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
>> that we  would initialize with object_initialize(). 
> 
> I think a sPAPRRTCState* would make more sense here - if you just see an
> Object* and are not familiar with the code, you wonder what this pointer
> is all about (and you then have to cast it to something different if you
> want to do anything with it) ... so IMHO either a DeviceState* or
> sPAPRRTCState* is the better choice here.

I think having a non-pointer is a better for the object lifecycle 
but I don't have a strong opinion on that. We will see what Dave 
prefers.

Thanks,

C. 

Re: [Qemu-devel] [PATCH] qmp: allow setting properties to empty string in qmp-shell

[John Snow]

On 03/02/2017 07:24 AM, Daniel P. Berrange wrote:
> The qmp-shell property parser currently rejects attempts to
> set string properties to the empty string eg
> 
>   (QEMU) migrate-set-parameters  tls-hostname=
>   Error while parsing command line: Expected a key=value pair, got 
> 'tls-hostname='
> command format:   [arg-name1=arg1] ... [arg-nameN=argN]
> 
> This is caused by checking the wrong condition after splitting
> the parameter on '='. The "partition" method will return "" for
> the separator field, if the seperator was not present, so that
> is the correct thing to check for malformed syntax.
> 

Ah, good point.

> Signed-off-by: Daniel P. Berrange 

Reviewed-by: John Snow 

> ---
>  scripts/qmp/qmp-shell | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index 0373b24..eccb88a 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -166,8 +166,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>  
>  def __cli_expr(self, tokens, parent):
>  for arg in tokens:
> -(key, _, val) = arg.partition('=')
> -if not val:
> +(key, sep, val) = arg.partition('=')
> +if sep != '=':
>  raise QMPShellError("Expected a key=value pair, got '%s'" % 
> arg)
>  
>  value = self.__parse_value(val)
>