Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Gonglei (Arei)]

>
> From: Kevin O'Connor [mailto:ke...@koconnor.net]
> Sent: Saturday, December 19, 2015 7:13 AM
> To: Gonglei (Arei)
> Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; seab...@seabios.org;
> Huangweidong (C); k...@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> problem on qemu-kvm platform
> 
> On Fri, Dec 18, 2015 at 03:04:58AM +, Gonglei (Arei) wrote:
> > Hi Kevin & Paolo,
> >
> > Luckily, I reproduced this problem last night. And I got the below log when
> SeaBIOS is stuck.
> [...]
> > [2015-12-18 10:38:10]  gonglei: finish while
> [...]
> > <...>-31509 [035] 154753.180077: kvm_exit: reason EXCEPTION_NMI rip 0x3
> info 0 8306
> > <...>-31509 [035] 154753.180077: kvm_emulate_insn: 0:3:f0 53 (real)
> > <...>-31509 [035] 154753.180077: kvm_inj_exception: #UD (0x0)
> > <...>-31509 [035] 154753.180077: kvm_entry: vcpu 0
> 
> This is an odd finding.  It seems to indicate that the code is caught
> in an infinite irq loop once irqs are enabled.  What doesn't make
> sense is that an NMI shouldn't depend on the cpu irq enable flag.
> Also, I can't explain why rip would be 0x03, nor why a #UD in an
> exception handler wouldn't result in a triple fault.  Maybe someone
> with more kvm knowledge could help here.
> 

Ccing Paolo and Radim.

> I did notice that you appear to be running with SeaBIOS v1.8.1 - I
> recommend you upgrade to the latest.  There were two important fixes
> in this area (8b9942fa and 3156b71a).  I don't think either of these
> fixes would explain the log above, but it would be best to eliminate
> the possibility.
> 
We can reproduce the problem using latest SeaBIOS too. :(


Regards,
-Gonglei

Re: [Qemu-devel] RFC: Operation Blockers in QEMU Block Nodes

[Jeff Cody]

On Fri, Dec 18, 2015 at 03:19:25PM +0100, Kevin Wolf wrote:
> Am 16.12.2015 um 07:25 hat Jeff Cody geschrieben:
> > Background:
> > 
> > Block jobs, and other QAPI operations, may modify and impact the
> > BlockDriverState graph in QEMU.  In order to support multiple
> > operations safely, we need a mechanism to block and gate operations,
> > 
> > We currently have op blockers, that are attached to each BDS.
> > However, in practice we check this on the device level, rather than on
> > the granularity of the individual BDS.  Also, due to limitations of
> > the current system, we only allow a single block job at a time.
> 
> As I already said on IRC, I think a really important part in order to
> make proper op blockers workable is that we actually enforce that you
> have to request permission before you can operate on an image. That is,
> if you were to write to an image, but you haven't requested write
> permission from the op blocker system before, you'd get an assertion
> failure.
> 
> The reason for this is that it's a long standing issue of the current
> system that it's not only possible, but even easy to forget adding the
> blocker code or updating it when the surrounding code changes. The
> result is a mechanism that is too restrictive for many cases on the one
> hand, but fails to actually provide the protection we need on the other
> hand (for example, bs->file is still unprotected).
> 
> Blockers are subtle and at the same time pervasive enough that I'm
> almost sure we wouldn't implement them correctly if we don't force
> ourselves to do it right by letting qemu crash whenever we don't.
>

I agree with this.  The (too brief) blurb at the end of my email was
the kernel of my thoughts:

 "It may be possible to later expand the NAC system, to provide
 handles for use by low-level operations in block.c."

I think to do enforced permissions, we would need to introduce
something like handles (or tokens) for operations, so that we would
fail if the handle/token was not provided for a given operation (or
had insufficient permissions).

I mentioned "later" because I think if the code itself is flexible to
it, it may be better to grow the block layer into it.  But you are
right, doing it right away may be best.


> > Proposed New Design Features:
> > --
> > This design would supersede the current op blocker system.
> > 
> > Rather than have the op blockers attached directly to each BDS, Block
> > Job access will be done through a separate Node Access Control system.
> > 
> > This new system will:
> > 
> > * Allow / Disallow operations to BDSs, (generally initiated by QAPI
> >   actions; i.e. BDS node operations other than guest read/writes)
> > 
> > * Not be limited to "block jobs" (e.g. block-commit, block-stream,
> >   etc..) - will also apply to other operations, such as
> >   blockdev-add, change-backing-file, etc.
> > 
> > * Allow granularity in options, and provide the safety needed to
> >   support multiple block jobs and operations.
> > 
> > * Reference each BDS by its node-name
> > 
> > * Be independent of the bdrv_states graph (i.e. does not reside in
> >   the BDS structure)
> 
> I agree that the BDS structure is probably not the right place (except
> possibly for caching the permission bitmasks), but I also wouldn't say
> that it should be completely independent structures. Otherwise keeping
> things in sync when the graph changes might turn out hard.
> 

And having some linking into the BDS will also make it more efficient,
and allow it to probe some for insights (such as child / parent node
relationships to check for adequate permissions, etc..).

I just want something that makes the NAC a distinct entity, that is
not necessarily constrained by the BDS graph (especially since we can
now unambiguously address all nodes independent of the graph).


> At the moment I can see two fundamentally different kinds of operations
> that are interesting for op blockers:
> 
> 1. I/O requests
> 
>We need a place to check I/O requests that knows both the user (and
>the permissions it has acquired) and the node that is accessed.
> 
>Any I/O request from outside the block layer (which includes guest
>devices, NBD servers and block jobs) should come through a
>BlockBackend. Even though we don't have it yet, we want to have a
>single BB per user. These requests could be dealt with by calling
>into the NAC whenever a BDS is attach to or detached from a BB;
>assertions can be checked in the appropriate blk_* functions.
> 

Can you elaborate more on "we want to have a single BB per user."?

I certainly agree that for most roles and operations, it is true that
the BB should be the representation.  However, most block jobs will
benefit from specifying node-names in addition to the BB, for easier
node identification.  I'd go as far as requiring it, if we are
implementing a new set of QAPI functions to go a

Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Gonglei (Arei)]

Hi Kevin & Paolo,

Luckily, I reproduced this problem last night. And I got the below log when 
SeaBIOS is stuck.

[BTW, the whole SeaBIOS log attached]

[2015-12-18 10:38:10] >gonglei: enter smp_setup()...
[2015-12-18 10:38:10] >gonglei: begine to enable local APIC...
[2015-12-18 10:38:10] >gonglei: finish enable local APIC...
[2015-12-18 10:38:10] >>>gonglei: cmos_smp_count=8
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=5
[2015-12-18 10:38:10] ===: CountCPUs=2, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=7
[2015-12-18 10:38:10] ===: CountCPUs=3, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=1
[2015-12-18 10:38:10] ===: CountCPUs=4, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=2
[2015-12-18 10:38:10] ===: CountCPUs=5, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=4
[2015-12-18 10:38:10] ===: CountCPUs=6, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=3
[2015-12-18 10:38:10] ===: CountCPUs=7, SMPStack=0x6d84
[2015-12-18 10:38:10] >>> enter handle_smp...
[2015-12-18 10:38:10] handle_smp: apic_id=6
[2015-12-18 10:38:10] ===: CountCPUs=8, SMPStack=0x6d84
[2015-12-18 10:38:10]  gonglei: finish while   

[pid 31509 is a vcpu thread used 100% cpu overhead]

# cat /proc/31509/stack  
[] vmx_vcpu_run+0x35c/0x580 [kvm_intel]
[] em_push+0x0/0x20 [kvm]
[] x86_emulate_instruction+0x20c/0x440 [kvm]
[] handle_exception+0xe4/0x1b58 [kvm_intel]
[] vcpu_enter_guest+0x565/0x790 [kvm]
[] vmx_get_segment_base+0x0/0xb0 [kvm_intel]
[] __vcpu_run+0x198/0x260 [kvm]
[] kvm_arch_vcpu_ioctl_run+0x68/0x1a0 [kvm]
[] vcpu_load+0x4e/0x80 [kvm]
[] kvm_vcpu_ioctl+0x38e/0x580 [kvm]
[] futex_wake+0xfd/0x110
[] security_file_permission+0x1c/0xa0
[] do_vfs_ioctl+0x8b/0x3b0
[] sys_ioctl+0xa1/0xb0
[] system_call_fastpath+0x16/0x1b
[] 0x

And kvm tracing information:

<...>-31509 [035] 154753.180077: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180077: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180077: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180077: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180078: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180078: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180079: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180079: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180079: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180080: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180080: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180080: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180081: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180081: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180081: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180081: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180082: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180083: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180083: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180083: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180084: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180084: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180084: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180084: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180085: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180085: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180085: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180085: kvm_entry: vcpu 0
<...>-31509 [035] 154753.180086: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306

Now, it's very clear that the guest stuck in yiled(), and then kvm encounter 
the exception #UD.

Do you have any thoughts? Thanks!


The Seabios patch below:

diff --git a/roms/seabios/src/boot.c b/roms/seabios/src/boot.c
index f23e9e1..552914a 100644
--- a/roms/seabios/src/boot.c
+++ b/roms/seabios/src/boot.c
@@ -93,7 +93,7 @@ glob_prefix(const char *glob, const char *str)
 static int
 find_prio(const char *glob)
 {
-dprintf(1, "Searching bootorder for: %s\n", glob);
+//dprintf(1, "Searching bootorder for: %s\n", glob);
 int i;
 for (i = 0; i < BootorderCount; i++)
 if (glob_prefix(glob, Bootorder[i]))
diff --git a/roms/seabios/src/fw/smp.c b/roms/seabios/src/fw/smp.c
index a466ea6..46ec607 100644
--- a/roms/seabios/src/fw/smp.c
+++ b/roms/seabios/src/fw/smp.c
@@ -46,12 +46,16 @@ int apic_id_is_present(u8 apic_id)
 return !!(FoundAPICIDs[apic_id/32] & (1u

[Qemu-devel] ping Re: [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

https://patchwork.ozlabs.org/patch/555945/

On Dec 11, 2015, at 10:27 PM, Programmingkid wrote:

> Mac OS X can be picky when it comes to allowing the user
> to use physical devices in QEMU. Most mounted volumes
> appear to be off limits to QEMU. If an issue is detected,
> a message is displayed showing the user how to unmount a
> volume. Now QEMU uses both CD and DVD media.
> 
> Signed-off-by: John Arbuckle 
> 
> ---
> Removed mediaType parameter from FindEjectableOpticalMedia().
> Added goto statements to hdev_open.
> Replaced snprintf() with g_strdup() in FindEjectableOpticalMedia().
> Put back return statement in hdev_open for Linux compatibility.
> 
> block/raw-posix.c |  163 -
> 1 files changed, 124 insertions(+), 39 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d9162fd..82e8e62 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -43,6 +43,7 @@
> #include 
> #include 
> //#include 
> +#include 
> #include 
> #endif
> 
> @@ -1975,33 +1976,46 @@ BlockDriver bdrv_file = {
> /* host device */
> 
> #if defined(__APPLE__) && defined(__MACH__)
> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> CFIndex maxPathSize, int flags);
> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
> +static char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
> {
> -kern_return_t   kernResult;
> +kern_return_t kernResult = KERN_FAILURE;
> mach_port_t masterPort;
> CFMutableDictionaryRef  classesToMatch;
> +const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
> +char *mediaType = NULL;
> 
> kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
> if ( KERN_SUCCESS != kernResult ) {
> printf( "IOMasterPort returned %d\n", kernResult );
> }
> 
> -classesToMatch = IOServiceMatching( kIOCDMediaClass );
> -if ( classesToMatch == NULL ) {
> -printf( "IOServiceMatching returned a NULL dictionary.\n" );
> -} else {
> -CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), 
> kCFBooleanTrue );
> -}
> -kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, 
> mediaIterator );
> -if ( KERN_SUCCESS != kernResult )
> -{
> -printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
> -}
> +int index;
> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
> +classesToMatch = IOServiceMatching(matching_array[index]);
> +if (classesToMatch == NULL) {
> +error_report("IOServiceMatching returned NULL for %s",
> + matching_array[index]);
> +continue;
> +}
> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> + kCFBooleanTrue);
> +kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> +  mediaIterator);
> +if (kernResult != KERN_SUCCESS) {
> +error_report("Note: IOServiceGetMatchingServices returned %d",
> + kernResult);
> +}
> 
> -return kernResult;
> +/* If a match was found, leave the loop */
> +if (*mediaIterator != 0) {
> +DPRINTF("Matching using %s\n", matching_array[index]);
> +mediaType = g_strdup(matching_array[index]);
> +break;
> +}
> +}
> +return mediaType;
> }
> 
> kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> @@ -2033,7 +2047,35 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
> char *bsdPath,
> return kernResult;
> }
> 
> -#endif
> +/* Sets up a real cdrom for use in QEMU */
> +static bool setup_cdrom(char *bsd_path, Error **errp)
> +{
> +int index, num_of_test_partitions = 2, fd;
> +char test_partition[MAXPATHLEN];
> +bool partition_found = false;
> +
> +/* look for a working partition */
> +for (index = 0; index < num_of_test_partitions; index++) {
> +snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
> + index);
> +fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
> +if (fd >= 0) {
> +partition_found = true;
> +qemu_close(fd);
> +break;
> +}
> +}
> +
> +/* if a working partition on the device was not found */
> +if (partition_found == false) {
> +error_setg(errp, "Failed to find a working partition on disc");
> +} else {
> +DPRINTF("Using %s as optical disc\n", test_partition);
> +pstrcpy(bsd_path, MAXPATHLEN, test_partition);
> +}
> +return partition_found;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> 
> static int hdev_probe_device(const char *

[Qemu-devel] [Bug 1527765] Re: sh4: ghc randomly segfaults on qemu-sh4-static

[Laurent Vivier]

Thank you for the 611 MB tar

The behavior is a little bit different on my system:

root@Quad:~# ls
ghc-7.8.4 ghc_7.8.4-9~bpo8+1.dsc
ghc_7.8.4-9~bpo8+1.debian.tar.xz  ghc_7.8.4.orig.tar.xz
root@Quad:~# cd ghc-7.8.4/utils/ghc-p
ghc-pkg/ ghc-pwd/ 
root@Quad:~# cd ghc-7.8.4/utils/ghc-p
ghc-pkg/ ghc-pwd/ 
root@Quad:~# cd ghc-7.8.4/utils/ghc-pwd/
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ls
Main  Main.hi  Main.hs  Main.o  Setup.hs  ghc-pwd.cabal  ghc.mk
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main
Main Main.hi  Main.hs  Main.o   
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
root@Quad:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
ghc: pthread_mutex_lock.c:80: __pthread_mutex_lock: Assertion 
`mutex->__data.__owner == 0' failed.
qemu: uncaught target signal 6 (Aborted) - core dumped
Aborted (core dumped)

The emulated "tas.b" instruction is not atomic, this is why sometimes
the locking fails...

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1527765

Title:
  sh4: ghc randomly segfaults on qemu-sh4-static

Status in QEMU:
  New

Bug description:
  Hello!

  I am currently in the process of bootstrapping ghc for the Debian sh4
  port and ran into a strange problem with qemu-sh4-static which
  randomly segfaults when running ghc to compile a Haskell source:

  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ls
  Main.hi  Main.hs  Setup.hs  ghc-pwd.cabal  ghc.mk
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  [1 of 1] Compiling Main ( Main.hs, Main.o )
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  [1 of 1] Compiling Main ( Main.hs, Main.o )
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  [1 of 1] Compiling Main ( Main.hs, Main.o )
  Bad interface file: 
/usr/local/lib/sh4-unknown-linux-gnu-ghc-7.10.3/time/dist-install/build/Data/Time/Format/Parse.hi
  ghc: panic! (the 'impossible' happened)
    (GHC version 7.10.3 for sh4-unknown-linux):
   getSymtabName:unknown known-key unique
  <>

  Please report this as a GHC bug:
  http://www.haskell.org/ghc/reportabug

  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  [1 of 1] Compiling Main ( Main.hs, Main.o )
  Linking Main ...
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd#

  As seen above, compiling a Haskell source code often results in a
  segfault but simply by retrying to run ghc over and over again, the
  compile process will eventually succeed and no segfault occurs.

  I have created a tarball which contains the sh4 chroot from the
  example above which also includes ghc, gcc and the source code in
  question (in /root/ghc-7.8.4/utils/ghc-pwd). To test, it's probably a
  good idea to replace the qemu-sh4-static binary in /usr/bin with a
  current git snapshot (which I tried but didn't help).

  > http://users.physik.fu-berlin.de/~glaubitz/sid-sh4-sbuild-ghc.tgz

  In case anyone wants to try ghc with their own sh4 chroot, here's my
  version of ghc:

  > https://people.debian.org/~glaubitz/sh4-unknown-linux-gnu-
  ghc-7.10.3.tar.gz

  Just extract this tarball into the root directory of the sh4 chroot.

  Please note, that it might be advisable on sh4 to apply the patches
  from these two bug reports as otherwise qemu-sh4-static won't work
  properly on amd64 and misses syscall 186:

  > https://bugs.launchpad.net/ubuntu/+source/qemu-li

Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Kevin O'Connor]

On Fri, Dec 18, 2015 at 03:04:58AM +, Gonglei (Arei) wrote:
> Hi Kevin & Paolo,
> 
> Luckily, I reproduced this problem last night. And I got the below log when 
> SeaBIOS is stuck.
[...]
> [2015-12-18 10:38:10]  gonglei: finish while   
[...]
> <...>-31509 [035] 154753.180077: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 
> 0 8306
> <...>-31509 [035] 154753.180077: kvm_emulate_insn: 0:3:f0 53 (real)
> <...>-31509 [035] 154753.180077: kvm_inj_exception: #UD (0x0)
> <...>-31509 [035] 154753.180077: kvm_entry: vcpu 0

This is an odd finding.  It seems to indicate that the code is caught
in an infinite irq loop once irqs are enabled.  What doesn't make
sense is that an NMI shouldn't depend on the cpu irq enable flag.
Also, I can't explain why rip would be 0x03, nor why a #UD in an
exception handler wouldn't result in a triple fault.  Maybe someone
with more kvm knowledge could help here.

I did notice that you appear to be running with SeaBIOS v1.8.1 - I
recommend you upgrade to the latest.  There were two important fixes
in this area (8b9942fa and 3156b71a).  I don't think either of these
fixes would explain the log above, but it would be best to eliminate
the possibility.

-Kevin

Re: [Qemu-devel] [PATCH 08/11] qjson: Simplify by using json-output-visitor

[Eric Blake]

On 12/11/2015 04:10 AM, Paolo Bonzini wrote:
> 
> 
> On 11/12/2015 00:53, Eric Blake wrote:
>> Instead of rolling our own limited JSON outputter, we can just
>> wrap the more full-featured JSON output Visitor.
>>
>> This slightly changes the output (different spacing), but the
>> result is still equivalent JSON contents.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  qjson.c | 61 ++---
>>  1 file changed, 22 insertions(+), 39 deletions(-)
> 
> I didn't find out which tree your patches apply to, but this:
> 

>  if (vmdesc) {
> -json_prop_int(vmdesc, "size", size);
> -json_start_array(vmdesc, "fields");
> -json_start_object(vmdesc, NULL);
> -json_prop_str(vmdesc, "name", "data");
> -json_prop_int(vmdesc, "size", size);
> -json_prop_str(vmdesc, "type", "buffer");
> -json_end_object(vmdesc);
> -json_end_array(vmdesc);
> +visit_type_int(vmdesc, size, "size", &error_abort);

visit_type_int() takes 'int64_t*', not 'int'.  In places, that means I'd
have to add a temporary variable to cover the fact that the input is not
the right type for writing '&size'

> @@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
> bool iterable_only)
>  return;
>  }
>  
> -vmdesc = qjson_new();
> -json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
> -json_start_array(vmdesc, "devices");
> +vmdesc_jov = json_output_visitor_new();
> +vmdesc = json_output_get_visitor(vmdesc_jov);
> +visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort);
> +visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", &error_abort);

...or even cover the case where it is a constant whose address does not
even exist.

> 
> compiles and is pretty much a 1:1 translation from the qjson.c API to the
> visitor API (using this patch as a guide).  Feel free to include it and
> remove qjson.c.  Alternatively, you can leave out this patch and I'll test
> and submit the transition.

So the patch to avoid qjson.c is not quite 1:1. Still, I'll go ahead and
complete the patch for my v2, to see if you still like it.

-- 
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] linux-user/syscall.c: Notice about lock bitmask translation for fcntl

[Chen Gang]

On 12/19/15 06:15, Laurent Vivier wrote:
> 
> Le 18/12/2015 23:12, Chen Gang a écrit :
>>

[...]

>>
>> OK, thank you very much. I shall config my email client again to notice
>> about it.
> 
> You should not use your email client to send patches, you should use
> "git send-email":
> 
> http://wiki.qemu.org/Contribute/SubmitAPatch
> 

OK, thanks.

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

[Qemu-devel] [Bug 1174654] Re: qemu-system-x86_64 takes 100% CPU after host machine resumed from suspend to ram

[varacanero]

Same for me here, Ubuntu wily.
I'm using vagrant-libvirt, and my hosts also very often run wild with 100% cpu 
usage after suspend.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1174654

Title:
  qemu-system-x86_64 takes 100% CPU after host machine resumed from
  suspend to ram

Status in QEMU:
  Confirmed
Status in qemu package in Ubuntu:
  Invalid

Bug description:
  I have Windows XP SP3  inside qemu VM. All works fine in 12.10. But
  after upgraiding to 13.04 i have to restart the VM each time i
  resuming my host machine, because qemu process starts to take CPU
  cycles and OS inside VM is very slow and sluggish. However it's still
  controllable and could be shutdown by itself.

  According to the taskmgr any active process takes 99% CPU. It's not
  stuck on some single process.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1174654/+subscriptions

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Notice about lock bitmask translation for fcntl

[Laurent Vivier]

Le 18/12/2015 23:12, Chen Gang a écrit :
> 
> On 12/19/15 05:58, Laurent Vivier wrote:
>>
>> Le 18/12/2015 22:40, Chen Gang a écrit :
>>>
> 
> [...]
> 
>>> I did not get any script/checkpatch.pl complains, originally.
>>>
>>> Does my email client configuration is incorrect, then cause incorrect
>>> mail format? I guess not. The related reason is below.
>>>
>>>  - I copy your full reply mail contents to a new file (diff.patch).
>>
>> If you copy, you loose the special characters. I do a "Save as".
>>
>>>
>>>  - Remove all '> ' in vi editor (1,% s/^> //g) (so get the original
>>>patch contents).
>>>
>>>  - ./script/checkpatch.pl diff.patch, it has no any complains.
>>
>> If I run "file" on the saved file, I have:
>>
>> $ file orig.eml
>> orig.eml: SMTP mail, ASCII text, with CRLF line terminators
>>
>> I can convert it with "tr":
>>
>> $ tr "\r" "\n" < orig.eml > new.eml
>>
>> $ file new.eml
>> new.eml: SMTP mail, ASCII text
>>
>> $ ./scripts/checkpatch.pl new.eml
>> total: 0 errors, 0 warnings, 54 lines checked
>>
>> new.eml has no obvious style problems and is ready for submission.
>>
>> Laurent
>>
> 
> OK, thank you very much. I shall config my email client again to notice
> about it.

You should not use your email client to send patches, you should use
"git send-email":

http://wiki.qemu.org/Contribute/SubmitAPatch

Laurent

[Qemu-devel] [Bug 1527765] [NEW] sh4: ghc randomly segfaults on qemu-sh4-static

[John Paul Adrian Glaubitz]

Public bug reported:

Hello!

I am currently in the process of bootstrapping ghc for the Debian sh4
port and ran into a strange problem with qemu-sh4-static which randomly
segfaults when running ghc to compile a Haskell source:

root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ls
Main.hi  Main.hs  Setup.hs  ghc-pwd.cabal  ghc.mk
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
[1 of 1] Compiling Main ( Main.hs, Main.o )
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
[1 of 1] Compiling Main ( Main.hs, Main.o )
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
[1 of 1] Compiling Main ( Main.hs, Main.o )
Bad interface file: 
/usr/local/lib/sh4-unknown-linux-gnu-ghc-7.10.3/time/dist-install/build/Data/Time/Format/Parse.hi
ghc: panic! (the 'impossible' happened)
  (GHC version 7.10.3 for sh4-unknown-linux):
 getSymtabName:unknown known-key unique
<>

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
[1 of 1] Compiling Main ( Main.hs, Main.o )
Linking Main ...
root@jessie32:~/ghc-7.8.4/utils/ghc-pwd#

As seen above, compiling a Haskell source code often results in a
segfault but simply by retrying to run ghc over and over again, the
compile process will eventually succeed and no segfault occurs.

I have created a tarball which contains the sh4 chroot from the example
above which also includes ghc, gcc and the source code in question (in
/root/ghc-7.8.4/utils/ghc-pwd). To test, it's probably a good idea to
replace the qemu-sh4-static binary in /usr/bin with a current git
snapshot (which I tried but didn't help).

> http://users.physik.fu-berlin.de/~glaubitz/sid-sh4-sbuild-ghc.tgz

In case anyone wants to try ghc with their own sh4 chroot, here's my
version of ghc:

> https://people.debian.org/~glaubitz/sh4-unknown-linux-gnu-
ghc-7.10.3.tar.gz

Just extract this tarball into the root directory of the sh4 chroot.

Please note, that it might be advisable on sh4 to apply the patches from
these two bug reports as otherwise qemu-sh4-static won't work properly
on amd64 and misses syscall 186:

> https://bugs.launchpad.net/ubuntu/+source/qemu-linaro/+bug/1254824
> https://bugs.launchpad.net/qemu/+bug/1516408

The above issue is reproducible with the two patches applied and
without. It's also reproducible with both libc6 2.19 and 2.21 in the
chroot. Thus, I am currently out of ideas what else to test.

Cheers,
Adrian

** Affects: qemu
 Importance: Undecided
 Status: New

** Description changed:

  Hello!
  
  I am currently in the process of bootstrapping ghc for the Debian sh4
  port and ran into a strange problem with qemu-sh4-static which randomly
  segfaults when running ghc to compile a Haskell source:
  
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ls
  Main.hi  Main.hs  Setup.hs  ghc-pwd.cabal  ghc.mk
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault
  root@jessie32:~/ghc-7.8.4/utils/ghc-pwd# ghc Main.hs
  /bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Notice about lock bitmask translation for fcntl

[Chen Gang]

On 12/19/15 05:58, Laurent Vivier wrote:
> 
> Le 18/12/2015 22:40, Chen Gang a écrit :
>>

[...]

>> I did not get any script/checkpatch.pl complains, originally.
>>
>> Does my email client configuration is incorrect, then cause incorrect
>> mail format? I guess not. The related reason is below.
>>
>>  - I copy your full reply mail contents to a new file (diff.patch).
> 
> If you copy, you loose the special characters. I do a "Save as".
> 
>>
>>  - Remove all '> ' in vi editor (1,% s/^> //g) (so get the original
>>patch contents).
>>
>>  - ./script/checkpatch.pl diff.patch, it has no any complains.
> 
> If I run "file" on the saved file, I have:
> 
> $ file orig.eml
> orig.eml: SMTP mail, ASCII text, with CRLF line terminators
> 
> I can convert it with "tr":
> 
> $ tr "\r" "\n" < orig.eml > new.eml
> 
> $ file new.eml
> new.eml: SMTP mail, ASCII text
> 
> $ ./scripts/checkpatch.pl new.eml
> total: 0 errors, 0 warnings, 54 lines checked
> 
> new.eml has no obvious style problems and is ready for submission.
> 
> Laurent
> 

OK, thank you very much. I shall config my email client again to notice
about it.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed.

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Notice about lock bitmask translation for fcntl

[Laurent Vivier]

Le 18/12/2015 22:40, Chen Gang a écrit :
> 
> On 12/18/15 17:37, Laurent Vivier wrote:
>>
>> Le 18/12/2015 07:26, Chen Gang a écrit :
>>>
>>> For fcntl, it always needs to notice about it, just like do_fcntl() has
>>> done, or it will cause issue (e.g. alpha host run i386 guest).
>>>
>>> Signed-off-by: Chen Gang 
>>> ---
>>>  linux-user/syscall.c |   18 --
>>>  1 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 0f8adeb..1a60e6f 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -9007,7 +9007,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  if (((CPUARMState *)cpu_env)->eabi) {
>>>  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>>>  goto efault;
>>> -fl.l_type = tswap16(target_efl->l_type);
>>> +fl.l_type = 
>>> target_to_host_bitmask(tswap16(target_fl->l_type),
>>> +   flock_tbl);
>>>  fl.l_whence = tswap16(target_efl->l_whence);
>>>  fl.l_start = tswap64(target_efl->l_start);
>>>  fl.l_len = tswap64(target_efl->l_len);
>>> @@ -9018,7 +9019,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  {
>>>  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>>>  goto efault;
>>> -fl.l_type = tswap16(target_fl->l_type);
>>> +fl.l_type = 
>>> target_to_host_bitmask(tswap16(target_fl->l_type),
>>> +   flock_tbl);
>>>  fl.l_whence = tswap16(target_fl->l_whence);
>>>  fl.l_start = tswap64(target_fl->l_start);
>>>  fl.l_len = tswap64(target_fl->l_len);
>>> @@ -9031,7 +9033,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  if (((CPUARMState *)cpu_env)->eabi) {
>>>  if (!lock_user_struct(VERIFY_WRITE, target_efl, arg3, 
>>> 0)) 
>>>  goto efault;
>>> -target_efl->l_type = tswap16(fl.l_type);
>>> +target_efl->l_type = host_to_target_bitmask(
>>> + tswap16(fl.l_type), 
>>> flock_tbl);
>>>  target_efl->l_whence = tswap16(fl.l_whence);
>>>  target_efl->l_start = tswap64(fl.l_start);
>>>  target_efl->l_len = tswap64(fl.l_len);
>>> @@ -9042,7 +9045,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  {
>>>  if (!lock_user_struct(VERIFY_WRITE, target_fl, arg3, 
>>> 0)) 
>>>  goto efault;
>>> -target_fl->l_type = tswap16(fl.l_type);
>>> +target_fl->l_type = host_to_target_bitmask(
>>> + tswap16(fl.l_type), 
>>> flock_tbl);
>>>  target_fl->l_whence = tswap16(fl.l_whence);
>>>  target_fl->l_start = tswap64(fl.l_start);
>>>  target_fl->l_len = tswap64(fl.l_len);
>>> @@ -9058,7 +9062,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  if (((CPUARMState *)cpu_env)->eabi) {
>>>  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>>>  goto efault;
>>> -fl.l_type = tswap16(target_efl->l_type);
>>> +fl.l_type = 
>>> target_to_host_bitmask(tswap16(target_fl->l_type),
>>> +   flock_tbl);
>>>  fl.l_whence = tswap16(target_efl->l_whence);
>>>  fl.l_start = tswap64(target_efl->l_start);
>>>  fl.l_len = tswap64(target_efl->l_len);
>>> @@ -9069,7 +9074,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>>> arg1,
>>>  {
>>>  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>>>  goto efault;
>>> -fl.l_type = tswap16(target_fl->l_type);
>>> +fl.l_type = 
>>> target_to_host_bitmask(tswap16(target_fl->l_type),
>>> +   flock_tbl);
>>>  fl.l_whence = tswap16(target_fl->l_whence);
>>>  fl.l_start = tswap64(target_fl->l_start);
>>>  fl.l_len = tswap64(target_fl->l_len);
>>>
>>
>> This patch looks good to me, except that script/checkpatch.pl complains
>> about "DOS line ending" and "line over 80 characters".
>>
> 
> I did not get any script/checkpatch.pl complains, originally.
> 
> Does my email client configuration is incorrect, then cause incorrect
> mail format? I guess not. The related reason is below.
> 
>  - I copy your full reply mail contents to a new file (diff.patch).

If you copy,

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-iotests: Reduce racy output in 028

[John Snow]

On 12/18/2015 03:46 PM, Eric Blake wrote:
> On 12/11/2015 06:23 PM, John Snow wrote:
>>
> 
> ping
> 
>>
>> On 12/10/2015 10:27 PM, Eric Blake wrote:
>>> On my machine, './check -qcow2 028' was failing about 80% of the
>>> time, due to a race in how many times the repeated attempts
>>> to run 'info block-jobs' could occur before the job was done,
>>> showing up as a failure of fewer '(qemu) ' prompts than in the
>>> expected output.  Silence the output during the repetitions, then
>>> add a final clean command to keep the expected output useful;
>>> once patched, I was finally able to run the test 20 times in a
>>> row with no failures.
>>>
>>
>> Son of a gun:
>>
>> 028 4s ... - output mismatch (see 028.out.bad)
>> --- /home/bos/jsnow/src/q3mu/tests/qemu-iotests/028.out  2015-10-09
>> 13:33:06.823758271 -0400
>> +++ 028.out.bad  2015-12-11 20:12:23.688952510 -0500
>> @@ -470,7 +470,7 @@
>>
>>  Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832
>> backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
>>  (qemu)
>> -(qemu) info block-jobs
>> +info block-jobs
>>  Type backup, device disk: Completed 0 of 4294968832 bytes, speed limit
>> 0 bytes/s
>>  info block-jobs
>>  No active jobs
>>
>>
>> Something must have changed to make it more prone to races, somehow.
>> Venus... Swamp gas...
> 
> Now that 2.6 is open, is my approach right, or do we need a more robust
> solution?
> 
>>
>>> Signed-off-by: Eric Blake 
>>> ---
>>>
>>> Not sure if this is the best fix, or if it is even appropriate
>>> for inclusion in 2.5 this late in the game.
>>>
>>>  tests/qemu-iotests/028 | 6 --
>>>  tests/qemu-iotests/028.out | 3 ---
>>>  2 files changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
>>> index a1f4423..756c335 100755
>>> --- a/tests/qemu-iotests/028
>>> +++ b/tests/qemu-iotests/028
>>> @@ -111,10 +111,12 @@ h=$QEMU_HANDLE
>>>  QEMU_COMM_TIMEOUT=1
>>>
>>>  # Silence output since it contains the disk image path and QEMU's readline
>>> -# character echoing makes it very hard to filter the output
>>> +# character echoing makes it very hard to filter the output. Plus, there
>>> +# is no telling how many times the command will repeat before succeeding.
>>>  _send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
>>>  _send_qemu_cmd $h "" "Formatting" | _filter_img_create
>>> -qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs"
>>> +qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs" 
>>> >/dev/null
>>> +_send_qemu_cmd $h "info block-jobs" "No active jobs"
>>>  _send_qemu_cmd $h 'quit' ""
>>>
>>>  # Base image sectors
>>> diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
>>> index 29c9972..e03452d 100644
>>> --- a/tests/qemu-iotests/028.out
>>> +++ b/tests/qemu-iotests/028.out
>>> @@ -469,10 +469,7 @@ No errors were found on the image.
>>>  block-backup
>>>
>>>  Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 
>>> backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
>>> -(qemu)
>>>  (qemu) iininfinfoinfo 
>>> info binfo 
>>> blinfo bloinfo 
>>> blocinfo 
>>> blockinfo 
>>> block-info 
>>> block-jinfo 
>>> block-joinfo 
>>> block-jobinfo block-jobs
>>> -Type backup, device disk: Completed 0 of 4294968832 bytes, speed limit 0 
>>> bytes/s
>>> -iininfinfoinfo 
>>> info binfo 
>>> blinfo bloinfo 
>>> blocinfo 
>>> blockinfo 
>>> block-info 
>>> block-jinfo 
>>> block-joinfo 
>>> block-jobinfo block-jobs
>>>  No active jobs
>>>  === IO: pattern 195
>>>  read 512/512 bytes at offset 3221194240
>>>
>>
> 

It looks sane to me, because you are deleting the insane portion of the
expected output. The final "info block" command serves as an assertion
that the loop polled successfully.

Reviewed-by: John Snow 

Re: [Qemu-devel] Jobs 2.0 QAPI [RFC]

[Eric Blake]

On 12/18/2015 02:24 PM, John Snow wrote:

>> To be subclassable, we need a flat union, and the discriminator must be
>> non-optional within that union.  Either 'options' is that flat union
>> (and we have a layer of {} nesting in QMP}, or we directly make the
>> 'data' of job-cancel be the flat union (no need for an "options":{}
>> nesting in QMP); the latter still depends on some of my pending patches,
>> but we'll get there in plenty of time.
>>
> 
> Ah, shame. It would have been nice to delay interpretation of the union
> pending the result of the Job lookup.

Yeah, I don't think dynamic parsing is going to fly in the qapi
generator; right now it works as "parse the entire JSON string into a
QObject, validate that it can be converted to a qapi type, then pass
that to the marshaller"; whereas a dynamic parse would require more like
"parse half the JSON, call the first marshaller to see what else to
parse, finish the parse, and now dispatch to the second marshaller".

> 
> If the discriminator must always be present, though, then so be it.

Maybe a future patch can make it optional (if the discriminator is
missing, do a tentative parse of all remaining fields, and as long as
exactly one branch is happy with the result, then we know what
discriminator to fill in).  But I'm not sure it's worth the complexity,
when it's easier to just tell clients to always pass a discriminator.


> I'll give it a college try and send some actual patches for x-job-query,
> x-job-cancel, etc.
> 
> For the flat union support on the top level as hinted for job-cancel,
> what branch of yours should I develop on top of?

http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi has all my
pending patches in the state last posted to the list, although it will
be non-fast-forwarded as I rebase to latest (I already know that today's
state of that branch doesn't build on today's qemu.git master, now that
2.6 is open and some churn has started creeping in).

-- 
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] linux-user/syscall.c: Notice about lock bitmask translation for fcntl

[Chen Gang]

On 12/18/15 17:37, Laurent Vivier wrote:
> 
> Le 18/12/2015 07:26, Chen Gang a écrit :
>>
>> For fcntl, it always needs to notice about it, just like do_fcntl() has
>> done, or it will cause issue (e.g. alpha host run i386 guest).
>>
>> Signed-off-by: Chen Gang 
>> ---
>>  linux-user/syscall.c |   18 --
>>  1 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 0f8adeb..1a60e6f 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -9007,7 +9007,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  if (((CPUARMState *)cpu_env)->eabi) {
>>  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>>  goto efault;
>> -fl.l_type = tswap16(target_efl->l_type);
>> +fl.l_type = 
>> target_to_host_bitmask(tswap16(target_fl->l_type),
>> +   flock_tbl);
>>  fl.l_whence = tswap16(target_efl->l_whence);
>>  fl.l_start = tswap64(target_efl->l_start);
>>  fl.l_len = tswap64(target_efl->l_len);
>> @@ -9018,7 +9019,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  {
>>  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>>  goto efault;
>> -fl.l_type = tswap16(target_fl->l_type);
>> +fl.l_type = 
>> target_to_host_bitmask(tswap16(target_fl->l_type),
>> +   flock_tbl);
>>  fl.l_whence = tswap16(target_fl->l_whence);
>>  fl.l_start = tswap64(target_fl->l_start);
>>  fl.l_len = tswap64(target_fl->l_len);
>> @@ -9031,7 +9033,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  if (((CPUARMState *)cpu_env)->eabi) {
>>  if (!lock_user_struct(VERIFY_WRITE, target_efl, arg3, 
>> 0)) 
>>  goto efault;
>> -target_efl->l_type = tswap16(fl.l_type);
>> +target_efl->l_type = host_to_target_bitmask(
>> + tswap16(fl.l_type), 
>> flock_tbl);
>>  target_efl->l_whence = tswap16(fl.l_whence);
>>  target_efl->l_start = tswap64(fl.l_start);
>>  target_efl->l_len = tswap64(fl.l_len);
>> @@ -9042,7 +9045,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  {
>>  if (!lock_user_struct(VERIFY_WRITE, target_fl, arg3, 
>> 0)) 
>>  goto efault;
>> -target_fl->l_type = tswap16(fl.l_type);
>> +target_fl->l_type = host_to_target_bitmask(
>> + tswap16(fl.l_type), 
>> flock_tbl);
>>  target_fl->l_whence = tswap16(fl.l_whence);
>>  target_fl->l_start = tswap64(fl.l_start);
>>  target_fl->l_len = tswap64(fl.l_len);
>> @@ -9058,7 +9062,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  if (((CPUARMState *)cpu_env)->eabi) {
>>  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>>  goto efault;
>> -fl.l_type = tswap16(target_efl->l_type);
>> +fl.l_type = 
>> target_to_host_bitmask(tswap16(target_fl->l_type),
>> +   flock_tbl);
>>  fl.l_whence = tswap16(target_efl->l_whence);
>>  fl.l_start = tswap64(target_efl->l_start);
>>  fl.l_len = tswap64(target_efl->l_len);
>> @@ -9069,7 +9074,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  {
>>  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>>  goto efault;
>> -fl.l_type = tswap16(target_fl->l_type);
>> +fl.l_type = 
>> target_to_host_bitmask(tswap16(target_fl->l_type),
>> +   flock_tbl);
>>  fl.l_whence = tswap16(target_fl->l_whence);
>>  fl.l_start = tswap64(target_fl->l_start);
>>  fl.l_len = tswap64(target_fl->l_len);
>>
> 
> This patch looks good to me, except that script/checkpatch.pl complains
> about "DOS line ending" and "line over 80 characters".
> 

I did not get any script/checkpatch.pl complains, originally.

Does my email client configuration is incorrect, then cause incorrect
mail format? I guess not. The related reason is below.

 - I copy your full reply mail contents to a new file (diff.patch).

 - Remove all '> ' in vi editor (1,% s/^> //g) (so get the original
   patch contents).

 - ./script/checkpatch.pl diff.patch, it has no any complains.

> Reviewed-

Re: [Qemu-devel] Jobs 2.0 QAPI [RFC]

[John Snow]

On 12/18/2015 01:07 PM, Eric Blake wrote:
> On 12/16/2015 05:50 PM, John Snow wrote:
>> In working through a prototype to enable multiple block jobs. A few
>> problem spots in our API compatibility become apparent.
>>
> 
>>  qmp_query_block_jobs 
>>
> 
>> Let's consider using a new command. I'm not sure if this is strictly
>> possible with current QAPI, but Eric will yell at me if it isn't --
>> forgive the wiggly pseudo-specification.
>>
>> query-jobs
>>
>> Will return a list of all jobs.
>>
>> query-jobs sys="block"
>>
>> Will return a list of all block jobs. This will be the only valid
>> subsystem to start with -- we don't have non-block jobs yet and it may
>> be some time before we do.
>>
>> Each subsystem will have a sys= enumeration, and the jobs for that
>> particular subsystem can subclass the default return type. The QAPI
>> commands can be defined to accept a union of the subclasses so we don't
>> have to specify in advance which type each command accepts, but we can
>> allow the responsible subsystem to interpret it on demand dynamically.
>>
>> The base type can be something along the lines of:
>>
>> { 'struct': 'JobInfo',
>>   'data': {
>> 'type': JobTypeEnum,
>> 'sys': JobSysEnum,
>> 'id': str,
>> 'busy': bool,
>> 'paused': bool,
>> 'ready': bool
>>   }
>> }
>>
>> And the BlockJobInfo can inherit/extend, looking like this:
>>
>> { 'struct': 'BlockJobInfo',
>>   'base': JobInfo
>>   'data': {
>> 'len': int,
>> 'offset': int,
>> 'speed': 'int',
>> 'io-status': BlockDeviceIoStatus,
>> 'devices': ,
>>   }
>> }
> 
> Done in QAPI via flat unions.  All the common fields, including the
> discriminator 'sys', are present in the base type, and then each
> subclass of a job adds additional parameters to the QMP type by
> specifying their subtype as a branch of the union.  So should be doable.
> 
>>
>> 'devices' will now report a more nuanced view of this Job's associated
>> nodes in our graph, as in the following:
>>
>> { "devices": [
>>   { "name": "drive0",
>> "nodes": [
>>   { "id": "#node123",
>> "permissions": "+RW-X"
>>   },
>>   { "id": "#node456",
>> "permissions": "+R"
>>   }
>> ]
>>   }
>> }
>>
>> This lets us show exactly what nodes we are touching, what BlockBackends
>> they belong to, and what permissions are involved. Unambiguously. You
>> can use your own imagination for a permissions string that will make
>> sense -- This depends on Jeff Cody's work primarily.
> 
> We'd probably want it to look more JSON-y, maybe something like:
> 
> { "id": "#node456",
>   "permissions": [ { "name":"read", "mode":"require" },
>{ "name":"write", "mode":"allow" },
>{ "name":"meta", "mode":"unspecified" } ] }
> 
> but I agree that how it is represented does not affect the proposal here
> of merely representing a list of all associated BDS affected by this
> job, broken out by permissions per BDS (as some jobs will affect
> multiple BDS, and not all of them with the same permissions).
> 

And as not shown here, some may affect multiple devices, so this should
give the full granularity.

The more JSON-y permissions is fine, also. It will likely evolve to
match whatever Jeff Cody needs for his fine-grained op blocker idea.

>>
>> The new command gives a chance to make a clean break and any users of
>> this new command won't be confused about the information they receive.
>>
>> The legacy qmp-query-block-jobs command can continue to operate
>> providing a best-effort approximation of the data it finds. The legacy
>> command will *abort* and return error if it detects any job that was
>> launched by a new-style command, e.g. if it detects there is more than
>> one job attached to any node under a single BlockBackend -- clearly the
>> user has been using new features -- and should abort announcing this
>> command is deprecated.
> 
> Makes sense. If an old client only uses the old commands, things will
> still work; while a new client, once converted to use the new commands,
> should do the conversion completely and not rely on the old view.
> 

Yes. All or nothing. I prefer this approach to an incomplete report. I
shouldn't have used "best-effort" above; I should have said:

"The legacy command will report back jobs if it has sufficient
resolution to do so." e.g. no conflicting device or BDS id results to
the query.

I don't very much like the idea of a query command that can give you
incomplete data, especially to an unwitting querier.

>>
>> If the graph looks reasonably plain, it can report back the information
>> it always has, except that it will now also report the "id" field in
>> addition to be perfectly unambiguous about how to issue commands to this
>> job, like I outlined in the first paragraph of this section.
> 
> Old clients won't care about the new information, but it doesn't hurt to
> supply it, especially if it lets us share code with the new query command.
> 

Th

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Markus Armbruster]

One short remark in addition to Eduardo's review.

Eduardo Habkost  writes:

> On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
>> Signed-off-by: Cao jin 
>> ---
>>  hw/pci-host/piix.c | 16 +---
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 715208b..e3840f0 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>  {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>  };
>>  
>> -static int host_pci_config_read(int pos, int len, uint32_t val)
>> +static int host_pci_config_read(int pos, int len, uint32_t val, Error 
>> **errp)
>
> You don't need the return value anymore, if you report errors
> through the errp parameter. The function can be void, now.
>
>>  {
>>  char path[PATH_MAX];
>>  int config_fd;
>> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
>> uint32_t val)
>>  int ret = 0;
>>  
>>  if (rc >= size || rc < 0) {
>> +error_setg(errp, "No such device");
>>  return -ENODEV;
>>  }
>>  
>>  config_fd = open(path, O_RDWR);
>>  if (config_fd < 0) {
>> +error_setg(errp, "No such device");
>>  return -ENODEV;
>>  }

Can we come up with nicer error messages?

[...]

Re: [Qemu-devel] [PATCH 2/2] qdev: safely fail device_add if unable to allocate device

[Markus Armbruster]

Eric Blake  writes:

> On 12/18/2015 09:48 AM, Daniel P. Berrange wrote:
>> On Fri, Dec 18, 2015 at 04:30:47PM +0100, Igor Mammedov wrote:
>>> qdev_device_add() currently uses object_new() which
>>> will abort if there memory allocation for device instance
>>> fails. While it's fine it startup, it is not desirable
>>> diring hotplug.
>>>
>>> Try to allocate memory for object first and fail safely
>>> if allocation fails.
>>>
>
>> This just avoids one small malloc failure.
>> 
>>> +object_initialize(dev, obj_size, driver);
>> 
>> This is going to call g_new many more times, so you'll
>> still hit OOM almost immediately. eg the call to
>> g_hash_table_new_full() in object_initialize_with_type
>> will abort on OOM, not to mention anything run in a
>> instance constructor function registered against the
>> class. There's no way to avoid this given that we have
>> chosen to use GLib in QEMU, so I don't really see any
>> point in replacing the 'object_new' call with g_try_malloc

Seconded.

> We just had a recent thread on it, and I tend to agree that this series
> isn't helpful.
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/threads.html#01238

Plenty of arguments there why recovering from scattered random
allocation failures isn't useful, and recovering from all of them isn't
practical.  Limit recovery attempts to big allocations, and spend (some
of) the saved cycles on actually testing the recovery code.

[...]

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-iotests: Reduce racy output in 028

[Eric Blake]

On 12/11/2015 06:23 PM, John Snow wrote:
> 

ping

> 
> On 12/10/2015 10:27 PM, Eric Blake wrote:
>> On my machine, './check -qcow2 028' was failing about 80% of the
>> time, due to a race in how many times the repeated attempts
>> to run 'info block-jobs' could occur before the job was done,
>> showing up as a failure of fewer '(qemu) ' prompts than in the
>> expected output.  Silence the output during the repetitions, then
>> add a final clean command to keep the expected output useful;
>> once patched, I was finally able to run the test 20 times in a
>> row with no failures.
>>
> 
> Son of a gun:
> 
> 028 4s ... - output mismatch (see 028.out.bad)
> --- /home/bos/jsnow/src/q3mu/tests/qemu-iotests/028.out   2015-10-09
> 13:33:06.823758271 -0400
> +++ 028.out.bad   2015-12-11 20:12:23.688952510 -0500
> @@ -470,7 +470,7 @@
> 
>  Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832
> backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
>  (qemu)
> -(qemu) info block-jobs
> +info block-jobs
>  Type backup, device disk: Completed 0 of 4294968832 bytes, speed limit
> 0 bytes/s
>  info block-jobs
>  No active jobs
> 
> 
> Something must have changed to make it more prone to races, somehow.
> Venus... Swamp gas...

Now that 2.6 is open, is my approach right, or do we need a more robust
solution?

> 
>> Signed-off-by: Eric Blake 
>> ---
>>
>> Not sure if this is the best fix, or if it is even appropriate
>> for inclusion in 2.5 this late in the game.
>>
>>  tests/qemu-iotests/028 | 6 --
>>  tests/qemu-iotests/028.out | 3 ---
>>  2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
>> index a1f4423..756c335 100755
>> --- a/tests/qemu-iotests/028
>> +++ b/tests/qemu-iotests/028
>> @@ -111,10 +111,12 @@ h=$QEMU_HANDLE
>>  QEMU_COMM_TIMEOUT=1
>>
>>  # Silence output since it contains the disk image path and QEMU's readline
>> -# character echoing makes it very hard to filter the output
>> +# character echoing makes it very hard to filter the output. Plus, there
>> +# is no telling how many times the command will repeat before succeeding.
>>  _send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
>>  _send_qemu_cmd $h "" "Formatting" | _filter_img_create
>> -qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs"
>> +qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs" 
>> >/dev/null
>> +_send_qemu_cmd $h "info block-jobs" "No active jobs"
>>  _send_qemu_cmd $h 'quit' ""
>>
>>  # Base image sectors
>> diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
>> index 29c9972..e03452d 100644
>> --- a/tests/qemu-iotests/028.out
>> +++ b/tests/qemu-iotests/028.out
>> @@ -469,10 +469,7 @@ No errors were found on the image.
>>  block-backup
>>
>>  Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 
>> backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
>> -(qemu)
>>  (qemu) iininfinfoinfo 
>> info binfo 
>> blinfo bloinfo 
>> blocinfo 
>> blockinfo 
>> block-info 
>> block-jinfo 
>> block-joinfo 
>> block-jobinfo block-jobs
>> -Type backup, device disk: Completed 0 of 4294968832 bytes, speed limit 0 
>> bytes/s
>> -iininfinfoinfo 
>> info binfo 
>> blinfo bloinfo 
>> blocinfo 
>> blockinfo 
>> block-info 
>> block-jinfo 
>> block-joinfo 
>> block-jobinfo block-jobs
>>  No active jobs
>>  === IO: pattern 195
>>  read 512/512 bytes at offset 3221194240
>>
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v3 1/2] i386: expose floppy-related objects in SSDT

[Roman Kagan]

On x86-based systems Linux determines the presence and the type of
floppy drives via a query of a CMOS field.  So does SeaBIOS when
populating the return data for int 0x13 function 0x08.

Windows doesn't; instead, it requests this information from BIOS via int
0x13/0x08 or through ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
(Floppy Drive Information).  On UEFI systems only ACPI-based detection
is supported.

QEMU used not to provide those objects in its ACPI tables; as a result
floppy drives were invisible to Windows on UEFI/OVMF.

This patch implements those objects in SSDT, populating them via AML
API.  For that, a couple of functions are extern-ified to facilitate
populating those objects in acpi-build.c.

Signed-off-by: Roman Kagan 
Signed-off-by: Denis V. Lunev 
CC: Michael S. Tsirkin 
CC: Igor Mammedov 
CC: Paolo Bonzini 
CC: Richard Henderson 
CC: Eduardo Habkost 
CC: John Snow 
CC: Kevin Wolf 
--
changes since v2:
 - explicit endianness for buffer data
 - reorder code to reduce conflicts with dynamic DSDT patchset

 hw/block/fdc.c | 11 +++
 hw/i386/acpi-build.c   | 78 ++
 hw/i386/pc.c   | 46 +
 include/hw/block/fdc.h |  2 ++
 include/hw/i386/pc.h   |  3 ++
 5 files changed, 121 insertions(+), 19 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4292ece..c858c5f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2408,6 +2408,17 @@ FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
 return isa->state.drives[i].drive;
 }
 
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+uint8_t *heads, uint8_t *sectors)
+{
+FDCtrlISABus *isa = ISA_FDC(fdc);
+FDrive *drv = &isa->state.drives[i];
+
+*cylinders = drv->max_track;
+*heads = (drv->flags & FDISK_DBL_SIDES) ? 2 : 1;
+*sectors = drv->last_sect;
+}
+
 static const VMStateDescription vmstate_isa_fdc ={
 .name = "fdc",
 .version_id = 2,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fa866f6..15e50fb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/loader.h"
 #include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
@@ -105,6 +106,13 @@ typedef struct AcpiPmInfo {
 uint16_t pcihp_io_len;
 } AcpiPmInfo;
 
+typedef struct AcpiFDInfo {
+uint8_t type;
+uint8_t cylinders;
+uint8_t heads;
+uint8_t sectors;
+} AcpiFDInfo;
+
 typedef struct AcpiMiscInfo {
 bool has_hpet;
 TPMVersion tpm_version;
@@ -112,6 +120,7 @@ typedef struct AcpiMiscInfo {
 unsigned dsdt_size;
 uint16_t pvpanic_port;
 uint16_t applesmc_io_base;
+AcpiFDInfo fdinfo[2];
 } AcpiMiscInfo;
 
 typedef struct AcpiBuildPciBusHotplugState {
@@ -235,10 +244,25 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
 
 static void acpi_get_misc_info(AcpiMiscInfo *info)
 {
+int i;
+ISADevice *fdc;
+
 info->has_hpet = hpet_find();
 info->tpm_version = tpm_get_version();
 info->pvpanic_port = pvpanic_port();
 info->applesmc_io_base = applesmc_port();
+
+fdc = pc_find_fdc0();
+if (fdc) {
+for (i = 0; i < ARRAY_SIZE(info->fdinfo); i++) {
+AcpiFDInfo *fdinfo = &info->fdinfo[i];
+fdinfo->type = isa_fdc_get_drive_type(fdc, i);
+if (fdinfo->type < FDRIVE_DRV_NONE) {
+isa_fdc_get_drive_geometry(fdc, i, &fdinfo->cylinders,
+   &fdinfo->heads, &fdinfo->sectors);
+}
+}
+}
 }
 
 /*
@@ -900,6 +924,40 @@ static Aml *build_crs(PCIHostState *host,
 return crs;
 }
 
+static Aml *build_fdinfo_aml(int idx, AcpiFDInfo *fdinfo)
+{
+Aml *dev, *fdi;
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(0x10);
+aml_append(fdi, aml_int(idx));  /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(fdinfo->type)));  /* Device Type */
+aml_append(fdi,
+aml_int(fdinfo->cylinders - 1));  /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(fdinfo->sectors));  /* Maximum Sector Number */
+aml_append(fdi, aml_int(fdinfo->heads - 1));  /* Maximum Head Number */
+/* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we */
+aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
+aml_append(fdi, aml_int(0x12));  /* disk_eot */
+aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
+aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
+aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
+  

[Qemu-devel] [PATCH v3 2/2] tests: update expected SSDT for floppy changes

[Roman Kagan]

Update the expected SSDTs to reflect the changes introduced in the
previous patch.

Signed-off-by: Roman Kagan 
Signed-off-by: Denis V. Lunev 
CC: Michael S. Tsirkin 
CC: Igor Mammedov 
CC: Paolo Bonzini 
CC: Richard Henderson 
CC: Eduardo Habkost 
CC: John Snow 
CC: Kevin Wolf 
---
changes since v2:
 - new patch

 tests/acpi-test-data/pc/SSDT | Bin 2486 -> 2588 bytes
 tests/acpi-test-data/pc/SSDT.bridge  | Bin 4345 -> 4447 bytes
 tests/acpi-test-data/q35/SSDT| Bin 691 -> 872 bytes
 tests/acpi-test-data/q35/SSDT.bridge | Bin 708 -> 889 bytes
 4 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/acpi-test-data/pc/SSDT b/tests/acpi-test-data/pc/SSDT
index 
210d6a71e58aa34ce8e94121d25bcf58c3bd503c..ba3fa272b8235bdea28eadb0eaa3751f1ef4f59a
 100644
GIT binary patch
delta 123
zcmdlcJV%5pIM^jbhKqrLQFJ3$G-Hx0TZ}$Se6Uk|fU~E8XRu?un~SqSbd#Z*PkLOY0aE2ED9$Cq$bbr%
KHYYNMasU8QB^ERQ

delta 24
gcmbOuvQ3yPIM^j*8z%z;WJQG-Hx0TZ}$Se6Uk|fU~E8XRu?un~SqSbd#Z*PkLOY0aE2ED9$Cq$bbr%
KHYYO9;0FNT%ono&

delta 24
gcmcbw^iz>5IM^lRrvL*3qryh6XvWQ_8K>|A0BC0i#{d8T

diff --git a/tests/acpi-test-data/q35/SSDT b/tests/acpi-test-data/q35/SSDT
index 
0970c67ddb1456707c0111f7e5e659ef385af408..7d12c6a562c6a7d14099d4819ab469797548549d
 100644
GIT binary patch
delta 203
zcmdnY`htxsIM^j5gPDPWaoI*Le#Uwi?ihWR_+Y2_0B27F&tS)RHy3Av=q7tNp8!XW
zct@8Y1`eQ*r;wfi0~ZV5e<)ypv$)oCF>$E^u@ILu*MF`Yu5VoYTpSPsoWKS!!VF-<
gVt~>A|JY3cX>t`5=MrIL0J;^3VSs6~DC0av01bIR_5c6?

delta 24
gcmaFCwwaYHIM^j*GZO;?W9>#Re#Xt`7-um809z#o3IG5A

diff --git a/tests/acpi-test-data/q35/SSDT.bridge 
b/tests/acpi-test-data/q35/SSDT.bridge
index 
a77868861754ce0bb2b7bc8091c03a53754d..f5c4b44b5c0f8a049f321c5aadcc825c261ba99f
 100644
GIT binary patch
delta 203
zcmX@Y`jd?-IM^kml9_>l(QP9aKV!WMcZ@zue6Uk|fU~E8XRu?un~SqSbd$ZCPkpxcx*EcSHE)IwRPGAEVVFoZ_
gF~I2mf9xiJG`R|jbBQoA0No12Fu=4~lyN;H0PbQx+5i9m

delta 24
gcmey#c7&BHIM^lR2onPXqwGd5e#Xt`7*{g_09%v>?f?J)

-- 
2.5.0

[Qemu-devel] [PATCH v3 0/2] i386: expose floppy-related objects in SSDT

[Roman Kagan]

Roman Kagan (2):
  i386: expose floppy-related objects in SSDT
  tests: update expected SSDT for floppy changes

Signed-off-by: Roman Kagan 
Signed-off-by: Denis V. Lunev 
CC: Michael S. Tsirkin 
CC: Igor Mammedov 
CC: Paolo Bonzini 
CC: Richard Henderson 
CC: Eduardo Habkost 
CC: John Snow 
CC: Kevin Wolf 
---
changes since v2:
 - explicit endianness for buffer data
 - reorder code to reduce conflicts with dynamic DSDT patchset
 - update test data


 hw/block/fdc.c   |  11 +
 hw/i386/acpi-build.c |  78 +++
 hw/i386/pc.c |  46 -
 include/hw/block/fdc.h   |   2 +
 include/hw/i386/pc.h |   3 ++
 tests/acpi-test-data/pc/SSDT | Bin 2486 -> 2588 bytes
 tests/acpi-test-data/pc/SSDT.bridge  | Bin 4345 -> 4447 bytes
 tests/acpi-test-data/q35/SSDT| Bin 691 -> 872 bytes
 tests/acpi-test-data/q35/SSDT.bridge | Bin 708 -> 889 bytes
 9 files changed, 121 insertions(+), 19 deletions(-)

-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject

[John Snow]

On 12/18/2015 11:11 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> Currently, fd_revalidate is called in two different places, with two
>> different expectations of behavior:
>>
>> (1) On initialization, as a routine to help pick the drive type and
>> initial geometries as a side-effect of the pick_geometry routine
>>
>> (2) On insert/eject, which either sets the geometries to a default value
>> or chooses new geometries based on the inserted diskette.
>>
>> Break this nonsense apart by creating a new function dedicated towards
>> picking the drive type on initialization.
>>
>> This has a few results:
>>
>> (1) fd_revalidate does not get called on boot anymore for drives with no
>> diskette.
>>
>> (2) pick_geometry will actually get called twice if we have a diskette
>> inserted, but this is harmless. (Once for the drive type, and once
>> as part of the media callback.)
>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/block/fdc.c | 36 +---
>>  1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index b587de8..e752758 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -167,6 +167,7 @@ static void fd_init(FDrive *drv)
>>  drv->disk = FLOPPY_DRIVE_TYPE_NONE;
>>  drv->last_sect = 0;
>>  drv->max_track = 0;
>> +drv->ro = true;
>>  }
>>  
>>  #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
>> @@ -249,13 +250,21 @@ static void fd_recalibrate(FDrive *drv)
>>  fd_seek(drv, 0, 0, 1, 1);
>>  }
>>  
>> -static void pick_geometry(FDrive *drv)
>> +/**
>> + * Determine geometry based on inserted diskette.
>> + */
>> +static bool pick_geometry(FDrive *drv)
> 
> Meaning of return value?
> 

1: "Modified diskette geometry values."

Will amend with documentation.

>>  {
>>  BlockBackend *blk = drv->blk;
>>  const FDFormat *parse;
>>  uint64_t nb_sectors, size;
>>  int i, first_match, match;
>>  
>> +/* We can only pick a geometry if we have a diskette. */
>> +if (!drv->media_inserted) {
>> +return false;
>> +}
>> +
>>  blk_get_geometry(blk, &nb_sectors);
>>  match = -1;
>>  first_match = -1;
>> @@ -293,8 +302,7 @@ static void pick_geometry(FDrive *drv)
>>  }
>>  drv->max_track = parse->max_track;
>>  drv->last_sect = parse->last_sect;
>> -drv->drive = parse->drive;
>> -drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
>> +drv->disk = parse->drive;
>>  drv->media_rate = parse->rate;
>>  
>>  if (drv->media_inserted) {
>> @@ -303,6 +311,14 @@ static void pick_geometry(FDrive *drv)
>> drv->max_track, drv->last_sect,
>> drv->ro ? "ro" : "rw");
>>  }
>> +return true;
>> +}
>> +
>> +static void pick_drive_type(FDrive *drv)
>> +{
>> +if (pick_geometry(drv)) {
>> +drv->drive = drv->disk;
>> +}
>>  }
>>  
>>  /* Revalidate a disk drive after a disk change */
>> @@ -311,15 +327,18 @@ static void fd_revalidate(FDrive *drv)
>>  FLOPPY_DPRINTF("revalidate\n");
>>  if (drv->blk != NULL) {
>>  drv->ro = blk_is_read_only(drv->blk);
>> -pick_geometry(drv);
>>  if (!drv->media_inserted) {
>>  FLOPPY_DPRINTF("No disk in drive\n");
>> +drv->disk = FLOPPY_DRIVE_TYPE_NONE;
> 
> The left hand side is the "current disk type" (@disk's comment says so).
> 
> The right hand side is "no drive connected" drive type (the enum's
> comment says so).
> 
> Thus, we set the current disk type to the "no drive connected" drive
> type.  Sounds nuts, doesn't it?  :)
> 
> Perhaps drv->disk isn't a disk type, but really into what an auto drive
> type should be morphed.  Correct?
> 

... es, it certainly informs us, if our type is auto, what we should
be setting the drive type to.

I'm definitely cheating, since there really should be two separate
enumerations, honestly:

DriveType: {120, 144, 288, auto, none}
DiskType: {120, 144, 288, none}

Though I concede the disk field isn't strictly needed, I was just
desperate to not have the one field mean two things simultaneously.

>> +} else {
>> +pick_geometry(drv);
>>  }
>>  } else {
>>  FLOPPY_DPRINTF("No drive connected\n");
>>  drv->last_sect = 0;
>>  drv->max_track = 0;
>>  drv->flags &= ~FDISK_DBL_SIDES;
>> +drv->disk = FLOPPY_DRIVE_TYPE_NONE;
>>  }
>>  }
>>  
>> @@ -2196,9 +2215,11 @@ static void fdctrl_change_cb(void *opaque, bool load)
>>  FDrive *drive = opaque;
>>  
>>  drive->media_inserted = load && drive->blk && 
>> blk_is_inserted(drive->blk);
>> -
>>  drive->media_changed = 1;
>> -fd_revalidate(drive);
>> +
>> +if (load) {
>> +fd_revalidate(drive);
>> +}
> 
> Hmm.  Looks like drv->last_sect, ->max_track and ->flags now remain
> after an eject.  If yes, why is that a good idea?
> 

It probably isn't. Overlooked. I c

Re: [Qemu-devel] x-input-send-event marked as experimental

[Markus Armbruster]

Marcelo Tosatti  writes:

> Gerd,
>
> commit df5b2adb7398d71016ee469f71e52075ed95e04e
> Author: Gerd Hoffmann 
> Date:   Tue Nov 25 14:54:17 2014 +0100
>
> input: move input-send-event into experimental namespace
> 
> Ongoing discussions on how we are going to specify the console,
> so tag the command as experiental so we can refine things in
> the 2.3 development cycle.
>
> So what is the problem here again? 
> input-send-event can send an input event to a 
> particular console, so that (the console number)
> has to be specified somehow.
>
> But, its already there, you can specify the console:
>
> -{ 'command': 'input-send-event',
> +{ 'command': 'x-input-send-event',
>'data': { '*console':'int', 'events': [ 'InputEvent' ] } }
>
> So what its waiting for to be moved out of experimental status?

See

* http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg03425.html
  Message-ID: <87zjbgvbp0@blackfin.pond.sub.org>
  and the thread around it

* http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg02935.html
  Message-ID: <1447319343.1400.39.ca...@redhat.com>

* commit 513e7cd input: Document why x-input-send-event is still
  experimental

Re: [Qemu-devel] [PATCH v3 6/9] target-arm: support QMP dump-guest-memory

[Andrew Jones]

On Fri, Dec 18, 2015 at 06:46:14PM +, Peter Maydell wrote:
> On 18 December 2015 at 18:05, Andrew Jones  wrote:
> > On Fri, Dec 18, 2015 at 04:31:13PM +, Peter Maydell wrote:
> >> On 18 December 2015 at 16:05, Andrew Jones  wrote:
> >> > On Fri, Dec 18, 2015 at 11:59:39AM +, Peter Maydell wrote:
> >> >> I don't understand why we need to do this. If this is an
> >> >> AArch64 dump then we should just treat it as an AArch64
> >> >> dump, and presumably the consumer of the dump knows enough
> >> >> to know what the "hypervisor view" of a CPU that's currently
> >> >> in 32-bit mode is. It has to anyway to be able to figure
> >> >> out where all the other registers are, so why can't it
> >> >> also figure out what mode the CPU is currently in and thus
> >> >> where r13 is in the xregs array?
> >> >
> >> > You're probably right that this shouldn't be necessary. But, in order for
> >> > it not to be necessary, I'll need to write another crash patch. 
> >> > Currently,
> >> > if you do a dump-guest-memory on a running guest, i.e. one where the 
> >> > kernel
> >> > has not called panic(), and thus the cpus are actually in 32-bit 
> >> > usermode,
> >> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> >> > segfaults if sp == xregs[31]. crash does properly decode the registers
> >> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
> >> > to aarch64_compat_sp here also allows crash to work properly in the non-
> >> > panic'd case.
> >>
> >> If crash segfaults then that's clearly a bug in crash...
> >> What is it expecting to see in the SP field?
> >
> > A valid stack pointer
> 
> But why? What does it do with it? (In any case a dump of a crashed
> system could have any random rubbish in SP so the tool has to handle
> it being something weird.)

The reason crash segfaults is because sp (xregs[31]) wasn't a user stack
address, and thus it expected the stack to include at least two frames,
which would mean fp would be non-zero, but it's not, and that leads to the
calculation of a bad stack pointer which then gets used as an offset into
the stack buffer, overflowing it. This is definitely a crash bug that
should be fixed. I'll report it.

Also, crash's arm64_get_dumpfile_stackframe() simply assumes sp will be
the stack pointer, and it doesn't check pstate for the MODE32 bit first.
I think it should be easy to fix this, as I only found two places that
should be changed. I'll send a patch for that.

> 
> >> > So, I could teach crash to do what I'm doing here in qemu instead, but
> >> > there's still one more reason why it may make sense to do it here. That
> >> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> >> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> >> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> >> > so why not set sp to the correct userspace sp?
> >>
> >> Well, what spec are we following here? The most logical view
> >
> > With the shoehorning approach we're following the aarch64 ptrace spec,
> > but putting arm32 registers in it. We then rely on the analysis tools
> > to do the right thing when they see pstate has PSR_MODE32_BIT set, which
> > all cpsr modes do.
> 
> Right, so the analysis tool already has to cope with the MODE32 bit
> being set, and it can also figure out where the SP is (and which
> other registers are currently live for 32-bit).
> 
> > The ptrace code in the kernel would return a real aarch32 view, i.e.
> > only registers up to r15 and a cpsr. We can't do that here because we've
> > committed to an EM_AARCH64 formatted core, and we only have one type of
> > PRSTATUS note for that core type. Furthermore, we want to be able to
> > return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.
> >
> >> to me seems to be to say "you get the view of the system
> >> that you get from a JTAG debugger or a hypervisor", which
> >> is to say you see a 64-bit set of registers and it's the
> >> debugger's job to decide which bits of those might be
> >> interesting to view as 32-bit and what is actually "live"
> >> 32 bit state.
> >
> > It appears that PRSTATUS aware tools don't currently work this way.
> >
> >>
> >> From that point of view there is no valid AArch64 SP register
> >> at this point in execution (xregs[31] for QEMU would be stale
> >> state, so not a good choice I think).
> >
> > I suppose zero or all 1's are the safest choices for "undefined", but
> > being undefined actually gives us freedom to use aarch64_compat_sp as
> > well, which, to me, looks like a safer and more useful value.
> 
> It just doesn't really make sense to me to do this one bit of
> work for the debug analysis tools when they already have to know
> that 32-bit modes are special. We seem to end up with a function
> in QEMU whose only purpose is working around a bug in the thing
> consuming the coredump.

OK, I've come around to your point of view. What value would 

Re: [Qemu-devel] [PATCH] gtk: use setlocale() for LC_MESSAGES only

[Markus Armbruster]

Alberto Garcia  writes:

>>> > We do however have translations for a few simple strings for the GTK+
>>> > menu items, so in order to run QEMU using the C locale, and yet have a
>>> > translated UI let's use setlocale() for LC_MESSAGES only.
>>> > 
>>> Not sure why I noticed it only now and if it's related to any recent
>>> package upgrade on my side (using RHEL 7), but I noticed that
>>> non-ASCII characters in the GTK UI strings are broken for me and git
>>> bisect pointed to this commit.
>>
>> I guess we need to set LC_CTYPE too.
>
> That affects functions in ctype.h (isalpha(), islower(), isupper(), ...)
> I guess that's safe?

If we're guessing, then I guess it isn't.  But we shouldn't be guessing.

"LC_CTYPE affects the behavior of the character handling functions and
the multibyte and wide character functions."

I doubt there's much use for the latter in QEMU itself, but in
libraries, all bets are off.  I guess this is what actually screws up
GTK.

We do use the former.  LC_CTYPE set to some sufficiently funky locale is
bound to upset these uses.

In short: nope, we can't just set LC_CTYPE, at least not without further
analysis.

We should've stayed out of the GUI business.

[...]

[Qemu-devel] x-input-send-event marked as experimental

[Marcelo Tosatti]

Gerd,

commit df5b2adb7398d71016ee469f71e52075ed95e04e
Author: Gerd Hoffmann 
Date:   Tue Nov 25 14:54:17 2014 +0100

input: move input-send-event into experimental namespace

Ongoing discussions on how we are going to specify the console,
so tag the command as experiental so we can refine things in
the 2.3 development cycle.

So what is the problem here again? 
input-send-event can send an input event to a 
particular console, so that (the console number)
has to be specified somehow.

But, its already there, you can specify the console:

-{ 'command': 'input-send-event',
+{ 'command': 'x-input-send-event',
   'data': { '*console':'int', 'events': [ 'InputEvent' ] } }

So what its waiting for to be moved out of experimental status?

Thanks

Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Marcel Apfelbaum]

On 12/18/2015 08:08 PM, Eduardo Habkost wrote:

On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote:

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:

We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
guest_info.


Hi Eduardo,

I like the idea of using qdev_get_machine() to get the machine instead of
keeping the reference around, however only in places we cannot get a reference
to it otherwise.


I was unsure if I should go to the extreme of removing all the
guest_info parameters, when doing that. I agree with your
suggestions, but v3 will have to wait until I get back from
vacations in Jan 18th. At least we will have less conflicts with
Igor's AML series. :)


Sure, and have fun in your vacation! :)
Marcel



Thanks!

Re: [Qemu-devel] [PATCH v3 6/9] target-arm: support QMP dump-guest-memory

[Peter Maydell]

On 18 December 2015 at 18:05, Andrew Jones  wrote:
> On Fri, Dec 18, 2015 at 04:31:13PM +, Peter Maydell wrote:
>> On 18 December 2015 at 16:05, Andrew Jones  wrote:
>> > On Fri, Dec 18, 2015 at 11:59:39AM +, Peter Maydell wrote:
>> >> I don't understand why we need to do this. If this is an
>> >> AArch64 dump then we should just treat it as an AArch64
>> >> dump, and presumably the consumer of the dump knows enough
>> >> to know what the "hypervisor view" of a CPU that's currently
>> >> in 32-bit mode is. It has to anyway to be able to figure
>> >> out where all the other registers are, so why can't it
>> >> also figure out what mode the CPU is currently in and thus
>> >> where r13 is in the xregs array?
>> >
>> > You're probably right that this shouldn't be necessary. But, in order for
>> > it not to be necessary, I'll need to write another crash patch. Currently,
>> > if you do a dump-guest-memory on a running guest, i.e. one where the kernel
>> > has not called panic(), and thus the cpus are actually in 32-bit usermode,
>> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
>> > segfaults if sp == xregs[31]. crash does properly decode the registers
>> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
>> > to aarch64_compat_sp here also allows crash to work properly in the non-
>> > panic'd case.
>>
>> If crash segfaults then that's clearly a bug in crash...
>> What is it expecting to see in the SP field?
>
> A valid stack pointer

But why? What does it do with it? (In any case a dump of a crashed
system could have any random rubbish in SP so the tool has to handle
it being something weird.)

>> > So, I could teach crash to do what I'm doing here in qemu instead, but
>> > there's still one more reason why it may make sense to do it here. That
>> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
>> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
>> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
>> > so why not set sp to the correct userspace sp?
>>
>> Well, what spec are we following here? The most logical view
>
> With the shoehorning approach we're following the aarch64 ptrace spec,
> but putting arm32 registers in it. We then rely on the analysis tools
> to do the right thing when they see pstate has PSR_MODE32_BIT set, which
> all cpsr modes do.

Right, so the analysis tool already has to cope with the MODE32 bit
being set, and it can also figure out where the SP is (and which
other registers are currently live for 32-bit).

> The ptrace code in the kernel would return a real aarch32 view, i.e.
> only registers up to r15 and a cpsr. We can't do that here because we've
> committed to an EM_AARCH64 formatted core, and we only have one type of
> PRSTATUS note for that core type. Furthermore, we want to be able to
> return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.
>
>> to me seems to be to say "you get the view of the system
>> that you get from a JTAG debugger or a hypervisor", which
>> is to say you see a 64-bit set of registers and it's the
>> debugger's job to decide which bits of those might be
>> interesting to view as 32-bit and what is actually "live"
>> 32 bit state.
>
> It appears that PRSTATUS aware tools don't currently work this way.
>
>>
>> From that point of view there is no valid AArch64 SP register
>> at this point in execution (xregs[31] for QEMU would be stale
>> state, so not a good choice I think).
>
> I suppose zero or all 1's are the safest choices for "undefined", but
> being undefined actually gives us freedom to use aarch64_compat_sp as
> well, which, to me, looks like a safer and more useful value.

It just doesn't really make sense to me to do this one bit of
work for the debug analysis tools when they already have to know
that 32-bit modes are special. We seem to end up with a function
in QEMU whose only purpose is working around a bug in the thing
consuming the coredump.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Roman Kagan]

On Fri, Dec 18, 2015 at 10:10:11AM -0800, Peter Hornyack wrote:
> On Fri, Dec 18, 2015 at 8:01 AM, Paolo Bonzini  wrote:
> > On 18/12/2015 16:19, Pavel Fedin wrote:
> >> As far as i understand this code, KVM_EXIT_HYPERV is called when one
> >> of three MSRs are accessed. But, shouldn't we have implemented
> >> instead something more generic, like KVM_EXIT_REG_IO, which would
> >> work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register
> >> code and value?
> >
> > Yes, we considered that.  There were actually patches for this as well.
> >  However, in this case the register is still emulated in the kernel, and
> > userspace just gets informed of the new value.
> 
> On brief inspection of Andrey's patch (I have not been following
> closely) it looks like the kvm_hyperv_exit struct that's returned to
> userspace contains more data (control, evt_page, and msg_page fields)
> than simply the value of the MSR, so would the desired SynIC exit fit
> into a general-purpose exit for MSR emulation?

Frankly I'm struggling trying to recall why we implemented it this way.
Actually all three fields are the values of respective MSRs and I don't
see any necessity to pass all three at the same time when any of them
gets updated.  The patch for QEMU adds an exit handler which processes
the fields individually, so I have a strong suspicion that union was
meant here rather than struct.

I hope Andrey will help to shed some light on that when he's back in the
office on Monday; meanwhile I think this peculiarity can be ignored.

Roman.

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Eduardo Habkost]

On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/pci-host/piix.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..e3840f0 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>  {0xa8, 4},  /* SNB: base of GTT stolen memory */
>  };
>  
> -static int host_pci_config_read(int pos, int len, uint32_t val)
> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)

You don't need the return value anymore, if you report errors
through the errp parameter. The function can be void, now.

>  {
>  char path[PATH_MAX];
>  int config_fd;
> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
> uint32_t val)
>  int ret = 0;
>  
>  if (rc >= size || rc < 0) {
> +error_setg(errp, "No such device");
>  return -ENODEV;
>  }
>  
>  config_fd = open(path, O_RDWR);
>  if (config_fd < 0) {
> +error_setg(errp, "No such device");
>  return -ENODEV;
>  }
>  
>  if (lseek(config_fd, pos, SEEK_SET) != pos) {
> +error_setg(errp, "lseek err: %s", strerror(errno));

What about error_setg_errno()?

>  ret = -errno;
>  goto out;
>  }
> @@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, 
> uint32_t val)
>  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  if (rc != len) {
>  ret = -errno;
> +error_setg(errp, "read err: %s", strerror(errno));

Same here.

>  }
>  out:
>  close(config_fd);
>  return ret;
>  }
>  
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
>  {
>  uint32_t val = 0;
>  int rc, i, num;
> @@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice 
> *pci_dev)
>  for (i = 0; i < num; i++) {
>  pos = igd_host_bridge_infos[i].offset;
>  len = igd_host_bridge_infos[i].len;
> -rc = host_pci_config_read(pos, len, val);
> +rc = host_pci_config_read(pos, len, val, errp);
>  if (rc) {

The usual pattern for error checking and propagation is:

Error *err;
host_pci_config_read(pos, len, val, &err);
if (err) {
error_propagate(errp, local_err);
return;
}

> -return -ENODEV;
> +return;
>  }
>  pci_default_write_config(pci_dev, pos, val, len);
>  }
> -
> -return 0;
>  }
>  
>  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->init = igd_pt_i440fx_initfn;
> +k->realize = igd_pt_i440fx_realize;
>  dc->desc = "IGD Passthrough Host bridge";
>  }
>  
> -- 
> 2.1.0
> 
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

['Roman Kagan']

On Fri, Dec 18, 2015 at 05:01:59PM +0100, Paolo Bonzini wrote:
> On 18/12/2015 16:19, Pavel Fedin wrote:
> > As far as i understand this code, KVM_EXIT_HYPERV is called when one
> > of three MSRs are accessed. But, shouldn't we have implemented 
> > instead something more generic, like KVM_EXIT_REG_IO, which would
> > work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register 
> > code and value?
> 
> Yes, we considered that.  There were actually patches for this as well.
>  However, in this case the register is still emulated in the kernel, and
> userspace just gets informed of the new value.
> 
> > This would allow us to solve the same task which we have done here,
> > but this solution would be reusable for other devices and other 
> > archirectures. What if in future we have more system registers to
> > emulate in userspace?
> 
> If we do get that, we will just rename KVM_EXIT_HYPERV to
> KVM_EXIT_MSR_ACCESS, and KVM_EXIT_HYPERV_SYNIC to
> KVM_EXIT_MSR_HYPERV_SYNIC, and struct kvm_hyperv_exit to kvm_msr_exit.

A generic implemenation will probably just convey (msr#, value) pair,
and KVM_EXIT_MSR_HYPERV_SYNIC wouldn't be needed at all.

I don't immediately see why it wouldn't work for us; we'd have reused
the infrastructure if it existed when we started our work.  I didn't see
Peter's patches yet; maybe we can come up with an interim solution to
fit in the merge window but expose a sufficiently generic API.

Roman.

Re: [Qemu-devel] [PATCH] hw/q35: fix pc-q35-1.6 machine options

[Eduardo Habkost]

On Thu, Dec 17, 2015 at 07:34:07PM +0200, Marcel Apfelbaum wrote:
> Following the convention each machine_options function should call
> the next machine options first.
> 
> Signed-off-by: Marcel Apfelbaum 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Paolo Bonzini]

On 18/12/2015 19:10, Peter Hornyack wrote:
> On brief inspection of Andrey's patch (I have not been following
> closely) it looks like the kvm_hyperv_exit struct that's returned to
> userspace contains more data (control, evt_page, and msg_page fields)
> than simply the value of the MSR, so would the desired SynIC exit fit
> into a general-purpose exit for MSR emulation?

This would be a special case that is used even if the hyperv MSRs are
emulated in the kernel.  Other exit subcodes than
KVM_EXIT_(MSR_)HYPERV_SYNIC would be used for your usecase of for MSR
emulation in userspace.

Paolo

Re: [Qemu-devel] [PATCH 10/10] iotests: 095: Use TEST_IMG override instead of "mv"

[Max Reitz]

On 16.12.2015 10:54, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  tests/qemu-iotests/095 | 12 ++--
>  tests/qemu-iotests/095.out |  8 
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
> index 6630181..cbe61bc 100755
> --- a/tests/qemu-iotests/095
> +++ b/tests/qemu-iotests/095
> @@ -50,17 +50,16 @@ _supported_os Linux
>  size_smaller=5M
>  size_larger=100M
>  
> -_make_test_img $size_smaller
> -mv "${TEST_IMG}" "${TEST_IMG}.base"
> +TEST_IMG="$TEST_IMG.base _make_test_img $size_smaller"

I think this should be:

TEST_IMG="$TEST_IMG.base" _make_test_img $size_smaller

>  
> -_make_test_img -b "${TEST_IMG}.base" $size_larger
> -mv "${TEST_IMG}" "${TEST_IMG}.snp1"
> +TEST_IMG="$TEST_IMG.snp1 _make_test_img -b "${TEST_IMG}.base" $size_larger"

TEST_IMG="$TEST_IMG.snp1" _make_test_img -b "${TEST_IMG}.base" \
$size_larger

(Also, you could strip the {} because the inconsistent usage looks a bit
strange inside of a single line.)

>  
>  _make_test_img -b "${TEST_IMG}.snp1" $size_larger
>  
>  echo
>  echo "=== Base image info before commit and resize ==="
> -TEST_IMG="${TEST_IMG}.base" _img_info
> +TEST_IMG="${TEST_IMG}.base" _img_info | \
> +sed -e 's/^cluster_size: .*$/cluster_size: XXX/'

How about using _filter_img_info instead?

Max

>  echo
>  echo === Running QEMU Live Commit Test ===
> @@ -78,7 +77,8 @@ _send_qemu_cmd $h "{ 'execute': 'block-commit',
>  
>  echo
>  echo "=== Base image info after commit and resize ==="
> -TEST_IMG="${TEST_IMG}.base" _img_info
> +TEST_IMG="${TEST_IMG}.base" _img_info | \
> +sed -e 's/^cluster_size: .*$/cluster_size: XXX/'
>  
>  # success, all done
>  echo "*** done"
> diff --git a/tests/qemu-iotests/095.out b/tests/qemu-iotests/095.out
> index 2360061..b95111e 100644
> --- a/tests/qemu-iotests/095.out
> +++ b/tests/qemu-iotests/095.out
> @@ -1,13 +1,13 @@
>  QA output created by 095
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=5242880
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 
> backing_file=TEST_DIR/t.IMGFMT.base
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=5242880
> +Formatting 'TEST_DIR/t.IMGFMT.snp1', fmt=IMGFMT size=104857600 
> backing_file=TEST_DIR/t.IMGFMT.base
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 
> backing_file=TEST_DIR/t.IMGFMT.snp1
>  
>  === Base image info before commit and resize ===
>  image: TEST_DIR/t.IMGFMT.base
>  file format: IMGFMT
>  virtual size: 5.0M (5242880 bytes)
> -cluster_size: 65536
> +cluster_size: XXX
>  
>  === Running QEMU Live Commit Test ===
>  
> @@ -19,5 +19,5 @@ cluster_size: 65536
>  image: TEST_DIR/t.IMGFMT.base
>  file format: IMGFMT
>  virtual size: 100M (104857600 bytes)
> -cluster_size: 65536
> +cluster_size: XXX
>  *** done
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 00/10] iotests: Clean up "mv $TEST_IMG $TEST_IMG.XXX"

[Max Reitz]

On 16.12.2015 10:54, Fam Zheng wrote:
> Commit 794d00f71d fixed two "mv" commands into the TEST_IMG override approach.
> There are still more occasions of "mv", this series fixes them.
> 
> The benefit is it drops the assumption that the created image is a single 
> file,
> which is not true for VMDK or the coming QBM format.  On top of this, those
> formats can be added to these cases later.
> 
> The patches stems from QBM work, but I will also take a look at adding
> applicable VMDK subformats once merged.
> 
> Please review.

Patches 1 to 9:

Reviewed-by: Max Reitz 

> 
> Fam
> 
> 
> Fam Zheng (10):
>   iotests: 018: Use TEST_IMG override instead of "mv"
>   iotests: 019: Use TEST_IMG override instead of "mv"
>   iotests: 020: Use TEST_IMG override instead of "mv"
>   iotests: 024: Use TEST_IMG override instead of "mv"
>   iotests: 028: Use TEST_IMG override instead of "mv"
>   iotests: 034: Use TEST_IMG override instead of "mv"
>   iotests: 037: Use TEST_IMG override instead of "mv"
>   iotests: 038: Use TEST_IMG override instead of "mv"
>   iotests: 050: Use TEST_IMG override instead of "mv"
>   iotests: 095: Use TEST_IMG override instead of "mv"
> 
>  tests/qemu-iotests/018 |  8 
>  tests/qemu-iotests/018.out |  2 +-
>  tests/qemu-iotests/019 | 13 +++--
>  tests/qemu-iotests/019.out |  4 ++--
>  tests/qemu-iotests/020 |  7 +--
>  tests/qemu-iotests/020.out |  2 +-
>  tests/qemu-iotests/024 | 10 --
>  tests/qemu-iotests/024.out |  4 ++--
>  tests/qemu-iotests/028 |  5 -
>  tests/qemu-iotests/028.out |  2 +-
>  tests/qemu-iotests/034 |  6 +-
>  tests/qemu-iotests/034.out |  2 +-
>  tests/qemu-iotests/037 |  5 -
>  tests/qemu-iotests/037.out |  2 +-
>  tests/qemu-iotests/038 |  5 -
>  tests/qemu-iotests/038.out |  2 +-
>  tests/qemu-iotests/050 |  9 +++--
>  tests/qemu-iotests/050.out |  4 ++--
>  tests/qemu-iotests/095 | 12 ++--
>  tests/qemu-iotests/095.out |  8 
>  20 files changed, 70 insertions(+), 42 deletions(-)
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Peter Hornyack]

On Fri, Dec 18, 2015 at 8:01 AM, Paolo Bonzini  wrote:
>
>
> On 18/12/2015 16:19, Pavel Fedin wrote:
>> As far as i understand this code, KVM_EXIT_HYPERV is called when one
>> of three MSRs are accessed. But, shouldn't we have implemented
>> instead something more generic, like KVM_EXIT_REG_IO, which would
>> work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register
>> code and value?
>
> Yes, we considered that.  There were actually patches for this as well.
>  However, in this case the register is still emulated in the kernel, and
> userspace just gets informed of the new value.

On brief inspection of Andrey's patch (I have not been following
closely) it looks like the kvm_hyperv_exit struct that's returned to
userspace contains more data (control, evt_page, and msg_page fields)
than simply the value of the MSR, so would the desired SynIC exit fit
into a general-purpose exit for MSR emulation?

>> This would allow us to solve the same task which we have done here,
>> but this solution would be reusable for other devices and other
>> archirectures. What if in future we have more system registers to
>> emulate in userspace?
>
> If we do get that, we will just rename KVM_EXIT_HYPERV to
> KVM_EXIT_MSR_ACCESS, and KVM_EXIT_HYPERV_SYNIC to
> KVM_EXIT_MSR_HYPERV_SYNIC, and struct kvm_hyperv_exit to kvm_msr_exit.
>
> Actually, we can do it now.  What do you guys think?  Peter?  I might
> even be convinced to document the capability (in Documentation/ and
> uapi/) even if the upstream kernel doesn't provide it.  We still have a
> lot of time until 4.5 is out, it can be done after the merge window even.

I will update and re-send that patchset for discussion.

>
> Paolo
>
>> I write this because at one point i suggested similar thing for ARM64
>> (but i never actually wrote it), to emulate physical CP15 timer. And
>> it would require exactly the same capability - process some trapped
>> system register accesses in userspace.

Peter

Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Eduardo Habkost]

On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote:
> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> >We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
> >guest_info.
> 
> Hi Eduardo,
> 
> I like the idea of using qdev_get_machine() to get the machine instead of
> keeping the reference around, however only in places we cannot get a reference
> to it otherwise.

I was unsure if I should go to the extreme of removing all the
guest_info parameters, when doing that. I agree with your
suggestions, but v3 will have to wait until I get back from
vacations in Jan 18th. At least we will have less conflicts with
Igor's AML series. :)

Thanks!

-- 
Eduardo

Re: [Qemu-devel] Jobs 2.0 QAPI [RFC]

[Eric Blake]

On 12/16/2015 05:50 PM, John Snow wrote:
> In working through a prototype to enable multiple block jobs. A few
> problem spots in our API compatibility become apparent.
> 

>  qmp_query_block_jobs 
> 

> Let's consider using a new command. I'm not sure if this is strictly
> possible with current QAPI, but Eric will yell at me if it isn't --
> forgive the wiggly pseudo-specification.
> 
> query-jobs
> 
> Will return a list of all jobs.
> 
> query-jobs sys="block"
> 
> Will return a list of all block jobs. This will be the only valid
> subsystem to start with -- we don't have non-block jobs yet and it may
> be some time before we do.
> 
> Each subsystem will have a sys= enumeration, and the jobs for that
> particular subsystem can subclass the default return type. The QAPI
> commands can be defined to accept a union of the subclasses so we don't
> have to specify in advance which type each command accepts, but we can
> allow the responsible subsystem to interpret it on demand dynamically.
> 
> The base type can be something along the lines of:
> 
> { 'struct': 'JobInfo',
>   'data': {
> 'type': JobTypeEnum,
> 'sys': JobSysEnum,
> 'id': str,
> 'busy': bool,
> 'paused': bool,
> 'ready': bool
>   }
> }
> 
> And the BlockJobInfo can inherit/extend, looking like this:
> 
> { 'struct': 'BlockJobInfo',
>   'base': JobInfo
>   'data': {
> 'len': int,
> 'offset': int,
> 'speed': 'int',
> 'io-status': BlockDeviceIoStatus,
> 'devices': ,
>   }
> }

Done in QAPI via flat unions.  All the common fields, including the
discriminator 'sys', are present in the base type, and then each
subclass of a job adds additional parameters to the QMP type by
specifying their subtype as a branch of the union.  So should be doable.

> 
> 'devices' will now report a more nuanced view of this Job's associated
> nodes in our graph, as in the following:
> 
> { "devices": [
>   { "name": "drive0",
> "nodes": [
>   { "id": "#node123",
> "permissions": "+RW-X"
>   },
>   { "id": "#node456",
> "permissions": "+R"
>   }
> ]
>   }
> }
> 
> This lets us show exactly what nodes we are touching, what BlockBackends
> they belong to, and what permissions are involved. Unambiguously. You
> can use your own imagination for a permissions string that will make
> sense -- This depends on Jeff Cody's work primarily.

We'd probably want it to look more JSON-y, maybe something like:

{ "id": "#node456",
  "permissions": [ { "name":"read", "mode":"require" },
   { "name":"write", "mode":"allow" },
   { "name":"meta", "mode":"unspecified" } ] }

but I agree that how it is represented does not affect the proposal here
of merely representing a list of all associated BDS affected by this
job, broken out by permissions per BDS (as some jobs will affect
multiple BDS, and not all of them with the same permissions).

> 
> The new command gives a chance to make a clean break and any users of
> this new command won't be confused about the information they receive.
> 
> The legacy qmp-query-block-jobs command can continue to operate
> providing a best-effort approximation of the data it finds. The legacy
> command will *abort* and return error if it detects any job that was
> launched by a new-style command, e.g. if it detects there is more than
> one job attached to any node under a single BlockBackend -- clearly the
> user has been using new features -- and should abort announcing this
> command is deprecated.

Makes sense. If an old client only uses the old commands, things will
still work; while a new client, once converted to use the new commands,
should do the conversion completely and not rely on the old view.

> 
> If the graph looks reasonably plain, it can report back the information
> it always has, except that it will now also report the "id" field in
> addition to be perfectly unambiguous about how to issue commands to this
> job, like I outlined in the first paragraph of this section.

Old clients won't care about the new information, but it doesn't hurt to
supply it, especially if it lets us share code with the new query command.

> 
> 
>  block-job-cancel 
> 
> This command can remain as a legacy command. Provided the "device"
> provided has precisely one job attached, we can allow this command to
> cancel it. If more complicated configurations are found, abort and
> encourage the user to use a new API.

Again, won't affect old client usage patterns, and new clients should
make the lock-step conversion to using only the new interface.  Sounds
reasonable.

> 
> We can refuse to augment block-job-cancel; forcing users who want to
> specify IDs to cancel to use the new API.
> 
> We can add a new "job-cancel" command which removes the block specificity.

Adding a new job-cancel sounds right.

> 
> We can introduce sub-classed arguments as needed for e.g. BlockJob
> cancellations:
> 
> { 'command': 'job-cancel',
>   'data': { 'id':

Re: [Qemu-devel] [PATCH v3 6/9] target-arm: support QMP dump-guest-memory

[Andrew Jones]

On Fri, Dec 18, 2015 at 04:31:13PM +, Peter Maydell wrote:
> On 18 December 2015 at 16:05, Andrew Jones  wrote:
> > On Fri, Dec 18, 2015 at 11:59:39AM +, Peter Maydell wrote:
> >> On 15 December 2015 at 22:51, Andrew Jones  wrote:
> >> > Add the support needed for creating prstatus elf notes. This
> >> > allows us to use QMP dump-guest-memory.
> >> >
> >> > Signed-off-by: Andrew Jones 
> >>
> >> > +int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> >> > + int cpuid, void *opaque)
> >> > +{
> >> > +struct aarch64_note note;
> >> > +CPUARMState *env = &ARM_CPU(cs)->env;
> >> > +DumpState *s = opaque;
> >> > +uint64_t pstate, sp;
> >> > +int ret, i;
> >> > +
> >> > +aarch64_note_init(¬e, s, "CORE", 5, NT_PRSTATUS, 
> >> > sizeof(note.prstatus));
> >> > +
> >> > +note.prstatus.pr_pid = cpu_to_dump32(s, cpuid);
> >> > +
> >> > +if (!is_a64(env)) {
> >> > +aarch64_sync_32_to_64(env);
> >> > +pstate = cpsr_read(env);
> >> > +sp = aarch64_compat_sp(env);
> >>
> >> I don't understand why we need to do this. If this is an
> >> AArch64 dump then we should just treat it as an AArch64
> >> dump, and presumably the consumer of the dump knows enough
> >> to know what the "hypervisor view" of a CPU that's currently
> >> in 32-bit mode is. It has to anyway to be able to figure
> >> out where all the other registers are, so why can't it
> >> also figure out what mode the CPU is currently in and thus
> >> where r13 is in the xregs array?
> >
> > You're probably right that this shouldn't be necessary. But, in order for
> > it not to be necessary, I'll need to write another crash patch. Currently,
> > if you do a dump-guest-memory on a running guest, i.e. one where the kernel
> > has not called panic(), and thus the cpus are actually in 32-bit usermode,
> > rather than in the 64-bit cpu-stop IPI handler, then the crash utility
> > segfaults if sp == xregs[31]. crash does properly decode the registers
> > it digs out of the stack frame on a panic'ed cpu though, and setting sp
> > to aarch64_compat_sp here also allows crash to work properly in the non-
> > panic'd case.
> 
> If crash segfaults then that's clearly a bug in crash...
> What is it expecting to see in the SP field?

A valid stack pointer

> 
> > So, I could teach crash to do what I'm doing here in qemu instead, but
> > there's still one more reason why it may make sense to do it here. That
> > reason is that I don't know what else to put in prstatus.pr_reg.sp. Does
> > xregs[31] make the most sense? or just zero? prstatus.pr_reg.pc is the
> > correct 32-bit userspace pc, prstatus.pr_reg.pstate is the correct cpsr,
> > so why not set sp to the correct userspace sp?
> 
> Well, what spec are we following here? The most logical view

With the shoehorning approach we're following the aarch64 ptrace spec,
but putting arm32 registers in it. We then rely on the analysis tools
to do the right thing when they see pstate has PSR_MODE32_BIT set, which
all cpsr modes do.

The ptrace code in the kernel would return a real aarch32 view, i.e.
only registers up to r15 and a cpsr. We can't do that here because we've
committed to an EM_AARCH64 formatted core, and we only have one type of
PRSTATUS note for that core type. Furthermore, we want to be able to
return all the registers to handle dumps of 64-bit EL2 with 32-bit EL1s.

> to me seems to be to say "you get the view of the system
> that you get from a JTAG debugger or a hypervisor", which
> is to say you see a 64-bit set of registers and it's the
> debugger's job to decide which bits of those might be
> interesting to view as 32-bit and what is actually "live"
> 32 bit state.

It appears that PRSTATUS aware tools don't currently work this way.

> 
> From that point of view there is no valid AArch64 SP register
> at this point in execution (xregs[31] for QEMU would be stale
> state, so not a good choice I think).

I suppose zero or all 1's are the safest choices for "undefined", but
being undefined actually gives us freedom to use aarch64_compat_sp as
well, which, to me, looks like a safer and more useful value.

Thanks,
drew

Re: [Qemu-devel] [PATCH] gtk: use setlocale() for LC_MESSAGES only

[Alberto Garcia]

>> > We do however have translations for a few simple strings for the GTK+
>> > menu items, so in order to run QEMU using the C locale, and yet have a
>> > translated UI let's use setlocale() for LC_MESSAGES only.
>> > 
>> Not sure why I noticed it only now and if it's related to any recent
>> package upgrade on my side (using RHEL 7), but I noticed that
>> non-ASCII characters in the GTK UI strings are broken for me and git
>> bisect pointed to this commit.
>
> I guess we need to set LC_CTYPE too.

That affects functions in ctype.h (isalpha(), islower(), isupper(), ...)
I guess that's safe?

> @@ -2044,8 +2044,9 @@ void gtk_display_init(DisplayState *ds, bool 
> full_screen, bool grab_on_hover)
>  
>  s->free_scale = FALSE;
>  
> -/* LC_MESSAGES only. See early_gtk_display_init() for details */
> +/* LC_MESSAGES+LC_CTYPE only. See early_gtk_display_init() for details */
>  setlocale(LC_MESSAGES, "");
> +setlocale(LC_CTYPE, "");
>  bindtextdomain("qemu", CONFIG_QEMU_LOCALEDIR);
>  textdomain("qemu");

You can also modify the comment in early_gtk_display_init() to say that
" we support importing LC_MESSAGES and LC_CTYPE from the environment ".

Berto

Re: [Qemu-devel] guest kernel in the host RAM

[Paolo Bonzini]

On 18/12/2015 17:28, Denis V. Lunev wrote:
>> Linux patches itself a lot, which
>> makes the memory savings minimal (if they exist at all).  What is the
>> decompression time for a kernel that is compressed with LZO?
>
> It seems that you are perfectly correct here!
> 
> With large pages enabled (and preferred 2 Mb pages mode)
> we will have exactly zero bonus.
> 
> Thank you for saving a lot of time and efforts :)))

I'm a lazy person, sometimes it helps. :)

Paolo

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Paolo Bonzini]

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/pci-bridge/pci_expander_bridge.c | 24 ++--
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c 
> b/hw/pci-bridge/pci_expander_bridge.c
> index 57f8a37..cc975f6 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
>   * Returns 0 on successs, -1 if i440fx host was not
>   * found or the bus number is already in use.
>   */
> -static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
> +static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error **errp)
>  {
>  PCIBus *bus = dev->bus;
>  int pxb_bus_num = pci_bus_num(pxb_bus);
>  
>  if (bus->parent_dev) {
> -error_report("PXB devices can be attached only to root bus.");
> +error_setg(errp, "PXB devices can be attached only to root bus.");
>  return -1;
>  }
>  
>  QLIST_FOREACH(bus, &bus->child, sibling) {
>  if (pci_bus_num(bus) == pxb_bus_num) {
> -error_report("Bus %d is already in use.", pxb_bus_num);
> +error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
>  return -1;
>  }
>  }
> @@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
> 0;
>  }
>  
> -static int pxb_dev_initfn(PCIDevice *dev)
> +static void pxb_dev_realize(PCIDevice *dev, Error **errp)
>  {
>  PXBDev *pxb = PXB_DEV(dev);
>  DeviceState *ds, *bds;
> @@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>  
>  if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>  pxb->numa_node >= nb_numa_nodes) {
> -error_report("Illegal numa node %d.", pxb->numa_node);
> -return -EINVAL;
> +error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
> +return;
>  }
>  
>  if (dev->qdev.id && *dev->qdev.id) {
> @@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>  
>  PCI_HOST_BRIDGE(ds)->bus = bus;
>  
> -if (pxb_register_bus(dev, bus)) {
> -return -EINVAL;
> +if (pxb_register_bus(dev, bus, errp)) {
> +goto err_register_bus;
>  }
>  
>  qdev_init_nofail(ds);
> @@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
>  pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>  
>  pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> -return 0;
> +
> +err_register_bus:
> +object_unref(OBJECT(ds));
> +object_unref(OBJECT(bds));
> +object_unref(OBJECT(bus));

I think these should be object_unparent, not unref.

Paolo

>  }
>  
>  static void pxb_dev_exitfn(PCIDevice *pci_dev)
> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void 
> *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->init = pxb_dev_initfn;
> +k->realize = pxb_dev_realize;
>  k->exit = pxb_dev_exitfn;
>  k->vendor_id = PCI_VENDOR_ID_REDHAT;
>  k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> 

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

['Roman Kagan']

On Fri, Dec 18, 2015 at 06:19:25PM +0300, Pavel Fedin wrote:
>  I dislike implementing architecture-dependent exit code where we could 
> implement an architecture-independent one.
> 
>  As far as i understand this code, KVM_EXIT_HYPERV is called when one of 
> three MSRs are accessed. But, shouldn't we have implemented
> instead something more generic, like KVM_EXIT_REG_IO, which would work 
> similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register
> code and value?
> 
>  This would allow us to solve the same task which we have done here, but this 
> solution would be reusable for other devices and other
> archirectures. What if in future we have more system registers to emulate in 
> userspace?
> 
>  I write this because at one point i suggested similar thing for ARM64 (but i 
> never actually wrote it), to emulate physical CP15
> timer. And it would require exactly the same capability - process some 
> trapped system register accesses in userspace.

Note that, as Paolo pointed out, in case of HyperV the value of the
register is of interest not only to the userspace but to the KVM, too.
Otherwise I don't see off hand why a generic infrastructure wouldn't fit
in our usecase.

Roman.

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Paolo Bonzini]

CCing Stefano, who is the maintainer.

Paolo

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/i386/xen/xen_pvdevice.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
> index c218947..a6c93d0 100644
> --- a/hw/i386/xen/xen_pvdevice.c
> +++ b/hw/i386/xen/xen_pvdevice.c
> @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static int xen_pv_init(PCIDevice *pci_dev)
> +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
>  {
>  XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
>  uint8_t *pci_conf;
>  
>  /* device-id property must always be supplied */
> -if (d->device_id == 0x)
> - return -1;
> +if (d->device_id == 0x) {
> +error_setg(errp, "Device ID invalid, it must always be supplied")
> + return;
> +}
>  
>  pci_conf = pci_dev->config;
>  
> @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
>  
>  pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>   &d->mmio);
> -
> -return 0;
>  }
>  
>  static Property xen_pv_props[] = {
> @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void 
> *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->init = xen_pv_init;
> +k->realize = xen_pv_realize;
>  k->class_id = PCI_CLASS_SYSTEM_OTHER;
>  dc->desc = "Xen PV Device";
>  dc->props = xen_pv_props;
> 

Re: [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()

[Paolo Bonzini]

Cc: qemu-triv...@nongnu.org


On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/mips/gt64xxx_pci.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index f76a9fd..c1f3c9c 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1193,7 +1193,7 @@ static int gt64120_init(SysBusDevice *dev)
>  return 0;
>  }
>  
> -static int gt64120_pci_init(PCIDevice *d)
> +static void gt64120_pci_realize(PCIDevice *d, Error **errp)
>  {
>  /* FIXME: Malta specific hw assumptions ahead */
>  pci_set_word(d->config + PCI_COMMAND, 0);
> @@ -1207,8 +1207,6 @@ static int gt64120_pci_init(PCIDevice *d)
>  pci_set_long(d->config + PCI_BASE_ADDRESS_4, 0x1400);
>  pci_set_long(d->config + PCI_BASE_ADDRESS_5, 0x1401);
>  pci_set_byte(d->config + 0x3d, 0x01);
> -
> -return 0;
>  }
>  
>  static void gt64120_pci_class_init(ObjectClass *klass, void *data)
> @@ -1216,7 +1214,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, 
> void *data)
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -k->init = gt64120_pci_init;
> +k->realize = gt64120_pci_realize;
>  k->vendor_id = PCI_VENDOR_ID_MARVELL;
>  k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
>  k->revision = 0x10;
> 

Re: [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()

[Paolo Bonzini]

Cc: qemu-triv...@nongnu.org

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/sh4/sh_pci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index a2f6d9e..4509053 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -151,12 +151,11 @@ static int sh_pci_device_init(SysBusDevice *dev)
>  return 0;
>  }
>  
> -static int sh_pci_host_init(PCIDevice *d)
> +static void sh_pci_host_realize(PCIDevice *d, Error **errp)
>  {
>  pci_set_word(d->config + PCI_COMMAND, PCI_COMMAND_WAIT);
>  pci_set_word(d->config + PCI_STATUS, PCI_STATUS_CAP_LIST |
>   PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
> -return 0;
>  }
>  
>  static void sh_pci_host_class_init(ObjectClass *klass, void *data)
> @@ -164,7 +163,7 @@ static void sh_pci_host_class_init(ObjectClass *klass, 
> void *data)
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -k->init = sh_pci_host_init;
> +k->realize = sh_pci_host_realize;
>  k->vendor_id = PCI_VENDOR_ID_HITACHI;
>  k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R;
>  /*
> 

Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices

[Paolo Bonzini]

On 18/12/2015 18:17, Alistair Francis wrote:
> Does anyone have any ideas on how we can do this?
> 
> AFAIK there is no way to currently do this, so we need to add
> something. What is the preferred way to expose the buses?

For now, what you're doing is okay for me, just moving the funky code in
zynq-specific files.

Thanks,

Paolo

> Thanks,
> 
> Alistair

[Qemu-devel] [PULL 1/1] numa: Clean up query-memdev error handling

[Eduardo Habkost]

From: Markus Armbruster 

qmp_query_memdev() has two error paths:

* When object_get_objects_root() returns null.  It never does, so
  simply drop the useless error handling.

* When query_memdev() fails.  It leaks err then.  But any failure
  there is actually a programming error.  Switch it to &error_abort,
  and drop the useless error handling.

Messed up in commit 76b5d85 "qmp: add query-memdev".

Signed-off-by: Markus Armbruster 
Reviewed-by: Eduardo Habkost 
Signed-off-by: Eduardo Habkost 
---
 numa.c | 59 ++-
 1 file changed, 10 insertions(+), 49 deletions(-)

diff --git a/numa.c b/numa.c
index fdfe294..1710946 100644
--- a/numa.c
+++ b/numa.c
@@ -517,7 +517,6 @@ static int query_memdev(Object *obj, void *opaque)
 {
 MemdevList **list = opaque;
 MemdevList *m = NULL;
-Error *err = NULL;
 
 if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
 m = g_malloc0(sizeof(*m));
@@ -525,72 +524,34 @@ static int query_memdev(Object *obj, void *opaque)
 m->value = g_malloc0(sizeof(*m->value));
 
 m->value->size = object_property_get_int(obj, "size",
- &err);
-if (err) {
-goto error;
-}
-
+ &error_abort);
 m->value->merge = object_property_get_bool(obj, "merge",
-   &err);
-if (err) {
-goto error;
-}
-
+   &error_abort);
 m->value->dump = object_property_get_bool(obj, "dump",
-  &err);
-if (err) {
-goto error;
-}
-
+  &error_abort);
 m->value->prealloc = object_property_get_bool(obj,
-  "prealloc", &err);
-if (err) {
-goto error;
-}
-
+  "prealloc",
+  &error_abort);
 m->value->policy = object_property_get_enum(obj,
 "policy",
 "HostMemPolicy",
-&err);
-if (err) {
-goto error;
-}
-
+&error_abort);
 object_property_get_uint16List(obj, "host-nodes",
-   &m->value->host_nodes, &err);
-if (err) {
-goto error;
-}
+   &m->value->host_nodes,
+   &error_abort);
 
 m->next = *list;
 *list = m;
 }
 
 return 0;
-error:
-g_free(m->value);
-g_free(m);
-
-return -1;
 }
 
 MemdevList *qmp_query_memdev(Error **errp)
 {
-Object *obj;
+Object *obj = object_get_objects_root();
 MemdevList *list = NULL;
 
-obj = object_get_objects_root();
-if (obj == NULL) {
-return NULL;
-}
-
-if (object_child_foreach(obj, query_memdev, &list) != 0) {
-goto error;
-}
-
+object_child_foreach(obj, query_memdev, &list);
 return list;
-
-error:
-qapi_free_MemdevList(list);
-return NULL;
 }
-- 
2.1.0

[Qemu-devel] [PULL 0/1] NUMA queue, 2015-12-18

[Eduardo Habkost]

The following changes since commit c688084506cf2cf2eba4ba9df4e91abb6e3dab83:

  Merge remote-tracking branch 
'remotes/berrange/tags/pull-qcrypto-secrets-base-2015-12-18-1' into staging 
(2015-12-18 17:04:15 +)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/numa-pull-request

for you to fetch changes up to 2f6f826e03e09eb3b65b3a764580d66b857e3a23:

  numa: Clean up query-memdev error handling (2015-12-18 15:50:24 -0200)


NUMA queue, 2015-12-18



Markus Armbruster (1):
  numa: Clean up query-memdev error handling

 numa.c | 59 ++-
 1 file changed, 10 insertions(+), 49 deletions(-)

-- 
2.1.0

Re: [Qemu-devel] [RESEND PATCH] q35: Remove old machine versions

[Eduardo Habkost]

On Fri, Dec 18, 2015 at 10:41:45AM +0100, Gerd Hoffmann wrote:
> On Do, 2015-12-17 at 15:27 -0200, Eduardo Habkost wrote:
> > Migration with q35 was not possible before commit
> > 04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35 unconditionally
> > creates an ich9-ahci device, that was marked as unmigratable. So all q35
> > machine classes before pc-q35-2.4 were not migratable, so there's no
> > point in keeping compatibility code for them.
> > 
> > Remove all old pc-q35 machine classes and keep only pc-q35-2.4.
> 
> > -static void pc_compat_1_6(MachineState *machine)
> > -{
> > -pc_compat_1_7(machine);
> > -rom_file_has_mr = false;
> > -has_acpi_build = false;
> 
> After applying this patch has_acpi_build is always true and can be
> dropped, together with some other code elsewhere which depends on
> has_acpi_build.  The same is probably true for other variables as well
> (gigabyte_align?).  Guess it's best to do this as one-per-variable
> incremental patches.

The static variables will be replaced by PCMachineClass fields. See:

  Subject: [Qemu-devel] [PATCH v3 0/6] pc: Initialization and compat function 
cleanup
  Date: Tue,  1 Dec 2015 20:58:02 -0200
  Message-Id: <1449010688-19205-1-git-send-email-ehabk...@redhat.com>

> 
> Reviewed-by: Gerd Hoffmann 

Thanks!

-- 
Eduardo

[Qemu-devel] [PATCH v2] qga: Support enum names in guest-file-seek

[Eric Blake]

Magic constants are a pain to use, especially when we run the
risk that our choice of '1' for QGA_SEEK_CUR might differ from
the host or guest's choice of SEEK_CUR.  Better is to use an
enum value, via a qapi alternate type for back-compatibility.

With this,
 {"command":"guest-file-seek", "arguments":{"handle":1,
  "offset":0, "whence":"cur"}}
becomes a synonym for the older
 {"command":"guest-file-seek", "arguments":{"handle":1,
  "offset":0, "whence":1}}

Signed-off-by: Eric Blake 

---
v2: rebase on top of qapi work that has now landed
v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05730.html
---
 qga/commands-posix.c   | 19 ++-
 qga/commands-win32.c   | 19 ++-
 qga/commands.c | 21 +
 qga/guest-agent-core.h |  9 ++---
 qga/qapi-schema.json   | 33 +++--
 tests/test-qga.c   |  9 -
 6 files changed, 70 insertions(+), 40 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c2ff970..a8c36aa 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -553,31 +553,24 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, 
const char *buf_b64,
 }

 struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
-  int64_t whence_code, Error **errp)
+  GuestFileWhence *whence_code,
+  Error **errp)
 {
 GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
 GuestFileSeek *seek_data = NULL;
 FILE *fh;
 int ret;
 int whence;
+Error *err = NULL;

 if (!gfh) {
 return NULL;
 }

 /* We stupidly exposed 'whence':'int' in our qapi */
-switch (whence_code) {
-case QGA_SEEK_SET:
-whence = SEEK_SET;
-break;
-case QGA_SEEK_CUR:
-whence = SEEK_CUR;
-break;
-case QGA_SEEK_END:
-whence = SEEK_END;
-break;
-default:
-error_setg(errp, "invalid whence code %"PRId64, whence_code);
+whence = ga_parse_whence(whence_code, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0654fe4..c6fed1e 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -382,7 +382,8 @@ done:
 }

 GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
-   int64_t whence_code, Error **errp)
+   GuestFileWhence *whence_code,
+   Error **errp)
 {
 GuestFileHandle *gfh;
 GuestFileSeek *seek_data;
@@ -391,6 +392,7 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t 
offset,
 off_pos.QuadPart = offset;
 BOOL res;
 int whence;
+Error *err = NULL;

 gfh = guest_file_handle_find(handle, errp);
 if (!gfh) {
@@ -398,18 +400,9 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t 
offset,
 }

 /* We stupidly exposed 'whence':'int' in our qapi */
-switch (whence_code) {
-case QGA_SEEK_SET:
-whence = SEEK_SET;
-break;
-case QGA_SEEK_CUR:
-whence = SEEK_CUR;
-break;
-case QGA_SEEK_END:
-whence = SEEK_END;
-break;
-default:
-error_setg(errp, "invalid whence code %"PRId64, whence_code);
+whence = ga_parse_whence(whence_code, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }

diff --git a/qga/commands.c b/qga/commands.c
index bb73e7d..b936d93 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -461,3 +461,24 @@ done:

 return ge;
 }
+
+/* Convert GuestFileWhence (either a raw integer or an enum value) into
+ * the guest's SEEK_ constants.  */
+int ga_parse_whence(GuestFileWhence *whence, Error **errp)
+{
+/* Exploit the fact that we picked values to match QGA_SEEK_*. */
+if (whence->type == QTYPE_QSTRING) {
+whence->type = QTYPE_QINT;
+whence->u.value = whence->u.name;
+}
+switch (whence->u.value) {
+case QGA_SEEK_SET:
+return SEEK_SET;
+case QGA_SEEK_CUR:
+return SEEK_CUR;
+case QGA_SEEK_END:
+return SEEK_END;
+}
+error_setg(errp, "invalid whence code %"PRId64, whence->u.value);
+return -1;
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 238dc6b..0a49516 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -12,16 +12,10 @@
  */
 #include "qapi/qmp/dispatch.h"
 #include "qemu-common.h"
+#include "qga-qmp-commands.h"

 #define QGA_READ_COUNT_DEFAULT 4096

-/* Mapping of whence codes used by guest-file-seek. */
-enum {
-QGA_SEEK_SET = 0,
-QGA_SEEK_CUR = 1,
-QGA_SEEK_END = 2,
-};
-
 typedef struct GAState GAState;
 typedef struct GACommandState GACommandState;
 extern GAState *ga_state;
@@ -44,6 +38,7 @@ void ga_set_frozen(GAState *s);
 void ga_unset_fr

Re: [Qemu-devel] [PULL v1 0/5] Support for securely passing secrets to QEMU

[Peter Maydell]

On 18 December 2015 at 16:53, Daniel P. Berrange  wrote:
> The following changes since commit 18f49881cf8359e89396aac12f5d3cf3f8a632ba:
>
>   configure: Fix shell syntax to placate OpenBSD's pdksh (2015-12-18 13:32:49 
> +)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu tags/pull-qcrypto-secrets-base-2015-12-18-1
>
> for you to fetch changes up to 1d7b5b4afdcd76e24ec3678d5418b29d4ff06ad9:
>
>   crypto: add support for loading encrypted x509 keys (2015-12-18 16:25:08 
> +)
>
> 
> Merge QCryptoSecret object support
>
> 
> Daniel P. Berrange (5):
>   util: add base64 decoding function
>   qemu-char: convert to use error checked base64 decode
>   qga: convert to use error checked base64 decode
>   crypto: add QCryptoSecret object class for password/key handling
>   crypto: add support for loading encrypted x509 keys

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option

[John Snow]

On 12/18/2015 10:57 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> Add the fallback drive type as an option so we can control
>> the behavior as a function of the QEMU machine version.
> 
> What's a "fallback drive type", and what does (or will) it do?
> 

I assume you mean "Make your commit messages better."

The fallback type accompanies the "auto" drive type as the fallback
drive type that gets selected if there is an issue auto-guessing from
the diskette.

It comes into play in two places:

(1) There's simply no diskette, or
(2) We couldn't figure out what kind of diskette it was.


The legacy behavior is implicitly type=auto,fallback=144. It is now
explicitly so, and the new behavior (at patch 11) is type=auto,fallback=288.

>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/block/fdc.c | 23 +++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index ad0e052..b587de8 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -155,6 +155,9 @@ typedef struct FDrive {
>>  bool media_inserted;  /* Is there a medium in the tray */
>>  } FDrive;
>>  
>> +
>> +static FloppyDriveType get_fallback_drive_type(FDrive *drv);
>> +
>>  static void fd_init(FDrive *drv)
>>  {
>>  /* Drive */
>> @@ -570,8 +573,15 @@ struct FDCtrl {
>>  uint8_t timer0;
>>  uint8_t timer1;
>>  
>> +FloppyDriveType fallback;
>>  };
>>  
>> +__attribute__((__unused__))
>> +static FloppyDriveType get_fallback_drive_type(FDrive *drv)
>> +{
>> +return drv->fdctrl->fallback;
>> +}
>> +
>>  #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>>  #define SYSBUS_FDC(obj) OBJECT_CHECK(FDCtrlSysBus, (obj), TYPE_SYSBUS_FDC)
>>  
>> @@ -2302,6 +2312,10 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, 
>> Error **errp)
>>  int i, j;
>>  static int command_tables_inited = 0;
>>  
>> +if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
>> +error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
>> +}
>> +
>>  /* Fill 'command_to_handler' lookup table */
>>  if (!command_tables_inited) {
>>  command_tables_inited = 1;
>> @@ -2433,6 +2447,9 @@ static Property isa_fdc_properties[] = {
>>  DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
>>  FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>>  FloppyDriveType),
>> +DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> +FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> +FloppyDriveType),
>>  DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> @@ -2487,6 +2504,9 @@ static Property sysbus_fdc_properties[] = {
>>  DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
>>  FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>>  FloppyDriveType),
>> +DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> +FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> +FloppyDriveType),
>>  DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> @@ -2510,6 +2530,9 @@ static Property sun4m_fdc_properties[] = {
>>  DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
>>  FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>>  FloppyDriveType),
>> +DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> +FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> +FloppyDriveType),
>>  DEFINE_PROP_END_OF_LIST(),
>>  };

Re: [Qemu-devel] [PATCH 2/2] qdev: safely fail device_add if unable to allocate device

[Eric Blake]

On 12/18/2015 09:48 AM, Daniel P. Berrange wrote:
> On Fri, Dec 18, 2015 at 04:30:47PM +0100, Igor Mammedov wrote:
>> qdev_device_add() currently uses object_new() which
>> will abort if there memory allocation for device instance
>> fails. While it's fine it startup, it is not desirable
>> diring hotplug.
>>
>> Try to allocate memory for object first and fail safely
>> if allocation fails.
>>

> This just avoids one small malloc failure.
> 
>> +object_initialize(dev, obj_size, driver);
> 
> This is going to call g_new many more times, so you'll
> still hit OOM almost immediately. eg the call to
> g_hash_table_new_full() in object_initialize_with_type
> will abort on OOM, not to mention anything run in a
> instance constructor function registered against the
> class. There's no way to avoid this given that we have
> chosen to use GLib in QEMU, so I don't really see any
> point in replacing the 'object_new' call with g_try_malloc

We just had a recent thread on it, and I tend to agree that this series
isn't helpful.

https://lists.gnu.org/archive/html/qemu-devel/2015-12/threads.html#01238

If the allocation is BIG (as in one megabyte or more), then doing
tentative allocation and reporting failure is useful (we still have
enough small memory left to meaningfully report the error).  But here,
objects are unlikely to require a megabyte each (and if you really do
have an obj_size that large, given that it is a sizeof(type), we may
have other problems on our hands - what does the .h look like that
defines a type that big, and how slow does it compile?).  We can assume
that small allocations will always succeed, because if they fail, even
the mere act of trying to gracefully recover and report that failure is
likely to malloc more memory, which will recursively fail and hose us
worse than exiting right away.

-- 
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 COLO-Frame v12 23/38] COLO: Implement failover work for Primary VM

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
> For PVM, if there is failover request from users.
> The colo thread will exit the loop while the failover BH does the
> cleanup work and resumes VM.
> 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Li Zhijian 
> ---
> v12:
> - Fix error report and remove unnecessary check in primary_vm_do_failover()
>  (Dave's suggestion)
> v11:
> - Don't call migration_end() in primary_vm_do_failover(),
>  The cleanup work will be done in migration_thread().
> - Remove vm_start() in primary_vm_do_failover() which also been done
>   in migraiton_thread()
> v10:
> - Call migration_end() in primary_vm_do_failover()
> 
> Signed-off-by: zhanghailiang 
> ---
>  include/migration/colo.h |  3 +++
>  include/migration/failover.h |  1 +
>  migration/colo-failover.c|  7 +-
>  migration/colo.c | 54 
> ++--
>  4 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index ba27719..0b02e95 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -32,4 +32,7 @@ void *colo_process_incoming_thread(void *opaque);
>  bool migration_incoming_in_colo_state(void);
>  
>  COLOMode get_colo_mode(void);
> +
> +/* failover */
> +void colo_do_failover(MigrationState *s);
>  #endif
> diff --git a/include/migration/failover.h b/include/migration/failover.h
> index 882c625..fba3931 100644
> --- a/include/migration/failover.h
> +++ b/include/migration/failover.h
> @@ -26,5 +26,6 @@ void failover_init_state(void);
>  int failover_set_state(int old_state, int new_state);
>  int failover_get_state(void);
>  void failover_request_active(Error **errp);
> +bool failover_request_is_active(void);
>  
>  #endif
> diff --git a/migration/colo-failover.c b/migration/colo-failover.c
> index 1b1be24..0c525da 100644
> --- a/migration/colo-failover.c
> +++ b/migration/colo-failover.c
> @@ -32,7 +32,7 @@ static void colo_failover_bh(void *opaque)
>  error_report("Unkown error for failover, old_state=%d", old_state);
>  return;
>  }
> -/*TODO: Do failover work */
> +colo_do_failover(NULL);
>  }
>  
>  void failover_request_active(Error **errp)
> @@ -67,6 +67,11 @@ int failover_get_state(void)
>  return atomic_read(&failover_state);
>  }
>  
> +bool failover_request_is_active(void)
> +{
> +return ((failover_get_state() != FAILOVER_STATUS_NONE));

You can remove the two sets of brackets.
But other than that:

Reviewed-by: Dr. David Alan Gilbert 


> +}
> +
>  void qmp_x_colo_lost_heartbeat(Error **errp)
>  {
>  if (get_colo_mode() == COLO_MODE_UNKNOWN) {
> diff --git a/migration/colo.c b/migration/colo.c
> index 176384e..977c8d8 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -41,6 +41,40 @@ bool migration_incoming_in_colo_state(void)
>  return mis && (mis->state == MIGRATION_STATUS_COLO);
>  }
>  
> +static bool colo_runstate_is_stopped(void)
> +{
> +return runstate_check(RUN_STATE_COLO) || !runstate_is_running();
> +}
> +
> +static void primary_vm_do_failover(void)
> +{
> +MigrationState *s = migrate_get_current();
> +int old_state;
> +
> +migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
> +  MIGRATION_STATUS_COMPLETED);
> +
> +old_state = failover_set_state(FAILOVER_STATUS_HANDLING,
> +   FAILOVER_STATUS_COMPLETED);
> +if (old_state != FAILOVER_STATUS_HANDLING) {
> +error_report("Incorrect state (%d) while doing failover for Primary 
> VM",
> + old_state);
> +return;
> +}
> +}
> +
> +void colo_do_failover(MigrationState *s)
> +{
> +/* Make sure vm stopped while failover */
> +if (!colo_runstate_is_stopped()) {
> +vm_stop_force_state(RUN_STATE_COLO);
> +}
> +
> +if (get_colo_mode() == COLO_MODE_PRIMARY) {
> +primary_vm_do_failover();
> +}
> +}
> +
>  static int colo_put_cmd(QEMUFile *f, uint32_t cmd)
>  {
>  int ret;
> @@ -150,9 +184,22 @@ static int colo_do_checkpoint_transaction(MigrationState 
> *s,
>  }
>  
>  qemu_mutex_lock_iothread();
> +if (failover_request_is_active()) {
> +qemu_mutex_unlock_iothread();
> +ret = -1;
> +goto out;
> +}
>  vm_stop_force_state(RUN_STATE_COLO);
>  qemu_mutex_unlock_iothread();
>  trace_colo_vm_state_change("run", "stop");
> +/*
> + * failover request bh could be called after
> + * vm_stop_force_state so we check failover_request_is_active() again.
> + */
> +if (failover_request_is_active()) {
> +ret = -1;
> +goto out;
> +}
>  
>  /* Disable block migration */
>  s->params.blk = 0;
> @@ -248,6 +295,11 @@ static void colo_process_checkpoint(MigrationState *s)
>  trace_colo_vm_state_change("stop", "run");
>  
>  while (s->state == MIGRATION_STATUS_COLO) {
> +if (failover_request_is_active()) {
> + 

Re: [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option

[John Snow]

On 12/18/2015 10:54 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> This patch adds a new explicit Floppy Drive Type option. The existing
>> behavior in QEMU is to automatically guess a drive type based on the
>> media inserted, or if a diskette is not present, arbitrarily assign one.
>>
>> This behavior can be described as "auto." This patch adds explicit
>> behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
>> is intended to mimick current behavior, while the other types pick
>> one explicitly.
>>
>> In a future patch, the goal is to change the FDC's default drive type
>> from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).
> 
> Err, will we have two different kinds of auto then?
> 

"No," the auto behavior will follow the same rules, but the fallback
type is configurable. Not that it's sane to do it, but it lets me
configure the new behavior in a way that's pretty solidly backwards
compatible using HW_COMPAT.

It's four steps:

1) Codify the old behavior as type=auto,fallback=144
2) Add drive sizes
3) Adjust pick_geometry to allow 1.44MB diskettes to be used in 2.88MB
drives
4) Set new behavior for 2.6+ to type=auto,fallback=288


"Why not just set the new default to type=288?"

Concerns and lack of testing that I will break esoteric targets. This
change is the safest, most minimal change I can make that accomplishes
my goal (2.88MB drives in most cases on x86 platforms to allow virtio
driver diskettes to be inserted post-boot.)

>> In order to allow users to obtain the old behaviors, though, a mechanism
>> for specifying the exact type of drive we want is needed.
> 
> I'd expect users who understand drive types sufficiently to pick one not
> to pick the old behavior, because it's *nuts*.
> 

Yes, I meant mostly if they wanted to force a 1.44MB drive type. libvirt
installations can just have you choose 120, 144 or 288 and be done with
it and always explicitly state which it wants. Easy.

Manual executions of QEMU still get the magic. The magic isn't *so* bad
now, honestly.

>> This patch adds the properties, but it is not acted on yet in favor of
>> making those changes a little more explicitly clear in standalone patches
>> later in this patch set.
>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/block/fdc.c   | 107 
>> ++-
>>  hw/core/qdev-properties.c|  11 +
>>  hw/i386/pc.c |  17 +++
>>  include/hw/block/fdc.h   |   9 +---
>>  include/hw/qdev-properties.h |   1 +
>>  qapi/block.json  |  16 +++
>>  6 files changed, 102 insertions(+), 59 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 13fef23..ad0e052 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -60,58 +60,60 @@ typedef enum FDriveRate {
>>  } FDriveRate;
>>  
>>  typedef struct FDFormat {
>> -FDriveType drive;
>> +FloppyDriveType drive;
>>  uint8_t last_sect;
>>  uint8_t max_track;
>>  uint8_t max_head;
>>  FDriveRate rate;
>>  } FDFormat;
>>  
>> +#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
>> +
>>  static const FDFormat fd_formats[] = {
>>  /* First entry is default format */
>>  /* 1.44 MB 3"1/2 floppy disks */
>> -{ FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
>> -{ FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 21, 83, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 22, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 23, 80, 1, FDRIVE_RATE_500K, },
>> +{ FLOPPY_DRIVE_TYPE_144, 24, 80, 1, FDRIVE_RATE_500K, },
>>  /* 2.88 MB 3"1/2 floppy disks */
>> -{ FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
>> -{ FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
>> -{ FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
>> -{ FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
>> -{ FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
>> +{ FLOPPY_DRIVE_TYPE_288, 36, 80, 1, FDRIVE_RATE_1M, },
>> +{ FLOPPY_DRIVE_TYPE_288, 39, 80, 1, FDRIVE_RATE_1M, },
>> +{ FLOPPY_DRIVE_TYPE_288, 40, 80, 1, FDRIVE_RATE_1M, },
>> +{ FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, },
>> +{ FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, },
>>  /* 720 kB 3"1/2 floppy disks */
>> -{ FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
>> -{ FDRIVE_DRV_144, 10, 80, 1, FDRIVE

[Qemu-devel] [PULL 48/48] block/qapi: allow best-effort query

[Kevin Wolf]

From: John Snow 

For more complex BDS trees that can be created under normal circumstances,
we lose the ability to issue query commands because of our inability to
re-construct the absolute filename.

Instead, omit this field when it is a problem and present as much information
as we can.

This will change the expected output in iotest 110, where we will now see a
json filename and the lack of an absolute filename instead of an error.

Signed-off-by: John Snow 
Message-id: 1450122916-4706-6-git-send-email-js...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 block/qapi.c   | 7 ---
 tests/qemu-iotests/110.out | 5 -
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 5587c64..fecac25 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -245,10 +245,11 @@ void bdrv_query_image_info(BlockDriverState *bs,
 info->has_backing_filename = true;
 bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
 if (err) {
-error_propagate(errp, err);
-qapi_free_ImageInfo(info);
+/* Can't reconstruct the full backing filename, so we must omit
+ * this field and apply a Best Effort to this query. */
 g_free(backing_filename2);
-return;
+backing_filename2 = NULL;
+error_free(err);
 }
 
 /* Always report the full_backing_filename if present, even if it's the
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 0270980..b3584ff 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -11,7 +11,10 @@ backing file: t.IMGFMT.base (actual path: 
TEST_DIR/t.IMGFMT.base)
 
 === Non-reconstructable filename ===
 
-qemu-img: Cannot use relative backing file names for 'json:{"driver": 
"IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", 
"filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", 
"set-state.0.new_state": 42}}'
+image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", 
"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": 
"blkdebug", "set-state.0.new_state": 42}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (cannot determine actual path)
 
 === Backing name is always relative to the backed image ===
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] vhdx: DIV_ROUND_UP() in vhdx_calc_bat_entries()

[Max Reitz]

We have DIV_ROUND_UP(), so we can use it to produce more easily readable
code. It may be slower than the bit shifting currently performed
(because it actually performs a division), but since
vhdx_calc_bat_entries() is never used in a hot path, this is completely
fine.

Signed-off-by: Max Reitz 
---
 block/vhdx.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 2fe9a5e..3955f7f 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -856,14 +856,8 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
 {
 uint32_t data_blocks_cnt, bitmap_blocks_cnt;
 
-data_blocks_cnt = s->virtual_disk_size >> s->block_size_bits;
-if (s->virtual_disk_size - (data_blocks_cnt << s->block_size_bits)) {
-data_blocks_cnt++;
-}
-bitmap_blocks_cnt = data_blocks_cnt >> s->chunk_ratio_bits;
-if (data_blocks_cnt - (bitmap_blocks_cnt << s->chunk_ratio_bits)) {
-bitmap_blocks_cnt++;
-}
+data_blocks_cnt = DIV_ROUND_UP(s->virtual_disk_size, s->block_size);
+bitmap_blocks_cnt = DIV_ROUND_UP(data_blocks_cnt, s->chunk_ratio);
 
 if (s->parent_entries) {
 s->bat_entries = bitmap_blocks_cnt * (s->chunk_ratio + 1);
-- 
2.6.4

[Qemu-devel] [PATCH v3 23/24] s390/sclp: Simplify control flow in sclp_realize()

[Markus Armbruster]

Suggested-by: David Hildenbrand 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
Acked-by: Cornelia Huck 
Reviewed-by: Eric Blake 
---
 hw/s390x/sclp.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 9a117c9..74f2b40 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -463,21 +463,18 @@ static void sclp_realize(DeviceState *dev, Error **errp)
 object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
  &err);
 if (err) {
-goto error;
+goto out;
 }
 
 ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
 if (ret == -E2BIG) {
 error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
hw_limit >> 30);
-goto error;
 } else if (ret) {
 error_setg(&err, "qemu: setting the guest size failed");
-goto error;
 }
-return;
-error:
-assert(err);
+
+out:
 error_propagate(errp, err);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices

[Alistair Francis]

On Thu, Dec 17, 2015 at 3:12 AM, Paolo Bonzini  wrote:
>
>
> On 17/12/2015 12:11, Peter Maydell wrote:
>> > It's a hack and I don't want it to become a sanctioned way to do it.
>> > It's already messing around pretty heavily with qdev internals, see the
>> > line right after QLIST_INSERT_HEAD:
>> >
>> > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling);
>>
>> Well, that doesn't look good either. I think my point still
>> stands -- we should be providing proper infrastructure at
>> the qdev level to allow SoC container devices to do the
>> things they need to do, not just letting the containers
>> mess with the qdev internals.
>
> I agree completely.

Does anyone have any ideas on how we can do this?

AFAIK there is no way to currently do this, so we need to add
something. What is the preferred way to expose the buses?

Thanks,

Alistair

>
> Paolo
>

[Qemu-devel] [PULL 26/48] block: Add opaque value to the amend CB

[Kevin Wolf]

From: Max Reitz 

Add an opaque value which is to be passed to the bdrv_amend_options()
status callback.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c   |  4 ++--
 block/qcow2-cluster.c | 14 --
 block/qcow2.c |  9 +
 block/qcow2.h |  3 ++-
 include/block/block.h |  4 ++--
 include/block/block_int.h |  3 ++-
 qemu-img.c|  5 +++--
 7 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index 1894f1b..411edbf 100644
--- a/block.c
+++ b/block.c
@@ -4068,12 +4068,12 @@ void bdrv_remove_aio_context_notifier(BlockDriverState 
*bs,
 }
 
 int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
-   BlockDriverAmendStatusCB *status_cb)
+   BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
 {
 if (!bs->drv->bdrv_amend_options) {
 return -ENOTSUP;
 }
-return bs->drv->bdrv_amend_options(bs, opts, status_cb);
+return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque);
 }
 
 /* This function will be called by the bdrv_recurse_is_first_non_filter method
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 24a60e2..34112c3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1641,7 +1641,8 @@ fail:
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
   int l1_size, int64_t *visited_l1_entries,
   int64_t l1_entries,
-  BlockDriverAmendStatusCB *status_cb)
+  BlockDriverAmendStatusCB *status_cb,
+  void *cb_opaque)
 {
 BDRVQcow2State *s = bs->opaque;
 bool is_active_l1 = (l1_table == s->l1_table);
@@ -1667,7 +1668,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 /* unallocated */
 (*visited_l1_entries)++;
 if (status_cb) {
-status_cb(bs, *visited_l1_entries, l1_entries);
+status_cb(bs, *visited_l1_entries, l1_entries, cb_opaque);
 }
 continue;
 }
@@ -1804,7 +1805,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 
 (*visited_l1_entries)++;
 if (status_cb) {
-status_cb(bs, *visited_l1_entries, l1_entries);
+status_cb(bs, *visited_l1_entries, l1_entries, cb_opaque);
 }
 }
 
@@ -1828,7 +1829,8 @@ fail:
  * qcow2 version which doesn't yet support metadata zero clusters.
  */
 int qcow2_expand_zero_clusters(BlockDriverState *bs,
-   BlockDriverAmendStatusCB *status_cb)
+   BlockDriverAmendStatusCB *status_cb,
+   void *cb_opaque)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t *l1_table = NULL;
@@ -1845,7 +1847,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
 
 ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size,
  &visited_l1_entries, l1_entries,
- status_cb);
+ status_cb, cb_opaque);
 if (ret < 0) {
 goto fail;
 }
@@ -1881,7 +1883,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
 
 ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size,
  &visited_l1_entries, l1_entries,
- status_cb);
+ status_cb, cb_opaque);
 if (ret < 0) {
 goto fail;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index fda1562..4be2c61 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2870,7 +2870,7 @@ static int qcow2_load_vmstate(BlockDriverState *bs, 
uint8_t *buf,
  * have to be removed.
  */
 static int qcow2_downgrade(BlockDriverState *bs, int target_version,
-   BlockDriverAmendStatusCB *status_cb)
+   BlockDriverAmendStatusCB *status_cb, void 
*cb_opaque)
 {
 BDRVQcow2State *s = bs->opaque;
 int current_version = s->qcow_version;
@@ -2919,7 +2919,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
 /* clearing autoclear features is trivial */
 s->autoclear_features = 0;
 
-ret = qcow2_expand_zero_clusters(bs, status_cb);
+ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
 if (ret < 0) {
 return ret;
 }
@@ -2934,7 +2934,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
 }
 
 static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
-   BlockDriverAmendStatusCB *status_cb)
+   Blo

[Qemu-devel] [PATCH 0/2] block/vhdx: Small cleanups

[Max Reitz]

Just some bits I discovered while going through the Coverity report.

(The issue reported by Coverity actually was not an issue.)


Max Reitz (2):
  vhdx: DIV_ROUND_UP() in vhdx_calc_bat_entries()
  vhdx: Simplify vhdx_set_shift_bits()

 block/vhdx.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

-- 
2.6.4

Re: [Qemu-devel] [PATCH COLO-Frame v12 14/38] ram: Split host_from_stream_offset() into two helper functions

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
> Split host_from_stream_offset() into two parts:
> One is to get ram block, which the block idstr may be get from migration
> stream, the other is to get hva (host) address from block and the offset.
> Besides, we will do the check working in a new helper offset_in_ramblock().
> 
> Signed-off-by: zhanghailiang 
> ---
> v12:
> - Remove the offset parameter for ram_block_from_stream() and
>   check the validity of the related value in a new helper. (Dave's suggestion)
> v11:
> - New patch
> 
> Signed-off-by: zhanghailiang 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  include/exec/ram_addr.h |  8 ++--
>  migration/ram.c | 40 +---
>  2 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 7115154..2b31279 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -38,10 +38,14 @@ struct RAMBlock {
>  int fd;
>  };
>  
> +static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> +{
> +return (b && b->host && offset < b->used_length) ? true : false;
> +}
> +
>  static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
>  {
> -assert(offset < block->used_length);
> -assert(block->host);
> +assert(offset_in_ramblock(block, offset));
>  return (char *)block->host + offset;
>  }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index a709471..09fe6e6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2138,28 +2138,24 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
> void *host)
>   * Returns a pointer from within the RCU-protected ram_list.
>   */
>  /*
> - * Read a RAMBlock ID from the stream f, find the host address of the
> - * start of that block and add on 'offset'
> + * Read a RAMBlock ID from the stream f.
>   *
>   * f: Stream to read from
> - * offset: Offset within the block
>   * flags: Page flags (mostly to see if it's a continuation of previous block)
>   */
> -static inline void *host_from_stream_offset(QEMUFile *f,
> -ram_addr_t offset,
> -int flags)
> +static inline RAMBlock *ram_block_from_stream(QEMUFile *f,
> +  int flags)
>  {
>  static RAMBlock *block = NULL;
>  char id[256];
>  uint8_t len;
>  
>  if (flags & RAM_SAVE_FLAG_CONTINUE) {
> -if (!block || block->max_length <= offset) {
> +if (!block) {
>  error_report("Ack, bad migration stream!");
>  return NULL;
>  }
> -
> -return block->host + offset;
> +return block;
>  }
>  
>  len = qemu_get_byte(f);
> @@ -2167,12 +2163,22 @@ static inline void *host_from_stream_offset(QEMUFile 
> *f,
>  id[len] = 0;
>  
>  block = qemu_ram_block_by_name(id);
> -if (block && block->max_length > offset) {
> -return block->host + offset;
> +if (!block) {
> +error_report("Can't find block %s", id);
> +return NULL;
>  }
>  
> -error_report("Can't find block %s", id);
> -return NULL;
> +return block;
> +}
> +
> +static inline void *host_from_ram_block_offset(RAMBlock *block,
> +   ram_addr_t offset)
> +{
> +if (!offset_in_ramblock(block, offset)) {
> +return NULL;
> +}
> +
> +return block->host + offset;
>  }
>  
>  /*
> @@ -2319,7 +2325,9 @@ static int ram_load_postcopy(QEMUFile *f)
>  trace_ram_load_postcopy_loop((uint64_t)addr, flags);
>  place_needed = false;
>  if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE)) {
> -host = host_from_stream_offset(f, addr, flags);
> +RAMBlock *block = ram_block_from_stream(f, flags);
> +
> +host = host_from_ram_block_offset(block, addr);
>  if (!host) {
>  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>  ret = -EINVAL;
> @@ -2450,7 +2458,9 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>  if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
>   RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> -host = host_from_stream_offset(f, addr, flags);
> +RAMBlock *block = ram_block_from_stream(f, flags);
> +
> +host = host_from_ram_block_offset(block, addr);
>  if (!host) {
>  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>  ret = -EINVAL;
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PULL 33/48] qcow2: Point to amend function in check

[Kevin Wolf]

From: Max Reitz 

If a reference count is not representable with the current refcount
order, the image check should point to qemu-img amend for increasing the
refcount order. However, qemu-img amend needs write access to the image
which cannot be provided if the image is marked corrupt; and the image
check will not mark the image consistent unless everything actually is
consistent.

Therefore, if an image is marked corrupt and the image check encounters
a reference count overflow, it cannot be fixed by using qemu-img amend
to increase the refcount order. Instead, one has to use qemu-img convert
to create a completely new copy of the image in this case.

Alternatively, we may want to give the user a way of manually removing
the corrupt flag, maybe through qemu-img amend, but this is not part of
this patch.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/qcow2-refcount.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e4020c6..af493f8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1345,6 +1345,9 @@ static int inc_refcounts(BlockDriverState *bs,
 if (refcount == s->refcount_max) {
 fprintf(stderr, "ERROR: overflow cluster offset=0x%" PRIx64
 "\n", cluster_offset);
+fprintf(stderr, "Use qemu-img amend to increase the refcount entry 
"
+"width or qemu-img convert to create a clean copy if the "
+"image cannot be opened for writing\n");
 res->corruptions++;
 continue;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 00/48] Block patches

[Peter Maydell]

On 18 December 2015 at 15:07, Kevin Wolf  wrote:
> The following changes since commit 67a708406221f476c0f8fa60c192c186150c5185:
>
>   Merge remote-tracking branch 
> 'remotes/berrange/tags/pull-io-channel-base-2015-12-18-1' into staging 
> (2015-12-18 12:42:10 +)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 9d4a6cf0ea471fb5aeaba9360fec863ef8a0ab44:
>
>   Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-12-18' 
> into queue-block (2015-12-18 14:40:35 +0100)
>
> 
>
> Block layer patches
>
> 

Applied, thanks.

-- PMM

[Qemu-devel] [PULL 12/48] block: Allow specifying child options in reopen

[Kevin Wolf]

If the child was defined in the same context (-drive argument or
blockdev-add QMP command) as its parent, a reopen of the parent should
work the same and allow changing options of the child.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 2c41fff..9c42b17 100644
--- a/block.c
+++ b/block.c
@@ -1720,15 +1720,23 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 flags &= ~BDRV_O_PROTOCOL;
 
 QLIST_FOREACH(child, &bs->children, next) {
+QDict *new_child_options;
+char *child_key_dot;
 int child_flags;
 
+/* reopen can only change the options of block devices that were
+ * implicitly created and inherited options. For other (referenced)
+ * block devices, a syntax like "backing.foo" results in an error. */
 if (child->bs->inherits_from != bs) {
 continue;
 }
 
+child_key_dot = g_strdup_printf("%s.", child->name);
+qdict_extract_subqdict(options, &new_child_options, child_key_dot);
+g_free(child_key_dot);
+
 child_flags = child->role->inherit_flags(flags);
-/* TODO Pass down child flags (backing.*, extents.*, ...) */
-bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
+bdrv_reopen_queue(bs_queue, child->bs, new_child_options, child_flags);
 }
 
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
-- 
1.8.3.1

[Qemu-devel] [PULL 0/2] xsa155

[Stefano Stabellini]

The following changes since commit 18f49881cf8359e89396aac12f5d3cf3f8a632ba:

  configure: Fix shell syntax to placate OpenBSD's pdksh (2015-12-18 13:32:49 
+)

are available in the git repository at:

  git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xsa155

for you to fetch changes up to 7ea11bf376aea4bf8340eb363de9777c7f93e556:

  xenfb: avoid reading twice the same fields from the shared page (2015-12-18 
15:10:09 +)


It would be great if they were backported to the QEMU stable trees too.



XSA-155 fixes


Stefano Stabellini (2):
  xen/blkif: Avoid double access to src->nr_segments
  xenfb: avoid reading twice the same fields from the shared page

 hw/block/xen_blkif.h |   12 
 hw/display/xenfb.c   |   10 ++
 2 files changed, 14 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH v3 20/24] vhdx: Fix "log that needs to be replayed" error message

[Markus Armbruster]

The arguments of error_setg_errno() should yield a short error string
without newlines.

Here, we try to append additional help to the error message by
embedding newlines in the error string.  That's nice, but it's doesn't
play nicely with the errno part.  tests/qemu-iotests/070.out shows the
resulting mess:

can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 
'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log 
that needs to be replayed.  To replay the log, execute:
 qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation 
not permitted

Switch to error_setg() and error_append_hint().  Result:

can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 
'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log 
that needs to be replayed
To replay the log, run:
qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx'

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/vhdx-log.c   | 13 +++--
 tests/qemu-iotests/070.out |  5 +++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47ae4b1..ab86416 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -784,12 +784,13 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState 
*s, bool *flushed,
 if (logs.valid) {
 if (bs->read_only) {
 ret = -EPERM;
-error_setg_errno(errp, EPERM,
- "VHDX image file '%s' opened read-only, but "
- "contains a log that needs to be replayed.  To "
- "replay the log, execute:\n qemu-img check -r "
- "all '%s'",
- bs->filename, bs->filename);
+error_setg(errp,
+   "VHDX image file '%s' opened read-only, but "
+   "contains a log that needs to be replayed",
+   bs->filename);
+error_append_hint(errp,  "To replay the log, run:\n"
+  "qemu-img check -r all '%s'\n",
+  bs->filename);
 goto exit;
 }
 /* now flush the log */
diff --git a/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out
index ffd4251..131a5b1 100644
--- a/tests/qemu-iotests/070.out
+++ b/tests/qemu-iotests/070.out
@@ -1,8 +1,9 @@
 QA output created by 070
 
 === Verify open image read-only fails, due to dirty log ===
-can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 
'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log 
that needs to be replayed.  To replay the log, execute:
- qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation not 
permitted
+can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 
'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log 
that needs to be replayed
+To replay the log, run:
+qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx'
  no file open, try 'help open'
 === Verify open image replays log  ===
 read 18874368/18874368 bytes at offset 0
-- 
2.4.3

[Qemu-devel] [PULL 44/48] block/qapi: do not redundantly print "actual path"

[Kevin Wolf]

From: John Snow 

If it happens to match the backing path, that was the actual path.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Message-id: 1450122916-4706-2-git-send-email-js...@redhat.com
Signed-off-by: Max Reitz 
---
 block/qapi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index c0e877e..563dd31 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -676,7 +676,9 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, 
void *f,
 
 if (info->has_backing_filename) {
 func_fprintf(f, "backing file: %s", info->backing_filename);
-if (info->has_full_backing_filename) {
+if (info->has_full_backing_filename &&
+(strcmp(info->backing_filename,
+info->full_backing_filename) != 0)) {
 func_fprintf(f, " (actual path: %s)", info->full_backing_filename);
 }
 func_fprintf(f, "\n");
-- 
1.8.3.1

[Qemu-devel] [PULL 08/48] block: Consider all block layer options in append_open_options

[Kevin Wolf]

The code already special-cased "node-name", which is currently the only
option passed in the QDict that isn't driver-specific. Generalise the
code to take all general block layer options into consideration.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 84b9bf4..0cc3c8b 100644
--- a/block.c
+++ b/block.c
@@ -3950,20 +3950,30 @@ out:
 static bool append_open_options(QDict *d, BlockDriverState *bs)
 {
 const QDictEntry *entry;
+QemuOptDesc *desc;
 bool found_any = false;
 
 for (entry = qdict_first(bs->options); entry;
  entry = qdict_next(bs->options, entry))
 {
-/* Only take options for this level and exclude all non-driver-specific
- * options */
-if (!strchr(qdict_entry_key(entry), '.') &&
-strcmp(qdict_entry_key(entry), "node-name"))
-{
-qobject_incref(qdict_entry_value(entry));
-qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
-found_any = true;
+/* Only take options for this level */
+if (strchr(qdict_entry_key(entry), '.')) {
+continue;
 }
+
+/* And exclude all non-driver-specific options */
+for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
+if (!strcmp(qdict_entry_key(entry), desc->name)) {
+break;
+}
+}
+if (desc->name) {
+continue;
+}
+
+qobject_incref(qdict_entry_value(entry));
+qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
+found_any = true;
 }
 
 return found_any;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.5??] configure: Fix shell syntax to placate OpenBSD's pdksh

[Peter Maydell]

On 14 December 2015 at 15:02, Peter Maydell  wrote:
> Unfortunately the OpenBSD pdksh does not like brackets inside
> the right part of a ${variable+word} parameter expansion:
>
>   $ echo "${a+($b)}"
>   ksh: ${a+($b)}": bad substitution
>
> though both bash and dash accept them. In any case this line
> was causing odd output in the case where nettle is not present:
>   nettleno ()
>
> (because if nettle is not present then $nettle will be "no",
> not a null string or unset).
>
> Rewrite it to just use an if.
>
> This bug was originally introduced in becaeb726 and was present
> in the 2.4.0 release.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1525682
> Reported-by: Dmitrij D. Czarkoff
> Signed-off-by: Peter Maydell 
> ---
> This fixes a problem where configure just falls over on OpenBSD,
> but on the other hand it is not a regression since 2.4.0...
> Opinions on whether we should put it in 2.5 as a last minute
> thing welcome.

Now committed to master since we're post-2.5. I added a
Cc: qemu-sta...@nongnu.org

too.

thanks
-- PMM

[Qemu-devel] [PULL 45/48] block/qapi: always report full_backing_filename

[Kevin Wolf]

From: John Snow 

Always report full_backing_filename, even if it's the same as
backing_filename. In the next patch, full_backing_filename may be
omitted if it cannot be generated instead of allowing e.g. drive_query
to abort if it runs into this scenario.

The presence or absence of the "full" field becomes useful information.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Message-id: 1450122916-4706-3-git-send-email-js...@redhat.com
Signed-off-by: Max Reitz 
---
 block/qapi.c   | 7 ---
 tests/qemu-iotests/043.out | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 563dd31..3d8e434 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -251,9 +251,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
 return;
 }
 
-if (strcmp(backing_filename, backing_filename2) != 0) {
-info->full_backing_filename =
-g_strdup(backing_filename2);
+/* Always report the full_backing_filename if present, even if it's the
+ * same as backing_filename. That they are same is useful info. */
+if (backing_filename2) {
+info->full_backing_filename = g_strdup(backing_filename2);
 info->has_full_backing_filename = true;
 }
 
diff --git a/tests/qemu-iotests/043.out b/tests/qemu-iotests/043.out
index 33f8cc3..b37d2a3 100644
--- a/tests/qemu-iotests/043.out
+++ b/tests/qemu-iotests/043.out
@@ -44,6 +44,7 @@ cluster_size: 65536
 "filename": "TEST_DIR/t.IMGFMT",
 "cluster-size": 65536,
 "format": "IMGFMT",
+"full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
 "backing-filename": "TEST_DIR/t.IMGFMT.2.base",
 "dirty-flag": false
 },
@@ -52,6 +53,7 @@ cluster_size: 65536
 "filename": "TEST_DIR/t.IMGFMT.2.base",
 "cluster-size": 65536,
 "format": "IMGFMT",
+"full-backing-filename": "TEST_DIR/t.IMGFMT.1.base",
 "backing-filename": "TEST_DIR/t.IMGFMT.1.base",
 "dirty-flag": false
 },
-- 
1.8.3.1

[Qemu-devel] [PULL 22/48] qemu-iotests: Try setting cache mode for children

[Kevin Wolf]

This is a basic test for specifying cache modes for child nodes on the
command line. It doesn't take much time and works without O_DIRECT
support.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/051 | 10 +++-
 tests/qemu-iotests/051.out | 60 ++
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index f6f0f4d..da90f59 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -61,7 +61,7 @@ function do_run_qemu()
 
 function run_qemu()
 {
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | 
_filter_generated_node_ids
 }
 
 size=128M
@@ -190,6 +190,14 @@ run_qemu -drive driver=null-co,cache=writethrough
 run_qemu -drive driver=null-co,cache=unsafe
 run_qemu -drive driver=null-co,cache=invalid_value
 
+# Can't test direct=on here because O_DIRECT might not be supported on this FS
+# Test 142 checks the direct=on cases
+
+for cache in writeback writethrough unsafe invalid_value; do
+echo -e "info block\ninfo block file\ninfo block backing\ninfo block 
backing-file" | \
+run_qemu -drive 
file="$TEST_IMG",cache=$cache,backing.file.filename="$TEST_IMG.base",backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file
 -nodefaults
+done
+
 echo
 echo === Specifying the protocol layer ===
 echo
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7a459a3..070d318 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -238,6 +238,66 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=null-co,cache=invalid_value
 QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
+Testing: -drive 
file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file
 -nodefaults
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo block
+ide0-hd0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+Cache mode:   writeback
+Backing file: TEST_DIR/t.qcow2.base (chain depth: 1)
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
finfo block 
fiinfo block 
filinfo block file
+
+file: TEST_DIR/t.qcow2 (file)
+Cache mode:   writeback
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
binfo block 
bainfo block 
bacinfo block 
backinfo block 
backiinfo block 
backininfo block 
backing
+backing: TEST_DIR/t.qcow2.base (qcow2, read-only)
+Cache mode:   writeback, ignore flushes
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
binfo block 
bainfo block 
bacinfo block 
backinfo block 
backiinfo block 
backininfo block 
backinginfo block 
backing-info block 
backing-finfo 
block backing-fi!
 info block 
backing-fil

Re: [Qemu-devel] [PATCH 6/6] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger  wrote:
> This patch allows the instantiation of the vfio-amd-xgbe device
> from the QEMU command line (-device vfio-amd-xgbe,host="").
>
> The guest is exposed with a device tree node that combines the description
> of both XGBE and PHY (representation supported from 4.2 onwards kernel):
> Documentation/devicetree/bindings/net/amd-xgbe.txt.
>
> There are 5 register regions, 6 interrupts including 4 optional
> edge-sensitive per-channel interrupts.
>
> Some property values are inherited from host device tree. Host device tree
> must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
>
> 2 clock nodes (dma and ptp) also are created. It is checked those clocks
> are fixed on host side.
>
> Signed-off-by: Eric Auger 
>
> ---
>
> RFC -> v1:
> - use qemu_fdt_getprop with Error **
> - free substrings in sysfs_to_dt_name
> - add some comments related to endianess in add_amd_xgbe_fdt_node
> - reword commit message (dtc binary dependency has disappeared)
> - check the host device has 5 regions meaning this is a combined
>   XGBE/PHY device
> ---
>  hw/arm/sysbus-fdt.c | 184 
> ++--
>  1 file changed, 178 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index c2d813b..7ec5623 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -22,6 +22,7 @@
>   */
>
>  #include 
> +#include 
>  #include "hw/arm/sysbus-fdt.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/device_tree.h"
> @@ -29,6 +30,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-platform.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
> +#include "hw/vfio/vfio-amd-xgbe.h"
>  #include "hw/arm/fdt.h"
>
>  /*
> @@ -120,12 +122,9 @@ static HostProperty clock_inherited_properties[] = {
>   * @host_phandle: phandle of the clock in host device tree
>   * @guest_phandle: phandle to assign to the guest node
>   */
> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> - uint32_t host_phandle,
> - uint32_t guest_phandle);
> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> - uint32_t host_phandle,
> - uint32_t guest_phandle)
> +static int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +uint32_t host_phandle,
> +uint32_t guest_phandle)
>  {
>  char node_path[256];
>  char *nodename;
> @@ -167,6 +166,22 @@ out:
>  return ret;
>  }
>
> +/**
> + * sysfs_to_dt_name
> + *
> + * convert the name found in sysfs into the node name
> + * for instance e090.xgmac is converted into xgmac@e090
> + */
> +static char *sysfs_to_dt_name(const char *sysfs_name)
> +{
> +gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
> +char *dt_name;
> +
> +dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
> +g_strfreev(substrings);
> +return dt_name;

This will crash if you feed it a string without a "." in it.
A bit more robustness would be a good idea.

> +}
> +
>  /* Device Specific Code */
>
>  /**
> @@ -234,9 +249,166 @@ fail_reg:
>  return ret;
>  }
>
> +
> +/* AMD xgbe properties whose values are copied/pasted from host */
> +static HostProperty amd_xgbe_inherited_properties[] = {
> +{"compatible", 0},
> +{"dma-coherent", 1},
> +{"amd,per-channel-interrupt", 1},
> +{"phy-mode", 0},
> +{"mac-address", 1},
> +{"amd,speed-set", 0},
> +{"amd,serdes-blwc", 1},
> +{"amd,serdes-cdr-rate", 1},
> +{"amd,serdes-pq-skew", 1},
> +{"amd,serdes-tx-amp", 1},
> +{"amd,serdes-dfe-tap-config", 1},
> +{"amd,serdes-dfe-tap-enable", 1},
> +{"clock-names", 0},

HostProperty's second field is type 'bool' so these initializers
should be using 'true' and 'false' rather than 1 and 0.

> +};
> +
> +/**
> + * add_amd_xgbe_fdt_node
> + *
> + * Generates the combined xgbe/phy node following kernel >=4.2
> + * binding documentation:
> + * Documentation/devicetree/bindings/net/amd-xgbe.txt:
> + * Also 2 clock nodes are created (dma and ptp)
> + */
> +static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +PlatformBusFDTData *data = opaque;
> +PlatformBusDevice *pbus = data->pbus;
> +VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +VFIODevice *vbasedev = &vdev->vbasedev;
> +VFIOINTp *intp;
> +const char *parent_node = data->pbus_node_name;
> +char *node_path, *nodename, *dt_name;
> +void *guest_fdt = data->fdt, *host_fdt;
> +const void *r;
> +int i, ret = -1, prop_len;
> +uint32_t *irq_attr, *reg_attr, *host_clock_phandles;
> +uint64_t mmio_base, irq_number;
> +uint32_t guest_clock_phandles[2];
> +
> +host_fdt = load_device_tree_from_sysfs();
> +if (!host_fdt) {
> +goto stop;
> +}
> +dt_name = sysfs_to_dt_name(vbasedev->name);
> +ret = qemu_fdt_node

[Qemu-devel] [PULL 35/48] qcow2: insert assert into qcow2_get_specific_info()

[Kevin Wolf]

From: "Denis V. Lunev" 

s->qcow_version is always set to 2 or 3. Let's assert if this is wrong.

Signed-off-by: Denis V. Lunev 
CC: Roman Kagan 
CC: Max Reitz 
CC: Kevin Wolf 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0304de6..1789af4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2803,6 +2803,10 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 .has_corrupt= true,
 .refcount_bits  = s->refcount_bits,
 };
+} else {
+/* if this assertion fails, this probably means a new version was
+ * added without having it covered here */
+assert(false);
 }
 
 return spec_info;
-- 
1.8.3.1

[Qemu-devel] [PULL 25/48] progress: Allow regressing progress

[Kevin Wolf]

From: Max Reitz 

Progress may regress; this should be displayed correctly by
qemu_progress_print().

While touching that area of code, drop the redundant parentheses in the
same condition.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 util/qemu-progress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-progress.c b/util/qemu-progress.c
index 4ee5cd0..532333e 100644
--- a/util/qemu-progress.c
+++ b/util/qemu-progress.c
@@ -152,7 +152,8 @@ void qemu_progress_print(float delta, int max)
 state.current = current;
 
 if (current > (state.last_print + state.min_skip) ||
-(current == 100) || (current == 0)) {
+current < (state.last_print - state.min_skip) ||
+current == 100 || current == 0) {
 state.last_print = state.current;
 state.print();
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 04/48] qcow2: Add .bdrv_join_options callback

[Kevin Wolf]

qcow2 accepts a few driver-specific options that overlap semantically
(e.g. "overlap-check" is an alias of "overlap-check.template", and any
missing cache size option is derived from the given ones).

When bdrv_reopen() merges the set of updated options with left out
options that should be kept at their old value, we need to consider this
and filter out any duplicates (which would generally cause errors
because new and old value would contradict each other).

This patch adds a .bdrv_join_options callback to BlockDriver and
implements it for qcow2.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c | 47 +++
 include/block/block_int.h |  1 +
 2 files changed, 48 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5b59fa3..fda1562 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1282,6 +1282,52 @@ static void qcow2_reopen_abort(BDRVReopenState *state)
 g_free(state->opaque);
 }
 
+static void qcow2_join_options(QDict *options, QDict *old_options)
+{
+bool has_new_overlap_template =
+qdict_haskey(options, QCOW2_OPT_OVERLAP) ||
+qdict_haskey(options, QCOW2_OPT_OVERLAP_TEMPLATE);
+bool has_new_total_cache_size =
+qdict_haskey(options, QCOW2_OPT_CACHE_SIZE);
+bool has_all_cache_options;
+
+/* New overlap template overrides all old overlap options */
+if (has_new_overlap_template) {
+qdict_del(old_options, QCOW2_OPT_OVERLAP);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_TEMPLATE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_MAIN_HEADER);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_ACTIVE_L1);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_ACTIVE_L2);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_REFCOUNT_TABLE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_INACTIVE_L1);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_INACTIVE_L2);
+}
+
+/* New total cache size overrides all old options */
+if (qdict_haskey(options, QCOW2_OPT_CACHE_SIZE)) {
+qdict_del(old_options, QCOW2_OPT_L2_CACHE_SIZE);
+qdict_del(old_options, QCOW2_OPT_REFCOUNT_CACHE_SIZE);
+}
+
+qdict_join(options, old_options, false);
+
+/*
+ * If after merging all cache size options are set, an old total size is
+ * overwritten. Do keep all options, however, if all three are new. The
+ * resulting error message is what we want to happen.
+ */
+has_all_cache_options =
+qdict_haskey(options, QCOW2_OPT_CACHE_SIZE) ||
+qdict_haskey(options, QCOW2_OPT_L2_CACHE_SIZE) ||
+qdict_haskey(options, QCOW2_OPT_REFCOUNT_CACHE_SIZE);
+
+if (has_all_cache_options && !has_new_total_cache_size) {
+qdict_del(options, QCOW2_OPT_CACHE_SIZE);
+}
+}
+
 static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -3145,6 +3191,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_reopen_prepare  = qcow2_reopen_prepare,
 .bdrv_reopen_commit   = qcow2_reopen_commit,
 .bdrv_reopen_abort= qcow2_reopen_abort,
+.bdrv_join_options= qcow2_join_options,
 .bdrv_create= qcow2_create,
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
 .bdrv_co_get_block_status = qcow2_co_get_block_status,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 66e208d..c2ce965 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -121,6 +121,7 @@ struct BlockDriver {
BlockReopenQueue *queue, Error **errp);
 void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
 void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
+void (*bdrv_join_options)(QDict *options, QDict *old_options);
 
 int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
  Error **errp);
-- 
1.8.3.1

[Qemu-devel] [PULL v1 5/5] crypto: add support for loading encrypted x509 keys

[Daniel P. Berrange]

Make use of the QCryptoSecret object to support loading of
encrypted x509 keys. The optional 'passwordid' parameter
to the tls-creds-x509 object type, provides the ID of a
secret object instance that holds the decryption password
for the PEM file.

 # printf "123456" > mypasswd.txt
 # $QEMU \
-object secret,id=sec0,filename=mypasswd.txt \
-object tls-creds-x509,passwordid=sec0,id=creds0,\
dir=/home/berrange/.pki/qemu,endpoint=server \
-vnc :1,tls-creds=creds0

This requires QEMU to be linked to GNUTLS >= 3.1.11. If
GNUTLS is too old an error will be reported if an attempt
is made to pass a decryption password.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 crypto/tlscredsx509.c | 48 +++
 include/crypto/tlscredsx509.h |  1 +
 qemu-options.hx   |  8 +++-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 26f18cb..d58fdea 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -20,6 +20,7 @@
 
 #include "crypto/tlscredsx509.h"
 #include "crypto/tlscredspriv.h"
+#include "crypto/secret.h"
 #include "qom/object_interfaces.h"
 #include "trace.h"
 
@@ -607,9 +608,30 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
 }
 
 if (cert != NULL && key != NULL) {
+#if GNUTLS_VERSION_NUMBER >= 0x030111
+char *password = NULL;
+if (creds->passwordid) {
+password = qcrypto_secret_lookup_as_utf8(creds->passwordid,
+ errp);
+if (!password) {
+goto cleanup;
+}
+}
+ret = gnutls_certificate_set_x509_key_file2(creds->data,
+cert, key,
+GNUTLS_X509_FMT_PEM,
+password,
+0);
+g_free(password);
+#else /* GNUTLS_VERSION_NUMBER < 0x030111 */
+if (creds->passwordid) {
+error_setg(errp, "PKCS8 decryption requires GNUTLS >= 3.1.11");
+goto cleanup;
+}
 ret = gnutls_certificate_set_x509_key_file(creds->data,
cert, key,
GNUTLS_X509_FMT_PEM);
+#endif /* GNUTLS_VERSION_NUMBER < 0x030111 */
 if (ret < 0) {
 error_setg(errp, "Cannot load certificate '%s' & key '%s': %s",
cert, key, gnutls_strerror(ret));
@@ -737,6 +759,27 @@ qcrypto_tls_creds_x509_prop_set_sanity(Object *obj,
 }
 
 
+static void
+qcrypto_tls_creds_x509_prop_set_passwordid(Object *obj,
+   const char *value,
+   Error **errp G_GNUC_UNUSED)
+{
+QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
+
+creds->passwordid = g_strdup(value);
+}
+
+
+static char *
+qcrypto_tls_creds_x509_prop_get_passwordid(Object *obj,
+   Error **errp G_GNUC_UNUSED)
+{
+QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
+
+return g_strdup(creds->passwordid);
+}
+
+
 static bool
 qcrypto_tls_creds_x509_prop_get_sanity(Object *obj,
Error **errp G_GNUC_UNUSED)
@@ -769,6 +812,10 @@ qcrypto_tls_creds_x509_init(Object *obj)
  qcrypto_tls_creds_x509_prop_get_sanity,
  qcrypto_tls_creds_x509_prop_set_sanity,
  NULL);
+object_property_add_str(obj, "passwordid",
+qcrypto_tls_creds_x509_prop_get_passwordid,
+qcrypto_tls_creds_x509_prop_set_passwordid,
+NULL);
 }
 
 
@@ -777,6 +824,7 @@ qcrypto_tls_creds_x509_finalize(Object *obj)
 {
 QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
 
+g_free(creds->passwordid);
 qcrypto_tls_creds_x509_unload(creds);
 }
 
diff --git a/include/crypto/tlscredsx509.h b/include/crypto/tlscredsx509.h
index b9785fd..25796d7 100644
--- a/include/crypto/tlscredsx509.h
+++ b/include/crypto/tlscredsx509.h
@@ -101,6 +101,7 @@ struct QCryptoTLSCredsX509 {
 gnutls_certificate_credentials_t data;
 #endif
 bool sanityCheck;
+char *passwordid;
 };
 
 
diff --git a/qemu-options.hx b/qemu-options.hx
index f37a2eb..49afe6c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3627,7 +3627,7 @@ expensive operation that consumes random pool entropy, so 
it is
 recommended that a persistent set of parameters be generated
 upfront and saved.
 
-@item -object 
tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},verify-peer=@var{on|off}
+@item -object 
tls-creds-x509,id=@var{id},endpoint=@var{endpoint},dir=@var{/path/to/cred/dir},verify-peer=@var{on|off},pass

Re: [Qemu-devel] [RFC PATCH 0/2] Add GIC ITS description in ACPI MADT table

[Peter Maydell]

On 29 November 2015 at 08:22, Shannon Zhao  wrote:
> From: Shannon Zhao 
>
> These two patches add ITS description in ACPI MADT table. It bases on
> Pavel Fedin's ITS series[1].
>
> [1]https://www.mail-archive.com/qemu-devel@nongnu.org/msg337421.html
>
> Shannon Zhao (2):
>   ACPI: Add GIC Interrupt Translation Service Structure definition
>   ARM: Virt: ACPI: Add GIC ITS description in ACPI MADT table
>
>  hw/arm/virt-acpi-build.c| 11 +++
>  include/hw/acpi/acpi-defs.h | 13 -
>  2 files changed, 23 insertions(+), 1 deletion(-)

Just a note to say these patches look fine and I'm assuming you'll
resend them when we get the ITS support in.

thanks
-- PMM

[Qemu-devel] [PULL v1 4/5] crypto: add QCryptoSecret object class for password/key handling

[Daniel P. Berrange]

Introduce a new QCryptoSecret object class which will be used
for providing passwords and keys to other objects which need
sensitive credentials.

The new object can provide secret values directly as properties,
or indirectly via a file. The latter includes support for file
descriptor passing syntax on UNIX platforms. Ordinarily passing
secret values directly as properties is insecure, since they
are visible in process listings, or in log files showing the
CLI args / QMP commands. It is possible to use AES-256-CBC to
encrypt the secret values though, in which case all that is
visible is the ciphertext.  For ad hoc developer testing though,
it is fine to provide the secrets directly without encryption
so this is not explicitly forbidden.

The anticipated scenario is that libvirtd will create a random
master key per QEMU instance (eg /var/run/libvirt/qemu/$VMNAME.key)
and will use that key to encrypt all passwords it provides to
QEMU via '-object secret,'.  This avoids the need for libvirt
(or other mgmt apps) to worry about file descriptor passing.

It also makes life easier for people who are scripting the
management of QEMU, for whom FD passing is significantly more
complex.

Providing data inline (insecure, only for ad hoc dev testing)

  $QEMU -object secret,id=sec0,data=letmein

Providing data indirectly in raw format

  printf "letmein" > mypasswd.txt
  $QEMU -object secret,id=sec0,file=mypasswd.txt

Providing data indirectly in base64 format

  $QEMU -object secret,id=sec0,file=mykey.b64,format=base64

Providing data with encryption

  $QEMU -object secret,id=master0,file=mykey.b64,format=base64 \
-object secret,id=sec0,data=[base64 ciphertext],\
   keyid=master0,iv=[base64 IV],format=base64

Note that 'format' here refers to the format of the ciphertext
data. The decrypted data must always be in raw byte format.

More examples are shown in the updated docs.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 crypto/Makefile.objs   |   1 +
 crypto/secret.c| 513 +
 include/crypto/secret.h| 148 +
 qapi/crypto.json   |  14 ++
 qemu-options.hx|  77 +++
 tests/.gitignore   |   1 +
 tests/Makefile |   2 +
 tests/test-crypto-secret.c | 452 +++
 8 files changed, 1208 insertions(+)
 create mode 100644 crypto/secret.c
 create mode 100644 include/crypto/secret.h
 create mode 100644 tests/test-crypto-secret.c

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index b2a0e0b..a3135f1 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -7,6 +7,7 @@ crypto-obj-y += tlscreds.o
 crypto-obj-y += tlscredsanon.o
 crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
+crypto-obj-y += secret.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o
diff --git a/crypto/secret.c b/crypto/secret.c
new file mode 100644
index 000..9a9257a
--- /dev/null
+++ b/crypto/secret.c
@@ -0,0 +1,513 @@
+/*
+ * QEMU crypto secret support
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#include "crypto/secret.h"
+#include "crypto/cipher.h"
+#include "qom/object_interfaces.h"
+#include "qemu/base64.h"
+#include "trace.h"
+
+
+static void
+qcrypto_secret_load_data(QCryptoSecret *secret,
+ uint8_t **output,
+ size_t *outputlen,
+ Error **errp)
+{
+char *data = NULL;
+size_t length = 0;
+GError *gerr = NULL;
+
+*output = NULL;
+*outputlen = 0;
+
+if (secret->file) {
+if (secret->data) {
+error_setg(errp,
+   "'file' and 'data' are mutually exclusive");
+return;
+}
+if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
+error_setg(errp,
+   "Unable to read %s: %s",
+   secret->file, gerr->message);
+g_error_free(gerr);
+return;
+}
+*output = (uint8_t *)data;
+*outputlen = length;
+} else if (secret->data) {
+*outputlen = strlen(secret->data);
+*output = (uint8_t *)g_strdup(secret->data);
+} else {
+error_setg(errp, "Eit

[Qemu-devel] [PULL v1 3/5] qga: convert to use error checked base64 decode

[Daniel P. Berrange]

Switch from using g_base64_decode over to qbase64_decode
in order to get error checking of the base64 input data.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qga/commands-posix.c | 11 +--
 qga/commands-win32.c | 11 +--
 qga/commands.c   | 13 -
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c2ff970..8fe708f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -29,6 +29,7 @@
 #include "qemu/queue.h"
 #include "qemu/host-utils.h"
 #include "qemu/sockets.h"
+#include "qemu/base64.h"
 
 #ifndef CONFIG_HAS_ENVIRON
 #ifdef __APPLE__
@@ -525,7 +526,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const 
char *buf_b64,
 gfh->state = RW_STATE_NEW;
 }
 
-buf = g_base64_decode(buf_b64, &buf_len);
+buf = qbase64_decode(buf_b64, -1, &buf_len, errp);
+if (!buf) {
+return NULL;
+}
 
 if (!has_count) {
 count = buf_len;
@@ -1963,7 +1967,10 @@ void qmp_guest_set_user_password(const char *username,
 char *chpasswddata = NULL;
 size_t chpasswdlen;
 
-rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
+rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
+if (!rawpasswddata) {
+return;
+}
 rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
 rawpasswddata[rawpasswdlen] = '\0';
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0654fe4..61ffbdf 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -34,6 +34,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/queue.h"
 #include "qemu/host-utils.h"
+#include "qemu/base64.h"
 
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x8000
@@ -357,7 +358,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const 
char *buf_b64,
 return NULL;
 }
 fh = gfh->fh;
-buf = g_base64_decode(buf_b64, &buf_len);
+buf = qbase64_decode(buf_b64, -1, &buf_len, errp);
+if (!buf) {
+return NULL;
+}
 
 if (!has_count) {
 count = buf_len;
@@ -1294,7 +1298,10 @@ void qmp_guest_set_user_password(const char *username,
 return;
 }
 
-rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
+rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
+if (!rawpasswddata) {
+return;
+}
 rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
 rawpasswddata[rawpasswdlen] = '\0';
 
diff --git a/qga/commands.c b/qga/commands.c
index bb73e7d..58568d8 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -14,6 +14,7 @@
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
+#include "qemu/base64.h"
 
 /* Maximum captured guest-exec out_data/err_data - 16MB */
 #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
@@ -393,10 +394,19 @@ GuestExec *qmp_guest_exec(const char *path,
 GIOChannel *in_ch, *out_ch, *err_ch;
 GSpawnFlags flags;
 bool has_output = (has_capture_output && capture_output);
+uint8_t *input = NULL;
+size_t ninput = 0;
 
 arglist.value = (char *)path;
 arglist.next = has_arg ? arg : NULL;
 
+if (has_input_data) {
+input = qbase64_decode(input_data, -1, &ninput, err);
+if (!input) {
+return NULL;
+}
+}
+
 argv = guest_exec_get_args(&arglist, true);
 envp = has_env ? guest_exec_get_args(env, false) : NULL;
 
@@ -425,7 +435,8 @@ GuestExec *qmp_guest_exec(const char *path,
 g_child_watch_add(pid, guest_exec_child_watch, gei);
 
 if (has_input_data) {
-gei->in.data = g_base64_decode(input_data, &gei->in.size);
+gei->in.data = input;
+gei->in.size = ninput;
 #ifdef G_OS_WIN32
 in_ch = g_io_channel_win32_new_fd(in_fd);
 #else
-- 
2.5.0

[Qemu-devel] [PATCH 1/2] qom: add object_class_get_instance_size()

[Igor Mammedov]

it will be used for allocating memory for a to be
created new object in safe manner.

Signed-off-by: Igor Mammedov 
---
 include/qom/object.h | 8 
 qom/object.c | 5 +
 2 files changed, 13 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 4509166..8f61b0b 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -880,6 +880,14 @@ bool object_class_is_abstract(ObjectClass *klass);
  */
 ObjectClass *object_class_by_name(const char *typename);
 
+/**
+ * object_class_get_instance_size:
+ * @klass: The #ObjectClass to obtain instance size
+ *
+ * Returns: instance size of @klass
+ */
+size_t object_class_get_instance_size(ObjectClass *klass);
+
 void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
   const char *implements_type, bool include_abstract,
   void *opaque);
diff --git a/qom/object.c b/qom/object.c
index d751569..2f141ee 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -771,6 +771,11 @@ ObjectClass *object_class_get_parent(ObjectClass *class)
 return type->class;
 }
 
+size_t object_class_get_instance_size(ObjectClass *klass)
+{
+   return klass->type->instance_size;
+}
+
 typedef struct OCFData
 {
 void (*fn)(ObjectClass *klass, void *opaque);
-- 
1.8.3.1

[Qemu-devel] [PULL 1/2] xen/blkif: Avoid double access to src->nr_segments

[Stefano Stabellini]

src is stored in shared memory and src->nr_segments is dereferenced
twice at the end of the function.  If a compiler decides to compile this
into two separate memory accesses then the size limitation could be
bypassed.

Fix it by removing the double access to src->nr_segments.

This is part of XSA-155.

Signed-off-by: Stefano Stabellini 
---
 hw/block/xen_blkif.h |   12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
index 711b692..c68487cb 100644
--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -85,8 +85,10 @@ static inline void blkif_get_x86_32_req(blkif_request_t 
*dst, blkif_x86_32_reque
d->nr_sectors = s->nr_sectors;
return;
}
-   if (n > src->nr_segments)
-   n = src->nr_segments;
+   /* prevent the compiler from optimizing the code and using 
src->nr_segments instead */
+   barrier();
+   if (n > dst->nr_segments)
+   n = dst->nr_segments;
for (i = 0; i < n; i++)
dst->seg[i] = src->seg[i];
 }
@@ -106,8 +108,10 @@ static inline void blkif_get_x86_64_req(blkif_request_t 
*dst, blkif_x86_64_reque
d->nr_sectors = s->nr_sectors;
return;
}
-   if (n > src->nr_segments)
-   n = src->nr_segments;
+   /* prevent the compiler from optimizing the code and using 
src->nr_segments instead */
+   barrier();
+   if (n > dst->nr_segments)
+   n = dst->nr_segments;
for (i = 0; i < n; i++)
dst->seg[i] = src->seg[i];
 }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] fix MSI injection on Xen

[Stefano Stabellini]

On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2015 at 05:16:18PM +, Stefano Stabellini wrote:
> > On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> > > On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> > > > On Xen MSIs can be remapped into pirqs, which are a type of event
> > > > channels. It's mostly for the benefit of PCI passthrough devices, to
> > > > avoid the overhead of interacting with the emulated lapic.
> > > > 
> > > > However remapping interrupts and MSIs is also supported for emulated
> > > > devices, such as the e1000 and virtio-net.
> > > > 
> > > > When an interrupt or an MSI is remapped into a pirq, masking and
> > > > unmasking is done by masking and unmasking the event channel. The
> > > > masking bit on the PCI config space or MSI-X table should be ignored,
> > > > but it isn't at the moment.
> > > > 
> > > > As a consequence emulated devices which use MSI or MSI-X, such as
> > > > virtio-net, don't work properly (the guest doesn't receive any
> > > > notifications). The mechanism was working properly when xen_apic was
> > > > introduced, but I haven't narrowed down which commit in particular is
> > > > causing the regression.
> > > > 
> > > > Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> > > > been remapped into pirqs.
> > > > 
> > > > Signed-off-by: Stefano Stabellini 
> > > > 
> > > 
> > > Is this a regression?
> > > If not I'd rather defer this until 2.6 when I can review properly.
> > 
> > Unfortunately it is a regression, but given that probably it has been
> > broken for multiple releases, I think you can wait until 2.6.
> > 
> 
> Let's do that then. Pls ping me after 2.6.

Ping


> > > > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > > > index f9c0484..3998725 100644
> > > > --- a/hw/pci/msi.c
> > > > +++ b/hw/pci/msi.c
> > > > @@ -19,6 +19,7 @@
> > > >   */
> > > >  
> > > >  #include "hw/pci/msi.h"
> > > > +#include "hw/xen/xen.h"
> > > >  #include "qemu/range.h"
> > > >  
> > > >  /* PCI_MSI_ADDRESS_LO */
> > > > @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
> > > >  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
> > > >  {
> > > >  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> > > > -uint32_t mask;
> > > > +uint32_t mask, data;
> > > > +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> > > >  assert(vector < PCI_MSI_VECTORS_MAX);
> > > >  
> > > >  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
> > > >  return false;
> > > >  }
> > > >  
> > > > +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> > > > +if (xen_is_pirq_msi(data)) {
> > > > +return false;
> > > > +}
> > > > +
> > > >  mask = pci_get_long(dev->config +
> > > >  msi_mask_off(dev, flags & 
> > > > PCI_MSI_FLAGS_64BIT));
> > > >  return mask & (1U << vector);
> > > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > > > index 7716bf3..96281c2 100644
> > > > --- a/hw/pci/msix.c
> > > > +++ b/hw/pci/msix.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include "hw/pci/msi.h"
> > > >  #include "hw/pci/msix.h"
> > > >  #include "hw/pci/pci.h"
> > > > +#include "hw/xen/xen.h"
> > > >  #include "qemu/range.h"
> > > >  
> > > >  #define MSIX_CAP_LENGTH 12
> > > > @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int 
> > > > vector)
> > > >  
> > > >  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, 
> > > > bool fmask)
> > > >  {
> > > > -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> > > > PCI_MSIX_ENTRY_VECTOR_CTRL;
> > > > -return fmask || dev->msix_table[offset] & 
> > > > PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > > +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> > > > +uint32_t *data = (uint32_t *)&dev->msix_table[offset + 
> > > > PCI_MSIX_ENTRY_DATA];
> > > > +/* MSIs on Xen can be remapped into pirqs. In those cases, masking
> > > > + * and unmasking go through the PV evtchn path. */
> > > > +if (xen_is_pirq_msi(*data)) {
> > > > +return false;
> > > > +}
> > > > +return fmask || dev->msix_table[offset + 
> > > > PCI_MSIX_ENTRY_VECTOR_CTRL] &
> > > > +PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > >  }
> > > >  
> > > >  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> > > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> > > > index 82de2bc..375707e 100644
> > > > --- a/hw/xen/xen_pt_msi.c
> > > > +++ b/hw/xen/xen_pt_msi.c
> > > > @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
> > > >  
> > > >  assert((!is_msix && msix_entry == 0) || is_msix);
> > > >  
> > > > -if (gvec == 0) {
> > > > -/* if gvec is 0, the guest is asking for a particular pirq that
> > > > - * is passed as dest_id */
> > > > +if (xen_is_pirq_msi(data)) {
> > > >  *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
> > > >  if (!*ppirq) {
> > > >  /* this probably identifi

[Qemu-devel] [PULL v1 2/5] qemu-char: convert to use error checked base64 decode

[Daniel P. Berrange]

Switch from using g_base64_decode over to qbase64_decode
in order to get error checking of the base64 input data.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qapi-schema.json | 2 --
 qemu-char.c  | 8 +++-
 qmp-commands.hx  | 2 --
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 516b145..2e31733 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -295,8 +295,6 @@
 # @format: #optional data encoding (default 'utf8').
 #  - base64: data must be base64 encoded text.  Its binary
 #decoding gets written.
-#Bug: invalid base64 is currently not rejected.
-#Whitespace *is* invalid.
 #  - utf8: data's UTF-8 encoding is written
 #  - data itself is always Unicode regardless of format, like
 #any other string.
diff --git a/qemu-char.c b/qemu-char.c
index 66703e3..00a7526 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -32,6 +32,7 @@
 #include "qapi/qmp-input-visitor.h"
 #include "qapi/qmp-output-visitor.h"
 #include "qapi-visit.h"
+#include "qemu/base64.h"
 
 #include 
 #include 
@@ -3264,7 +3265,12 @@ void qmp_ringbuf_write(const char *device, const char 
*data,
 }
 
 if (has_format && (format == DATA_FORMAT_BASE64)) {
-write_data = g_base64_decode(data, &write_count);
+write_data = qbase64_decode(data, -1,
+&write_count,
+errp);
+if (!write_data) {
+return;
+}
 } else {
 write_data = (uint8_t *)data;
 write_count = strlen(data);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6f3a25d..7b235ee 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -512,8 +512,6 @@ Arguments:
 - "data": data to write (json-string)
 - "format": data format (json-string, optional)
   - Possible values: "utf8" (default), "base64"
-Bug: invalid base64 is currently not rejected.
-Whitespace *is* invalid.
 
 Example:
 
-- 
2.5.0

[Qemu-devel] [PULL v1 1/5] util: add base64 decoding function

[Daniel P. Berrange]

The standard glib provided g_base64_decode doesn't provide any
kind of sensible error checking on its input. Add a QEMU custom
wrapper qbase64_decode which can be used with untrustworthy
input that can contain invalid base64 characters, embedded
NUL characters, or not be NUL terminated at all.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 include/qemu/base64.h |  58 +++
 tests/.gitignore  |   1 +
 tests/Makefile|   3 ++
 tests/test-base64.c   | 109 ++
 util/Makefile.objs|   1 +
 util/base64.c |  60 +++
 6 files changed, 232 insertions(+)
 create mode 100644 include/qemu/base64.h
 create mode 100644 tests/test-base64.c
 create mode 100644 util/base64.c

diff --git a/include/qemu/base64.h b/include/qemu/base64.h
new file mode 100644
index 000..793708d
--- /dev/null
+++ b/include/qemu/base64.h
@@ -0,0 +1,58 @@
+/*
+ * QEMU base64 helpers
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#ifndef QEMU_BASE64_H__
+#define QEMU_BASE64_H__
+
+#include "qemu-common.h"
+
+
+/**
+ * qbase64_decode:
+ * @input: the (possibly) base64 encoded text
+ * @in_len: length of @input or -1 if NUL terminated
+ * @out_len: filled with length of decoded data
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Attempt to decode the (possibly) base64 encoded
+ * text provided in @input. If the @input text may
+ * contain embedded NUL characters, or may not be
+ * NUL terminated, then @in_len must be set to the
+ * known size of the @input buffer.
+ *
+ * Note that embedded NULs, or lack of a NUL terminator
+ * are considered invalid base64 data and errors
+ * will be reported to this effect.
+ *
+ * If decoding is successful, the decoded data will
+ * be returned and @out_len set to indicate the
+ * number of bytes in the decoded data. The caller
+ * must use g_free() to free the returned data when
+ * it is no longer required.
+ *
+ * Returns: the decoded data or NULL
+ */
+uint8_t *qbase64_decode(const char *input,
+size_t in_len,
+size_t *out_len,
+Error **errp);
+
+
+#endif /* QEMU_BUFFER_H__ */
diff --git a/tests/.gitignore b/tests/.gitignore
index 77aaba6..596fef3 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -8,6 +8,7 @@ check-qom-interface
 check-qom-proplist
 rcutorture
 test-aio
+test-base64
 test-bitops
 test-blockjob-txn
 test-coroutine
diff --git a/tests/Makefile b/tests/Makefile
index 6ff4627..3ae57b8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -90,6 +90,7 @@ check-unit-y += tests/test-io-channel-file$(EXESUF)
 check-unit-$(CONFIG_GNUTLS) += tests/test-io-channel-tls$(EXESUF)
 check-unit-y += tests/test-io-channel-command$(EXESUF)
 check-unit-y += tests/test-io-channel-buffer$(EXESUF)
+check-unit-y += tests/test-base64$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -423,6 +424,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
$(test-qom-obj-y)
 tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
$(test-util-obj-y)
+tests/test-base64$(EXESUF): tests/test-base64.o \
+   libqemuutil.a libqemustub.a
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json 
$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
diff --git a/tests/test-base64.c b/tests/test-base64.c
new file mode 100644
index 000..a0ca2d8
--- /dev/null
+++ b/tests/test-base64.c
@@ -0,0 +1,109 @@
+/*
+ * QEMU base64 helper test
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 

[Qemu-devel] [PULL 27/48] qcow2: Use error_report() in qcow2_amend_options()

[Kevin Wolf]

From: Max Reitz 

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c  | 14 ++
 tests/qemu-iotests/061.out | 14 +++---
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4be2c61..01b20d9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2964,11 +2964,11 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 } else if (!strcmp(compat, "1.1")) {
 new_version = 3;
 } else {
-fprintf(stderr, "Unknown compatibility level %s.\n", compat);
+error_report("Unknown compatibility level %s", compat);
 return -EINVAL;
 }
 } else if (!strcmp(desc->name, BLOCK_OPT_PREALLOC)) {
-fprintf(stderr, "Cannot change preallocation mode.\n");
+error_report("Cannot change preallocation mode");
 return -ENOTSUP;
 } else if (!strcmp(desc->name, BLOCK_OPT_SIZE)) {
 new_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
@@ -2981,16 +2981,14 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 !!s->cipher);
 
 if (encrypt != !!s->cipher) {
-fprintf(stderr, "Changing the encryption flag is not "
-"supported.\n");
+error_report("Changing the encryption flag is not supported");
 return -ENOTSUP;
 }
 } else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {
 cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
  cluster_size);
 if (cluster_size != s->cluster_size) {
-fprintf(stderr, "Changing the cluster size is not "
-"supported.\n");
+error_report("Changing the cluster size is not supported");
 return -ENOTSUP;
 }
 } else if (!strcmp(desc->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
@@ -3037,8 +3035,8 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 if (s->use_lazy_refcounts != lazy_refcounts) {
 if (lazy_refcounts) {
 if (s->qcow_version < 3) {
-fprintf(stderr, "Lazy refcounts only supported with 
compatibility "
-"level 1.1 and above (use compat=1.1 or greater)\n");
+error_report("Lazy refcounts only supported with compatibility 
"
+ "level 1.1 and above (use compat=1.1 or 
greater)");
 return -EINVAL;
 }
 s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index f2598a8..57aae28 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -291,18 +291,18 @@ No errors were found on the image.
 === Testing invalid configurations ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Lazy refcounts only supported with compatibility level 1.1 and above (use 
compat=1.1 or greater)
+qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above 
(use compat=1.1 or greater)
 qemu-img: Error while amending options: Invalid argument
-Lazy refcounts only supported with compatibility level 1.1 and above (use 
compat=1.1 or greater)
+qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above 
(use compat=1.1 or greater)
 qemu-img: Error while amending options: Invalid argument
-Unknown compatibility level 0.42.
+qemu-img: Unknown compatibility level 0.42
 qemu-img: Error while amending options: Invalid argument
 qemu-img: Invalid parameter 'foo'
-Changing the cluster size is not supported.
+qemu-img: Changing the cluster size is not supported
 qemu-img: Error while amending options: Operation not supported
-Changing the encryption flag is not supported.
+qemu-img: Changing the encryption flag is not supported
 qemu-img: Error while amending options: Operation not supported
-Cannot change preallocation mode.
+qemu-img: Cannot change preallocation mode
 qemu-img: Error while amending options: Operation not supported
 
 === Testing correct handling of unset value ===
@@ -310,7 +310,7 @@ qemu-img: Error while amending options: Operation not 
supported
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 Should work:
 Should not work:
-Changing the cluster size is not supported.
+qemu-img: Changing the cluster size is not supported
 qemu-img: Error while amending options: Operation not supported
 
 === Testing zero expansion on inactive clusters ===
-- 
1.8.3.1

[Qemu-devel] [PULL 11/48] block: Keep "driver" in bs->options

[Kevin Wolf]

Instead of passing a separate drv argument to bdrv_open_common(), just
make sure that a "driver" option is set in the QDict. This also means
that a "driver" entry is consistently present in bs->options now.

This is another step towards keeping all options in the QDict (which is
the represenation of the blockdev-add QMP command).

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 57 -
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index c204d13..2c41fff 100644
--- a/block.c
+++ b/block.c
@@ -817,6 +817,11 @@ static QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "Node name of the block device node",
 },
+{
+.name = "driver",
+.type = QEMU_OPT_STRING,
+.help = "Block driver to use for the node",
+},
 { /* end of list */ }
 },
 };
@@ -827,18 +832,31 @@ static QemuOptsList bdrv_runtime_opts = {
  * Removes all processed options from *options.
  */
 static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
-QDict *options, int flags, BlockDriver *drv, Error **errp)
+QDict *options, int flags, Error **errp)
 {
 int ret, open_flags;
 const char *filename;
+const char *driver_name = NULL;
 const char *node_name = NULL;
 QemuOpts *opts;
+BlockDriver *drv;
 Error *local_err = NULL;
 
-assert(drv != NULL);
 assert(bs->file == NULL);
 assert(options != NULL && bs->options != options);
 
+opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+qemu_opts_absorb_qdict(opts, options, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail_opts;
+}
+
+driver_name = qemu_opt_get(opts, "driver");
+drv = bdrv_find_format(driver_name);
+assert(drv != NULL);
+
 if (file != NULL) {
 filename = file->bs->filename;
 } else {
@@ -848,19 +866,12 @@ static int bdrv_open_common(BlockDriverState *bs, 
BdrvChild *file,
 if (drv->bdrv_needs_filename && !filename) {
 error_setg(errp, "The '%s' block driver requires a file name",
drv->format_name);
-return -EINVAL;
-}
-
-trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
-
-opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
 ret = -EINVAL;
 goto fail_opts;
 }
 
+trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
+
 node_name = qemu_opt_get(opts, "node-name");
 bdrv_assign_node_name(bs, node_name, &local_err);
 if (local_err) {
@@ -1477,11 +1488,14 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 goto fail;
 }
 
+bs->open_flags = flags;
+bs->options = options;
+options = qdict_clone_shallow(options);
+
 /* Find the right image format driver */
 drvname = qdict_get_try_str(options, "driver");
 if (drvname) {
 drv = bdrv_find_format(drvname);
-qdict_del(options, "driver");
 if (!drv) {
 error_setg(errp, "Unknown driver: '%s'", drvname);
 ret = -EINVAL;
@@ -1497,10 +1511,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 qdict_del(options, "backing");
 }
 
-bs->open_flags = flags;
-bs->options = options;
-options = qdict_clone_shallow(options);
-
 /* Open image file without format layer */
 if ((flags & BDRV_O_PROTOCOL) == 0) {
 if (flags & BDRV_O_RDWR) {
@@ -1528,6 +1538,19 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 if (ret < 0) {
 goto fail;
 }
+/*
+ * This option update would logically belong in bdrv_fill_options(),
+ * but we first need to open bs->file for the probing to work, while
+ * opening bs->file already requires the (mostly) final set of options
+ * so that cache mode etc. can be inherited.
+ *
+ * Adding the driver later is somewhat ugly, but it's not an option
+ * that would ever be inherited, so it's correct. We just need to make
+ * sure to update both bs->options (which has the full effective
+ * options for bs) and options (which has file.* already removed).
+ */
+qdict_put(bs->options, "driver", qstring_from_str(drv->format_name));
+qdict_put(options, "driver", qstring_from_str(drv->format_name));
 } else if (!drv) {
 error_setg(errp, "Must specify either driver or file");
 ret = -EINVAL;
@@ -1541,7 +1564,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 assert(!(flags & BDRV_O_PROTOCOL) || !file

[Qemu-devel] [PULL 2/2] xenfb: avoid reading twice the same fields from the shared page

[Stefano Stabellini]

Reading twice the same field could give the guest an attack of
opportunity. In the case of event->type, gcc could compile the switch
statement into a jump table, effectively ending up reading the type
field multiple times.

This is part of XSA-155.

Signed-off-by: Stefano Stabellini 
---
 hw/display/xenfb.c |   10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 5e324ef..4e2a27a 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -784,18 +784,20 @@ static void xenfb_invalidate(void *opaque)
 
 static void xenfb_handle_events(struct XenFB *xenfb)
 {
-uint32_t prod, cons;
+uint32_t prod, cons, out_cons;
 struct xenfb_page *page = xenfb->c.page;
 
 prod = page->out_prod;
-if (prod == page->out_cons)
+out_cons = page->out_cons;
+if (prod == out_cons)
return;
 xen_rmb(); /* ensure we see ring contents up to prod */
-for (cons = page->out_cons; cons != prod; cons++) {
+for (cons = out_cons; cons != prod; cons++) {
union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
+uint8_t type = event->type;
int x, y, w, h;
 
-   switch (event->type) {
+   switch (type) {
case XENFB_TYPE_UPDATE:
if (xenfb->up_count == UP_QUEUE)
xenfb->up_fullscreen = 1;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] linux-user, sh4: fix signal retcode address

[Laurent Vivier]

Ping ?

Le 23/11/2015 11:38, Laurent Vivier a écrit :
> To return from a signal, setup_frame() puts an instruction to
> be executed in the stack. This sequence calls the syscall sigreturn().
> 
> The address of the instruction must be set in the PR register
> to be executed.
> 
> This patch fixes this: the current code sets the register to the address
> of the instruction in the host address space (which can be 64bit whereas
> PR is only 32bit), but the virtual CPU can't access this address space,
> so we put in PR the address of the instruction in the guest address space.
> 
> This patch also removes an useless variable (ret) in the modified functions.
> 
> Signed-off-by: Laurent Vivier 
> ---
>  linux-user/signal.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 55e5405..5e8f6d8 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -3215,7 +3215,6 @@ static void setup_frame(int sig, struct 
> target_sigaction *ka,
>  struct target_sigframe *frame;
>  abi_ulong frame_addr;
>  int i;
> -int err = 0;
>  
>  frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
>  if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> @@ -3233,15 +3232,14 @@ static void setup_frame(int sig, struct 
> target_sigaction *ka,
>  regs->pr = (unsigned long) ka->sa_restorer;
>  } else {
>  /* Generate return code (system call to sigreturn) */
> +abi_ulong retcode_addr = frame_addr +
> + offsetof(struct target_sigframe, retcode);
>  __put_user(MOVW(2), &frame->retcode[0]);
>  __put_user(TRAP_NOARG, &frame->retcode[1]);
>  __put_user((TARGET_NR_sigreturn), &frame->retcode[2]);
> -regs->pr = (unsigned long) frame->retcode;
> +regs->pr = (unsigned long) retcode_addr;
>  }
>  
> -if (err)
> -goto give_sigsegv;
> -
>  /* Set up registers for signal handler */
>  regs->gregs[15] = frame_addr;
>  regs->gregs[4] = sig; /* Arg for signal handler */
> @@ -3264,7 +3262,6 @@ static void setup_rt_frame(int sig, struct 
> target_sigaction *ka,
>  struct target_rt_sigframe *frame;
>  abi_ulong frame_addr;
>  int i;
> -int err = 0;
>  
>  frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
>  if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> @@ -3293,15 +3290,14 @@ static void setup_rt_frame(int sig, struct 
> target_sigaction *ka,
>  regs->pr = (unsigned long) ka->sa_restorer;
>  } else {
>  /* Generate return code (system call to sigreturn) */
> +abi_ulong retcode_addr = frame_addr +
> + offsetof(struct target_rt_sigframe, 
> retcode);
>  __put_user(MOVW(2), &frame->retcode[0]);
>  __put_user(TRAP_NOARG, &frame->retcode[1]);
>  __put_user((TARGET_NR_rt_sigreturn), &frame->retcode[2]);
> -regs->pr = (unsigned long) frame->retcode;
> +regs->pr = (unsigned long) retcode_addr;
>  }
>  
> -if (err)
> -goto give_sigsegv;
> -
>  /* Set up registers for signal handler */
>  regs->gregs[15] = frame_addr;
>  regs->gregs[4] = sig; /* Arg for signal handler */
> 

[Qemu-devel] [PULL 13/48] block: reopen: Document option precedence and refactor accordingly

[Kevin Wolf]

The interesting part of reopening an image is from which sources the
effective options should be taken, i.e. which options take precedence
over which other options. This patch documents the precedence that will
be implemented in the following patches.

It also refactors bdrv_reopen_queue(), so that the top-level reopened
node is handled the same way as children are. Option/flag inheritance
from the parent becomes just one item in the list and is done at the
beginning of the function, similar to how the other items are/will be
handled.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 39 +--
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 9c42b17..5fd6c44 100644
--- a/block.c
+++ b/block.c
@@ -1693,9 +1693,13 @@ typedef struct BlockReopenQueueEntry {
  * bs_queue, or the existing bs_queue being used.
  *
  */
-BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs,
-QDict *options, int flags)
+static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
+ BlockDriverState *bs,
+ QDict *options,
+ int flags,
+ const BdrvChildRole *role,
+ QDict *parent_options,
+ int parent_flags)
 {
 assert(bs != NULL);
 
@@ -1712,6 +1716,22 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 options = qdict_new();
 }
 
+/*
+ * Precedence of options:
+ * 1. Explicitly passed in options (highest)
+ * 2. TODO Set in flags (only for top level)
+ * 3. TODO Retained from explicitly set options of bs
+ * 4. TODO Inherited from parent node
+ * 5. Retained from effective options of bs
+ */
+
+/* Inherit from parent node */
+if (parent_options) {
+assert(!flags);
+flags = role->inherit_flags(parent_flags);
+}
+
+/* Old values are used for options that aren't set yet */
 old_options = qdict_clone_shallow(bs->options);
 bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
@@ -1722,7 +1742,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 QLIST_FOREACH(child, &bs->children, next) {
 QDict *new_child_options;
 char *child_key_dot;
-int child_flags;
 
 /* reopen can only change the options of block devices that were
  * implicitly created and inherited options. For other (referenced)
@@ -1735,8 +1754,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 qdict_extract_subqdict(options, &new_child_options, child_key_dot);
 g_free(child_key_dot);
 
-child_flags = child->role->inherit_flags(flags);
-bdrv_reopen_queue(bs_queue, child->bs, new_child_options, child_flags);
+bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
+child->role, options, flags);
 }
 
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
@@ -1749,6 +1768,14 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 return bs_queue;
 }
 
+BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
+BlockDriverState *bs,
+QDict *options, int flags)
+{
+return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
+   NULL, NULL, 0);
+}
+
 /*
  * Reopen multiple BlockDriverStates atomically & transactionally.
  *
-- 
1.8.3.1

[Qemu-devel] [PULL 07/48] block: Allow references for backing files

[Kevin Wolf]

For bs->file, using references to existing BDSes has been possible for a
while already. This patch enables the same for bs->backing.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c   | 48 +---
 block/mirror.c|  2 +-
 include/block/block.h |  3 ++-
 3 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 57bf8fc..84b9bf4 100644
--- a/block.c
+++ b/block.c
@@ -1182,30 +1182,43 @@ out:
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
- * options is a QDict of options to pass to the block drivers, or NULL for an
- * empty set of options. The reference to the QDict is transferred to this
- * function (even on failure), so if the caller intends to reuse the 
dictionary,
- * it needs to use QINCREF() before calling bdrv_file_open.
+ * bdref_key specifies the key for the image's BlockdevRef in the options 
QDict.
+ * That QDict has to be flattened; therefore, if the BlockdevRef is a QDict
+ * itself, all options starting with "${bdref_key}." are considered part of the
+ * BlockdevRef.
+ *
+ * TODO Can this be unified with bdrv_open_image()?
  */
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
+   const char *bdref_key, Error **errp)
 {
 char *backing_filename = g_malloc0(PATH_MAX);
+char *bdref_key_dot;
+const char *reference = NULL;
 int ret = 0;
 BlockDriverState *backing_hd;
+QDict *options;
+QDict *tmp_parent_options = NULL;
 Error *local_err = NULL;
 
 if (bs->backing != NULL) {
-QDECREF(options);
 goto free_exit;
 }
 
 /* NULL means an empty set of options */
-if (options == NULL) {
-options = qdict_new();
+if (parent_options == NULL) {
+tmp_parent_options = qdict_new();
+parent_options = tmp_parent_options;
 }
 
 bs->open_flags &= ~BDRV_O_NO_BACKING;
-if (qdict_haskey(options, "file.filename")) {
+
+bdref_key_dot = g_strdup_printf("%s.", bdref_key);
+qdict_extract_subqdict(parent_options, &options, bdref_key_dot);
+g_free(bdref_key_dot);
+
+reference = qdict_get_try_str(parent_options, bdref_key);
+if (reference || qdict_haskey(options, "file.filename")) {
 backing_filename[0] = '\0';
 } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
 QDECREF(options);
@@ -1228,19 +1241,16 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 goto free_exit;
 }
 
-backing_hd = bdrv_new();
-
 if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
 qdict_put(options, "driver", qstring_from_str(bs->backing_format));
 }
 
-assert(bs->backing == NULL);
+backing_hd = NULL;
 ret = bdrv_open_inherit(&backing_hd,
 *backing_filename ? backing_filename : NULL,
-NULL, options, 0, bs, &child_backing, &local_err);
+reference, options, 0, bs, &child_backing,
+&local_err);
 if (ret < 0) {
-bdrv_unref(backing_hd);
-backing_hd = NULL;
 bs->open_flags |= BDRV_O_NO_BACKING;
 error_setg(errp, "Could not open backing file: %s",
error_get_pretty(local_err));
@@ -1253,8 +1263,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 bdrv_set_backing_hd(bs, backing_hd);
 bdrv_unref(backing_hd);
 
+qdict_del(parent_options, bdref_key);
+
 free_exit:
 g_free(backing_filename);
+QDECREF(tmp_parent_options);
 return ret;
 }
 
@@ -1537,10 +1550,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 
 /* If there is a backing file, use it */
 if ((flags & BDRV_O_NO_BACKING) == 0) {
-QDict *backing_options;
-
-qdict_extract_subqdict(options, &backing_options, "backing.");
-ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+ret = bdrv_open_backing_file(bs, options, "backing", &local_err);
 if (ret < 0) {
 goto close_and_fail;
 }
diff --git a/block/mirror.c b/block/mirror.c
index 8e3f524..fc34a9c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -652,7 +652,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
 Error *local_err = NULL;
 int ret;
 
-ret = bdrv_open_backing_file(s->target, NULL, &local_err);
+ret = bdrv_open_backing_file(s->target, NULL, "backing", &local_err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 return;
diff --git a/include/block/block.h b/include/block/block.h
index d048bbf..2e77288 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -210,7 +210,8 @@ BdrvChild *bdrv_open_child(const char *filename,
co

[Qemu-devel] [PULL 36/48] raw-posix: Make aio=native option binding

[Kevin Wolf]

Traditionally, aio=native was treated as an advice that could simply be
ignored if an error occurs while initialising Linux AIO or the feature
wasn't compiled in. This behaviour was deprecated in commit 96518254
(qemu 2.3; error during init) and commit 1501ecc1 (qemu 2.5; not
compiled in).

This patch changes raw-posix to error out in these cases instead of
printing a deprecation warning.

Signed-off-by: Kevin Wolf 
Acked-by: Christian Borntraeger 
Reviewed-by: Stefan Hajnoczi 
---
 block/raw-posix.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ffeebe1..076d070 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -500,21 +500,17 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 goto fail;
 }
 if (!s->use_aio && (bdrv_flags & BDRV_O_NATIVE_AIO)) {
-error_printf("WARNING: aio=native was specified for '%s', but "
- "it requires cache.direct=on, which was not "
- "specified. Falling back to aio=threads.\n"
- " This will become an error condition in "
- "future QEMU versions.\n",
- bs->filename);
+error_setg(errp, "aio=native was specified, but it requires "
+ "cache.direct=on, which was not specified.");
+ret = -EINVAL;
+goto fail;
 }
 #else
 if (bdrv_flags & BDRV_O_NATIVE_AIO) {
-error_printf("WARNING: aio=native was specified for '%s', but "
- "is not supported in this build. Falling back to "
- "aio=threads.\n"
- " This will become an error condition in "
- "future QEMU versions.\n",
- bs->filename);
+error_setg(errp, "aio=native was specified, but is not supported "
+ "in this build.");
+ret = -EINVAL;
+goto fail;
 }
 #endif /* !defined(CONFIG_LINUX_AIO) */
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 11/24] error: Use error_reportf_err() where it makes obvious sense

[Markus Armbruster]

Done with this Coccinelle semantic patch

@@
expression FMT, E, S;
expression list ARGS;
@@
-error_report(FMT, ARGS, error_get_pretty(E));
+error_reportf_err(E, FMT/*@@@*/, ARGS);
(
-error_free(E);
|
 exit(S);
|
 abort();
)

followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
because I can't figure out how to make Coccinelle transform strings.

We now use the error whole instead of just its message obtained with
error_get_pretty().  This avoids suppressing its hint (see commit
50b7b00), but I can't see how the errors touched in this commit could
come with hints.

Signed-off-by: Markus Armbruster 
---
 arch_init.c   |  4 +---
 block/sheepdog.c  |  5 ++---
 blockdev.c| 12 +---
 hw/arm/cubieboard.c   |  9 -
 hw/arm/digic_boards.c |  3 +--
 hw/core/qdev-properties.c |  6 ++
 hw/core/qdev.c|  5 ++---
 hw/i386/pc.c  |  5 ++---
 hw/ppc/e500.c |  4 ++--
 hw/usb/bus.c  |  5 ++---
 qemu-img.c| 33 +
 qemu-nbd.c| 11 +--
 replay/replay.c   |  3 +--
 tests/test-aio.c  |  4 +---
 tests/test-thread-pool.c  |  4 +---
 ui/vnc.c  |  4 +---
 vl.c  |  6 ++
 17 files changed, 47 insertions(+), 76 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 38f5fb9..d1383b3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
 
 acpi_table_add(opts, &err);
 if (err) {
-error_report("Wrong acpi table provided: %s",
- error_get_pretty(err));
-error_free(err);
+error_reportf_err(err, "Wrong acpi table provided: ");
 exit(1);
 }
 #endif
diff --git a/block/sheepdog.c b/block/sheepdog.c
index dd8301b..6986be8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2405,9 +2405,8 @@ static int sd_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
 
 ret = do_sd_create(s, &new_vid, 1, &local_err);
 if (ret < 0) {
-error_report("failed to create inode for snapshot: %s",
- error_get_pretty(local_err));
-error_free(local_err);
+error_reportf_err(local_err,
+  "failed to create inode for snapshot: ");
 goto cleanup;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 13eaa77..f93a6ee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1583,13 +1583,11 @@ static void internal_snapshot_abort(BlkActionState 
*common)
 }
 
 if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
-error_report("Failed to delete snapshot with id '%s' and name '%s' on "
- "device '%s' in abort: %s",
- sn->id_str,
- sn->name,
- bdrv_get_device_name(bs),
- error_get_pretty(local_error));
-error_free(local_error);
+error_reportf_err(local_error,
+  "Failed to delete snapshot with id '%s' and "
+  "name '%s' on device '%s' in abort: ",
+  sn->id_str, sn->name,
+  bdrv_get_device_name(bs));
 }
 }
 
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index bf068cd..a71e43c 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -39,27 +39,26 @@ static void cubieboard_init(MachineState *machine)
 
 object_property_set_int(OBJECT(&s->a10->emac), 1, "phy-addr", &err);
 if (err != NULL) {
-error_report("Couldn't set phy address: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set phy address: ");
 exit(1);
 }
 
 object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
 if (err != NULL) {
-error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set clk0 frequency: ");
 exit(1);
 }
 
 object_property_set_int(OBJECT(&s->a10->timer), 2400, "clk1-freq",
 &err);
 if (err != NULL) {
-error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
+error_reportf_err(err, "Couldn't set clk1 frequency: ");
 exit(1);
 }
 
 object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
 if (err != NULL) {
-error_report("Couldn't realize Allwinner A10: %s",
- error_get_pretty(err));
+error_reportf_err(err, "Couldn't realize Allwinner A10: ");
 exit(1);
 }
 
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 710045a..dfaed25 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -64,8 +64,7 @@ static void digic4_board_init(DigicBoard *board)
 s->digic = DIGIC(object_new(TYPE_DIGIC));
 o

Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage

[Daniel P. Berrange]

On Fri, Dec 18, 2015 at 01:43:19PM +, Peter Maydell wrote:
> On 18 December 2015 at 13:41, Paolo Bonzini  wrote:
> >
> >
> > On 18/12/2015 14:35, Dr. David Alan Gilbert wrote:
> >> > That would not be recursive make, but rather a completely separate
> >> > Makefile to be manually invoked with -f.
> >>
> >> Hmm but wouldn't this Makefile also be a good place for small-fast
> >> style check scripts that could be included in  make check  ?
> >
> > Yes, but there is no benefit from inclusion, since these tests do not
> > depend on anything having been built already.
> 
> In particular, if it doesn't require a preceding configure or
> build I can just run it once as part of my merge testing rather
> than farming it out to be run in parallel on five machines and
> ten different configs :-)

Ok, yeah, that makes total sense - it certainly shouldn't depend on
any configure / build step.

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

[Qemu-devel] [PATCH v3 21/24] error: Clean up errors with embedded newlines (again)

[Markus Armbruster]

The arguments of error_report() should yield a short error string
without newlines.

A few places try to print additional help after the error message by
embedding newlines in the error string.  That's nice, but let's do it
the right way.  Commit 474c213 cleaned up some, but they keep coming
back.  Offenders tracked down with the Coccinelle semantic patch from
commit 312fd5f.

Cc: Laszlo Ersek 
Cc: Pavel Fedin 
Signed-off-by: Markus Armbruster 
Reviewed-by: Laszlo Ersek 
Reviewed-by: Eric Blake 
Signed-off-by: Markus Armbruster 
---
 hw/i386/pc.c | 4 ++--
 kvm-all.c| 6 +++---
 qemu-nbd.c   | 5 ++---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ba7b414..f41c448 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -419,8 +419,8 @@ static void pc_cmos_init_late(void *opaque)
 
 if (state.multiple) {
 error_report("warning: multiple floppy disk controllers with "
- "iobase=0x3f0 have been found;\n"
- "the one being picked for CMOS setup might not reflect "
+ "iobase=0x3f0 have been found");
+error_printf("the one being picked for CMOS setup might not reflect "
  "your intent");
 }
 pc_cmos_init_floppy(s, state.floppy);
diff --git a/kvm-all.c b/kvm-all.c
index bd9e764..9148889 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2063,9 +2063,9 @@ void kvm_device_access(int fd, int group, uint64_t attr,
write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
&kvmattr);
 if (err < 0) {
-error_report("KVM_%s_DEVICE_ATTR failed: %s\n"
- "Group %d attr 0x%016" PRIx64, write ? "SET" : "GET",
- strerror(-err), group, attr);
+error_report("KVM_%s_DEVICE_ATTR failed: %s",
+ write ? "SET" : "GET", strerror(-err));
+error_printf("Group %d attr 0x%016" PRIx64, group, attr);
 abort();
 }
 }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 023eacd..a4cf847 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -599,9 +599,8 @@ int main(int argc, char **argv)
 }
 
 if ((argc - optind) != 1) {
-error_report("Invalid number of argument.\n"
- "Try `%s --help' for more information.",
- argv[0]);
+error_report("Invalid number of arguments");
+error_printf("Try `%s --help' for more information.\n", argv[0]);
 exit(EXIT_FAILURE);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH COLO-Frame v12 21/38] COLO failover: Introduce a new command to trigger a failover

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
> We leave users to choose whatever heartbeat solution they want, if the 
> heartbeat
> is lost, or other errors they detect, they can use experimental command
> 'x_colo_lost_heartbeat' to tell COLO to do failover, COLO will do operations
> accordingly.
> 
> For example, if the command is sent to the PVM, the Primary side will
> exit COLO mode and take over operation. If sent to the Secondary, the
> secondary will run failover work, then take over server operation to
> become the new Primary.
> 
> Cc: Luiz Capitulino 
> Cc: Eric Blake 
> Cc: Markus Armbruster 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Li Zhijian 

Reviewed-by: Dr. David Alan Gilbert 

> ---
> v11:
> - Add more comments for x-colo-lost-heartbeat command (Eric's suggestion)
> - Return 'enum' instead of 'int' for get_colo_mode() (Eric's suggestion)
> v10:
> - Rename command colo_lost_hearbeat to experimental 'x_colo_lost_heartbeat'
> 
> Signed-off-by: zhanghailiang 
> ---
>  hmp-commands.hx  | 15 +++
>  hmp.c|  8 
>  hmp.h|  1 +
>  include/migration/colo.h |  3 +++
>  include/migration/failover.h | 20 
>  migration/Makefile.objs  |  2 +-
>  migration/colo-comm.c| 11 +++
>  migration/colo-failover.c| 41 +
>  migration/colo.c |  1 +
>  qapi-schema.json | 29 +
>  qmp-commands.hx  | 19 +++
>  stubs/migration-colo.c   |  8 
>  12 files changed, 157 insertions(+), 1 deletion(-)
>  create mode 100644 include/migration/failover.h
>  create mode 100644 migration/colo-failover.c
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb52e4d..a381b0b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1039,6 +1039,21 @@ migration (or once already in postcopy).
>  ETEXI
>  
>  {
> +.name   = "x_colo_lost_heartbeat",
> +.args_type  = "",
> +.params = "",
> +.help   = "Tell COLO that heartbeat is lost,\n\t\t\t"
> +  "a failover or takeover is needed.",
> +.mhandler.cmd = hmp_x_colo_lost_heartbeat,
> +},
> +
> +STEXI
> +@item x_colo_lost_heartbeat
> +@findex x_colo_lost_heartbeat
> +Tell COLO that heartbeat is lost, a failover or takeover is needed.
> +ETEXI
> +
> +{
>  .name   = "client_migrate_info",
>  .args_type  = 
> "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>  .params = "protocol hostname port tls-port cert-subject",
> diff --git a/hmp.c b/hmp.c
> index ee87d38..dc6dc30 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1310,6 +1310,14 @@ void hmp_migrate_start_postcopy(Monitor *mon, const 
> QDict *qdict)
>  hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
> +{
> +Error *err = NULL;
> +
> +qmp_x_colo_lost_heartbeat(&err);
> +hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_set_password(Monitor *mon, const QDict *qdict)
>  {
>  const char *protocol  = qdict_get_str(qdict, "protocol");
> diff --git a/hmp.h b/hmp.h
> index a8c5b5a..864a300 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -70,6 +70,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
> *qdict);
>  void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
>  void hmp_client_migrate_info(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict);
> +void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict);
>  void hmp_set_password(Monitor *mon, const QDict *qdict);
>  void hmp_expire_password(Monitor *mon, const QDict *qdict);
>  void hmp_eject(Monitor *mon, const QDict *qdict);
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index 2676c4a..ba27719 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -17,6 +17,7 @@
>  #include "migration/migration.h"
>  #include "qemu/coroutine_int.h"
>  #include "qemu/thread.h"
> +#include "qemu/main-loop.h"
>  
>  bool colo_supported(void);
>  void colo_info_mig_init(void);
> @@ -29,4 +30,6 @@ bool migration_incoming_enable_colo(void);
>  void migration_incoming_exit_colo(void);
>  void *colo_process_incoming_thread(void *opaque);
>  bool migration_incoming_in_colo_state(void);
> +
> +COLOMode get_colo_mode(void);
>  #endif
> diff --git a/include/migration/failover.h b/include/migration/failover.h
> new file mode 100644
> index 000..1785b52
> --- /dev/null
> +++ b/include/migration/failover.h
> @@ -0,0 +1,20 @@
> +/*
> + *  COarse-grain LOck-stepping Virtual Machines for Non-stop Service (COLO)
> + *  (a.k.a. Fault Tolerance or Continuous Replication)
> + *
> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
> + * Copyright (c) 2015 FUJITSU LIMITED
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * This work

Re: [Qemu-devel] [PATCH 5/6] hw/arm/sysbus-fdt: helpers for clock node generation

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger  wrote:
> Some passthrough'ed devices depend on clock nodes. Those need to be
> generated in the guest device tree. This patch introduces some helpers
> to build a clock node from information retrieved in the host device tree.
>
> - inherit_properties copies properties from a host device tree node to
>   a guest device tree node
> - fdt_build_clock_node builds a guest clock node and checks the host
>   fellow clock is a fixed one.
>
> fdt_build_clock_node will become static as soon as it gets used. A
> dummy pre-declaration is needed for compilation of this patch.
>
> Signed-off-by: Eric Auger 
>
> ---
>
> RFC -> v1:
> - use the new proto of qemu_fdt_getprop
> - remove newline in error_report
> - fix some style issues
> ---
>  hw/arm/sysbus-fdt.c | 111 
> 
>  1 file changed, 111 insertions(+)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 9d28797..c2d813b 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -21,6 +21,7 @@
>   *
>   */
>
> +#include 
>  #include "hw/arm/sysbus-fdt.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/device_tree.h"
> @@ -56,6 +57,116 @@ typedef struct NodeCreationPair {
>  int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>  } NodeCreationPair;
>
> +/* helpers */
> +
> +struct HostProperty {
> +const char *name;
> +bool optional;
> +};
> +
> +typedef struct HostProperty HostProperty;

You can combine the typedef and the struct declaration if you like.

> +
> +/**
> + * inherit_properties
> + *
> + * copies properties listed in an array from host device tree to
> + * guest device tree. If a non optional property is not found, the
> + * function self-asserts. An optional property is ignored if not found
> + * in the host device tree.
> + * @props: array of HostProperty to copy
> + * @nb_props: number of properties in the array
> + * @host_dt: host device tree blob
> + * @guest_dt: guest device tree blob
> + * @node_path: host dt node path where the property is supposed to be
> +  found
> + * @nodename: guest node name the properties should be added to
> + */
> +static void inherit_properties(HostProperty *props, int nb_props,
> +   void *host_fdt, void *guest_fdt,
> +   char *node_path, char *nodename)
> +{
> +int i, prop_len;
> +const void *r;
> +
> +for (i = 0; i < nb_props; i++) {
> +r = qemu_fdt_getprop(host_fdt, node_path,
> + props[i].name,
> + &prop_len,
> + props[i].optional ? NULL : &error_fatal);

We'll get an error here if the host device tree doesn't match
up correctly, right? Is the error message going to be sufficiently
informative to the end user about what's gone wrong? (What does
it end up looking like?)

> +if (r) {
> +qemu_fdt_setprop(guest_fdt, nodename,
> + props[i].name, r, prop_len);
> +}
> +}
> +}
> +
> +/* clock properties whose values are copied/pasted from host */
> +static HostProperty clock_inherited_properties[] = {
> +{"compatible", 0},
> +{"#clock-cells", 0},
> +{"clock-frequency", 1},
> +{"clock-output-names", 1},
> +};
> +
> +/**
> + * fdt_build_clock_node
> + *
> + * Build a guest clock node, used as a dependency from a passthrough'ed
> + * device. Most information are retrieved from the host clock node.
> + * Also check the host clock is a fixed one.
> + *
> + * @host_fdt: host device tree blob from which info are retrieved
> + * @guest_fdt: guest device tree blob where the clock node is added
> + * @host_phandle: phandle of the clock in host device tree
> + * @guest_phandle: phandle to assign to the guest node
> + */
> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> + uint32_t host_phandle,
> + uint32_t guest_phandle);
> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> + uint32_t host_phandle,
> + uint32_t guest_phandle)
> +{
> +char node_path[256];

Please don't use hardcoded fixed buffer lengths (see previous patch
review comments).

> +char *nodename;
> +const void *r;
> +int ret, prop_len;
> +
> +ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
> +if (ret <= 0) {
> +error_report("not able to locate clock handle %d in host device 
> tree",
> + host_phandle);
> +goto out;
> +}

If we're going to return to the caller in the error case rather than just
exiting here, I think we should return the error to the caller via an
Error** rather than calling error_report() directly ourselves.

> +ret = fdt_get_path(host_fdt, ret, node_path, 256);
> +if (ret < 0) {
> +error_report("not able to retrieve node path for clock handle %d",
> +