Re: [Qemu-devel] [PATCH RFC] linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is defined

[Felix Janda]

Peter Maydell wrote:
> On 30 September 2016 at 16:41, Felix Janda  wrote:
> > This fixes a compilation error with the musl c library.
> > ---
> > I don't really understand the purpose of the hack, which was
> > introduced in
> >
> > http://git.qemu.org/?p=qemu.git;a=commit;h=624f7979058b84cbf81c76d45f302ce757b213ca
> >
> > but musl does not have a separate thread library (it is included in
> > libc.so), so I doubt that the hack is applies to it.
> 
> The reason for the hack is that glibc uses SIGRTMAX (ie signal 63)
> for its own internal purposes (it uses it for between-thread
> communication to implement the posix threading APIs). If we
> didn't reverse SIGRTMIN and SIGRTMAX then both the glibc in
> the host process and the glibc in the guest process would try
> to use the same signal and the guest's threading APIs would
> break.

Thanks for explaining this.

> I'm pretty sure musl is going to need to use a signal to
> implement threads too, so I expect this hack will be
> needed there too, not just on glibc.

musl uses signal 34 (internally called SIGSYNCCALL) to implement
threads.

> It would be better to figure out how to identify the
> range of RT signals on musl...

musl has SIGRTMIN and SIGRTMAX, but these resolve to functions
__libc_current_sigrtmin() and __libc_current_sigrtmax(). The first
always returns 35, the second _NSIG-1.

But how do these matter? Isn't it only important that signal 34 gets
remapped?

Felix

Re: [Qemu-devel] [PATCH] linux-user: include for F_EXLCK and F_SHLCK

[Felix Janda]

Peter Maydell wrote:
> On 30 September 2016 at 16:39, Felix Janda  wrote:
> > The F_EXLCK and F_SHLCK fcntl lock constants are obsolete synonyms for
> > F_WRLCK and F_RDLCK.
> 
> This seems unlikely, since on for instance Alpha F_EXLCK is
> 16, F_SHLCK is 32, F_RDLCK is 1 and F_WRLCK is 2, so they're
> all distinct:
> http://lxr.free-electrons.com/source/arch/alpha/include/uapi/asm/fcntl.h#L52

Except for headers, the only use of F_EXLCK in the kernel is in
fs/cifs/file.c, where it has the same effect as F_WRLCK (except
for a debug message). Similarly for F_SHLCK and F_RDLCK.

Thanks,
Felix

Re: [Qemu-devel] [PATCH v7 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[ashish mittal]

Hi Stefan, others,

Thank you for all the review comments.

On Wed, Sep 28, 2016 at 4:13 AM, Stefan Hajnoczi  wrote:
> On Tue, Sep 27, 2016 at 09:09:49PM -0700, Ashish Mittal wrote:
>> +vxhs_bdrv_init(const char c) "Registering VxHS AIO driver%c"
>
> Why do several trace events have a %c format specifier at the end and it
> always takes a '.' value?
>
>> +#define QNIO_CONNECT_TIMOUT_SECS120
>
> This isn't used and there is a typo (s/TIMOUT/TIMEOUT/).  Can it be
> dropped?
>
>> +static int32_t
>> +vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcode, int64_t 
>> *in,
>> +void *ctx, uint32_t flags)
>> +{
>> +int   ret = 0;
>> +
>> +switch (opcode) {
>> +case VDISK_STAT:
>
> It seems unnecessary to abstract the iio_ioctl() constants and then have
> a switch statement to translate to the actual library constants.  It
> makes little sense since the flags argument already uses the library
> constants.  Just use the library's constants.
>
>> +ret = iio_ioctl(apictx, rfd, IOR_VDISK_STAT,
>> + in, ctx, flags);
>> +break;
>> +
>> +case VDISK_AIO_FLUSH:
>> +ret = iio_ioctl(apictx, rfd, IOR_VDISK_FLUSH,
>> + in, ctx, flags);
>> +break;
>> +
>> +case VDISK_CHECK_IO_FAILOVER_READY:
>> +ret = iio_ioctl(apictx, rfd, IOR_VDISK_CHECK_IO_FAILOVER_READY,
>> + in, ctx, flags);
>> +break;
>> +
>> +default:
>> +ret = -ENOTSUP;
>> +break;
>> +}
>> +
>> +if (ret) {
>> +*in = 0;
>
> Some callers pass in = NULL so this will crash.
>
> The naming seems wrong: this is an output argument, not an input
> argument.  Please call it "out_val" or similar.
>
>> +res = vxhs_reopen_vdisk(s, s->vdisk_ask_failover_idx);
>> +if (res == 0) {
>> +res = vxhs_qnio_iio_ioctl(s->qnio_ctx,
>> +  s->vdisk_hostinfo[s->vdisk_ask_failover_idx].vdisk_rfd,
>> +  VDISK_CHECK_IO_FAILOVER_READY, NULL, s, flags);
>
> Looking at iio_ioctl(), I'm not sure how this can ever work.  The fourth
> argument is NULL and iio_ioctl() will attempt *vdisk_size = 0 so this
> will crash.
>
> Do you have tests that exercise this code path?
>

You are right. This bug crept in to the FAILOVER path when I moved the
qnio shim code to qemu. Earlier code in libqnio did *in = 0 on a
per-case basis and skipped it for VDISK_CHECK_IO_FAILOVER_READY. I
will fix this.

We do thoroughly test these code paths, but the problem is that the
existing tests do not fully work with the new changes I am doing. I do
not yet have test case to test failover with the latest code. I do
frequently test using qemu-io (open a vdisk, read, write and re-read
to check written data) and also try to bring up an existing guest VM
using latest qemu-system-x86_64 binary to make sure I don't regress
main functionality. I did not however run these tests for v7 patch,
therefore some of the v7 changes do break the code. This patch had
some major code reconfiguration over v6, therefore my intention was to
just get a feel for whether the main code structure looks good.

A lot of changes have been proposed. I will discuss these with the
team and get back with inputs. I guess having a test framework is
really important at this time.

Regards,
Ashish

On Fri, Sep 30, 2016 at 1:36 AM, Stefan Hajnoczi  wrote:
> On Tue, Sep 27, 2016 at 09:09:49PM -0700, Ashish Mittal wrote:
>> This patch adds support for a new block device type called "vxhs".
>> Source code for the library that this code loads can be downloaded from:
>> https://github.com/MittalAshish/libqnio.git
>
> The QEMU block driver should deal with BlockDriver<->libqnio integration
> and libqnio should deal with vxhs logic (network protocol, failover,
> etc).  Right now the vxhs logic is spread between both components.  If
> responsibilities aren't cleanly separated between QEMU and libqnio then
> I see no point in having libqnio.
>
> Failover code should move into libqnio so that programs using libqnio
> avoid duplicating the failover code.
>
> Similarly IIO_IO_BUF_SIZE/segments should be handled internally by
> libqnio so programs using libqnio do not duplicate this code.
>
> libqnio itself can be simplified significantly:
>
> The multi-threading is not necessary and adds complexity.  Right now
> there seem to be two reasons for multi-threading: shared contexts and
> the epoll thread.  Both can be eliminated as follows.
>
> Shared contexts do not make sense in a multi-disk, multi-core
> environment.  Why is it advantages to tie disks to a single context?
> It's simpler and more multi-core friendly to let every disk have its own
> connection.
>
> The epoll thread forces library users to use thread synchronization when
> processing callbacks.  Look at libiscsi for an example of how to
> eliminate it.  Two APIs are defined: int 

Re: [Qemu-devel] [PATCH] linux-user: include for F_EXLCK and F_SHLCK

[Peter Maydell]

On 30 September 2016 at 16:39, Felix Janda  wrote:
> The F_EXLCK and F_SHLCK fcntl lock constants are obsolete synonyms for
> F_WRLCK and F_RDLCK.

This seems unlikely, since on for instance Alpha F_EXLCK is
16, F_SHLCK is 32, F_RDLCK is 1 and F_WRLCK is 2, so they're
all distinct:
http://lxr.free-electrons.com/source/arch/alpha/include/uapi/asm/fcntl.h#L52

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: use libc wrapper instead of direct mremap syscall

[Peter Maydell]

On 30 September 2016 at 16:39, Felix Janda  wrote:
> This commit essentially reverts commit
> 3af72a4d98dca033492102603734cbc63cd2694a, which has replaced
> five-argument calls to mremap() by direct mremap syscalls for
> compatibility with glibc older than version 2.4.
>
> The direct syscall was buggy for 64bit targets on 32bit hosts
> because of the default integer type promotions. Since glibc-2.4
> is now a decade old, we can remove this workaround.
>
> Signed-off-by: Felix Janda 


Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC] linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is defined

[Peter Maydell]

On 30 September 2016 at 16:41, Felix Janda  wrote:
> This fixes a compilation error with the musl c library.
> ---
> I don't really understand the purpose of the hack, which was
> introduced in
>
> http://git.qemu.org/?p=qemu.git;a=commit;h=624f7979058b84cbf81c76d45f302ce757b213ca
>
> but musl does not have a separate thread library (it is included in
> libc.so), so I doubt that the hack is applies to it.

The reason for the hack is that glibc uses SIGRTMAX (ie signal 63)
for its own internal purposes (it uses it for between-thread
communication to implement the posix threading APIs). If we
didn't reverse SIGRTMIN and SIGRTMAX then both the glibc in
the host process and the glibc in the guest process would try
to use the same signal and the guest's threading APIs would
break.

I'm pretty sure musl is going to need to use a signal to
implement threads too, so I expect this hack will be
needed there too, not just on glibc.

It would be better to figure out how to identify the
range of RT signals on musl...


> ---
>  linux-user/signal.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index c750053..c89f83d 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -73,12 +73,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>  [SIGPWR] = TARGET_SIGPWR,
>  [SIGSYS] = TARGET_SIGSYS,
>  /* next signals stay the same */
> +#ifdef __SIGRTMIN
>  /* Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
> host libpthread signals.  This assumes no one actually uses SIGRTMAX 
> :-/
> To fix this properly we need to do manual signal delivery multiplexed
> over a single host signal.  */
>  [__SIGRTMIN] = __SIGRTMAX,
>  [__SIGRTMAX] = __SIGRTMIN,
> +#endif
>  };
>  static uint8_t target_to_host_signal_table[_NSIG];
>
> --
> 2.7.3
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar Iglesias]

Thanks :-)

Edgar

---

Sent from my phone

 Original Message 

Subject: Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of 
iss_sf in disas_ld_lit

From: Peter Maydell 

Date: 1 Oct 2016, 01:20

To: Edgar Iglesias 

On 30 September 2016 at 14:42, Edgar Iglesias  wrote:
> Peter, feel free to change the syntax to whatever you feel is best.

Applied to target-arm.next, thanks. I picked "iss_sf = opc != 0;"

-- PMM


This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

Re: [Qemu-devel] [PATCH RFC] linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is defined

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20160930234101.GD5887@nyan
Subject: [Qemu-devel] [PATCH RFC] linux-user: peform __SIGRTMIN hack only when 
__SIGRTMIN is defined

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20160930234021.GC5887@nyan -> 
patchew/20160930234021.GC5887@nyan
 * [new tag] patchew/20160930234101.GD5887@nyan -> 
patchew/20160930234101.GD5887@nyan
Switched to a new branch 'test'
5a29f11 linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is defined

=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is 
defined...
WARNING: architecture specific defines should be avoided
#18: FILE: linux-user/signal.c:76:
+#ifdef __SIGRTMIN

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 14 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] linux-user: include instead of

[Felix Janda]

This removes the last usage of  in the code base.

Signed-off-by: Felix Janda 
---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3d1f694..4bfb671 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -42,7 +42,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
-- 
2.7.3

[Qemu-devel] [PATCH RFC] linux-user: peform __SIGRTMIN hack only when __SIGRTMIN is defined

[Felix Janda]

This fixes a compilation error with the musl c library.
---
I don't really understand the purpose of the hack, which was
introduced in

http://git.qemu.org/?p=qemu.git;a=commit;h=624f7979058b84cbf81c76d45f302ce757b213ca

but musl does not have a separate thread library (it is included in
libc.so), so I doubt that the hack is applies to it.
---
 linux-user/signal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index c750053..c89f83d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -73,12 +73,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
 [SIGPWR] = TARGET_SIGPWR,
 [SIGSYS] = TARGET_SIGSYS,
 /* next signals stay the same */
+#ifdef __SIGRTMIN
 /* Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
host libpthread signals.  This assumes no one actually uses SIGRTMAX :-/
To fix this properly we need to do manual signal delivery multiplexed
over a single host signal.  */
 [__SIGRTMIN] = __SIGRTMAX,
 [__SIGRTMAX] = __SIGRTMIN,
+#endif
 };
 static uint8_t target_to_host_signal_table[_NSIG];
 
-- 
2.7.3

[Qemu-devel] [PATCH] linux-user: include for F_EXLCK and F_SHLCK

[Felix Janda]

The F_EXLCK and F_SHLCK fcntl lock constants are obsolete synonyms for
F_WRLCK and F_RDLCK. Include  to fix compilation with
the musl c library, which does not expose these constants.

Signed-off-by: Felix Janda 
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0815f30..3d1f694 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -107,6 +107,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include 
 #endif
 #include 
+#include 
 #include "linux_loop.h"
 #include "uname.h"
 
-- 
2.7.3

[Qemu-devel] [PATCH] linux-user: use libc wrapper instead of direct mremap syscall

[Felix Janda]

This commit essentially reverts commit
3af72a4d98dca033492102603734cbc63cd2694a, which has replaced
five-argument calls to mremap() by direct mremap syscalls for
compatibility with glibc older than version 2.4.

The direct syscall was buggy for 64bit targets on 32bit hosts
because of the default integer type promotions. Since glibc-2.4
is now a decade old, we can remove this workaround.

Signed-off-by: Felix Janda 
---
 linux-user/mmap.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index c4371d9..ffd099d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -17,8 +17,6 @@
  *  along with this program; if not, see .
  */
 #include "qemu/osdep.h"
-#include 
-#include 
 
 #include "qemu.h"
 #include "qemu-common.h"
@@ -681,10 +679,8 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
old_size,
 mmap_lock();
 
 if (flags & MREMAP_FIXED) {
-host_addr = (void *) syscall(__NR_mremap, g2h(old_addr),
- old_size, new_size,
- flags,
- g2h(new_addr));
+host_addr = mremap(g2h(old_addr), old_size, new_size,
+   flags, g2h(new_addr));
 
 if (reserved_va && host_addr != MAP_FAILED) {
 /* If new and old addresses overlap then the above mremap will
@@ -700,10 +696,8 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
old_size,
 errno = ENOMEM;
 host_addr = MAP_FAILED;
 } else {
-host_addr = (void *) syscall(__NR_mremap, g2h(old_addr),
- old_size, new_size,
- flags | MREMAP_FIXED,
- g2h(mmap_start));
+host_addr = mremap(g2h(old_addr), old_size, new_size,
+   flags | MREMAP_FIXED, g2h(mmap_start));
 if (reserved_va) {
 mmap_reserve(old_addr, old_size);
 }
-- 
2.7.3

Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Peter Maydell]

On 30 September 2016 at 15:46, Tom Hanson  wrote:
> On 09/29/2016 07:27 PM, Peter Maydell wrote:
> ...
>>> This work was not done at this time since the changes could not be tested
>>> with current CPU models.  Comments have been added to flag the locations
>>> where this will need to be fixed once a model is available.
>>
>> This is *not* why we haven't done this work. We haven't done it
>> because the maximum virtual address size permitted by the
>> architecture is less than 56 bits, and so this is a "can't happen"
>> situation.
>
> But, in an earlier discussion which we had about the desire to use QEMU
> to test potential new ARM-based architectures with large address spaces
> I suggested that these changes be made now.  You said that the changes
> shouldn't be made because:
> where there is no supported guest CPU that could use
> that code, the code shouldn't be there because it's untested
> and untestable
> Isn't that the same thing I said above?

That's a general statement of principle about what I think we
should or shouldn't write code for in QEMU. In this particular case,
it's true, but the reason it's true isn't just that we don't
currently have any 56 bit-VA CPUs implemented, but because such
a CPU is not permitted by the architecture. That's a stronger
statement and I think it's worth making.

>>> 3 comments added in same file to identify cases in a switch.
>>
>> This should be a separate patch, because it is unrelated to the
>> tagged address stuff.
>
> As part of that same conversation you suggested adding these
> comments rather than making the changes:
> If we can assert, or failing that have a comment in the place
> that would be modified anyway for 56 bit addresses then that
> ought to catch the future case I think.

Yes, I still think this. What does it have to do with adding
"SVC", "HVC", etc comments to the switch cases? Those have
nothing to do with tagged addresses or 56 bit VAs, and should
not be in this patch (though I don't object to them inherently).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Peter Maydell]

On 30 September 2016 at 14:42, Edgar Iglesias  wrote:
> Peter, feel free to change the syntax to whatever you feel is best.

Applied to target-arm.next, thanks. I picked "iss_sf = opc != 0;"

-- PMM

Re: [Qemu-devel] [PULL 0/8] next patches for s390x/kvm

[Peter Maydell]

On 30 September 2016 at 03:47, Christian Borntraeger
 wrote:
> Peter,
>
> The following changes since commit 25930ed60aad49f1fdd7de05272317c86ce1275b:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
> staging (2016-09-27 23:10:12 +0100)
>
> are available in the git repository at:
>
>   git://github.com/borntraeger/qemu.git tags/s390x-20160927
>
> for you to fetch changes up to 794afd7096f1ef3ea632b5cf75998562a2f8029a:
>
>   s390x/kvm: fix build against qemu_uuid (2016-09-28 13:24:51 +0200)
>
> 
> Couple of s390x patches:
> - some PCI cleanups
> - fix build error due to uuid rework
> - fix potential deadlock in sigp handling
> - enable ccw devices in BIOS and enforce checking in QEMU

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Edgar Iglesias]

Peter, feel free to change the syntax to whatever you feel is best.

Cheers,

Edgar

---

Sent from my phone

 Original Message 

Subject: Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of 
iss_sf in disas_ld_lit

From: Peter Maydell 

Date: 30 Sep 2016, 19:34

To: "Edgar E. Iglesias" 

On 30 September 2016 at 03:49, Edgar E. Iglesias
 wrote:
> On Fri, Sep 30, 2016 at 12:42:27PM +0200, Thomas Huth wrote:
>> On 30.09.2016 12:19, Edgar E. Iglesias wrote:
>> > From: "Edgar E. Iglesias" 
>> >
>> > Fix the decoding of iss_sf in disas_ld_lit.
>> > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome)
>> > is a bit that specifies the width of the register that the
>> > instruction loads to.
>> >
>> > If cleared it specifies 32 bits.
>> > If set it specifies 64 bits.
>> >
>> > Signed-off-by: Edgar E. Iglesias 
>> > ---
>> >  target-arm/translate-a64.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> > index ddf52f5..eae25c3 100644
>> > --- a/target-arm/translate-a64.c
>> > +++ b/target-arm/translate-a64.c
>> > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t 
>> > insn)
>> >  do_fp_ld(s, rt, tcg_addr, size);
>> >  } else {
>> >  /* Only unsigned 32bit loads target 32bit registers.  */
>> > -bool iss_sf = opc == 0 ? 32 : 64;
>> > +bool iss_sf = opc == 0 ? false : true;
>>
>> You could simplify that to:
>>
>>   bool iss_sf = !(opc == 0);
>
> I don't really see how that is simpler/clearer.
>
> I considered:
> bool iss_sf = opc != 0;
>
> but felt the expanded one was clearer in particular as a patch for review.

I would probably personally have written it without the ?:, but
I don't think it's significant enough to bother respinning.

thanks
-- PMM


This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence

[Jonathan Neuschäfer]

On Fri, Sep 30, 2016 at 11:45:19PM +0100, Alex Bennée wrote:
> 
> Jonathan Neuschäfer  writes:
> 
> > On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote:
> >> From: Paolo Bonzini 
> >>
> >> There is a data race if the sequence is written concurrently to the
> >> read.  In C11 this has undefined behavior.  Use atomic_set; the
> >> read side is already using atomic_read.
> >>
> >> Reported-by: Alex Bennée 
> >> Signed-off-by: Paolo Bonzini 
> >> Signed-off-by: Alex Bennée 
> >> ---
> >>  include/qemu/seqlock.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
> >> index 2e2be4c..8dee11d 100644
> >> --- a/include/qemu/seqlock.h
> >> +++ b/include/qemu/seqlock.h
> >> @@ -31,7 +31,7 @@ static inline void seqlock_init(QemuSeqLock *sl)
> >>  /* Lock out other writers and update the count.  */
> >>  static inline void seqlock_write_begin(QemuSeqLock *sl)
> >>  {
> >> -++sl->sequence;
> >> +atomic_set(>sequence, sl->sequence + 1);
> >
> > I'm not an atomics expert, but as I read the code, the load of
> > sl->sequence (on the right side) isn't atomic relative to the store
> > (atomic_set). Should this be atomic_inc(>sequence) instead, or am I
> > missing something?
> 
> There can only be one seqlock_write going on at a time (that is
> protected by a lock). The atomic_set only ensures that the seqlock_read
> side unambiguously sees one value or the other - you don't need to use
> an atomic_inc in this case.

Ah, good, that makes sense.


Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Tom Hanson]

On 09/29/2016 07:27 PM, Peter Maydell wrote:
...
>> This work was not done at this time since the changes could not be tested
>> with current CPU models.  Comments have been added to flag the locations
>> where this will need to be fixed once a model is available.
> 
> This is *not* why we haven't done this work. We haven't done it
> because the maximum virtual address size permitted by the
> architecture is less than 56 bits, and so this is a "can't happen"
> situation.

But, in an earlier discussion which we had about the desire to use QEMU to test 
potential new ARM-based architectures with large address spaces I suggested 
that these changes be made now.  You said that the changes shouldn't be made 
because:
where there is no supported guest CPU that could use
that code, the code shouldn't be there because it's untested
and untestable
Isn't that the same thing I said above?

>> 3 comments added in same file to identify cases in a switch.
> 
> This should be a separate patch, because it is unrelated to the
> tagged address stuff.

As part of that same conversation you suggested adding these comments rather 
than making the changes:
If we can assert, or failing that have a comment in the place
that would be modified anyway for 56 bit addresses then that
ought to catch the future case I think.

Re: [Qemu-devel] [PULL 0/2] target-mips queue

[Peter Maydell]

On 30 September 2016 at 01:16, Yongbok Kim  wrote:
>>
>> Your GPG key hasn't been signed by anybody -- are you in a position
>> to get it signed by somebody else who can in-person verify your identity
>> (eg Leon)?
>
> Hi Peter,
>
> We have just arranged the key signing and Leon has signed my key.

Thanks; I have applied the pull request to master.

-- PMM

Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence

[Alex Bennée]

Jonathan Neuschäfer  writes:

> On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote:
>> From: Paolo Bonzini 
>>
>> There is a data race if the sequence is written concurrently to the
>> read.  In C11 this has undefined behavior.  Use atomic_set; the
>> read side is already using atomic_read.
>>
>> Reported-by: Alex Bennée 
>> Signed-off-by: Paolo Bonzini 
>> Signed-off-by: Alex Bennée 
>> ---
>>  include/qemu/seqlock.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
>> index 2e2be4c..8dee11d 100644
>> --- a/include/qemu/seqlock.h
>> +++ b/include/qemu/seqlock.h
>> @@ -31,7 +31,7 @@ static inline void seqlock_init(QemuSeqLock *sl)
>>  /* Lock out other writers and update the count.  */
>>  static inline void seqlock_write_begin(QemuSeqLock *sl)
>>  {
>> -++sl->sequence;
>> +atomic_set(>sequence, sl->sequence + 1);
>
> I'm not an atomics expert, but as I read the code, the load of
> sl->sequence (on the right side) isn't atomic relative to the store
> (atomic_set). Should this be atomic_inc(>sequence) instead, or am I
> missing something?

There can only be one seqlock_write going on at a time (that is
protected by a lock). The atomic_set only ensures that the seqlock_read
side unambiguously sees one value or the other - you don't need to use
an atomic_inc in this case.

>
> In particular, I'm worried about this situation:
>
>   thread 0thread 1
>   seqlock_write_begin:seqlock_write_begin:
> unsigned tmp = sl->sequence unsigned tmp = sl->sequence
> tmp += 1tmp += 1
> atomic_set(>sequence, tmp)
> atomic_set(>sequence, tmp)
>
> ... where sl->sequence will effectively only be incremented once (as far
> as I can see).
>
>
> Regards,
> Jonathan Neuschäfer


--
Alex Bennée

Re: [Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence

[Jonathan Neuschäfer]

On Fri, Sep 30, 2016 at 10:30:56PM +0100, Alex Bennée wrote:
> From: Paolo Bonzini 
> 
> There is a data race if the sequence is written concurrently to the
> read.  In C11 this has undefined behavior.  Use atomic_set; the
> read side is already using atomic_read.
> 
> Reported-by: Alex Bennée 
> Signed-off-by: Paolo Bonzini 
> Signed-off-by: Alex Bennée 
> ---
>  include/qemu/seqlock.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
> index 2e2be4c..8dee11d 100644
> --- a/include/qemu/seqlock.h
> +++ b/include/qemu/seqlock.h
> @@ -31,7 +31,7 @@ static inline void seqlock_init(QemuSeqLock *sl)
>  /* Lock out other writers and update the count.  */
>  static inline void seqlock_write_begin(QemuSeqLock *sl)
>  {
> -++sl->sequence;
> +atomic_set(>sequence, sl->sequence + 1);

I'm not an atomics expert, but as I read the code, the load of
sl->sequence (on the right side) isn't atomic relative to the store
(atomic_set). Should this be atomic_inc(>sequence) instead, or am I
missing something?

In particular, I'm worried about this situation:

thread 0thread 1
seqlock_write_begin:seqlock_write_begin:
  unsigned tmp = sl->sequence unsigned tmp = sl->sequence
  tmp += 1tmp += 1
  atomic_set(>sequence, tmp)
  atomic_set(>sequence, tmp)

... where sl->sequence will effectively only be incremented once (as far
as I can see).


Regards,
Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 00/11] blockjobs: Fix transactional race condition

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1475272849-19990-1-git-send-email-js...@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/11] blockjobs: Fix transactional race 
condition

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/1475272849-19990-1-git-send-email-js...@redhat.com 
-> patchew/1475272849-19990-1-git-send-email-js...@redhat.com
Switched to a new branch 'test'
d3bf4cf iotests: add transactional failure race test
ea3adad blockjob: refactor backup_start as backup_job_create
6b5e7c6 blockjob: add block_job_start
fc02e4d blockjob: add .start field
f34ba3c blockjob: add .clean property
6dc649d blockjobs: fix documentation
3e472b6 blockjobs: split interface into public/private
6cbfc4c blockjobs: Always use block_job_get_aio_context
b2ec7f6 Blockjobs: Internalize user_pause logic
e76a966 blockjob: centralize QMP event emissions
cb59e5e blockjob: fix dead pointer in txn list

=== OUTPUT BEGIN ===
Checking PATCH 1/11: blockjob: fix dead pointer in txn list...
Checking PATCH 2/11: blockjob: centralize QMP event emissions...
Checking PATCH 3/11: Blockjobs: Internalize user_pause logic...
Checking PATCH 4/11: blockjobs: Always use block_job_get_aio_context...
Checking PATCH 5/11: blockjobs: split interface into public/private...
ERROR: struct BlockJobDriver should normally be const
#301: FILE: include/block/blockjob.h:31:
+typedef struct BlockJobDriver BlockJobDriver;

ERROR: struct BlockJobDriver should normally be const
#535: FILE: include/block/blockjob_int.h:37:
+struct BlockJobDriver {

ERROR: space prohibited between function name and open parenthesis '('
#579: FILE: include/block/blockjob_int.h:81:
+void coroutine_fn (*pause)(BlockJob *job);

ERROR: space prohibited between function name and open parenthesis '('
#586: FILE: include/block/blockjob_int.h:88:
+void coroutine_fn (*resume)(BlockJob *job);

total: 4 errors, 0 warnings, 805 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 6/11: blockjobs: fix documentation...
Checking PATCH 7/11: blockjob: add .clean property...
Checking PATCH 8/11: blockjob: add .start field...
Checking PATCH 9/11: blockjob: add block_job_start...
Checking PATCH 10/11: blockjob: refactor backup_start as backup_job_create...
Checking PATCH 11/11: iotests: add transactional failure race test...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH v2 10/11] blockjob: refactor backup_start as backup_job_create

[John Snow]

Refactor backup_start as backup_job_create, which only creates the job,
but does not automatically start it. The old interface, 'backup_start',
is not kept in favor of limiting the number of nearly-identical iterfaces
that would have to be edited to keep up with QAPI changes in the future.

Callers that wish to synchronously start the backup_block_job can
instead just call block_job_start immediately after calling
backup_job_create.

Transactions are updated to use the new interface, calling block_job_start
only during the .commit phase, which helps prevent race conditions where
jobs may finish before we even finish building the transaction. This may
happen, for instance, during empty block backup jobs.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c| 26 ---
 block/replication.c   | 11 ---
 blockdev.c| 81 +++
 include/block/block_int.h | 21 ++--
 4 files changed, 86 insertions(+), 53 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 7294169..aad69eb 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -527,7 +527,7 @@ static const BlockJobDriver backup_job_driver = {
 .attached_aio_context   = backup_attached_aio_context,
 };
 
-void backup_start(const char *job_id, BlockDriverState *bs,
+BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
   bool compress,
@@ -546,52 +546,52 @@ void backup_start(const char *job_id, BlockDriverState 
*bs,
 
 if (bs == target) {
 error_setg(errp, "Source and target cannot be the same");
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(bs)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(target)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (compress && target->drv->bdrv_co_pwritev_compressed == NULL) {
 error_setg(errp, "Compression is not supported for this drive %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
-return;
+return NULL;
 }
 
 if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 if (!sync_bitmap) {
 error_setg(errp, "must provide a valid bitmap name for "
  "\"incremental\" sync mode");
-return;
+return NULL;
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
 if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
-return;
+return NULL;
 }
 } else if (sync_bitmap) {
 error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
MirrorSyncMode_lookup[sync_mode]);
-return;
+return NULL;
 }
 
 len = bdrv_getlength(bs);
@@ -638,8 +638,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
 block_job_txn_add_job(txn, >common);
-block_job_start(>common);
-return;
+
+return >common;
 
  error:
 if (sync_bitmap) {
@@ -649,4 +649,6 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 blk_unref(job->target);
 block_job_unref(>common);
 }
+
+return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index b604b93..d9cdc36 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -409,6 +409,7 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 int64_t active_length, hidden_length, disk_length;
 AioContext *aio_context;
 Error *local_err = NULL;
+BlockJob *job;
 
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
@@ -496,16 +497,18 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 bdrv_op_block_all(top_bs, s->blocker);
 bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-backup_start("replication-backup", s->secondary_disk->bs,
- s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, false,
- BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
- backup_job_completed, s, NULL, _err);
+job = 

[Qemu-devel] [PATCH v2 08/11] blockjob: add .start field

[John Snow]

Add an explicit start field to specify the entrypoint. We already have
ownership of the coroutine itself AND managing the lifetime of the
coroutine, let's take control of creation of the coroutine, too.

This will allow us to delay creation of the actual coroutine until we
know we'll actually start a BlockJob in block_job_start. This avoids
the sticky question of how to "un-create" a Coroutine that hasn't been
started yet.

Signed-off-by: John Snow 
---
 block/backup.c   | 23 ---
 block/commit.c   |  3 ++-
 block/mirror.c   |  4 +++-
 block/stream.c   |  3 ++-
 include/block/blockjob_int.h |  3 +++
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 42ff4c0..58dfc58 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -307,16 +307,6 @@ void backup_cow_request_end(CowRequest *req)
 cow_request_end(req);
 }
 
-static const BlockJobDriver backup_job_driver = {
-.instance_size  = sizeof(BackupBlockJob),
-.job_type   = BLOCK_JOB_TYPE_BACKUP,
-.set_speed  = backup_set_speed,
-.commit = backup_commit,
-.abort  = backup_abort,
-.clean  = backup_clean,
-.attached_aio_context   = backup_attached_aio_context,
-};
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
 bool read, int error)
 {
@@ -526,6 +516,17 @@ static void coroutine_fn backup_run(void *opaque)
 block_job_defer_to_main_loop(>common, backup_complete, data);
 }
 
+static const BlockJobDriver backup_job_driver = {
+.instance_size  = sizeof(BackupBlockJob),
+.job_type   = BLOCK_JOB_TYPE_BACKUP,
+.start  = backup_run,
+.set_speed  = backup_set_speed,
+.commit = backup_commit,
+.abort  = backup_abort,
+.clean  = backup_clean,
+.attached_aio_context   = backup_attached_aio_context,
+};
+
 void backup_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -636,7 +637,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
-job->common.co = qemu_coroutine_create(backup_run, job);
+job->common.co = qemu_coroutine_create(job->common.driver->start, job);
 block_job_txn_add_job(txn, >common);
 qemu_coroutine_enter(job->common.co);
 return;
diff --git a/block/commit.c b/block/commit.c
index fb5bede..dbaf39e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -205,6 +205,7 @@ static const BlockJobDriver commit_job_driver = {
 .instance_size = sizeof(CommitBlockJob),
 .job_type  = BLOCK_JOB_TYPE_COMMIT,
 .set_speed = commit_set_speed,
+.start = commit_run,
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
@@ -274,7 +275,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(commit_run, s);
+s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 
 trace_commit_start(bs, base, top, s, s->common.co, opaque);
 qemu_coroutine_enter(s->common.co);
diff --git a/block/mirror.c b/block/mirror.c
index cc62fb0..ef54e5b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -891,6 +891,7 @@ static const BlockJobDriver mirror_job_driver = {
 .instance_size  = sizeof(MirrorBlockJob),
 .job_type   = BLOCK_JOB_TYPE_MIRROR,
 .set_speed  = mirror_set_speed,
+.start  = mirror_run,
 .complete   = mirror_complete,
 .pause  = mirror_pause,
 .attached_aio_context   = mirror_attached_aio_context,
@@ -900,6 +901,7 @@ static const BlockJobDriver commit_active_job_driver = {
 .instance_size  = sizeof(MirrorBlockJob),
 .job_type   = BLOCK_JOB_TYPE_COMMIT,
 .set_speed  = mirror_set_speed,
+.start  = mirror_run,
 .complete   = mirror_complete,
 .pause  = mirror_pause,
 .attached_aio_context   = mirror_attached_aio_context,
@@ -967,7 +969,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 
 bdrv_op_block_all(target, s->common.blocker);
 
-s->common.co = qemu_coroutine_create(mirror_run, s);
+s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_mirror_start(bs, s, s->common.co, opaque);
 qemu_coroutine_enter(s->common.co);
 }
diff --git a/block/stream.c b/block/stream.c
index 71d0e7a..2a1c814 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -212,6 

[Qemu-devel] [PATCH v2 05/11] blockjobs: split interface into public/private

[John Snow]

To make it a little more obvious which functions are intended to be
public interface and which are intended to be for use only by jobs
themselves, split the interface into "public" and "private" files.

Convert blockjobs (e.g. block/backup) to using the private interface.
Leave blockdev and others on the public interface.

Give up and let qemu-img use the internal interface, though it doesn't
strictly need to be using it.

As a side-effect, hide the BlockJob and BlockJobDriver implementation
from most of the QEMU codebase.

Signed-off-by: John Snow 
---
 block/backup.c   |   2 +-
 block/commit.c   |   2 +-
 block/mirror.c   |   2 +-
 block/replication.c  |   2 +-
 block/stream.c   |   2 +-
 blockjob.c   |   2 +-
 include/block/block.h|   3 +-
 include/block/blockjob.h | 325 +--
 include/block/blockjob_int.h | 355 +++
 qemu-img.c   |   2 +-
 tests/test-blockjob-txn.c|   2 +-
 tests/test-blockjob.c|   2 +-
 12 files changed, 368 insertions(+), 333 deletions(-)
 create mode 100644 include/block/blockjob_int.h

diff --git a/block/backup.c b/block/backup.c
index 582bd0f..d667482 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -16,7 +16,7 @@
 #include "trace.h"
 #include "block/block.h"
 #include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "block/block_backup.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
diff --git a/block/commit.c b/block/commit.c
index 9f67a8b..fb5bede 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -15,7 +15,7 @@
 #include "qemu/osdep.h"
 #include "trace.h"
 #include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..cc62fb0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -13,7 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "trace.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "block/block_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
diff --git a/block/replication.c b/block/replication.c
index 3bd1cf1..b604b93 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -15,7 +15,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "block/nbd.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "block/block_int.h"
 #include "block/block_backup.h"
 #include "sysemu/block-backend.h"
diff --git a/block/stream.c b/block/stream.c
index 3187481..71d0e7a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -14,7 +14,7 @@
 #include "qemu/osdep.h"
 #include "trace.h"
 #include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
diff --git a/blockjob.c b/blockjob.c
index 073d9ce..09fb602 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -27,7 +27,7 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block/block.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
 #include "block/block_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qmp/qerror.h"
diff --git a/include/block/block.h b/include/block/block.h
index e18233a..f556345 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,16 +7,15 @@
 #include "qemu/coroutine.h"
 #include "block/accounting.h"
 #include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi-types.h"
 #include "qemu/hbitmap.h"
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
-typedef struct BlockJob BlockJob;
 typedef struct BdrvChild BdrvChild;
 typedef struct BdrvChildRole BdrvChildRole;
-typedef struct BlockJobTxn BlockJobTxn;
 
 typedef struct BlockDriverInfo {
 /* in bytes, 0 if irrelevant */
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 6f28c73..efae26b 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -28,177 +28,9 @@
 
 #include "block/block.h"
 
-/**
- * BlockJobDriver:
- *
- * A class type for block job driver.
- */
-typedef struct BlockJobDriver {
-/** Derived BlockJob struct size */
-size_t instance_size;
-
-/** String describing the operation, part of query-block-jobs QMP API */
-BlockJobType job_type;
-
-/** Optional callback for job types that support setting a speed limit */
-void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
-
-/** Optional callback for job types that need to forward I/O status reset 
*/
-void (*iostatus_reset)(BlockJob *job);
-
-/**
- * Optional callback for job types whose completion must be triggered
- * manually.
- */
-void (*complete)(BlockJob *job, Error **errp);
-
-/**
- * If the callback 

[Qemu-devel] [PATCH v2 07/11] blockjob: add .clean property

[John Snow]

Cleaning up after we have deferred to the main thread but before the
transaction has converged can be dangerous and result in deadlocks
if the job cleanup invokes any BH polling loops.

A job may attempt to begin cleaning up, but may induce another job to
enter its cleanup routine. The second job, part of our same transaction,
will block waiting for the first job to finish, so neither job may now
make progress.

To rectify this, allow jobs to register a cleanup operation that will
always run regardless of if the job was in a transaction or not, and
if the transaction job group completed successfully or not.

Move sensitive cleanup to this callback instead which is guaranteed to
be run only after the transaction has converged, which removes sensitive
timing constraints from said cleanup.

Furthermore, in future patches these cleanup operations will be performed
regardless of whether or not we actually started the job. Therefore,
cleanup callbacks should essentially confine themselves to undoing create
operations, e.g. setup actions taken in what is now backup_run.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c   | 11 ---
 blockjob.c   |  5 +++--
 include/block/blockjob_int.h |  8 
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d667482..42ff4c0 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -242,6 +242,13 @@ static void backup_abort(BlockJob *job)
 }
 }
 
+static void backup_clean(BlockJob *job)
+{
+BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+assert(s->target);
+blk_unref(s->target);
+}
+
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
@@ -306,6 +313,7 @@ static const BlockJobDriver backup_job_driver = {
 .set_speed  = backup_set_speed,
 .commit = backup_commit,
 .abort  = backup_abort,
+.clean  = backup_clean,
 .attached_aio_context   = backup_attached_aio_context,
 };
 
@@ -327,11 +335,8 @@ typedef struct {
 
 static void backup_complete(BlockJob *job, void *opaque)
 {
-BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 BackupCompleteData *data = opaque;
 
-blk_unref(s->target);
-
 block_job_completed(job, data->ret);
 g_free(data);
 }
diff --git a/blockjob.c b/blockjob.c
index 09fb602..44cbf6c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -217,7 +217,9 @@ static void block_job_completed_single(BlockJob *job)
 job->driver->abort(job);
 }
 }
-
+if (job->driver->clean) {
+job->driver->clean(job);
+}
 if (job->cb) {
 job->cb(job->opaque, job->ret);
 }
@@ -230,7 +232,6 @@ static void block_job_completed_single(BlockJob *job)
 }
 block_job_event_completed(job, msg);
 }
-
 if (job->txn) {
 QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c6da7e4..b7aeaef 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -74,6 +74,14 @@ struct BlockJobDriver {
 void (*abort)(BlockJob *job);
 
 /**
+ * If the callback is not NULL, it will be invoked after a call to either
+ * .commit() or .abort(). Regardless of which callback is invoked after
+ * completion, .clean() will always be called, even if the job does not
+ * belong to a transaction group.
+ */
+void (*clean)(BlockJob *job);
+
+/**
  * If the callback is not NULL, it will be invoked when the job transitions
  * into the paused state.  Paused jobs must not perform any asynchronous
  * I/O or event loop activity.  This callback is used to quiesce jobs.
-- 
2.7.4

[Qemu-devel] [PATCH] fixup! tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write

[Alex Bennée]

---
 cpu-exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 0e6b3d3..8b8be25 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -291,7 +291,8 @@ static inline TranslationBlock *tb_find(CPUState *cpu,
is executed. */
 cpu_get_tb_cpu_state(env, , _base, );
 tb = atomic_rcu_read(>tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);
-if (unlikely(!tb || atomic_read(>pc) != pc || 
atomic_read(>cs_base) != cs_base ||
+if (unlikely(!tb || atomic_read(>pc) != pc ||
+ atomic_read(>cs_base) != cs_base ||
  atomic_read(>flags) != flags)) {
 tb = tb_htable_lookup(cpu, pc, cs_base, flags);
 if (!tb) {
-- 
2.9.3

[Qemu-devel] [PATCH v2 06/11] blockjobs: fix documentation

[John Snow]

Wrong function names in documentation.

Signed-off-by: John Snow 
---
 include/block/blockjob_int.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 0a2d41e..c6da7e4 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -305,7 +305,7 @@ void block_job_enter(BlockJob *job);
 void block_job_event_cancelled(BlockJob *job);
 
 /**
- * block_job_ready:
+ * block_job_event_completed:
  * @job: The job which is now ready to complete.
  * @msg: Error message. Only present on failure.
  *
@@ -314,8 +314,8 @@ void block_job_event_cancelled(BlockJob *job);
 void block_job_event_completed(BlockJob *job, const char *msg);
 
 /**
- * block_job_ready:
- * @job: The job which is now ready to complete.
+ * block_job_event_ready:
+ * @job: The job which is now ready to be completed.
  *
  * Send a BLOCK_JOB_READY event for the specified job.
  */
-- 
2.7.4

[Qemu-devel] [PATCH v2 04/11] blockjobs: Always use block_job_get_aio_context

[John Snow]

There are a few places where we're fishing it out for ourselves.
Let's not do that and instead use the helper.

Signed-off-by: John Snow 
---
 block/io.c   | 4 ++--
 blockdev.c   | 4 ++--
 blockjob.c   | 2 +-
 include/block/blockjob.h | 9 +
 qemu-img.c   | 2 +-
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index 868b065..f26a503 100644
--- a/block/io.c
+++ b/block/io.c
@@ -288,7 +288,7 @@ void bdrv_drain_all(void)
 GSList *aio_ctxs = NULL, *ctx;
 
 while ((job = block_job_next(job))) {
-AioContext *aio_context = blk_get_aio_context(job->blk);
+AioContext *aio_context = block_job_get_aio_context(job);
 
 aio_context_acquire(aio_context);
 block_job_pause(job, false);
@@ -347,7 +347,7 @@ void bdrv_drain_all(void)
 
 job = NULL;
 while ((job = block_job_next(job))) {
-AioContext *aio_context = blk_get_aio_context(job->blk);
+AioContext *aio_context = block_job_get_aio_context(job);
 
 aio_context_acquire(aio_context);
 block_job_resume(job);
diff --git a/blockdev.c b/blockdev.c
index 268452f..0ac507f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3596,7 +3596,7 @@ static BlockJob *find_block_job(const char *id, 
AioContext **aio_context,
 return NULL;
 }
 
-*aio_context = blk_get_aio_context(job->blk);
+*aio_context = block_job_get_aio_context(job);
 aio_context_acquire(*aio_context);
 
 return job;
@@ -3956,7 +3956,7 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 
 for (job = block_job_next(NULL); job; job = block_job_next(job)) {
 BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
-AioContext *aio_context = blk_get_aio_context(job->blk);
+AioContext *aio_context = block_job_get_aio_context(job);
 
 aio_context_acquire(aio_context);
 elem->value = block_job_query(job);
diff --git a/blockjob.c b/blockjob.c
index 2a35f50..073d9ce 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -78,7 +78,7 @@ BlockJob *block_job_get(const char *id)
  * block_job_defer_to_main_loop() where it runs in the QEMU main loop.  Code
  * that supports both cases uses this helper function.
  */
-static AioContext *block_job_get_aio_context(BlockJob *job)
+AioContext *block_job_get_aio_context(BlockJob *job)
 {
 return job->deferred_to_main_loop ?
qemu_get_aio_context() :
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 081f6c2..6f28c73 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -513,4 +513,13 @@ void block_job_txn_unref(BlockJobTxn *txn);
  */
 void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
 
+/**
+ * block_job_get_aio_context:
+ * @job: Job to get the aio_context for
+ *
+ * Fetch the current context for the given BlockJob. May be the main loop if
+ * the job has already deferred to main for final cleanup.
+ */
+AioContext *block_job_get_aio_context(BlockJob *job);
+
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index ceffefe..204fa9c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -793,7 +793,7 @@ static void common_block_job_cb(void *opaque, int ret)
 
 static void run_block_job(BlockJob *job, Error **errp)
 {
-AioContext *aio_context = blk_get_aio_context(job->blk);
+AioContext *aio_context = block_job_get_aio_context(job);
 
 do {
 aio_poll(aio_context, true);
-- 
2.7.4

[Qemu-devel] [PATCH v2 00/11] blockjobs: Fix transactional race condition

[John Snow]

There are a few problems with transactional job completion right now.

First, if jobs complete so quickly they complete before remaining jobs
get a chance to join the transaction, the completion mode can leave well
known state and the QLIST can get corrupted and the transactional jobs
can complete in batches or phases instead of all together.

Second, if two or more jobs defer to the main loop at roughly the same
time, it's possible for one job's cleanup to directly invoke the other
job's cleanup from within the same thread, leading to a situation that
will deadlock the entire transaction.

Thanks to Vladimir for pointing out these modes of failure.

This series also does a little digging into refactoring Jobs into public
and private interfaces. It's somewhat unrelated, but it was easier to
include this with this series than separate it out and send it later.
This comprises patches 2-6. The actual fixes here are in patches 1 and
7-10. A new test to catch Vladimir's failure scenario is in patch 11.

v2:
 - Lots of differences in patches 2-9.
 - Cancel should now work on an "unstarted" blockjob.
 - New refactoring patches.
 - Added "start" property for BlockJob Drivers.



For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch job-manual-start
https://github.com/jnsnow/qemu/tree/job-manual-start

This version is tagged job-manual-start-v2:
https://github.com/jnsnow/qemu/releases/tag/job-manual-start-v2

John Snow (10):
  blockjob: centralize QMP event emissions
  Blockjobs: Internalize user_pause logic
  blockjobs: Always use block_job_get_aio_context
  blockjobs: split interface into public/private
  blockjobs: fix documentation
  blockjob: add .clean property
  blockjob: add .start field
  blockjob: add block_job_start
  blockjob: refactor backup_start as backup_job_create
  iotests: add transactional failure race test

Vladimir Sementsov-Ogievskiy (1):
  blockjob: fix dead pointer in txn list

 block/backup.c   |  59 ---
 block/commit.c   |   6 +-
 block/io.c   |   6 +-
 block/mirror.c   |   7 +-
 block/replication.c  |  13 +-
 block/stream.c   |   6 +-
 blockdev.c   | 128 +++
 blockjob.c   |  72 +++--
 include/block/block.h|   3 +-
 include/block/block_int.h|  29 ++--
 include/block/blockjob.h | 345 +++-
 include/block/blockjob_int.h | 366 +++
 qemu-img.c   |   4 +-
 tests/qemu-iotests/124   |  91 +++
 tests/qemu-iotests/124.out   |   4 +-
 tests/test-blockjob-txn.c|  14 +-
 tests/test-blockjob.c|   2 +-
 17 files changed, 688 insertions(+), 467 deletions(-)
 create mode 100644 include/block/blockjob_int.h

-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 30 September 2016 at 14:48, Tom Hanson  wrote:
> On 09/29/2016 07:37 PM, Peter Maydell wrote:
>>
>> On 16 September 2016 at 10:34, Thomas Hanson 
>> wrote:
>>>
>>>  If tagged addresses are enabled, then addresses being loaded into
>>> the
>>>  PC must be cleaned up by overwriting the tag bits with either all
>>> 0's
>>>  or all 1's as specified in the ARM ARM spec.  The decision process
>>> is
>>>  dependent on whether the code will be running in EL0/1 or in EL2/3
>>> and
>>>  is controlled by a combination of Top Byte Ignored (TBI) bits in the
>>>  TCR and the value of bit 55 in the address being loaded.
>>>
>>>  TBI values are extracted from the appropriate TCR and made available
>>>  to TCG code generation routines by inserting them into the TB flags
>>>  field and then transferring them to DisasContext structure in
>>>  gen_intermediate_code_a64().
>>>
>>>  New function gen_a64_set_pc_reg() encapsulates the logic required to
>>>  determine whether clean up of the tag byte is required and then
>>>  generating the code to correctly load the PC.
>>>
>>>  In addition to those instruction which can directly load a tagged
>>>  address into the PC, there are others which increment or add a value
>>> to
>>>  the PC.  If 56 bit addressing is used, these instructions can cause
>>> an
>>>  arithmetic roll-over into the tag bits.  The ARM ARM specification
>>> for
>>>  handling tagged addresses requires that these cases also be
>>> addressed
>>>  by cleaning up the tag field.  This work has been deferred because
>>>  there is currently no CPU model available for testing with 56 bit
>>>  addresses.
>>
>> These changes are OK (other than the comments I've made on the
>> patches), but do not cover all the cases where values can be
>> loaded into the PC and may need to be cleansed of their tags.
>>
>> In particular:
>>   * on exception entry to AArch64 we may need to clean a tag out of
>> the vector table base address register VBAR_ELx
>> (in QEMU this would be in arm_cpu_do_interrupt_aarch64())
>>   * on exception return to AArch64 we may need to clean a tag out of
>> the return address we got from ELR_ELx
>> (in QEMU, in the exception_return helper)
>>
>> Note that D4.1.1 of the ARM ARM describes a potential relaxation
>> of the requirement that tag bits not be propagated into the PC
>> in the case of an illegal exception return; I recommend not
>> taking advantage of that relaxation unless it really does fall
>> out of the implementation much more trivially that way.
>>
>> Watch out that you use the TBI bits for the destination EL in
>> each case, not the EL you start in...
>>
>> thanks
>> -- PMM
>
> Peter,
>
> As I read arm_cpu_do_interrupt_aarch64() it sets the return address in
> env->elr_el[new_el] to env->pc (for AArch64).
>
> Since the PC is alway clean, how can a tagged address get saved off? Am I
> missing something?

That's the code that saves the old PC into ELR_ELx. For exception
entry the bit that needs changing is where we put the new vector
entry point address (which is calculated from VBAR_ELx) into the PC.

thanks
-- PMM

[Qemu-devel] [PATCH v2 03/11] Blockjobs: Internalize user_pause logic

[John Snow]

BlockJobs will begin hiding their state in preparation for some
refactorings anyway, so let's internalize the user_pause mechanism
instead of leaving it to callers to correctly manage.

Signed-off-by: John Snow 
---
 block/io.c   |  2 +-
 blockdev.c   | 10 --
 blockjob.c   | 16 
 include/block/blockjob.h | 11 ++-
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index fdf7080..868b065 100644
--- a/block/io.c
+++ b/block/io.c
@@ -291,7 +291,7 @@ void bdrv_drain_all(void)
 AioContext *aio_context = blk_get_aio_context(job->blk);
 
 aio_context_acquire(aio_context);
-block_job_pause(job);
+block_job_pause(job, false);
 aio_context_release(aio_context);
 }
 
diff --git a/blockdev.c b/blockdev.c
index 03200e7..268452f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3629,7 +3629,7 @@ void qmp_block_job_cancel(const char *device,
 force = false;
 }
 
-if (job->user_paused && !force) {
+if (block_job_paused(job) && !force) {
 error_setg(errp, "The block job for device '%s' is currently paused",
device);
 goto out;
@@ -3646,13 +3646,12 @@ void qmp_block_job_pause(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);
 
-if (!job || job->user_paused) {
+if (!job || block_job_paused(job)) {
 return;
 }
 
-job->user_paused = true;
 trace_qmp_block_job_pause(job);
-block_job_pause(job);
+block_job_pause(job, true);
 aio_context_release(aio_context);
 }
 
@@ -3661,11 +3660,10 @@ void qmp_block_job_resume(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);
 
-if (!job || !job->user_paused) {
+if (!job || !block_job_paused(job)) {
 return;
 }
 
-job->user_paused = false;
 trace_qmp_block_job_resume(job);
 block_job_iostatus_reset(job);
 block_job_resume(job);
diff --git a/blockjob.c b/blockjob.c
index 6a300ba..2a35f50 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -104,7 +104,7 @@ static void block_job_detach_aio_context(void *opaque)
 /* In case the job terminates during aio_poll()... */
 block_job_ref(job);
 
-block_job_pause(job);
+block_job_pause(job, false);
 
 if (!job->paused) {
 /* If job is !job->busy this kicks it into the next pause point. */
@@ -343,9 +343,12 @@ void block_job_complete(BlockJob *job, Error **errp)
 job->driver->complete(job, errp);
 }
 
-void block_job_pause(BlockJob *job)
+void block_job_pause(BlockJob *job, bool user)
 {
 job->pause_count++;
+if (user) {
+job->user_paused = true;
+}
 }
 
 static bool block_job_should_pause(BlockJob *job)
@@ -353,6 +356,11 @@ static bool block_job_should_pause(BlockJob *job)
 return job->pause_count > 0;
 }
 
+bool block_job_paused(BlockJob *job)
+{
+return job ? job->user_paused : 0;
+}
+
 void coroutine_fn block_job_pause_point(BlockJob *job)
 {
 if (!block_job_should_pause(job)) {
@@ -386,6 +394,7 @@ void block_job_resume(BlockJob *job)
 if (job->pause_count) {
 return;
 }
+job->user_paused = false;
 block_job_enter(job);
 }
 
@@ -592,8 +601,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 action, _abort);
 if (action == BLOCK_ERROR_ACTION_STOP) {
 /* make the pause user visible, which will be resumed from QMP. */
-job->user_paused = true;
-block_job_pause(job);
+block_job_pause(job, true);
 block_job_iostatus_set_err(job, error);
 }
 return action;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ddb4ae..081f6c2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -347,10 +347,19 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
 /**
  * block_job_pause:
  * @job: The job to be paused.
+ * @user: Requested explicitly via user?
  *
  * Asynchronously pause the specified job.
  */
-void block_job_pause(BlockJob *job);
+void block_job_pause(BlockJob *job, bool user);
+
+/**
+ * block_job_paused:
+ * @job: The job to query.
+ *
+ * Returns true if the job is user-paused.
+ */
+bool block_job_paused(BlockJob *job);
 
 /**
  * block_job_resume:
-- 
2.7.4

[Qemu-devel] [PATCH v2 11/11] iotests: add transactional failure race test

[John Snow]

Add a regression test for the case found by Vladimir.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/124 | 91 ++
 tests/qemu-iotests/124.out |  4 +-
 2 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2f0bc24..b8f7dad 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -512,6 +512,97 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 self.check_backups()
 
 
+def test_transaction_failure_race(self):
+'''Test: Verify that transactions with jobs that have no data to
+transfer do not cause race conditions in the cancellation of the entire
+transaction job group.
+'''
+
+# Create a second drive, with pattern:
+drive1 = self.add_node('drive1')
+self.img_create(drive1['file'], drive1['fmt'])
+io_write_patterns(drive1['file'], (('0x14', 0, 512),
+   ('0x5d', '1M', '32k'),
+   ('0xcd', '32M', '124k')))
+
+# Create a blkdebug interface to this img as 'drive1'
+result = self.vm.qmp('blockdev-add', options={
+'node-name': drive1['id'],
+'driver': drive1['fmt'],
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': drive1['file']
+},
+'set-state': [{
+'event': 'flush_to_disk',
+'state': 1,
+'new_state': 2
+}],
+'inject-error': [{
+'event': 'read_aio',
+'errno': 5,
+'state': 2,
+'immediately': False,
+'once': True
+}],
+}
+})
+self.assert_qmp(result, 'return', {})
+
+# Create bitmaps and full backups for both drives
+drive0 = self.drives[0]
+dr0bm0 = self.add_bitmap('bitmap0', drive0)
+dr1bm0 = self.add_bitmap('bitmap0', drive1)
+self.create_anchor_backup(drive0)
+self.create_anchor_backup(drive1)
+self.assert_no_active_block_jobs()
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+# Emulate some writes
+self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+  ('0xef', '16M', '256k'),
+  ('0x46', '32736k', '64k')))
+
+# Create incremental backup targets
+target0 = self.prepare_backup(dr0bm0)
+target1 = self.prepare_backup(dr1bm0)
+
+# Ask for a new incremental backup per-each drive, expecting drive1's
+# backup to fail and attempt to cancel the empty drive0 job.
+transaction = [
+transaction_drive_backup(drive0['id'], target0, sync='incremental',
+ format=drive0['fmt'], mode='existing',
+ bitmap=dr0bm0.name),
+transaction_drive_backup(drive1['id'], target1, sync='incremental',
+ format=drive1['fmt'], mode='existing',
+ bitmap=dr1bm0.name)
+]
+result = self.vm.qmp('transaction', actions=transaction,
+ properties={'completion-mode': 'grouped'} )
+self.assert_qmp(result, 'return', {})
+
+# Observe that drive0's backup is cancelled and drive1 completes with
+# an error.
+self.wait_qmp_backup_cancelled(drive0['id'])
+self.assertFalse(self.wait_qmp_backup(drive1['id']))
+error = self.vm.event_wait('BLOCK_JOB_ERROR')
+self.assert_qmp(error, 'data', {'device': drive1['id'],
+'action': 'report',
+'operation': 'read'})
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+self.assert_no_active_block_jobs()
+
+# Delete drive0's successful target and eliminate our record of the
+# unsuccessful drive1 target.
+dr0bm0.del_target()
+dr1bm0.del_target()
+
+self.vm.shutdown()
+
+
+
 def test_sync_dirty_bitmap_missing(self):
 self.assert_no_active_block_jobs()
 self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 36376be..e56cae0 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-..
+...
 --
-Ran 10 tests
+Ran 11 tests
 
 OK
-- 
2.7.4

[Qemu-devel] [PATCH v2 09/11] blockjob: add block_job_start

[John Snow]

Instead of automatically starting jobs at creation time via backup_start
et al, we'd like to return a job object pointer that can be started
manually at later point in time.

For now, add the block_job_start mechanism and start the jobs
automatically as we have been doing, with conversions job-by-job coming
in later patches.

Of note: cancellation of unstarted jobs will perform all the normal
cleanup as if the job had started, particularly abort and clean. The
only difference is that we will not emit any events, because the job
never actually started.

Signed-off-by: John Snow 
---
 block/backup.c|  3 +--
 block/commit.c|  3 +--
 block/mirror.c|  3 +--
 block/stream.c|  3 +--
 blockjob.c| 48 +++
 include/block/block_int.h |  8 
 tests/test-blockjob-txn.c | 12 ++--
 7 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 58dfc58..7294169 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -637,9 +637,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
-job->common.co = qemu_coroutine_create(job->common.driver->start, job);
 block_job_txn_add_job(txn, >common);
-qemu_coroutine_enter(job->common.co);
+block_job_start(>common);
 return;
 
  error:
diff --git a/block/commit.c b/block/commit.c
index dbaf39e..e64fdf3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -275,10 +275,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 
 trace_commit_start(bs, base, top, s, s->common.co, opaque);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 
diff --git a/block/mirror.c b/block/mirror.c
index ef54e5b..2a6a662 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -969,9 +969,8 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 
 bdrv_op_block_all(target, s->common.blocker);
 
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_mirror_start(bs, s, s->common.co, opaque);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 2a1c814..55cbbe7 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -232,7 +232,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_stream_start(bs, base, s, s->common.co, opaque);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
diff --git a/blockjob.c b/blockjob.c
index 44cbf6c..ac9a68d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -161,7 +161,8 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 job->blk   = blk;
 job->cb= cb;
 job->opaque= opaque;
-job->busy  = true;
+job->busy  = false;
+job->paused= true;
 job->refcnt= 1;
 bs->job = job;
 
@@ -184,6 +185,15 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 return job;
 }
 
+void block_job_start(BlockJob *job)
+{
+assert(job && !job->co && job->paused && !job->busy && job->driver->start);
+job->paused = false;
+job->busy = true;
+job->co = qemu_coroutine_create(job->driver->start, job);
+qemu_coroutine_enter(job->co);
+}
+
 void block_job_ref(BlockJob *job)
 {
 ++job->refcnt;
@@ -220,18 +230,24 @@ static void block_job_completed_single(BlockJob *job)
 if (job->driver->clean) {
 job->driver->clean(job);
 }
+
 if (job->cb) {
 job->cb(job->opaque, job->ret);
 }
-if (block_job_is_cancelled(job)) {
-block_job_event_cancelled(job);
-} else {
-const char *msg = NULL;
-if (job->ret < 0) {
-msg = strerror(-job->ret);
+
+/* Emit events only if we actually started */
+if (job->co) {
+if (block_job_is_cancelled(job)) {
+block_job_event_cancelled(job);
+} else {
+const char *msg = NULL;
+if (job->ret < 0) {
+msg = strerror(-job->ret);
+}
+block_job_event_completed(job, msg);
 }
-block_job_event_completed(job, msg);
 }
+
 if (job->txn) {
 QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
@@ -335,7 +351,8 @@ void block_job_set_speed(BlockJob *job, int64_t speed, 
Error **errp)
 
 void block_job_complete(BlockJob *job, Error **errp)
 {
-if (job->pause_count || 

[Qemu-devel] [PATCH v2 01/11] blockjob: fix dead pointer in txn list

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Though it is not intended to be reached through normal circumstances,
if we do not gracefully deconstruct the transaction QLIST, we may wind
up with stale pointers in the list.

The rest of this series attempts to address the underlying issues,
but this should fix list inconsistencies.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Tested-by: John Snow 
Reviewed-by: John Snow 
[Rewrote commit message. --js]
Signed-off-by: John Snow 
Reviewed-by: Eric Blake 

Signed-off-by: John Snow 
---
 blockjob.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockjob.c b/blockjob.c
index a167f96..13e7134 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -220,6 +220,7 @@ static void block_job_completed_single(BlockJob *job)
 }
 job->cb(job->opaque, job->ret);
 if (job->txn) {
+QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
 }
 block_job_unref(job);
-- 
2.7.4

[Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions

[John Snow]

There's no reason to leave this to blockdev; we can do it in blockjobs
directly and get rid of an extra callback for most users.

Signed-off-by: John Snow 
---
 blockdev.c | 37 ++---
 blockjob.c | 16 ++--
 2 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 29c6561..03200e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2957,31 +2957,6 @@ out:
 aio_context_release(aio_context);
 }
 
-static void block_job_cb(void *opaque, int ret)
-{
-/* Note that this function may be executed from another AioContext besides
- * the QEMU main loop.  If you need to access anything that assumes the
- * QEMU global mutex, use a BH or introduce a mutex.
- */
-
-BlockDriverState *bs = opaque;
-const char *msg = NULL;
-
-trace_block_job_cb(bs, bs->job, ret);
-
-assert(bs->job);
-
-if (ret < 0) {
-msg = strerror(-ret);
-}
-
-if (block_job_is_cancelled(bs->job)) {
-block_job_event_cancelled(bs->job);
-} else {
-block_job_event_completed(bs->job, msg);
-}
-}
-
 void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
   bool has_base, const char *base,
   bool has_backing_file, const char *backing_file,
@@ -3033,7 +3008,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 base_name = has_backing_file ? backing_file : base_name;
 
 stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- has_speed ? speed : 0, on_error, block_job_cb, bs, 
_err);
+ has_speed ? speed : 0, on_error, NULL, bs, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
@@ -3136,10 +3111,10 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
 goto out;
 }
 commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
-on_error, block_job_cb, bs, _err, false);
+on_error, NULL, bs, _err, false);
 } else {
 commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
- on_error, block_job_cb, bs,
+ on_error, NULL, bs,
  has_backing_file ? backing_file : NULL, _err);
 }
 if (local_err != NULL) {
@@ -3260,7 +3235,7 @@ static void do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn, Error **errp)
 
 backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
  bmap, backup->compress, backup->on_source_error,
- backup->on_target_error, block_job_cb, bs, txn, _err);
+ backup->on_target_error, NULL, bs, txn, _err);
 bdrv_unref(target_bs);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
@@ -3330,7 +3305,7 @@ void do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn, Error **errp)
 }
 backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
  NULL, backup->compress, backup->on_source_error,
- backup->on_target_error, block_job_cb, bs, txn, _err);
+ backup->on_target_error, NULL, bs, txn, _err);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
 }
@@ -3410,7 +3385,7 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
  has_replaces ? replaces : NULL,
  speed, granularity, buf_size, sync, backing_mode,
  on_source_error, on_target_error, unmap,
- block_job_cb, bs, errp);
+ NULL, bs, errp);
 }
 
 void qmp_drive_mirror(DriveMirror *arg, Error **errp)
diff --git a/blockjob.c b/blockjob.c
index 13e7134..6a300ba 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -124,7 +124,6 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 BlockBackend *blk;
 BlockJob *job;
 
-assert(cb);
 if (bs->job) {
 error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
 return NULL;
@@ -218,7 +217,20 @@ static void block_job_completed_single(BlockJob *job)
 job->driver->abort(job);
 }
 }
-job->cb(job->opaque, job->ret);
+
+if (job->cb) {
+job->cb(job->opaque, job->ret);
+}
+if (block_job_is_cancelled(job)) {
+block_job_event_cancelled(job);
+} else {
+const char *msg = NULL;
+if (job->ret < 0) {
+msg = strerror(-job->ret);
+}
+block_job_event_completed(job, msg);
+}
+
 if (job->txn) {
 QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 00/15] A number of fixes for ThreadSanitizer

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20160930213106.20186-1-alex.ben...@linaro.org
Subject: [Qemu-devel] [PATCH v3 00/15] A number of fixes for ThreadSanitizer

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/1475246744-29302-1-git-send-email-berra...@redhat.com -> 
patchew/1475246744-29302-1-git-send-email-berra...@redhat.com
 * [new tag] patchew/20160930213106.20186-1-alex.ben...@linaro.org -> 
patchew/20160930213106.20186-1-alex.ben...@linaro.org
Switched to a new branch 'test'
4f429c2 translate-all: mark updates to PageDesc as atomic
d6cfc0a tcg: update remaining TranslationBuffer fields atomically
831c9b0 tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write
f0c76b4 .travis.yml: add gcc sanitizer build
27926c7 qga/command: use QEMU atomic primitives
7b7d449 linux-user/syscall: extend lock around cpu-list
9659c21 util/qht: atomically set b->hashes
e5625e0 cpu: atomically modify cpu->exit_request
1a2afa5 qom/cpu: atomically clear the tb_jmp_cache
857940f qom/object: update class cache atomically
d0019b1 seqlock: use atomic writes for the sequence
58cdc77 tcg/optimize: move default return out of if statement
35c6de8 exec-all.h: revert tb_page_addr_t to target_ulong
ec4be39 atomic.h: comment on use of atomic_read/set
0f57650 atomic.h: fix __SANITIZE_THREAD__ build

=== OUTPUT BEGIN ===
Checking PATCH 1/15: atomic.h: fix __SANITIZE_THREAD__ build...
Checking PATCH 2/15: atomic.h: comment on use of atomic_read/set...
Checking PATCH 3/15: exec-all.h: revert tb_page_addr_t to target_ulong...
Checking PATCH 4/15: tcg/optimize: move default return out of if statement...
Checking PATCH 5/15: seqlock: use atomic writes for the sequence...
Checking PATCH 6/15: qom/object: update class cache atomically...
Checking PATCH 7/15: qom/cpu: atomically clear the tb_jmp_cache...
Checking PATCH 8/15: cpu: atomically modify cpu->exit_request...
Checking PATCH 9/15: util/qht: atomically set b->hashes...
Checking PATCH 10/15: linux-user/syscall: extend lock around cpu-list...
Checking PATCH 11/15: qga/command: use QEMU atomic primitives...
Checking PATCH 12/15: .travis.yml: add gcc sanitizer build...
Checking PATCH 13/15: tcg: ensure cpu_tb_exec/tb_gen_code use 
atomic_read/write...
ERROR: line over 90 characters
#38: FILE: cpu-exec.c:294:
+if (unlikely(!tb || atomic_read(>pc) != pc || 
atomic_read(>cs_base) != cs_base ||

total: 1 errors, 0 warnings, 32 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 14/15: tcg: update remaining TranslationBuffer fields 
atomically...
Checking PATCH 15/15: translate-all: mark updates to PageDesc as atomic...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Tom Hanson]

On 09/29/2016 07:37 PM, Peter Maydell wrote:

On 16 September 2016 at 10:34, Thomas Hanson  wrote:

 If tagged addresses are enabled, then addresses being loaded into the
 PC must be cleaned up by overwriting the tag bits with either all 0's
 or all 1's as specified in the ARM ARM spec.  The decision process is
 dependent on whether the code will be running in EL0/1 or in EL2/3 and
 is controlled by a combination of Top Byte Ignored (TBI) bits in the
 TCR and the value of bit 55 in the address being loaded.

 TBI values are extracted from the appropriate TCR and made available
 to TCG code generation routines by inserting them into the TB flags
 field and then transferring them to DisasContext structure in
 gen_intermediate_code_a64().

 New function gen_a64_set_pc_reg() encapsulates the logic required to
 determine whether clean up of the tag byte is required and then
 generating the code to correctly load the PC.

 In addition to those instruction which can directly load a tagged
 address into the PC, there are others which increment or add a value to
 the PC.  If 56 bit addressing is used, these instructions can cause an
 arithmetic roll-over into the tag bits.  The ARM ARM specification for
 handling tagged addresses requires that these cases also be addressed
 by cleaning up the tag field.  This work has been deferred because
 there is currently no CPU model available for testing with 56 bit
 addresses.

These changes are OK (other than the comments I've made on the
patches), but do not cover all the cases where values can be
loaded into the PC and may need to be cleansed of their tags.

In particular:
  * on exception entry to AArch64 we may need to clean a tag out of
the vector table base address register VBAR_ELx
(in QEMU this would be in arm_cpu_do_interrupt_aarch64())
  * on exception return to AArch64 we may need to clean a tag out of
the return address we got from ELR_ELx
(in QEMU, in the exception_return helper)

Note that D4.1.1 of the ARM ARM describes a potential relaxation
of the requirement that tag bits not be propagated into the PC
in the case of an illegal exception return; I recommend not
taking advantage of that relaxation unless it really does fall
out of the implementation much more trivially that way.

Watch out that you use the TBI bits for the destination EL in
each case, not the EL you start in...

thanks
-- PMM

Peter,

As I read arm_cpu_do_interrupt_aarch64() it sets the return address in 
env->elr_el[new_el] to env->pc (for AArch64).


Since the PC is alway clean, how can a tagged address get saved off? Am 
I missing something?


-Tom

[Qemu-devel] [PATCH v3 14/15] tcg: update remaining TranslationBuffer fields atomically

[Alex Bennée]

The TranslationBuffer is one of those heavily accessed across threads.
To meet defined C11 behaviour across threads we update the accesses to
use the relaxed atomic helpers. Care is still taken with locking and
barriers for when flags are updated and when newly generated buffers are
made visible to the rest of the system.

Signed-off-by: Alex Bennée 
---
 cpu-exec.c  | 16 
 include/exec/exec-all.h | 11 +++
 translate-all.c | 11 ++-
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 99c906b..0e6b3d3 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -185,7 +185,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, 
TranslationBlock *itb)
 cc->synchronize_from_tb(cpu, last_tb);
 } else {
 assert(cc->set_pc);
-cc->set_pc(cpu, last_tb->pc);
+cc->set_pc(cpu, atomic_read(_tb->pc));
 }
 }
 if (tb_exit == TB_EXIT_REQUESTED) {
@@ -235,13 +235,13 @@ static bool tb_cmp(const void *p, const void *d)
 const TranslationBlock *tb = p;
 const struct tb_desc *desc = d;
 
-if (tb->pc == desc->pc &&
-tb->page_addr[0] == desc->phys_page1 &&
-tb->cs_base == desc->cs_base &&
-tb->flags == desc->flags &&
+if (atomic_read(>pc) == desc->pc &&
+atomic_read(>page_addr[0]) == desc->phys_page1 &&
+atomic_read(>cs_base) == desc->cs_base &&
+atomic_read(>flags) == desc->flags &&
 !atomic_read(>invalid)) {
 /* check next page if needed */
-if (tb->page_addr[1] == -1) {
+if (atomic_read(>page_addr[1]) == -1) {
 return true;
 } else {
 tb_page_addr_t phys_page2;
@@ -249,7 +249,7 @@ static bool tb_cmp(const void *p, const void *d)
 
 virt_page2 = (desc->pc & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
 phys_page2 = get_page_addr_code(desc->env, virt_page2);
-if (tb->page_addr[1] == phys_page2) {
+if (atomic_read(>page_addr[1]) == phys_page2) {
 return true;
 }
 }
@@ -507,7 +507,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 return;
 }
 
-trace_exec_tb(tb, tb->pc);
+trace_exec_tb(tb, atomic_read(>pc));
 ret = cpu_tb_exec(cpu, tb);
 *last_tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
 *tb_exit = ret & TB_EXIT_MASK;
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index c3596a6..ff34a0d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -200,6 +200,17 @@ static inline void tlb_flush_by_mmuidx(CPUState *cpu, ...)
 #define USE_DIRECT_JUMP
 #endif
 
+/*
+ * TranslationBlock
+ *
+ * This structure represents a single translated block of code. The
+ * actual code is referenced via tc_ptr. This structure is accessed
+ * across multiple QEMU threads so for C11 compliance all fields
+ * should be access with at least relaxed atomic primitives. Fields
+ * that are updated after initial generation, mainly those involved
+ * with patching jumps and chaining TBs, need stronger guarantees to
+ * prevent corruption.
+ */
 struct TranslationBlock {
 target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS 
base) */
 target_ulong cs_base; /* CS base for this block */
diff --git a/translate-all.c b/translate-all.c
index 0f13d4d..a6262ae 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -773,9 +773,10 @@ static TranslationBlock *tb_alloc(target_ulong pc)
 return NULL;
 }
 tb = _ctx.tb_ctx.tbs[tcg_ctx.tb_ctx.nb_tbs++];
-tb->pc = pc;
-tb->cflags = 0;
-tb->invalid = false;
+
+atomic_set(>pc, pc);
+atomic_set(>cflags, 0);
+atomic_set(>invalid, false);
 return tb;
 }
 
@@ -1095,7 +1096,7 @@ static inline void tb_alloc_page(TranslationBlock *tb,
 bool page_already_protected;
 #endif
 
-tb->page_addr[n] = page_addr;
+atomic_set(>page_addr[n], page_addr);
 p = page_find_alloc(page_addr >> TARGET_PAGE_BITS, 1);
 tb->page_next[n] = p->first_tb;
 #ifndef CONFIG_USER_ONLY
@@ -1156,7 +1157,7 @@ static void tb_link_page(TranslationBlock *tb, 
tb_page_addr_t phys_pc,
 if (phys_page2 != -1) {
 tb_alloc_page(tb, 1, phys_page2);
 } else {
-tb->page_addr[1] = -1;
+atomic_set(>page_addr[1], -1);
 }
 
 /* add in the hash table */
-- 
2.9.3

[Qemu-devel] [PATCH v3 13/15] tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write

[Alex Bennée]

To meet C11 semantics for shared data access we need to use relaxed
atomic accesses. While the completion of data writes w.r.t reads is
ensured by QHT's explicit barriers when a newly generated TB is inserted
ThreadSanitizer will still complain. By using the relaxed accesses the
same code gets generated but instrumentation does not have to worry
about a potentially undefined interaction between plain loads/stores.

Signed-off-by: Alex Bennée 
---
 cpu-exec.c  | 6 +++---
 translate-all.c | 8 
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index e114fcd..99c906b 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -140,7 +140,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, 
TranslationBlock *itb)
 uintptr_t ret;
 TranslationBlock *last_tb;
 int tb_exit;
-uint8_t *tb_ptr = itb->tc_ptr;
+uint8_t *tb_ptr = atomic_read(>tc_ptr);
 
 qemu_log_mask_and_addr(CPU_LOG_EXEC, itb->pc,
"Trace %p [" TARGET_FMT_lx "] %s\n",
@@ -291,8 +291,8 @@ static inline TranslationBlock *tb_find(CPUState *cpu,
is executed. */
 cpu_get_tb_cpu_state(env, , _base, );
 tb = atomic_rcu_read(>tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);
-if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
- tb->flags != flags)) {
+if (unlikely(!tb || atomic_read(>pc) != pc || 
atomic_read(>cs_base) != cs_base ||
+ atomic_read(>flags) != flags)) {
 tb = tb_htable_lookup(cpu, pc, cs_base, flags);
 if (!tb) {
 
diff --git a/translate-all.c b/translate-all.c
index 8ca393c..0f13d4d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1198,10 +1198,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 }
 
 gen_code_buf = tcg_ctx.code_gen_ptr;
-tb->tc_ptr = gen_code_buf;
-tb->cs_base = cs_base;
-tb->flags = flags;
-tb->cflags = cflags;
+atomic_set(>tc_ptr, gen_code_buf);
+atomic_set(>cs_base, cs_base);
+atomic_set(>flags, flags);
+atomic_set(>cflags, cflags);
 
 #ifdef CONFIG_PROFILER
 tcg_ctx.tb_count1++; /* includes aborted translations because of
-- 
2.9.3

[Qemu-devel] [PATCH v3 12/15] .travis.yml: add gcc sanitizer build

[Alex Bennée]

As it seems easy to break the ThreadSanitizer build we should defend it to
ensure that fixes get applied when it breaks. We use the Ubuntu GCC PPA
to get the latest GCC goodness.

As we need to use the -fuse-ld=gold work around we have to disable the
linux-user targets as these trip up the linker.

The make check run is also disabled for Travis but this can be
re-enabled once the check targets have been fixed.

Signed-off-by: Alex Bennée 

---
v2
  - move to container instead of using Trusty
  - work around link problem with -fuse-ld=gold
  - disable linux-user builds
  - drop blacklist (gcc doesn't support it)
  - dump config.log if configure step fails
  - disable make check
  - ensure we use gthread coroutines
---
 .travis.yml | 45 +
 1 file changed, 45 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index f30b10e..9916178 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,6 +9,7 @@ cache: ccache
 addons:
   apt:
 packages:
+  # Build dependencies
   - libaio-dev
   - libattr1-dev
   - libbrlapi-dev
@@ -89,6 +90,7 @@ matrix:
 - env: CONFIG=""
   os: osx
   compiler: clang
+# Plain Trusty Build
 - env: CONFIG=""
   sudo: required
   addons:
@@ -99,3 +101,46 @@ matrix:
 - sudo apt-get build-dep -qq qemu
 - wget -O - 
http://people.linaro.org/~alex.bennee/qemu-submodule-git-seed.tar.xz | tar -xvJ
 - git submodule update --init --recursive
+# Using newer GCC with sanitizers
+- addons:
+apt:
+  sources:
+# PPAs for newer toolchains
+- ubuntu-toolchain-r-test
+  packages:
+# Extra toolchains
+- gcc-5
+- g++-5
+# Build dependencies
+- libaio-dev
+- libattr1-dev
+- libbrlapi-dev
+- libcap-ng-dev
+- libgnutls-dev
+- libgtk-3-dev
+- libiscsi-dev
+- liblttng-ust-dev
+- libnfs-dev
+- libncurses5-dev
+- libnss3-dev
+- libpixman-1-dev
+- libpng12-dev
+- librados-dev
+- libsdl1.2-dev
+- libseccomp-dev
+- libspice-protocol-dev
+- libspice-server-dev
+- libssh2-1-dev
+- liburcu-dev
+- libusb-1.0-0-dev
+- libvte-2.90-dev
+- sparse
+- uuid-dev
+  language: generic
+  compiler: none
+  env:
+- COMPILER_NAME=gcc CXX=g++-5 CC=gcc-5
+- CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user 
--with-coroutine=gthread"
+- TEST_CMD=""
+  before_script:
+- ./configure ${CONFIG} --extra-cflags="-g3 -O0 -fsanitize=thread 
-fuse-ld=gold" || cat config.log
-- 
2.9.3

[Qemu-devel] [PATCH v3 05/15] seqlock: use atomic writes for the sequence

[Alex Bennée]

From: Paolo Bonzini 

There is a data race if the sequence is written concurrently to the
read.  In C11 this has undefined behavior.  Use atomic_set; the
read side is already using atomic_read.

Reported-by: Alex Bennée 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Alex Bennée 
---
 include/qemu/seqlock.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
index 2e2be4c..8dee11d 100644
--- a/include/qemu/seqlock.h
+++ b/include/qemu/seqlock.h
@@ -31,7 +31,7 @@ static inline void seqlock_init(QemuSeqLock *sl)
 /* Lock out other writers and update the count.  */
 static inline void seqlock_write_begin(QemuSeqLock *sl)
 {
-++sl->sequence;
+atomic_set(>sequence, sl->sequence + 1);
 
 /* Write sequence before updating other fields.  */
 smp_wmb();
@@ -42,7 +42,7 @@ static inline void seqlock_write_end(QemuSeqLock *sl)
 /* Write other fields before finalizing sequence.  */
 smp_wmb();
 
-++sl->sequence;
+atomic_set(>sequence, sl->sequence + 1);
 }
 
 static inline unsigned seqlock_read_begin(QemuSeqLock *sl)
-- 
2.9.3

[Qemu-devel] [PATCH v3 11/15] qga/command: use QEMU atomic primitives

[Alex Bennée]

The guest client's use of the glib's g_atomic primitives causes newer
GCC's to barf when built on Travis. As QEMU has its own primitives with
well understood semantics we might as well use them.

The use of atomics was a little inconsistent so I've also ensure the
values are correctly set with atomic primitives at the same time.

I also made the usage of bool consistent while I was at it.

Signed-off-by: Alex Bennée 
---
 qga/commands.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index 50fd26a..edd3e83 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -16,6 +16,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
+#include "qemu/atomic.h"
 
 /* Maximum captured guest-exec out_data/err_data - 16MB */
 #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
@@ -82,7 +83,7 @@ struct GuestExecIOData {
 guchar *data;
 gsize size;
 gsize length;
-gint closed;
+bool closed;
 bool truncated;
 const char *name;
 };
@@ -93,7 +94,7 @@ struct GuestExecInfo {
 int64_t pid_numeric;
 gint status;
 bool has_output;
-gint finished;
+bool finished;
 GuestExecIOData in;
 GuestExecIOData out;
 GuestExecIOData err;
@@ -156,13 +157,13 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error 
**err)
 
 ges = g_new0(GuestExecStatus, 1);
 
-bool finished = g_atomic_int_get(>finished);
+bool finished = atomic_mb_read(>finished);
 
 /* need to wait till output channels are closed
  * to be sure we captured all output at this point */
 if (gei->has_output) {
-finished = finished && g_atomic_int_get(>out.closed);
-finished = finished && g_atomic_int_get(>err.closed);
+finished = finished && atomic_mb_read(>out.closed);
+finished = finished && atomic_mb_read(>err.closed);
 }
 
 ges->exited = finished;
@@ -264,7 +265,7 @@ static void guest_exec_child_watch(GPid pid, gint status, 
gpointer data)
 (int32_t)gpid_to_int64(pid), (uint32_t)status);
 
 gei->status = status;
-gei->finished = true;
+atomic_mb_set(>finished, true);
 
 g_spawn_close_pid(pid);
 }
@@ -320,7 +321,7 @@ static gboolean guest_exec_input_watch(GIOChannel *ch,
 done:
 g_io_channel_shutdown(ch, true, NULL);
 g_io_channel_unref(ch);
-g_atomic_int_set(>closed, 1);
+atomic_mb_set(>closed, true);
 g_free(p->data);
 
 return false;
@@ -374,7 +375,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
 close:
 g_io_channel_shutdown(ch, true, NULL);
 g_io_channel_unref(ch);
-g_atomic_int_set(>closed, 1);
+atomic_mb_set(>closed, true);
 return false;
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 0/9] A couple of fixes for ThreadSanitizer

[Alex Bennée]

Paolo Bonzini  writes:

> On 22/09/2016 12:13, Alex Bennée wrote:
>> Hi,
>>

>
> Queued patches 2-8 (1 is already in and 9 is outside my knowledge), thanks.

Actually could you take them from the v3 I've just posted? I've cleaned
up a bunch of the commit messages and dropped the blacklist patch.

--
Alex Bennée

[Qemu-devel] [PATCH v3 09/15] util/qht: atomically set b->hashes

[Alex Bennée]

ThreadSanitizer detects a possible race between reading/writing the
hashes. The ordering semantics are already documented for QHT however
for true C11 compliance we should use relaxed atomic primitives for
accesses that are done across threads. On x86 this slightly changes to
the code to not do a load/compare in a single instruction leading to a
slight performance degradation.

Running 'taskset -c 0 tests/qht-bench -n 1 -d 10' (i.e. all lookups) 10
times, we get:

before the patch:
 $ ./mean.pl 34.04 34.24 34.38 34.25 34.18 34.51 34.46 34.44 34.29 34.08
 34.287 +- 0.160072900059109
after:
 $ ./mean.pl 33.94 34.00 33.52 33.46 33.55 33.71 34.27 34.06 34.28 34.58
 33.937 +- 0.374731014640279

Signed-off-by: Alex Bennée 
Reviewed-by: Emilio G. Cota 
---
 util/qht.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/qht.c b/util/qht.c
index 16a8d79..571639d 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -379,7 +379,7 @@ static void qht_bucket_reset__locked(struct qht_bucket 
*head)
 if (b->pointers[i] == NULL) {
 goto done;
 }
-b->hashes[i] = 0;
+atomic_set(>hashes[i], 0);
 atomic_set(>pointers[i], NULL);
 }
 b = b->next;
@@ -444,7 +444,7 @@ void *qht_do_lookup(struct qht_bucket *head, 
qht_lookup_func_t func,
 
 do {
 for (i = 0; i < QHT_BUCKET_ENTRIES; i++) {
-if (b->hashes[i] == hash) {
+if (atomic_read(>hashes[i]) == hash) {
 /* The pointer is dereferenced before seqlock_read_retry,
  * so (unlike qht_insert__locked) we need to use
  * atomic_rcu_read here.
@@ -538,8 +538,8 @@ static bool qht_insert__locked(struct qht *ht, struct 
qht_map *map,
 if (new) {
 atomic_rcu_set(>next, b);
 }
-b->hashes[i] = hash;
 /* smp_wmb() implicit in seqlock_write_begin.  */
+atomic_set(>hashes[i], hash);
 atomic_set(>pointers[i], p);
 seqlock_write_end(>sequence);
 return true;
@@ -607,10 +607,10 @@ qht_entry_move(struct qht_bucket *to, int i, struct 
qht_bucket *from, int j)
 qht_debug_assert(to->pointers[i]);
 qht_debug_assert(from->pointers[j]);
 
-to->hashes[i] = from->hashes[j];
+atomic_set(>hashes[i], from->hashes[j]);
 atomic_set(>pointers[i], from->pointers[j]);
 
-from->hashes[j] = 0;
+atomic_set(>hashes[j], 0);
 atomic_set(>pointers[j], NULL);
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v3 02/15] atomic.h: comment on use of atomic_read/set

[Alex Bennée]

Add some notes on the use of the relaxed atomic access helpers and their
importance for defined behaviour in C11's multi-threaded memory model.

Signed-off-by: Alex Bennée 
---
 include/qemu/atomic.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index c493f89..c4f6950 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -92,6 +92,12 @@
 /* Weak atomic operations prevent the compiler moving other
  * loads/stores past the atomic operation load/store. However there is
  * no explicit memory barrier for the processor.
+ *
+ * The C11 memory model says that variables that are accessed from
+ * different threads should at least be done with __ATOMIC_RELAXED
+ * primitives or the result is undefined. Generally this has little to
+ * no effect on the generated code but not using the atomic primitives
+ * will get flagged by sanitizers as a violation.
  */
 #define atomic_read(ptr)  \
 ({\
-- 
2.9.3

[Qemu-devel] [PATCH v3 07/15] qom/cpu: atomically clear the tb_jmp_cache

[Alex Bennée]

The ThreadSanitizer rightly complains that something initialised with a
normal access is later updated and read atomically.

Signed-off-by: Alex Bennée 
---
 qom/cpu.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index 484c493..ef905da 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -253,6 +253,7 @@ void cpu_reset(CPUState *cpu)
 static void cpu_common_reset(CPUState *cpu)
 {
 CPUClass *cc = CPU_GET_CLASS(cpu);
+int i;
 
 if (qemu_loglevel_mask(CPU_LOG_RESET)) {
 qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
@@ -268,7 +269,10 @@ static void cpu_common_reset(CPUState *cpu)
 cpu->can_do_io = 1;
 cpu->exception_index = -1;
 cpu->crash_occurred = false;
-memset(cpu->tb_jmp_cache, 0, TB_JMP_CACHE_SIZE * sizeof(void *));
+
+for (i = 0; i < TB_JMP_CACHE_SIZE; ++i) {
+atomic_set(>tb_jmp_cache[i], NULL);
+}
 }
 
 static bool cpu_common_has_work(CPUState *cs)
-- 
2.9.3

[Qemu-devel] [PATCH v3 00/15] A number of fixes for ThreadSanitizer

[Alex Bennée]

Hi,

This is v3 of the ThreadSanitizer fixes. Changes from the last
version:

  - added some more review tags
  - made clear C11 undefined behaviour is the main issue
  - added two minor fixes to atomic.h
  - change type of tb_page_addr_t back to target_ulong
  - dropped ui/vnc-enc-tight patch (already picked up an merged)
  - dropped the blacklist patch (not needed with gcc)
  - fixed a number of remaining issues with:
- TranslationBuffer access
- PageDesc access

A number of the patches fix fairly rare race conditions. In fact I had
to repeat my test case a number of times to trigger some of them:

  retry.py -n 100 -c -- ./arm-linux-user/qemu-arm ./pigz.armhf \
-c -9 linux-4.6.3.tar > /dev/null 2> tsan-user-async.log

On a build with a light patch to reduce the size of the translation
buffer so we trigger lots of flushes.

With this series applied you can enable ThreadSanitizer with the
following command line:

  ./configure --extra-cflags="-g3 -O0 -fsanitize=thread -D__SANITIZE_THREAD__" \
--with-coroutine=gthread --disable-pie --enable-debug --enable-debug-info

breakdown:
  -fsanitize=thread - enables sanitizer
  --with-coroutine=gthread - tsan chokes on other forms of coroutine
  --disable-pie - tsan no longer works with PIE
  --enable-debug --enable-debug-info - better backtraces

Remaining work:

Running make check with the sanitzer enabled flags up a number of
warnings. I'm going to leave those for others to investigate as I need
to press on with the next MTTCG re-base ;-)

Alex Bennée (14):
  atomic.h: fix __SANITIZE_THREAD__ build
  atomic.h: comment on use of atomic_read/set
  exec-all.h: revert tb_page_addr_t to target_ulong
  tcg/optimize: move default return out of if statement
  qom/object: update class cache atomically
  qom/cpu: atomically clear the tb_jmp_cache
  cpu: atomically modify cpu->exit_request
  util/qht: atomically set b->hashes
  linux-user/syscall: extend lock around cpu-list
  qga/command: use QEMU atomic primitives
  .travis.yml: add gcc sanitizer build
  tcg: ensure cpu_tb_exec/tb_gen_code use atomic_read/write
  tcg: update remaining TranslationBuffer fields atomically
  translate-all: mark updates to PageDesc as atomic

Paolo Bonzini (1):
  seqlock: use atomic writes for the sequence

 .travis.yml | 45 ++
 cpu-exec.c  | 30 -
 include/exec/exec-all.h | 13 +++-
 include/qemu/atomic.h   |  8 -
 include/qemu/seqlock.h  |  4 +--
 linux-user/syscall.c|  7 +++-
 qga/commands.c  | 17 +-
 qom/cpu.c   | 10 --
 qom/object.c| 15 +
 tcg/optimize.c  |  3 +-
 translate-all.c | 86 +
 util/qht.c  | 10 +++---
 12 files changed, 161 insertions(+), 87 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong

[Alex Bennée]

Commit b480d9b74 converted tb_page_addr_t to abi_ulong which while the
right size imposes additional alignment restrictions on the type. This
gets in the way of using atomic accesses on certain guest platforms
which allow finer alignments. As tb_page_addr_t isn't actually visible
to the guest we can revert the change.

This is potentially less efficient for ILP32 style guests but it is the
simpler change to make.

Signed-off-by: Alex Bennée 
---
 include/exec/exec-all.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 336a57c..c3596a6 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -30,7 +30,7 @@
addresses in userspace mode.  Define tb_page_addr_t to be an appropriate
type.  */
 #if defined(CONFIG_USER_ONLY)
-typedef abi_ulong tb_page_addr_t;
+typedef target_ulong tb_page_addr_t;
 #else
 typedef ram_addr_t tb_page_addr_t;
 #endif
-- 
2.9.3

[Qemu-devel] [PATCH v3 08/15] cpu: atomically modify cpu->exit_request

[Alex Bennée]

ThreadSanitizer picks up potential races although we already use
barriers to ensure things are in the correct order when processing exit
requests. For true C11 defined behaviour across threads we need to use
relaxed atomic_set/atomic_read semantics to reassure tsan.

Signed-off-by: Alex Bennée 
---
 cpu-exec.c | 8 
 qom/cpu.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 8823d23..e114fcd 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -192,7 +192,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, 
TranslationBlock *itb)
 /* We were asked to stop executing TBs (probably a pending
  * interrupt. We've now stopped, so clear the flag.
  */
-cpu->tcg_exit_req = 0;
+atomic_set(>tcg_exit_req, 0);
 }
 return ret;
 }
@@ -490,8 +490,8 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
 *last_tb = NULL;
 }
 }
-if (unlikely(cpu->exit_request || replay_has_interrupt())) {
-cpu->exit_request = 0;
+if (unlikely(atomic_read(>exit_request) || replay_has_interrupt())) {
+atomic_set(>exit_request, 0);
 cpu->exception_index = EXCP_INTERRUPT;
 cpu_loop_exit(cpu);
 }
@@ -503,7 +503,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
TranslationBlock *tb,
 {
 uintptr_t ret;
 
-if (unlikely(cpu->exit_request)) {
+if (unlikely(atomic_read(>exit_request))) {
 return;
 }
 
diff --git a/qom/cpu.c b/qom/cpu.c
index ef905da..e765bc0 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -120,10 +120,10 @@ void cpu_reset_interrupt(CPUState *cpu, int mask)
 
 void cpu_exit(CPUState *cpu)
 {
-cpu->exit_request = 1;
+atomic_set(>exit_request, 1);
 /* Ensure cpu_exec will see the exit request after TCG has exited.  */
 smp_wmb();
-cpu->tcg_exit_req = 1;
+atomic_set(>tcg_exit_req, 1);
 }
 
 int cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
-- 
2.9.3

[Qemu-devel] [PATCH v3 15/15] translate-all: mark updates to PageDesc as atomic

[Alex Bennée]

Updates to the internal page table are protected by the mmap_lock.
However for defined C11 semantics things that are access across threads
need to accessed using at least relaxed atomics.

Signed-off-by: Alex Bennée 
---
 translate-all.c | 67 +
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index a6262ae..2d6c0e8 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -805,22 +805,21 @@ static inline void invalidate_page_bitmap(PageDesc *p)
 static void page_flush_tb_1(int level, void **lp)
 {
 int i;
+PageDesc *pd = atomic_rcu_read(lp);
 
-if (*lp == NULL) {
-return;
-}
-if (level == 0) {
-PageDesc *pd = *lp;
+if (pd) {
+if (level == 0) {
 
-for (i = 0; i < V_L2_SIZE; ++i) {
-pd[i].first_tb = NULL;
-invalidate_page_bitmap(pd + i);
-}
-} else {
-void **pp = *lp;
+for (i = 0; i < V_L2_SIZE; ++i) {
+atomic_set([i].first_tb, NULL);
+invalidate_page_bitmap(pd + i);
+}
+} else {
+void **pp = (void **) pd;
 
-for (i = 0; i < V_L2_SIZE; ++i) {
-page_flush_tb_1(level - 1, pp + i);
+for (i = 0; i < V_L2_SIZE; ++i) {
+page_flush_tb_1(level - 1, pp + i);
+}
 }
 }
 }
@@ -1360,7 +1359,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, 
tb_page_addr_t end,
 /* we remove all the TBs in the range [start, end[ */
 /* XXX: see if in some cases it could be faster to invalidate all
the code */
-tb = p->first_tb;
+tb = atomic_read(>first_tb);
 while (tb != NULL) {
 n = (uintptr_t)tb & 3;
 tb = (TranslationBlock *)((uintptr_t)tb & ~3);
@@ -1968,16 +1967,15 @@ void page_set_flags(target_ulong start, target_ulong 
end, int flags)
the code inside.  */
 if (!(p->flags & PAGE_WRITE) &&
 (flags & PAGE_WRITE) &&
-p->first_tb) {
+atomic_read(>first_tb)) {
 tb_invalidate_phys_page(addr, 0);
 }
-p->flags = flags;
+atomic_set(>flags, flags);
 }
 }
 
 int page_check_range(target_ulong start, target_ulong len, int flags)
 {
-PageDesc *p;
 target_ulong end;
 target_ulong addr;
 
@@ -2003,28 +2001,31 @@ int page_check_range(target_ulong start, target_ulong 
len, int flags)
 for (addr = start, len = end - start;
  len != 0;
  len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
-p = page_find(addr >> TARGET_PAGE_BITS);
-if (!p) {
-return -1;
-}
-if (!(p->flags & PAGE_VALID)) {
-return -1;
-}
+PageDesc *p = page_find(addr >> TARGET_PAGE_BITS);
+if (p) {
+int cur_flags = atomic_read(>flags);
 
-if ((flags & PAGE_READ) && !(p->flags & PAGE_READ)) {
-return -1;
-}
-if (flags & PAGE_WRITE) {
-if (!(p->flags & PAGE_WRITE_ORG)) {
+if (!(cur_flags & PAGE_VALID)) {
 return -1;
 }
-/* unprotect the page if it was put read-only because it
-   contains translated code */
-if (!(p->flags & PAGE_WRITE)) {
-if (!page_unprotect(addr, 0)) {
+
+if ((flags & PAGE_READ) && !(cur_flags & PAGE_READ)) {
+return -1;
+}
+if (flags & PAGE_WRITE) {
+if (!(cur_flags & PAGE_WRITE_ORG)) {
 return -1;
 }
+/* unprotect the page if it was put read-only because it
+   contains translated code */
+if (!(cur_flags & PAGE_WRITE)) {
+if (!page_unprotect(addr, 0)) {
+return -1;
+}
+}
 }
+} else {
+return -1;
 }
 }
 return 0;
-- 
2.9.3

[Qemu-devel] [PATCH v3 01/15] atomic.h: fix __SANITIZE_THREAD__ build

[Alex Bennée]

Only very modern GCC's actually set this define when building with the
ThreadSanitizer so this little typo slipped though.

Signed-off-by: Alex Bennée 
---
 include/qemu/atomic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 0cce246..c493f89 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -82,7 +82,7 @@
  */
 #if defined(__SANITIZE_THREAD__)
 #define smp_read_barrier_depends() ({ barrier(); 
__atomic_thread_fence(__ATOMIC_CONSUME); })
-#elsif defined(__alpha__)
+#elif defined(__alpha__)
 #define smp_read_barrier_depends()   asm volatile("mb":::"memory")
 #else
 #define smp_read_barrier_depends()   barrier()
-- 
2.9.3

[Qemu-devel] [PATCH v3 10/15] linux-user/syscall: extend lock around cpu-list

[Alex Bennée]

There is a potential race if several threads exit at once. To serialise
the exits extend the lock above the initial checking of the CPU list.

Signed-off-by: Alex Bennée 
---
 linux-user/syscall.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0815f30..fa559be 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7476,13 +7476,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 break;
 }
 
+cpu_list_lock();
+
 if (CPU_NEXT(first_cpu)) {
 TaskState *ts;
 
-cpu_list_lock();
 /* Remove the CPU from the list.  */
 QTAILQ_REMOVE(, cpu, node);
+
 cpu_list_unlock();
+
 ts = cpu->opaque;
 if (ts->child_tidptr) {
 put_user_u32(0, ts->child_tidptr);
@@ -7495,6 +7498,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 rcu_unregister_thread();
 pthread_exit(NULL);
 }
+
+cpu_list_unlock();
 #ifdef TARGET_GPROF
 _mcleanup();
 #endif
-- 
2.9.3

[Qemu-devel] [PATCH v3 04/15] tcg/optimize: move default return out of if statement

[Alex Bennée]

This is to appease sanitizer builds which complain that:

  "error: control reaches end of non-void function"

Signed-off-by: Alex Bennée 
Reviewed-by: Marc-André Lureau 
---
 tcg/optimize.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 9998ac7..0f13490 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -468,9 +468,8 @@ static TCGArg do_constant_folding_cond(TCGOpcode op, TCGArg 
x,
 default:
 return 2;
 }
-} else {
-return 2;
 }
+return 2;
 }
 
 /* Return 2 if the condition can't be simplified, and the result
-- 
2.9.3

[Qemu-devel] [PATCH v3 06/15] qom/object: update class cache atomically

[Alex Bennée]

The idiom CPU_GET_CLASS(cpu) is fairly extensively used in various
threads and trips of ThreadSanitizer due to the fact it updates
obj->class->object_cast_cache behind the scenes. As this is just a
fast-path cache there is no need to lock updates.

However to ensure defined C11 behaviour across threads we need to use
the plain atomic_read/set primitives and keep the sanitizer happy.

Signed-off-by: Alex Bennée 
Reviewed-by: Marc-André Lureau 
---
 qom/object.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 8166b7d..7a05e35 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -614,7 +614,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char 
*typename,
 Object *inst;
 
 for (i = 0; obj && i < OBJECT_CLASS_CAST_CACHE; i++) {
-if (obj->class->object_cast_cache[i] == typename) {
+if (atomic_read(>class->object_cast_cache[i]) == typename) {
 goto out;
 }
 }
@@ -631,10 +631,10 @@ Object *object_dynamic_cast_assert(Object *obj, const 
char *typename,
 
 if (obj && obj == inst) {
 for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
-obj->class->object_cast_cache[i - 1] =
-obj->class->object_cast_cache[i];
+atomic_set(>class->object_cast_cache[i - 1],
+   atomic_read(>class->object_cast_cache[i]));
 }
-obj->class->object_cast_cache[i - 1] = typename;
+atomic_set(>class->object_cast_cache[i - 1], typename);
 }
 
 out:
@@ -704,7 +704,7 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass 
*class,
 int i;
 
 for (i = 0; class && i < OBJECT_CLASS_CAST_CACHE; i++) {
-if (class->class_cast_cache[i] == typename) {
+if (atomic_read(>class_cast_cache[i]) == typename) {
 ret = class;
 goto out;
 }
@@ -725,9 +725,10 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass 
*class,
 #ifdef CONFIG_QOM_CAST_DEBUG
 if (class && ret == class) {
 for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
-class->class_cast_cache[i - 1] = class->class_cast_cache[i];
+atomic_set(>class_cast_cache[i - 1],
+   atomic_read(>class_cast_cache[i]));
 }
-class->class_cast_cache[i - 1] = typename;
+atomic_set(>class_cast_cache[i - 1], typename);
 }
 out:
 #endif
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Paolo Bonzini]

On 30/09/2016 20:33, Eduardo Habkost wrote:
> On Fri, Sep 30, 2016 at 09:55:33AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 29/09/2016 23:14, Eduardo Habkost wrote:
>>> + * "-foo" overrides "+foo"
>>> + * "[+-]foo" overrides "foo=..."
>>
>> Is this something that people are actually using?  Can we detect it and
>> deprecate it in 2.8, and drop it in 2.9?
> 
> We can, but I would like to keep the test cases there in 2.8, at
> least. I will update the test case to note that this is legacy
> behavior that we plan to remove in 2.9, and send a separate
> follow-up patch to detect when people mix both formats.

Yes, of course!  Sounds like a very good plan.

Paolo

Re: [Qemu-devel] [libvirt] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 02:56:42PM -0500, Eric Blake wrote:
> On 09/30/2016 01:49 PM, Eduardo Habkost wrote:
> > Registering the actual names containing underscores as aliases
> > will allow management software to be aware that the old
> > compatibility names are suported, and will make feat2prop() calls
> 
> s/suported/supported/

Fixed, thanks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> If given an option string such as
> 
>   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
> 
> the qemu_opts_to_qdict() method will currently overwrite
> the values for repeated option keys, so only the last
> value is in the returned dict:
> 
> size=QString("1024")
> nodes=QString("1-2")
> policy=QString("bind")
> 
> With this change the caller can optionally ask for all
> the repeated values to be stored in a QList. In the
> above example that would result in 'nodes' being a
> QList, so the returned dict would contain
> 
> size=QString("1024")
> nodes=QList([QString("10"),
>  QString("4-5"),
>  QString("1-2")])
> policy=QString("bind")

I think the last time around you were converting keys (turning "nodes"
into "nodes.0"; I think your idea here of keeping a single key tied to
QList makes more sense.

> 
> Note that the conversion has no way of knowing whether
> any given key is expected to be a list upfront - it can
> only figure that out when seeing the first duplicated
> key. Thus the caller has to be prepared to deal with the
> fact that if a key 'foo' is a list, then the returned
> qdict may contain either a QString or a QList for the
> key 'foo'.
> 
> In a third mode, it is possible to ask for repeated
> options to be reported as an error, rather than silently
> dropping all but the last one.
> 
> All existing callers are all converted to explicitly
> request the historical behaviour of only reporting the
> last key. Later patches will make use of the new modes.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

> -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict);
> +typedef enum {
> +/* Last occurrence of a duplicate option silently replaces earlier */
> +QEMU_OPTS_REPEAT_POLICY_LAST,
> +/* Each occurrence of a duplicate option converts value to a QList */
> +QEMU_OPTS_REPEAT_POLICY_ALL,
> +/* First occurrence of a duplicate option causes an error */
> +QEMU_OPTS_REPEAT_POLICY_ERROR,
> +} QemuOptsRepeatPolicy;
> +

Nicer.  Thanks!

> +++ b/tests/test-qemu-opts.c
> @@ -421,6 +421,130 @@ static void test_qemu_opts_set(void)
>  g_assert(opts == NULL);
>  }
>  

> +static void test_qemu_opts_to_qdict_repeat_all(void)
> +{
> +QemuOpts *opts;
> +QDict *dict;
> +QList *list;
> +const QListEntry *ent;
> +QString *str;
> +
> +/* dynamically initialized (parsed) opts */
> +opts = qemu_opts_parse(_list_03,
> +   
> "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind",
> +   false, NULL);
> +g_assert(opts);
> +
> +dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ALL,
> +  _abort);
> +g_assert(dict);
> +
> +g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024");
> +g_assert(qdict_haskey(dict, "nodes"));
> +list = qobject_to_qlist(qdict_get(dict, "nodes"));
> +g_assert(list);
> +
> +ent = qlist_first(list);
> +g_assert(ent);
> +str = qobject_to_qstring(ent->value);
> +g_assert(str);
> +g_assert_cmpstr(qstring_get_str(str), ==, "10");
> +
> +ent = qlist_next(ent);
> +g_assert(ent);
> +str = qobject_to_qstring(ent->value);
> +g_assert(str);
> +g_assert_cmpstr(qstring_get_str(str), ==, "4-5");
> +
> +ent = qlist_next(ent);
> +g_assert(ent);
> +str = qobject_to_qstring(ent->value);
> +g_assert(str);
> +g_assert_cmpstr(qstring_get_str(str), ==, "1-2");

Yes, this is different from v13, but does look nicer with the value
turning into QList instead of the key being rewritten into dotted form.

> +++ b/util/qemu-option.c
> @@ -1057,23 +1057,73 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict 
> *qdict, Error **errp)
>   * The QDict values are of type QString.
>   * TODO We'll want to use types appropriate for opt->desc->type, but
>   * this is enough for now.
> + *
> + * If the @opts contains multiple occurrences of the same key,
> + * then the @repeatPolicy parameter determines how they are to
> + * be handled. Traditional behaviour was to only store the
> + * last occurrence, but if @repeatPolicy is set to
> + * QEMU_OPTS_REPEAT_POLICY_ALL, then a QList will be returned
> + * containing all values, for any key with multiple occurrences.
> + * The QEMU_OPTS_REPEAT_POLICY_ERROR value can be used to fail
> + * conversion with an error if multiple occurrences of a key
> + * are seen.
>   */
> -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict)
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict,
> +  QemuOptsRepeatPolicy repeatPolicy,
> +  Error **errp)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [libvirt] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eric Blake]

On 09/30/2016 01:49 PM, Eduardo Habkost wrote:
> Registering the actual names containing underscores as aliases
> will allow management software to be aware that the old
> compatibility names are suported, and will make feat2prop() calls

s/suported/supported/

> unnecessary when using feature names.
> 
> Also, this will help us avoid making the code support underscores
> on feature names that never had them in the first place. e.g.
> "+tsc_deadline" was never supported and doesn't need to be
> translated to "+tsc-deadline".
> 
> In other word: this will require less magic translation of
> strings, and simple 1:1 match between the config options and
> actual QOM properties.
> 
> Note that the underscores are still present in the
> FeatureWordInfo::feat_names arrays, because
> add_flagname_to_bitmaps() needs them to be kept. The next patches
> will remove add_flagname_to_bitmaps() and will allow us to
> finally remove the aliases from feat_names.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  target-i386/cpu.c | 22 ++
>  1 file changed, 22 insertions(+)
> 

-- 
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 v14 11/21] qapi: add integer range support for QObjectInputVisitor

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> The traditional CLI arg syntax allows two ways to specify
> integer lists, either one value per key, or a range of
> values per key. eg the following are identical:
> 
>   -arg foo=5,foo=6,foo=7
>   -arg foo=5-7
> 
> This extends the QObjectInputVisitor so that it is able
> to parse ranges and turn them into distinct list entries.
> 
> This means that
> 
>   -arg foo=5-7
> 
> is treated as equivalent to
> 
>   -arg foo.0=5,foo.1=6,foo.2=7
> 
> Edge case tests are copied from test-opts-visitor to
> ensure identical behaviour when parsing.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

> @@ -71,6 +77,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The value given determines how many levels of structs are allowed to
>   * be flattened in this way.
>   *
> + * If @permit_int_ranges is true, then when visiting a list of integers,
> + * the integer value strings may encode ranges eg a single element

My usual complaint that 'e.g.' is usually spelled with dots :)

> + * containing "5-7" is treated as if there were three elements "5", "6",
> + * "7". This should only be used if compatibility is required with the
> + * OptsVisitor which would allow integer ranges. e.g. set this to true

Ah. Here you DID use dots, but used the wrong abbreviation.
Substituting "For example" in this location does not make as much sense
as "i.e." would.  Or go for simplicity; it reads just fine as:

"...would allow integer ranges; so only set this to true if..."

> + * if you have compatibility requirements for
> + *
> + *   -arg val=5-8
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + *   -arg val.0=5,val.1=6,val.2=7,val.3=8
> + *

> +
> +/*
> + * If we have string that represents an integer range (5-24),
> + * parse the end of the range and set things up so we'll process
> + * the rest of the range before consuming another element
> + * from the QList.
> + */
> +if (end && *end) {
> +if (!qiv->permit_int_ranges) {
> +error_setg(errp,
> +   "Integer ranges are not permitted here");
> +return;
> +}
> +if (!inlist) {
> +error_setg(errp,
> +   "Integer ranges are only permitted when "
> +   "visiting list parameters");
> +return;
> +}
> +if (*end != '-') {
> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +   "a number range");
> +return;
> +}
> +end++;
> +if (qemu_strtoll(end, NULL, 0, ) < 0) {

Signed parse...

> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
> +return;
> +}
> +if (*obj > ret) {

...and signed comparison...

> +error_setg(errp,
> +   "Parameter '%s' range start %" PRIu64
> +   " must be less than (or equal to) %" PRIu64,

...but unsigned numbers in the error message.  Looks a bit odd (but not
necessarily worse than what our OptsVisitor was already doing).

> +   name, *obj, ret);
> +return;
> +}
> +
> +if ((*obj <= (INT64_MAX - QIV_RANGE_MAX)) &&
> +(ret >= (*obj + QIV_RANGE_MAX))) {
> +error_setg(errp,
> +   "Parameter '%s' range must be less than %d",
> +   name, QIV_RANGE_MAX);
> +return;
> +}
> +if (*obj != ret) {
> +tos->range_val = *obj;
> +tos->range_limit = ret;
> +}
> +}
>  }

Overall, I found this a bit easier to read than the OptsVisitor state
machine.  Which makes me worried that we may have some subtle
translation bugs; but I hope it's the same end result.

My consolation is that if there are any differences, they would be in
the corner cases, where a user is probably going to get surprising
behavior from the command line anyway, and shouldn't have been
requesting ranges in that corner case.

>  
>  static void qobject_input_type_uint64(Visitor *v, const char *name,
> @@ -366,21 +438,85 @@ static void qobject_input_type_uint64_autocast(Visitor 
> *v, const char *name,
> uint64_t *obj, Error **errp)
>  {
>  QObjectInputVisitor *qiv = to_qiv(v);

Feels like a lot of code duplication; but I don't know if there's any
sane way to share more of the code while handling both signed and
unsigned types.

The testsuite additions are reassuring; I confirmed that they match
test-opts-visitor.c in coverage.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 4/4] qemu-iotests: Test creating floppy drives

[Kevin Wolf]

This tests the different supported methods to create floppy drives and
how they interact.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1205 
 tests/qemu-iotests/group   |1 +
 3 files changed, 1448 insertions(+)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
new file mode 100755
index 000..8bb6443
--- /dev/null
+++ b/tests/qemu-iotests/172
@@ -0,0 +1,242 @@
+#!/bin/bash
+#
+# Test floppy configuration
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
+function do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@"
+echo
+}
+
+function check_floppy_qtree()
+{
+echo
+echo Testing: "$@" | _filter_testdir
+
+# QEMU_OPTIONS contains -nodefaults, we don't want that here because the
+# defaults are part of what should be checked here
+echo "info qtree" |
+QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 |
+grep -zo '[[:cntrl:]]\( *\)dev: isa-fdc.*\([[:cntrl:]]\1 .*\)*[[:cntrl:]] 
*dev:'
+}
+
+function check_cache_mode()
+{
+echo "info block none0" |
+QEMU_OPTIONS="" do_run_qemu -drive if=none,file="$TEST_IMG" "$@" |
+_filter_win32 | grep "Cache mode"
+}
+
+
+size=720k
+
+_make_test_img $size
+
+# Default drive semantics:
+#
+# By default you get a single empty floppy drive. You can override it with
+# -drive and using the same index, but if you use -drive to add a floppy to a
+# different index, you get both of them. However, as soon as you use any
+# '-device floppy', even to a different slot, the default drive is disabled.
+
+echo
+echo
+echo === Default ===
+
+check_floppy_qtree
+
+echo
+echo
+echo === Using -fda/-fdb options ===
+
+check_floppy_qtree -fda "$TEST_IMG"
+check_floppy_qtree -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+
+
+echo
+echo
+echo === Using -drive options ===
+
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+
+echo
+echo
+echo === Using -drive if=none and -global ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
+
+echo
+echo
+echo === Using -drive if=none and -device ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -device floppy,drive=none0 -device floppy,drive=none1,unit=1
+
+echo
+echo
+echo === Mixing -fdX and -global ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+
+# Conflicting (-fdX wins)
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+
+echo
+echo
+echo === Mixing -fdX and -device ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive 

[Qemu-devel] [PATCH v2 3/4] fdc: Move qdev properties to FloppyDrive

[Kevin Wolf]

This makes the FloppyDrive qdev object actually useful: Now that it has
all properties that don't belong to the controller, you can actually
use '-device floppy' and get a working result.

Command line semantics is consistent with CD-ROM drives: By default you
get a single empty floppy drive. You can override it with -drive and
using the same index, but if you use -drive to add a floppy to a
different index, you get both of them. However, as soon as you use any
'-device floppy', even to a different slot, the default drive is
disabled.

Using '-device floppy' without specifying the unit will choose the first
free slot on the controller.

Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 112 ++---
 vl.c   |   1 +
 2 files changed, 85 insertions(+), 28 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5aa8e52..00c0ec6 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -35,6 +35,7 @@
 #include "qemu/timer.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -487,12 +488,18 @@ static const BlockDevOps fd_block_ops = {
  OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
 
 typedef struct FloppyDrive {
-DeviceState qdev;
-uint32_tunit;
+DeviceState qdev;
+uint32_tunit;
+BlockConf   conf;
+FloppyDriveType type;
 } FloppyDrive;
 
 static Property floppy_drive_properties[] = {
 DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_BLOCK_PROPERTIES(FloppyDrive, conf),
+DEFINE_PROP_DEFAULT("drive-type", FloppyDrive, type,
+FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+FloppyDriveType),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -501,6 +508,7 @@ static int floppy_drive_init(DeviceState *qdev)
 FloppyDrive *dev = FLOPPY_DRIVE(qdev);
 FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
 FDrive *drive;
+int ret;
 
 if (dev->unit == -1) {
 for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
@@ -517,29 +525,57 @@ static int floppy_drive_init(DeviceState *qdev)
 return -1;
 }
 
-/* TODO Check whether unit is in use */
-
 drive = get_drv(bus->fdc, dev->unit);
-
 if (drive->blk) {
-if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-error_report("fdc doesn't support drive option werror");
-return -1;
-}
-if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-error_report("fdc doesn't support drive option rerror");
-return -1;
-}
-} else {
+error_report("Floppy unit %d is in use", dev->unit);
+return -1;
+}
+
+if (!dev->conf.blk) {
 /* Anonymous BlockBackend for an empty drive */
-drive->blk = blk_new();
+dev->conf.blk = blk_new();
+ret = blk_attach_dev(dev->conf.blk, dev);
+assert(ret == 0);
 }
 
-fd_init(drive);
-if (drive->blk) {
-blk_set_dev_ops(drive->blk, _block_ops, drive);
-pick_drive_type(drive);
+blkconf_blocksizes(>conf);
+if (dev->conf.logical_block_size != 512 ||
+dev->conf.physical_block_size != 512)
+{
+error_report("Physical and logical block size must be 512 for floppy");
+return -1;
+}
+
+/* rerror/werror aren't supported by fdc and therefore not even registered
+ * with qdev. So set the defaults manually before they are used in
+ * blkconf_apply_backend_options(). */
+dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
+dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
+blkconf_apply_backend_options(>conf);
+
+/* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
+ * for empty drives. */
+if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
+blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option werror");
+return -1;
 }
+if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+
+drive->blk = dev->conf.blk;
+drive->fdctrl = bus->fdc;
+
+fd_init(drive);
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+
+/* Keep 'type' qdev property and FDrive->drive in sync */
+drive->drive = dev->type;
+pick_drive_type(drive);
+dev->type = drive->drive;
+
 fd_revalidate(drive);
 
 return 0;
@@ -808,6 +844,10 @@ struct FDCtrl {
 FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
+struct {
+BlockBackend *blk;
+FloppyDriveType type;
+} qdev_for_drives[MAX_FD];
 int reset_sensei;
 uint32_t check_media_rate;
 FloppyDriveType fallback; /* 

[Qemu-devel] [PATCH v2 1/4] fdc: Add a floppy qbus

[Kevin Wolf]

This adds a qbus to the floppy controller that should contain the floppy
drives eventually. At the moment it just exists and is empty.

Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 40 +++-
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b79873a..a3afb62 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -52,6 +52,33 @@
 }   \
 } while (0)
 
+
+//
+/* qdev floppy bus  */
+
+#define TYPE_FLOPPY_BUS "Floppy"
+#define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FloppyBus {
+BusState bus;
+FDCtrl *fdc;
+} FloppyBus;
+
+static const TypeInfo floppy_bus_info = {
+.name = TYPE_FLOPPY_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(FloppyBus),
+};
+
+static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
+{
+qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL);
+bus->fdc = fdc;
+}
+
+
 //
 /* Floppy drive emulation   */
 
@@ -148,8 +175,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC   2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
 typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
@@ -684,6 +709,7 @@ struct FDCtrl {
 /* Power down config (also with status regB access mode */
 uint8_t pwrd;
 /* Floppy drives */
+FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
 int reset_sensei;
@@ -2442,7 +2468,8 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
 *fdc_tc = qdev_get_gpio_in(dev, 0);
 }
 
-static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
+  Error **errp)
 {
 int i, j;
 static int command_tables_inited = 0;
@@ -2480,6 +2507,8 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error 
**errp)
 k->register_channel(fdctrl->dma, fdctrl->dma_chann,
 _transfer_handler, fdctrl);
 }
+
+floppy_bus_create(fdctrl, >bus, dev);
 fdctrl_connect_drives(fdctrl, errp);
 }
 
@@ -2508,7 +2537,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error 
**errp)
 }
 
 qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-fdctrl_realize_common(fdctrl, );
+fdctrl_realize_common(dev, fdctrl, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
@@ -2559,7 +2588,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, 
Error **errp)
 FDCtrlSysBus *sys = SYSBUS_FDC(dev);
 FDCtrl *fdctrl = >state;
 
-fdctrl_realize_common(fdctrl, errp);
+fdctrl_realize_common(dev, fdctrl, errp);
 }
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
@@ -2744,6 +2773,7 @@ static void fdc_register_types(void)
 type_register_static(_fdc_type_info);
 type_register_static(_fdc_info);
 type_register_static(_fdc_info);
+type_register_static(_bus_info);
 }
 
 type_init(fdc_register_types)
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 0/4] fdc: Use separate qdev device for drives

[Kevin Wolf]

We have been complaining for a long time about how the floppy controller and
floppy drives are combined in a single qdev device and how this makes the
device awkward to work with because it behaves different from all other block
devices.

The latest reason to complain was when I noticed that using qdev device names
in QMP commands (e.g. for media change) doesn't really work when only the
controller is a qdev device, but the drives aren't.

So I decided to have a go at it, and this is the result.

It doesn't actually change any of the inner workings of the floppy controller,
but it wires things up differently on the qdev layer so that a floppy
controller now exposes a bus on which the floppy drives sit. This results in a
structure that is similar to IDE where the actual drive state is still in the
controller and the qdev device basically just contains the qdev properties -
not pretty, but quite workable.

The commit message of patch 3 explains how to use it. In short, there is a
'-device floppy' now and it does what you would expect if you ever used ide-cd.

The other problem is old command lines, especially those using things like
'-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
the property to an internally created floppy drive device. This is a bit like
usb-storage, which we know is ugly, but works well enough in practice. The good
news here is that in contrast to usb-storage, the floppy controller only does
the forwarding for legacy configurations; as soon as you start using '-device
floppy', it doesn't happen any more.

So as you may have expected, this conversion doesn't result in a perfect
device, but I think it's definitely an improvement over the old state. I hope
you like it despite the warts. :-)

v2:
- Added patch 4 (qemu-iotests case for floppy config on the command line)
- Patch 2: Create a floppy device only if a BlockBackend exists instead of
  always creating two of them
- Patch 2: Initialise drive->fdctrl even if no drive is attached, it is
  accessed anyway during migration
- Patch 3: Keep 'type' qdev property and FDrive->drive in sync
- Patch 3: Removed if with condition that is always true

Kevin Wolf (4):
  fdc: Add a floppy qbus
  fdc: Add a floppy drive qdev
  fdc: Move qdev properties to FloppyDrive
  qemu-iotests: Test creating floppy drives

 hw/block/fdc.c |  263 --
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1205 
 tests/qemu-iotests/group   |1 +
 vl.c   |1 +
 5 files changed, 1668 insertions(+), 44 deletions(-)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/4] fdc: Add a floppy drive qdev

[Kevin Wolf]

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)

Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 151 +
 1 file changed, 120 insertions(+), 31 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a3afb62..5aa8e52 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
 
 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);
 
 typedef struct FloppyBus {
 BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;
 
-typedef struct FDrive {
+struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
 uint8_t media_rate;   /* Data rate of medium*/
 
 bool media_validated; /* Have we validated the media? */
-} FDrive;
+};
 
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,100 @@ static void fd_revalidate(FDrive *drv)
 }
 }
 
+static void fd_change_cb(void *opaque, bool load)
+{
+FDrive *drive = opaque;
+
+drive->media_changed = 1;
+drive->media_validated = false;
+fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+.change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+ OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+DeviceState qdev;
+uint32_tunit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
+FDrive *drive;
+
+if (dev->unit == -1) {
+for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+drive = get_drv(bus->fdc, dev->unit);
+if (!drive->blk) {
+break;
+}
+}
+}
+
+if (dev->unit >= MAX_FD) {
+error_report("Can't create floppy unit %d, bus supports only %d units",
+ dev->unit, MAX_FD);
+return -1;
+}
+
+/* TODO Check whether unit is in use */
+
+drive = get_drv(bus->fdc, dev->unit);
+
+if (drive->blk) {
+if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+error_report("fdc doesn't support drive option werror");
+return -1;
+}
+if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+} else {
+/* Anonymous BlockBackend for an empty drive */
+drive->blk = blk_new();
+}
+
+fd_init(drive);
+if (drive->blk) {
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+pick_drive_type(drive);
+}
+fd_revalidate(drive);
+
+return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *k = DEVICE_CLASS(klass);
+k->init = floppy_drive_init;
+set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+k->bus_type = TYPE_FLOPPY_BUS;
+k->props = floppy_drive_properties;
+k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+.name = TYPE_FLOPPY_DRIVE,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(FloppyDrive),
+.class_init = floppy_drive_class_init,
+};
+
 //
 /* Intel 82078 floppy disk controller emulation  */
 
@@ -1185,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif
 
-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {
-switch (fdctrl->cur_drv) {
+switch (unit) {
 case 0: return drv0(fdctrl);
 case 1: return drv1(fdctrl);
 #if MAX_FD == 4
@@ -1198,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
 }
 }
 
+static FDrive *get_cur_drv(FDCtrl *fdctrl)
+{
+return get_drv(fdctrl, fdctrl->cur_drv);
+}
+
 /* Status A register : 0x00 (read-only) */
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
 {
@@ -2357,46 +2458,33 @@ static void fdctrl_result_timer(void *opaque)
 }
 }
 
-static void fdctrl_change_cb(void *opaque, bool load)
-{
-FDrive *drive = opaque;
-
-drive->media_changed = 1;
-drive->media_validated = false;
-fd_revalidate(drive);
-}
-
-static const BlockDevOps fdctrl_block_ops = {
-.change_media_cb = fdctrl_change_cb,
-};
-
 /* Init functions */
 static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
 {
 unsigned int i;
 FDrive *drive;
+

Re: [Qemu-devel] [PATCH v4 03/12] block/nbd: Default port in nbd_refresh_filename()

[Eric Blake]

On 09/28/2016 03:55 PM, Max Reitz wrote:
> Instead of not emitting the port in nbd_refresh_filename(), just set it
> to the default if the user did not specify it. This makes the logic a
> bit simpler.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 19 ++-
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature

[Michael S. Tsirkin]

On Fri, Sep 30, 2016 at 02:05:10PM +0200, Maxime Coquelin wrote:
> 
> 
> On 09/29/2016 11:23 PM, Maxime Coquelin wrote:
> > 
> > 
> > On 09/29/2016 10:21 PM, Michael S. Tsirkin wrote:
> > > On Thu, Sep 29, 2016 at 10:05:22PM +0200, Maxime Coquelin wrote:
> > > > 
> > > > 
> > > > On 09/29/2016 07:57 PM, Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 29, 2016 at 05:30:53PM +0200, Maxime Coquelin wrote:
> > > > ...
> > > > > > 
> > > > > > Before enabling anything by default, we should first optimize the 1
> > > > > > slot
> > > > > > case. Indeed, micro-benchmark using testpmd in txonly[0] shows ~17%
> > > > > > perf regression for 64 bytes case:
> > > > > >  - 2 descs per packet: 11.6Mpps
> > > > > >  - 1 desc per packet: 9.6Mpps
> > > > > > 
> > > > > > This is due to the virtio header clearing in 
> > > > > > virtqueue_enqueue_xmit().
> > > > > > Removing it, we get better results than with 2 descs (1.20Mpps).
> > > > > > Since the Virtio PMD doesn't support offloads, I wonder whether we 
> > > > > > can
> > > > > > just drop the memset?
> > > > > 
> > > > > What will happen? Will the header be uninitialized?
> > > > Yes..
> > > > I didn't look closely at the spec, but just looked at DPDK's and Linux
> > > > vhost implementations. IIUC, the header is just skipped in the two
> > > > implementations.
> > > 
> > > In linux guest skbs are initialized AFAIK. See virtio_net_hdr_from_skb
> > > first thing it does is
> > > memset(hdr, 0, sizeof(*hdr));
> > 
> > I meant in vhost-net linux implementation, the header is just skipped:
> > 
> > static void handle_tx(struct vhost_net *net)
> > {
> > ...
> > /* Skip header. TODO: support TSO. */
> > len = iov_length(vq->iov, out);
> > iov_iter_init(_iter, WRITE, vq->iov, out, len);
> > iov_iter_advance(_iter, hdr_size);
> > 
> > And the same is done is done in DPDK:
> > 
> > static inline int __attribute__((always_inline))
> > copy_desc_to_mbuf(struct virtio_net *dev, struct vring_desc *descs,
> >   uint16_t max_desc, struct rte_mbuf *m, uint16_t desc_idx,
> >   struct rte_mempool *mbuf_pool)
> > {
> > ...
> > /*
> >  * A virtio driver normally uses at least 2 desc buffers
> >  * for Tx: the first for storing the header, and others
> >  * for storing the data.
> >  */
> > if (likely((desc->len == dev->vhost_hlen) &&
> >(desc->flags & VRING_DESC_F_NEXT) != 0)) {
> > desc = [desc->next];
> > if (unlikely(desc->flags & VRING_DESC_F_INDIRECT))
> > return -1;
> > 
> > desc_addr = gpa_to_vva(dev, desc->addr);
> > if (unlikely(!desc_addr))
> > return -1;
> > 
> > rte_prefetch0((void *)(uintptr_t)desc_addr);
> > 
> > desc_offset = 0;
> > desc_avail  = desc->len;
> > nr_desc+= 1;
> > 
> > PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
> > } else {
> > desc_avail  = desc->len - dev->vhost_hlen;
> > desc_offset = dev->vhost_hlen;
> > }
> 
> Actually, the header is parsed in DPDK vhost implementation.
> But as Virtio PMD provides a zero'ed header, we could just parse
> the header only if VIRTIO_NET_F_NO_TX_HEADER is not negotiated.

host can always skip the header parse if it wants to.
It didn't seem worth it to add branches there but
if I'm wrong, by all means code it up.


> > > 
> > > 
> > > 
> > > > > 
> > > > > The spec says:
> > > > > The driver can send a completely checksummed packet. In this
> > > > > case, flags
> > > > > will be zero, and gso_type
> > > > > will be VIRTIO_NET_HDR_GSO_NONE.
> > > > > 
> > > > > and
> > > > > The driver MUST set num_buffers to zero.
> > > > > If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> > > > > flags to
> > > > > zero and SHOULD supply a fully
> > > > > checksummed packet to the device.
> > > > > 
> > > > > and
> > > > > If none of the VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO options have
> > > > > been
> > > > > negotiated, the driver MUST
> > > > > set gso_type to VIRTIO_NET_HDR_GSO_NONE.
> > > > > 
> > > > > so doing this unconditionally would be a spec violation, but if you 
> > > > > see
> > > > > value in this, we can add a feature bit.
> > > > Right it would be a spec violation, so it should be done conditionally.
> > > > If a feature bit is to be added, what about VIRTIO_NET_F_NO_TX_HEADER?
> > > > It would imply VIRTIO_NET_F_CSUM not set, and no GSO features set.
> > > > If negotiated, we wouldn't need to prepend a header.
> > > 
> > > Yes but two points.
> > > 
> > > 1. why is this memset expensive? Is the test completely skipping looking
> > >at the packet otherwise?
> > Yes.
> > > 
> > > 2. As long as we are doing this, see
> > > Alignment vs. Networking
> > > 
> > > in Documentation/unaligned-memory-access.txt
> > Thanks, I'll have a look tomorrow.
> 
> I did a rough prototype which removes Tx headers unconditionally, 

Re: [Qemu-devel] [PATCH v14 00/21] QAPI/QOM work for non-scalar object properties

[Eric Blake]

On 09/30/2016 10:45 AM,
no-re...@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> Hi,
> 
> Your series failed automatic build test. Please find the testing commands and
> their output below. If you have docker installed, you can probably reproduce 
> it
> locally.
> 

>   CCqapi/string-output-visitor.o
> /tmp/qemu-test/src/qapi/qobject-input-visitor.c: In function 
> ‘qobject_input_get_object’:
> /tmp/qemu-test/src/qapi/qobject-input-visitor.c:97: warning: implicit 
> declaration of function ‘g_hash_table_contains’
> /tmp/qemu-test/src/qapi/qobject-input-visitor.c:97: warning: nested extern 
> declaration of ‘g_hash_table_contains’

> 
> tests/docker/Makefile.include:107: recipe for target 
> 'docker-run-test-quick@centos6' failed
> make: *** [docker-run-test-quick@centos6] Error 2

Cool - the autobuilder is now detecting use of too-new glib functions :)

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC] Tracing guest register usage

[Richard Henderson]

On 09/30/2016 08:13 AM, Lluís Vilanova wrote:

(2) an internal state change
to DisasContext, reflected in INDEX_op_insn_start, with no changes to any TCG
registers.  So you'll not actually be tracking eflags at all.


I don't follow what you mean. AFAIK INDEX_op_insn_start does not change the
guest eflags.


It sets cc_op, which affects how eflags is computed.


r~

[Qemu-devel] [PATCH v5 11/12] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Add a new optional field to query-cpu-definitions schema:
"unavailable-features". It will contain a list of QOM properties
that prevent the CPU model from running in the current host.

Cc: David Hildenbrand 
Cc: Michael Mueller 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Jiri Denemark 
Cc: libvir-l...@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Changed doc to "since 2.8" as we missed 2.7
  * Reported-by: Eric Blake 

Changes v2 -> v3:
* Small documentation reword
  * Suggested-by: Markus Armbruster 

Changes v1 -> v2:
* Remove @runnable field, non-empty @unavailable-features is
  enough to report CPU model as not runnable.
* Documentation updates:
  * Changed to "(since 2.7)";
  * Add more details about the exact meaning of
unavailable-features, and what it would mean to see
read-only QOM properties in the list, and that
implementations can return "type" if there's
no extra information available;
---
 qapi-schema.json | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index c3dcf11..abb163b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3114,10 +3114,31 @@
 #  QEMU version, machine type, machine options and accelerator options.
 #  A static model is always migration-safe. (since 2.8)
 #
+# @unavailable-features: #optional List of properties that prevent
+#the CPU model from running in the current
+#host. (since 2.8)
+#
+# @unavailable-features is a list of QOM property names that
+# represent CPU model attributes that prevent the CPU from running.
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU model run in the current host. Implementations
+# that choose not to provide specific information return the
+# property name "type".
+# If the property is read-write, it means that it MAY be possible
+# to run the CPU model in the current host if that property is
+# changed. Management software can use it as hints to suggest or
+# choose an alternative for the user, or just to generate meaningful
+# error messages explaining why the CPU model can't be used.
+# If @unavailable-features is an empty list, the CPU model is
+# runnable using the current host and machine-type.
+# If @unavailable-features is not present, runnability
+# information for the CPU is not available.
+#
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool' } }
+  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
+'*unavailable-features': [ 'str' ] } }
 
 ##
 # @query-cpu-definitions:
-- 
2.7.4

[Qemu-devel] [PATCH v5 12/12] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "unavailable-features" field on the x86 implementation
of query-cpu-definitions.

Cc: Jiri Denemark 
Cc: libvir-l...@redhat.com
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Handle missing XSAVE components cleanly, but looking up
  the original feature that required it
* Use x86_cpu_load_features() function

Changes v2 -> v3:
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Updated to the new schema: no @runnable field, and
  always report @unavailable-features as present
---
 target-i386/cpu.c | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e4b45e3..620889f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
 }
 }
 
+/* Return the feature property name for a feature flag bit */
+static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
+{
+/* XSAVE components are automatically enabled by other features,
+ * so return the original feature name instead
+ */
+if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
+int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
+
+if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
+x86_ext_save_areas[comp].bits) {
+w = x86_ext_save_areas[comp].feature;
+bitnr = ctz32(x86_ext_save_areas[comp].bits);
+}
+}
+
+assert(bitnr < 32);
+assert(w < FEATURE_WORDS);
+return feature_word_info[w].feat_names[bitnr];
+}
+
 /* Compatibily hack to maintain legacy +-feat semantic,
  * where +-feat overwrites any feature set by
  * feat=on|feat even if the later is parsed after +-feat
@@ -2030,6 +2051,56 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 }
 }
 
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
+
+/* Check for missing features that may prevent the CPU class from
+ * running using the current machine and accelerator.
+ */
+static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
+ strList **missing_feats)
+{
+X86CPU *xc;
+FeatureWord w;
+Error *err = NULL;
+strList **next = missing_feats;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("kvm");;
+*missing_feats = new;
+return;
+}
+
+xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
+
+x86_cpu_load_features(xc, );
+if (err) {
+/* Errors at x86_cpu_load_features should never happen,
+ * but in case it does, just report the model as not
+ * runnable at all using the "type" property.
+ */
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("type");
+*next = new;
+next = >next;
+}
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+uint32_t filtered = xc->filtered_features[w];
+int i;
+for (i = 0; i < 32; i++) {
+if (filtered & (1UL << i)) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup(x86_cpu_feature_name(w, i));
+*next = new;
+next = >next;
+}
+}
+}
+
+object_unref(OBJECT(xc));
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2122,6 +2193,8 @@ static void x86_cpu_definition_entry(gpointer data, 
gpointer user_data)
 
 info = g_malloc0(sizeof(*info));
 info->name = x86_cpu_class_get_model_name(cc);
+x86_cpu_class_check_missing_features(cc, >unavailable_features);
+info->has_unavailable_features = true;
 
 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.7.4

[Qemu-devel] [PATCH v5 10/12] target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

When probing for CPU model information, we need to reuse the code
that initializes CPUID fields, but not the remaining side-effects
of x86_cpu_realizefn(). Move that code to a separate function
that can be reused later.

Signed-off-by: Eduardo Habkost 
---
Changes v4 -> v5:
* Fix typo on x86_cpu_load_features() comment
  Reported-by: Paolo Bonzini 

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 66 +++
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 636e629..e4b45e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2991,34 +2991,13 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
 }
 
-#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
-   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
-   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
-#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
- (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
- (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
-static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+/* Load CPUID data based on configured features */
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
 {
-CPUState *cs = CPU(dev);
-X86CPU *cpu = X86_CPU(dev);
-X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
 CPUX86State *env = >env;
-Error *local_err = NULL;
-static bool ht_warned;
 FeatureWord w;
 GList *l;
-
-if (xcc->kvm_required && !kvm_enabled()) {
-char *name = x86_cpu_class_get_model_name(xcc);
-error_setg(_err, "CPU model '%s' requires KVM", name);
-g_free(name);
-goto out;
-}
-
-if (cpu->apic_id == UNASSIGNED_APIC_ID) {
-error_setg(errp, "apic-id property was not initialized properly");
-return;
-}
+Error *local_err = NULL;
 
 /*TODO: cpu->host_features incorrectly overwrites features
  * set using "feat=on|off". Once we fix this, we can convert
@@ -3085,6 +3064,45 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 x86_cpu_filter_features(cpu);
+
+out:
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+}
+}
+
+#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
+   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
+   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
+#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
+ (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
+ (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+CPUState *cs = CPU(dev);
+X86CPU *cpu = X86_CPU(dev);
+X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+CPUX86State *env = >env;
+Error *local_err = NULL;
+static bool ht_warned;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+char *name = x86_cpu_class_get_model_name(xcc);
+error_setg(_err, "CPU model '%s' requires KVM", name);
+g_free(name);
+goto out;
+}
+
+if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+error_setg(errp, "apic-id property was not initialized properly");
+return;
+}
+
+x86_cpu_load_features(cpu, _err);
+if (local_err) {
+goto out;
+}
+
 if (cpu->check_cpuid || cpu->enforce_cpuid) {
 if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
 error_setg(_err,
-- 
2.7.4

Re: [Qemu-devel] backup notifier fail policy

[Vladimir Sementsov-Ogievskiy]

On 30.09.2016 20:11, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

Please, can somebody explain me, why we fail guest request in case of 
io error in write notifier? I think guest consistency is more 
important than success of unfinished backup. Or, what am I missing?


I'm saying about this code:

static int coroutine_fn backup_before_write_notify(
NotifierWithReturn *notifier,
void *opaque)
{
BackupBlockJob *job = container_of(notifier, BackupBlockJob, 
before_write);

BdrvTrackedRequest *req = opaque;
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;

assert(req->bs == blk_bs(job->common.blk));
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);

return backup_do_cow(job, sector_num, nb_sectors, NULL, true);
}

So, what about something like

ret = backup_do_cow(job, ...
if (ret < 0 && job->notif_ret == 0) {
   job->notif_ret = ret;
}

return 0;

and fail block job if notif_ret < 0 in other places of backup code?



And second question about notifiers in backup block job. If block job is 
paused, notifiers still works and can copy data. Is it ok? So, user 
thinks that job is paused, so he can do something with target disk.. But 
really, this 'something' will race with write-notifiers. So, what 
assumptions may user actually have about paused backup job? Is there any 
agreements? Also, on query-block-jobs we will see job.busy = false, when 
actually copy-on-write may be in flight..


--
Best regards,
Vladimir

[Qemu-devel] [PATCH v5 08/12] target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas

[Eduardo Habkost]

Instead of treating the FP and SSE bits as special cases, add
them to the x86_ext_save_areas array. This will simplify the code
that calculates the supported xsave components and the size of
the xsave area.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c018122..477990d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -535,6 +535,20 @@ typedef struct ExtSaveArea {
 } ExtSaveArea;
 
 static const ExtSaveArea x86_ext_save_areas[] = {
+[XSTATE_FP_BIT] = {
+/* x87 FP state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* x87 state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
+[XSTATE_SSE_BIT] = {
+/* SSE state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* SSE state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
 [XSTATE_YMM_BIT] =
   { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
 .offset = offsetof(X86XSaveArea, avx_state),
@@ -568,9 +582,9 @@ static const ExtSaveArea x86_ext_save_areas[] = {
 static uint32_t xsave_area_size(uint64_t mask)
 {
 int i;
-uint64_t ret = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader);
+uint64_t ret = 0;
 
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if ((mask >> i) & 1) {
 ret = MAX(ret, esa->offset + esa->size);
@@ -2961,8 +2975,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 return;
 }
 
-mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK);
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+mask = 0;
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if (env->features[esa->feature] & esa->bits) {
 mask |= (1ULL << i);
-- 
2.7.4

[Qemu-devel] [PATCH v5 07/12] target-i386: Register properties for feature aliases manually

[Eduardo Habkost]

Instead of keeping the aliases inside the feature name arrays and
require parsing the strings, just register alias properties
manually. This simplifies the code for property registration and
lookup.

Signed-off-by: Eduardo Habkost 
---
Changes v4 -> v5:
* Refresh after the previous patches were changed

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 43 +--
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ec65aef..c018122 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -278,12 +278,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_1_ECX] = {
 .feat_names = {
-"pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
+"pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
 "ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4-1",
-"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1",
+"sse4.2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -302,9 +302,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* cx8 */, NULL /* apic */, NULL, "syscall",
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
-"nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
-NULL, "lm|i64", "3dnowext", "3dnow",
+"nx", NULL, "mmxext", NULL /* mmx */,
+NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
+NULL, "lm", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
 .tcg_features = TCG_EXT2_FEATURES,
@@ -3321,31 +3321,22 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
FeatureWord w,
int bitnr)
 {
-Object *obj = OBJECT(cpu);
-int i;
-char **names;
 FeatureWordInfo *fi = _word_info[w];
+const char *name = fi->feat_names[bitnr];
 
-if (!fi->feat_names[bitnr]) {
+if (!name) {
 return;
 }
 
-names = g_strsplit(fi->feat_names[bitnr], "|", 0);
-
 /* Property names should use "-" instead of "_".
  * Old names containing underscores are registered as aliases
  * using object_property_add_alias()
  */
-assert(!strchr(names[0], '_'));
-x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
-
-for (i = 1; names[i]; i++) {
-assert(!strchr(names[i], '_'));
-object_property_add_alias(obj, names[i], obj, names[0],
-  _abort);
-}
-
-g_strfreev(names);
+assert(!strchr(name, '_'));
+/* aliases don't use "|" delimiters anymore, they are registered
+ * manually using object_property_add_alias() */
+assert(!strchr(name, '|'));
+x86_cpu_register_bit_prop(cpu, name, >env.features[w], bitnr);
 }
 
 static void x86_cpu_initfn(Object *obj)
@@ -3393,6 +3384,14 @@ static void x86_cpu_initfn(Object *obj)
 }
 }
 
+object_property_add_alias(obj, "sse3", obj, "pni", _abort);
+object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", _abort);
+object_property_add_alias(obj, "sse4-1", obj, "sse4.1", _abort);
+object_property_add_alias(obj, "sse4-2", obj, "sse4.2", _abort);
+object_property_add_alias(obj, "xd", obj, "nx", _abort);
+object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", _abort);
+object_property_add_alias(obj, "i64", obj, "lm", _abort);
+
 object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", _abort);
 object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", 
_abort);
 object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", _abort);
-- 
2.7.4

[Qemu-devel] [PATCH v5 06/12] target-i386: Remove underscores from feat_names arrays

[Eduardo Habkost]

Instead of translating the feature name entries when adding
property names, store the actual property names in the feature
name array.

For reference, here is the full list of functions that use
FeatureWordInfo::feat_names:

* x86_cpu_get_migratable_flags(): not affected, as it just
  check for non-NULL values.
* report_unavailable_features(): informative only. It will
  start printing feature names with hyphens.
* x86_cpu_list(): informative only. It will start printing
  feature names with hyphens
* x86_cpu_register_feature_bit_props(): not affected, as it
  was already calling feat2prop(). Now we can remove the
  feat2prop() calls safely.

So, the only user-visible effect of this patch are the new names
being used in help and error messages for users.

Signed-off-by: Eduardo Habkost 
---
Changes v4 -> v5:
* Remove the feat2prop() call from the alias registration
  loop, too
* Commit message update to enumerate all code that uses
  feat_names

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 36 
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cef4ecd..ec65aef 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 [FEAT_1_ECX] = {
 .feat_names = {
 "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
-"ds_cpl", "vmx", "smx", "est",
+"ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4_1",
-"sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1|sse4-1",
+"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -303,7 +303,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
 "nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb", "rdtscp",
+NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
 NULL, "lm|i64", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
@@ -311,13 +311,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_8000_0001_ECX] = {
 .feat_names = {
-"lahf_lm", "cmp_legacy", "svm", "extapic",
+"lahf-lm", "cmp-legacy", "svm", "extapic",
 "cr8legacy", "abm", "sse4a", "misalignsse",
 "3dnowprefetch", "osvw", "ibs", "xop",
 "skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid_msr",
-NULL, "tbm", "topoext", "perfctr_core",
-"perfctr_nb", NULL, NULL, NULL,
+"fma4", "tce", NULL, "nodeid-msr",
+NULL, "tbm", "topoext", "perfctr-core",
+"perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_ECX,
@@ -339,8 +339,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_KVM] = {
 .feat_names = {
-"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -400,9 +400,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_SVM] = {
 .feat_names = {
-"npt", "lbrv", "svm_lock", "nrip_save",
-"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-NULL, NULL, "pause_filter", NULL,
+"npt", "lbrv", "svm-lock", "nrip-save",
+"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+NULL, NULL, "pause-filter", NULL,
 "pfthreshold", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -414,7 +414,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_7_0_EBX] = {
 .feat_names = {
-"fsgsbase", "tsc_adjust", NULL, "bmi1",
+"fsgsbase", "tsc-adjust", NULL, "bmi1",
 "hle", "avx2", NULL, "smep",
 "bmi2", "erms", "invpcid", "rtm",
 NULL, NULL, "mpx", NULL,
@@ -3332,11 +3332,15 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
 
 names = g_strsplit(fi->feat_names[bitnr], "|", 

[Qemu-devel] [PATCH v5 03/12] target-i386: Disable VME by default with TCG

[Eduardo Habkost]

VME is already disabled automatically when using TCG. So, instead
of pretending it is there when reporting CPU model data on
query-cpu-* QMP commands (making every CPU model to be reported
as not runnable), we can disable it by default on all CPU models
when using TCG.

Do that by adding a tcg_default_props array that will work like
kvm_default_props.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ac3646e..3d3f64e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,6 +1550,14 @@ static PropValue kvm_default_props[] = {
 { NULL, NULL },
 };
 
+/* TCG-specific defaults that override all CPU models when using TCG
+ */
+static PropValue tcg_default_props[] = {
+{ "vme", "off" },
+{ NULL, NULL },
+};
+
+
 void x86_cpu_change_kvm_default(const char *prop, const char *value)
 {
 PropValue *pv;
@@ -2283,6 +2291,8 @@ static void x86_cpu_load_def(X86CPU *cpu, 
X86CPUDefinition *def, Error **errp)
 }
 
 x86_cpu_apply_props(cpu, kvm_default_props);
+} else if (tcg_enabled()) {
+x86_cpu_apply_props(cpu, tcg_default_props);
 }
 
 env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
-- 
2.7.4

[Qemu-devel] [PATCH v5 00/12] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

This series extends query-cpu-definitions to include an extra
field: "unavailable-features". The new field can be used to find
out reasons that prevent the CPU model from running in the
current host.

This will return information based on the current machine and
accelerator only. In the future we may extend these mechanisms to
allow querying other machines and other accelerators without
restarting QEMU, but it will require some reorganization of
QEMU's main code.

To be able to implement this more cleanly, the series rework some
of the feature/property name code.

This series can be seen in the git branch at:
  https://github.com/ehabkost/qemu-hacks.git 
work/query-cpu-definitions-runnable-info

The series is based on my x86-next branch:
   https://github.com/ehabkost/qemu.git x86-next

Changes v4 -> v5:
* New patch: "target-i386: Register aliases for feature names with underscores"
* On patch: "tests: Add test case for x86 feature parsing compatibility":
  * Fix typo on commit message
Reported-by: Jonathan Neuschäfer 
  * Add comment noting that the "[+-]feature" compatibility mode
will be removed soon
* On patch: "target-i386: Make plus_features/minus_features QOM-based":
  * Removed feat2prop() call on , as we now have property aliases
for the old names containing underscores
* On patch: "target-i386: Remove underscores from feat_names arrays":
  * Remove the feat2prop() call from the alias registration
loop, too
  * Commit message update to enumerate all code that uses
feat_names
* On patch: "target-i386: x86_cpu_load_features() function":
  * Fix typo on x86_cpu_load_features() comment
Reported-by: Paolo Bonzini 

Diff v4 ->v5:

  diff --git a/target-i386/cpu.c b/target-i386/cpu.c
  index 4dd3aee..620889f 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -2002,12 +2002,10 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,

   /* Compatibility syntax: */
   if (featurestr[0] == '+') {
  -feat2prop(featurestr + 1);
   plus_features = g_list_append(plus_features,
 g_strdup(featurestr + 1));
   continue;
   } else if (featurestr[0] == '-') {
  -feat2prop(featurestr + 1);
   minus_features = g_list_append(minus_features,
  g_strdup(featurestr + 1));
   continue;
  @@ -3066,8 +3064,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
   env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
   }

  -/* Load CPUID data based on configureured features
  - */
  +/* Load CPUID data based on configured features */
   static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
   {
   CPUX86State *env = >env;
  @@ -3443,7 +3440,10 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
   return;
   }

  -/* Property names should use "-" instead of "_" */
  +/* Property names should use "-" instead of "_".
  + * Old names containing underscores are registered as aliases
  + * using object_property_add_alias()
  + */
   assert(!strchr(name, '_'));
   /* aliases don't use "|" delimiters anymore, they are registered
* manually using object_property_add_alias() */
  @@ -3496,7 +3496,6 @@ static void x86_cpu_initfn(Object *obj)
   }
   }

  -/* Alias for feature properties: */
   object_property_add_alias(obj, "sse3", obj, "pni", _abort);
   object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", 
_abort);
   object_property_add_alias(obj, "sse4-1", obj, "sse4.1", _abort);
  @@ -3505,6 +3504,28 @@ static void x86_cpu_initfn(Object *obj)
   object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", _abort);
   object_property_add_alias(obj, "i64", obj, "lm", _abort);

  +object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", _abort);
  +object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", 
_abort);
  +object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", 
_abort);
  +object_property_add_alias(obj, "lahf_lm", obj, "lahf-lm", _abort);
  +object_property_add_alias(obj, "cmp_legacy", obj, "cmp-legacy", 
_abort);
  +object_property_add_alias(obj, "nodeid_msr", obj, "nodeid-msr", 
_abort);
  +object_property_add_alias(obj, "perfctr_core", obj, "perfctr-core", 
_abort);
  +object_property_add_alias(obj, "perfctr_nb", obj, "perfctr-nb", 
_abort);
  +object_property_add_alias(obj, "kvm_nopiodelay", obj, "kvm-nopiodelay", 
_abort);
  +object_property_add_alias(obj, "kvm_mmu", obj, "kvm-mmu", _abort);
  +object_property_add_alias(obj, "kvm_asyncpf", obj, "kvm-asyncpf", 
_abort);
  +object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", 
_abort);
  +object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", 
_abort);
  +

[Qemu-devel] [PATCH v5 09/12] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

x86_cpu_filter_features() will be reused by code that shouldn't
print any warning. Move the warning code to a new
x86_cpu_report_filtered_features() function, and call it from
x86_cpu_realizefn().

Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Made x86_cpu_filter_features() void, make
  x86_cpu_report_filtered_features() return true if
  some features were filtered
---
 target-i386/cpu.c | 41 -
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 477990d..636e629 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2161,14 +2161,11 @@ static uint32_t 
x86_cpu_get_supported_feature_word(FeatureWord w,
 
 /*
  * Filters CPU feature words based on host availability of each feature.
- *
- * Returns: 0 if all flags are supported by the host, non-zero otherwise.
  */
-static int x86_cpu_filter_features(X86CPU *cpu)
+static void x86_cpu_filter_features(X86CPU *cpu)
 {
 CPUX86State *env = >env;
 FeatureWord w;
-int rv = 0;
 
 for (w = 0; w < FEATURE_WORDS; w++) {
 uint32_t host_feat =
@@ -2176,15 +2173,22 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 uint32_t requested_features = env->features[w];
 env->features[w] &= host_feat;
 cpu->filtered_features[w] = requested_features & ~env->features[w];
-if (cpu->filtered_features[w]) {
-if (cpu->check_cpuid || cpu->enforce_cpuid) {
-report_unavailable_features(w, cpu->filtered_features[w]);
-}
-rv = 1;
-}
 }
+}
 
-return rv;
+/* Report list of filtered features to stderr.
+ * Returns true if some features were found to be filtered, false otherwise
+ */
+static bool x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+FeatureWord w;
+uint32_t filtered = 0;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+filtered |= cpu->filtered_features[w];
+report_unavailable_features(w, cpu->filtered_features[w]);
+}
+return filtered;
 }
 
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
@@ -3080,12 +3084,15 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
 }
 
-if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-error_setg(_err,
-   kvm_enabled() ?
-   "Host doesn't support requested features" :
-   "TCG doesn't support requested features");
-goto out;
+x86_cpu_filter_features(cpu);
+if (cpu->check_cpuid || cpu->enforce_cpuid) {
+if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
+error_setg(_err,
+   kvm_enabled() ?
+   "Host doesn't support requested features" :
+   "TCG doesn't support requested features");
+goto out;
+}
 }
 
 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-- 
2.7.4

[Qemu-devel] [PATCH v5 05/12] target-i386: Make plus_features/minus_features QOM-based

[Eduardo Habkost]

Instead of using custom feature name lookup code for
plus_features/minus_features, save the property names used in
"[+-]feature" and use object_property_set_bool() to set them.

We don't need a feat2prop() call because we now have alias
properties for the old names containing underscores.

Signed-off-by: Eduardo Habkost 
---
Changes v4 -> v5:
* Removed feat2prop() call, as we now have property aliases for
  the old names containing underscores

Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 106 +++---
 1 file changed, 20 insertions(+), 86 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6b5bf59..cef4ecd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -650,85 +650,6 @@ void host_cpuid(uint32_t function, uint32_t count,
 *edx = vec[3];
 }
 
-#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
-
-/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
- * a substring.  ex if !NULL points to the first char after a substring,
- * otherwise the string is assumed to sized by a terminating nul.
- * Return lexical ordering of *s1:*s2.
- */
-static int sstrcmp(const char *s1, const char *e1,
-   const char *s2, const char *e2)
-{
-for (;;) {
-if (!*s1 || !*s2 || *s1 != *s2)
-return (*s1 - *s2);
-++s1, ++s2;
-if (s1 == e1 && s2 == e2)
-return (0);
-else if (s1 == e1)
-return (*s2);
-else if (s2 == e2)
-return (*s1);
-}
-}
-
-/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
- * '|' delimited (possibly empty) strings in which case search for a match
- * within the alternatives proceeds left to right.  Return 0 for success,
- * non-zero otherwise.
- */
-static int altcmp(const char *s, const char *e, const char *altstr)
-{
-const char *p, *q;
-
-for (q = p = altstr; ; ) {
-while (*p && *p != '|')
-++p;
-if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
-return (0);
-if (!*p)
-return (1);
-else
-q = ++p;
-}
-}
-
-/* search featureset for flag *[s..e), if found set corresponding bit in
- * *pval and return true, otherwise return false
- */
-static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
-   const char **featureset)
-{
-uint32_t mask;
-const char **ppc;
-bool found = false;
-
-for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
-if (*ppc && !altcmp(s, e, *ppc)) {
-*pval |= mask;
-found = true;
-}
-}
-return found;
-}
-
-static void add_flagname_to_bitmaps(const char *flagname,
-FeatureWordArray words,
-Error **errp)
-{
-FeatureWord w;
-for (w = 0; w < FEATURE_WORDS; w++) {
-FeatureWordInfo *wi = _word_info[w];
-if (lookup_feature([w], flagname, NULL, wi->feat_names)) {
-break;
-}
-}
-if (w == FEATURE_WORDS) {
-error_setg(errp, "CPU feature %s not found", flagname);
-}
-}
-
 /* CPU class name definitions: */
 
 #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU
@@ -2015,8 +1936,7 @@ static inline void feat2prop(char *s)
  * feat=on|feat even if the later is parsed after +-feat
  * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
  */
-static FeatureWordArray plus_features = { 0 };
-static FeatureWordArray minus_features = { 0 };
+static GList *plus_features, *minus_features;
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
@@ -2047,10 +1967,12 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 
 /* Compatibility syntax: */
 if (featurestr[0] == '+') {
-add_flagname_to_bitmaps(featurestr + 1, plus_features, _err);
+plus_features = g_list_append(plus_features,
+  g_strdup(featurestr + 1));
 continue;
 } else if (featurestr[0] == '-') {
-add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);
+minus_features = g_list_append(minus_features,
+   g_strdup(featurestr + 1));
 continue;
 }
 
@@ -3066,6 +2988,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 Error *local_err = NULL;
 static bool ht_warned;
 FeatureWord w;
+GList *l;
 
 if (xcc->kvm_required && !kvm_enabled()) {
 char *name = x86_cpu_class_get_model_name(xcc);
@@ -3091,9 +3014,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 }
 
-for (w = 0; w < FEATURE_WORDS; w++) {
-cpu->env.features[w] |= plus_features[w];
-cpu->env.features[w] &= ~minus_features[w];
+for (l = plus_features; l; l 

[Qemu-devel] [PATCH v5 04/12] target-i386: Register aliases for feature names with underscores

[Eduardo Habkost]

Registering the actual names containing underscores as aliases
will allow management software to be aware that the old
compatibility names are suported, and will make feat2prop() calls
unnecessary when using feature names.

Also, this will help us avoid making the code support underscores
on feature names that never had them in the first place. e.g.
"+tsc_deadline" was never supported and doesn't need to be
translated to "+tsc-deadline".

In other word: this will require less magic translation of
strings, and simple 1:1 match between the config options and
actual QOM properties.

Note that the underscores are still present in the
FeatureWordInfo::feat_names arrays, because
add_flagname_to_bitmaps() needs them to be kept. The next patches
will remove add_flagname_to_bitmaps() and will allow us to
finally remove the aliases from feat_names.

Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3d3f64e..6b5bf59 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3455,6 +3455,28 @@ static void x86_cpu_initfn(Object *obj)
 }
 }
 
+object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", _abort);
+object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", 
_abort);
+object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", _abort);
+object_property_add_alias(obj, "lahf_lm", obj, "lahf-lm", _abort);
+object_property_add_alias(obj, "cmp_legacy", obj, "cmp-legacy", 
_abort);
+object_property_add_alias(obj, "nodeid_msr", obj, "nodeid-msr", 
_abort);
+object_property_add_alias(obj, "perfctr_core", obj, "perfctr-core", 
_abort);
+object_property_add_alias(obj, "perfctr_nb", obj, "perfctr-nb", 
_abort);
+object_property_add_alias(obj, "kvm_nopiodelay", obj, "kvm-nopiodelay", 
_abort);
+object_property_add_alias(obj, "kvm_mmu", obj, "kvm-mmu", _abort);
+object_property_add_alias(obj, "kvm_asyncpf", obj, "kvm-asyncpf", 
_abort);
+object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", 
_abort);
+object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", 
_abort);
+object_property_add_alias(obj, "kvm_pv_unhalt", obj, "kvm-pv-unhalt", 
_abort);
+object_property_add_alias(obj, "svm_lock", obj, "svm-lock", _abort);
+object_property_add_alias(obj, "nrip_save", obj, "nrip-save", 
_abort);
+object_property_add_alias(obj, "tsc_scale", obj, "tsc-scale", 
_abort);
+object_property_add_alias(obj, "vmcb_clean", obj, "vmcb-clean", 
_abort);
+object_property_add_alias(obj, "pause_filter", obj, "pause-filter", 
_abort);
+object_property_add_alias(obj, "sse4_1", obj, "sse4.1", _abort);
+object_property_add_alias(obj, "sse4_2", obj, "sse4.2", _abort);
+
 x86_cpu_load_def(cpu, xcc->cpu_def, _abort);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v5 01/12] tests: Add test case for x86 feature parsing compatibility

[Eduardo Habkost]

Add a new test case to ensure the existing behavior of the
feature parsing code will be kept.

Signed-off-by: Eduardo Habkost 
---
Changes series v4 -> v5:
* Fix typo on commit message
  Reported-by: Jonathan Neuschäfer 
* Add comment noting that the "[+-]feature" compatibility mode
  will be removed soon

Changes series v3 -> v4:
* New patch added to series
---
 tests/test-x86-cpuid-compat.c | 44 +++
 1 file changed, 44 insertions(+)

diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 83162a4..260dd27 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -3,6 +3,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "libqtest.h"
 
 static char *get_cpu0_qom_path(void)
@@ -34,6 +35,15 @@ static QObject *qom_get(const char *path, const char *prop)
 return ret;
 }
 
+static bool qom_get_bool(const char *path, const char *prop)
+{
+QBool *value = qobject_to_qbool(qom_get(path, prop));
+bool b = qbool_get_bool(value);
+
+QDECREF(value);
+return b;
+}
+
 typedef struct CpuidTestArgs {
 const char *cmdline;
 const char *property;
@@ -66,10 +76,44 @@ static void add_cpuid_test(const char *name, const char 
*cmdline,
 qtest_add_data_func(name, args, test_cpuid_prop);
 }
 
+static void test_plus_minus(void)
+{
+char *path;
+
+/* Rules:
+ * 1)"-foo" overrides "+foo"
+ * 2) "[+-]foo" overrides "foo=..."
+ * 3) Old feature names with underscores (e.g. "sse4_2")
+ *should keep working
+ *
+ * Note: rules 1 and 2 are planned to be removed soon, but we
+ * need to keep compatibility for a while until we start
+ * warning users about it.
+ */
+qtest_start("-cpu 
pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on");
+path = get_cpu0_qom_path();
+
+g_assert_false(qom_get_bool(path, "fpu"));
+g_assert_false(qom_get_bool(path, "mce"));
+g_assert_true(qom_get_bool(path, "cx8"));
+
+/* Test both the original and the alias feature names: */
+g_assert_true(qom_get_bool(path, "sse4-1"));
+g_assert_true(qom_get_bool(path, "sse4.1"));
+
+g_assert_true(qom_get_bool(path, "sse4-2"));
+g_assert_true(qom_get_bool(path, "sse4.2"));
+
+qtest_end();
+g_free(path);
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
 
+qtest_add_func("x86/cpuid/parsing-plus-minus", test_plus_minus);
+
 /* Original level values for CPU models: */
 add_cpuid_test("x86/cpuid/phenom/level",
"-cpu phenom", "level", 5);
-- 
2.7.4

[Qemu-devel] [PATCH v5 02/12] target-i386: List CPU models using subclass list

[Eduardo Habkost]

Instead of using the builtin_x86_defs array, use the QOM subclass
list to list CPU models on "-cpu ?" and "query-cpu-definitions".

Signed-off-by: Andreas Färber 
[ehabkost: copied code from a patch by Andreas:
 "target-i386: QOM'ify CPU", from March 2012]
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |   4 ++
 target-i386/cpu.c | 103 --
 2 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5dde658..e724004 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -63,6 +63,10 @@ typedef struct X86CPUClass {
 
 bool kvm_required;
 
+/* Optional description of CPU model.
+ * If unavailable, cpu_def->model_id is used */
+const char *model_description;
+
 DeviceRealize parent_realize;
 void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0807e92..ac3646e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1628,6 +1628,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void 
*data)
 cpu_x86_fill_model_id(host_cpudef.model_id);
 
 xcc->cpu_def = _cpudef;
+xcc->model_description =
+"KVM processor with all supported host features "
+"(only available in KVM mode)";
 
 /* level, xlevel, xlevel2, and the feature words are initialized on
  * instance_init, because they require KVM to be initialized.
@@ -2098,23 +2101,62 @@ static void listflags(FILE *f, fprintf_function print, 
const char **featureset)
 }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *class_a = (ObjectClass *)a;
+ObjectClass *class_b = (ObjectClass *)b;
+X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+const char *name_a, *name_b;
+
+if (cc_a->kvm_required != cc_b->kvm_required) {
+/* kvm_required items go last */
+return cc_a->kvm_required ? 1 : -1;
+} else {
+name_a = object_class_get_name(class_a);
+name_b = object_class_get_name(class_b);
+return strcmp(name_a, name_b);
+}
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+list = g_slist_sort(list, x86_cpu_list_compare);
+return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CPUListState *s = user_data;
+char *name = x86_cpu_class_get_model_name(cc);
+const char *desc = cc->model_description;
+if (!desc) {
+desc = cc->cpu_def->model_id;
+}
+
+(*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+  name, desc);
+g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-X86CPUDefinition *def;
-char buf[256];
 int i;
+CPUListState s = {
+.file = f,
+.cpu_fprintf = cpu_fprintf,
+};
+GSList *list;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-def = _x86_defs[i];
-snprintf(buf, sizeof(buf), "%s", def->name);
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-}
-#ifdef CONFIG_KVM
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-   "KVM processor with all supported host features "
-   "(only available in KVM mode)");
-#endif
+(*cpu_fprintf)(f, "Available CPUs:\n");
+list = get_sorted_cpu_model_list();
+g_slist_foreach(list, x86_cpu_list_entry, );
+g_slist_free(list);
 
 (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2126,26 +2168,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 }
 }
 
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
-CpuDefinitionInfoList *cpu_list = NULL;
-X86CPUDefinition *def;
-int i;
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CpuDefinitionInfoList **cpu_list = user_data;
+CpuDefinitionInfoList *entry;
+CpuDefinitionInfo *info;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-CpuDefinitionInfoList *entry;
-CpuDefinitionInfo *info;
-
-def = _x86_defs[i];
-info = g_malloc0(sizeof(*info));
-info->name = g_strdup(def->name);
+info = g_malloc0(sizeof(*info));
+info->name = x86_cpu_class_get_model_name(cc);
 
-entry = g_malloc0(sizeof(*entry));
-entry->value = info;
-entry->next = cpu_list;
-cpu_list = entry;
-}
+entry = g_malloc0(sizeof(*entry));
+

Re: [Qemu-devel] [PATCH] usb: Change *_exitfn return type from void to int

[Eric Blake]

On 09/30/2016 12:51 PM, Akanksha Srivastava wrote:
> The *_exitfn functions cannot fail and should not be
> returning int.
> Suggested as a Bite-sized task
> Signed-off-by: Akanksha Srivastava 
> ---
>  hw/usb/ccid-card-emulated.c | 4 ++--
>  hw/usb/ccid-card-passthru.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index 3213f9f..abb2773 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
>  return 0;
>  }
>  
> -static int emulated_exitfn(CCIDCardState *base)
> +static void emulated_exitfn(CCIDCardState *base)
>  {
>  EmulatedState *card = EMULATED_CCID_CARD(base);
>  VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
> @@ -564,7 +564,7 @@ static int emulated_exitfn(CCIDCardState *base)
>  qemu_mutex_destroy(>handle_apdu_mutex);
>  qemu_mutex_destroy(>vreader_mutex);
>  qemu_mutex_destroy(>event_list_mutex);
> -return 0;
> +return;
>  }

Ending a function that returns void with 'return' is redundant; just
drop the return statement altogether if it is the last thing that can be
reached.

>  
>  static Property emulated_card_properties[] = {
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 2eacea7..2bd9cb2 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -364,9 +364,9 @@ static int passthru_initfn(CCIDCardState *base)
>  return 0;
>  }
>  
> -static int passthru_exitfn(CCIDCardState *base)
> +static void passthru_exitfn(CCIDCardState *base)
>  {
> -return 0;
> +return;
>  }

And again.  Since the callback does nothing, do you even need it to
exist, or can you do further cleanups that allow the callback to be
optional if it would otherwise be a no-op?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 02/12] block/nbd: Reject port parameter without host

[Eric Blake]

On 09/28/2016 03:55 PM, Max Reitz wrote:
> Currently, a port that is passed along with a UNIX socket path is
> silently ignored. That is not exactly ideal, it should be an error
> instead.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake 


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/6] io: add ability to set a name for IO channels

[Eric Blake]

On 09/30/2016 10:16 AM, Daniel P. Berrange wrote:
> The GSource object has ability to have a name, which is useful
> when debugging performance problems with the mainloop event
> callbacks that take too long. By associating a name with an
> QIOChannel object, we can then set the name on any GSource

'an QIOChannel' sounds funny.

> associated with the channel.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/io/channel.h | 13 +
>  io/channel.c | 24 +++-
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Transactions, Jobs, and Cancellation

[John Snow]

On 09/30/2016 02:30 PM, Eric Blake wrote:

On 09/30/2016 11:35 AM, John Snow wrote:

Hi Eric (as a proxy for Libvirt);

I want to make a change to transactions such that they do not actually
start the jobs until the entire transaction is error-checked for validity.

This would be a change from the current setup where:

- Some jobs are started
- One job cannot start
- Existing jobs are cancelled, emitting job events
- QMP transaction returns failure.


In this case, the number of events emitted is less than the number of
events comprising the overall transaction, right?



Yes, because some may fail to execute %jobtype%_start(), and the 
transaction halts and returns ERROR. Some jobs may have already started, 
and those are canceled, producing an event.




to something more like:

- Some jobs are queued to start
- One job cannot start
- Existing queued jobs are un-created
- No events are emitted, but the QMP transaction fails.


Is the failure guaranteed to be synchronous to the QMP command that
requested the 'transaction' command?  If so, I think you're fine: in
general, libvirt is looking for events (and presumably N events for a
'transaction' with N components) _only_ if the 'transaction' command
itself succeeded; but if the 'transaction' command fails, then there is
no expectation of events.



Yep, the synchronous case -- before we return any info on the QMP 
transaction at all.



If the failure is asynchronous (and can happen even after 'transaction'
returns success), then it gets a bit weirder; but libvirt still tries to
operate on the principle that events are best-effort notifications and
may be missed, so as long as it is always possible to poll QMP and learn
the same information as would have been presented in the omitted events,
we are probably still okay.



Yeah, no changes to the async case, regardless of the value of the 
trasnactional-cancel property.




If I understand correctly, it's possible for the transaction to fail
even after it has started several jobs because they begin operating
during the prepare phase.

Then, because the transaction preparation has failed, QEMU will cancel
the transaction instead of proceeding, which will generate some
BLOCK_JOB_CANCELLED events.

This may affect libvirt and others if I change these semantics.

Does that sound appropriate to you, or would you from a libvirt
perspective RATHER get events for "uncreated" jobs like you do now? I
could make it do either, but I'd rather prefer to simply not emit events
for jobs that didn't truly never start.


Avoiding events for jobs that never start makes sense if the transaction
itself returns failure.



I can also begin emitting events for jobs that *actually* start if it
would help to disambiguate the cases between old and new transactions.

Note: This has nothing to do with the transactional-cancel property,
which only impacts what happens when jobs created by a transaction fail
AFTER a successful return from qmp_transaction.


Ah, so it sounds like this is ALL about the synchronous case (that is,
the old behavior was that _even_ when 'transaction' fails, some events
were possibly leaked for < N portions of the transaction, if the earlier
portions were started to the point that they could emit a cancelled
event as a result of the later portions never starting).  So I think
your idea makes sense.



Great!

--js

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Eric Blake]

On 09/30/2016 12:34 PM, Peter Maydell wrote:
 +bool iss_sf = opc == 0 ? false : true;

Feels like a double negative.

>>>
>>> You could simplify that to:
>>>
>>>   bool iss_sf = !(opc == 0);
>>
>> I don't really see how that is simpler/clearer.
>>
>> I considered:
>> bool iss_sf = opc != 0;

Or even shorter:

bool iss_sf = !opc;

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 09:55:33AM +0200, Paolo Bonzini wrote:
> 
> 
> On 29/09/2016 23:14, Eduardo Habkost wrote:
> > + * "-foo" overrides "+foo"
> > + * "[+-]foo" overrides "foo=..."
> 
> Is this something that people are actually using?  Can we detect it and
> deprecate it in 2.8, and drop it in 2.9?

We can, but I would like to keep the test cases there in 2.8, at
least. I will update the test case to note that this is legacy
behavior that we plan to remove in 2.9, and send a separate
follow-up patch to detect when people mix both formats.

-- 
Eduardo

Re: [Qemu-devel] Transactions, Jobs, and Cancellation

[Eric Blake]

On 09/30/2016 11:35 AM, John Snow wrote:
> Hi Eric (as a proxy for Libvirt);
> 
> I want to make a change to transactions such that they do not actually
> start the jobs until the entire transaction is error-checked for validity.
> 
> This would be a change from the current setup where:
> 
> - Some jobs are started
> - One job cannot start
> - Existing jobs are cancelled, emitting job events
> - QMP transaction returns failure.

In this case, the number of events emitted is less than the number of
events comprising the overall transaction, right?

> 
> to something more like:
> 
> - Some jobs are queued to start
> - One job cannot start
> - Existing queued jobs are un-created
> - No events are emitted, but the QMP transaction fails.

Is the failure guaranteed to be synchronous to the QMP command that
requested the 'transaction' command?  If so, I think you're fine: in
general, libvirt is looking for events (and presumably N events for a
'transaction' with N components) _only_ if the 'transaction' command
itself succeeded; but if the 'transaction' command fails, then there is
no expectation of events.

If the failure is asynchronous (and can happen even after 'transaction'
returns success), then it gets a bit weirder; but libvirt still tries to
operate on the principle that events are best-effort notifications and
may be missed, so as long as it is always possible to poll QMP and learn
the same information as would have been presented in the omitted events,
we are probably still okay.

> 
> If I understand correctly, it's possible for the transaction to fail
> even after it has started several jobs because they begin operating
> during the prepare phase.
> 
> Then, because the transaction preparation has failed, QEMU will cancel
> the transaction instead of proceeding, which will generate some
> BLOCK_JOB_CANCELLED events.
> 
> This may affect libvirt and others if I change these semantics.
> 
> Does that sound appropriate to you, or would you from a libvirt
> perspective RATHER get events for "uncreated" jobs like you do now? I
> could make it do either, but I'd rather prefer to simply not emit events
> for jobs that didn't truly never start.

Avoiding events for jobs that never start makes sense if the transaction
itself returns failure.

> 
> I can also begin emitting events for jobs that *actually* start if it
> would help to disambiguate the cases between old and new transactions.
> 
> Note: This has nothing to do with the transactional-cancel property,
> which only impacts what happens when jobs created by a transaction fail
> AFTER a successful return from qmp_transaction.

Ah, so it sounds like this is ALL about the synchronous case (that is,
the old behavior was that _even_ when 'transaction' fails, some events
were possibly leaked for < N portions of the transaction, if the earlier
portions were started to the point that they could emit a cancelled
event as a result of the later portions never starting).  So I think
your idea makes sense.

-- 
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 v14 10/21] qapi: permit auto-creating nested structs

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> Some of the historical command line opts that had their
> keys in in a completely flat namespace are now represented
> by QAPI schemas that use a nested structs. When converting

singular/plural mismatch; either s/a // or s/structs/struct/

> the QemuOpts to QObject, there is no information about
> compound types available, so the QObject will be completely
> flat, even after the qdict_crumple() call. So when starting
> a struct, we may not have a QDict available in the input
> data, so we auto-create a QDict containing all the currently
> unvisited input data keys. Not all historical command line
> opts require this, so the behaviour is opt-in, by specifying
> how many levels of structs are permitted to be auto-created.
> 
> Note that this only works if the child struct is the last
> field to the visited in the parent struct. This is always
> the case for currently existing legacy command line options.
> 
> The example is the NetLegacy type which has 3 levels of
> structs. The modern way to represent this in QemuOpts would
> be the dot-separated component approach
> 
>   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>opts.data.fd=3,opts.data.script=ifup
> 
> The legacy syntax will just be presenting
> 
>   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup

So basically, any key=values that were not consumed as known keys in the
parent struct are reparsed as the (presumably lone) remaining QDict
child, recursing as needed to follow the QAPI nesting.

Does this still work if the command line is presented in a different order:

-net script=ifup,vlan=1,type=tap,fd=3,id=foo,name=bar

My guess is yes, but if the testsuite doesn't cover it, you may want to
add a test.

> 
> So we need to auto-create 3 levels of struct when visiting.
> 
> The implementation here will enable visiting in both the
> modern and legacy syntax, compared to OptsVisitor which
> only allows the legacy syntax.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h |  22 +-
>  qapi/qobject-input-visitor.c |  59 ++--
>  tests/test-qobject-input-visitor.c   | 130 
> ---
>  3 files changed, 194 insertions(+), 17 deletions(-)
> 
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 1809f48..94051f3 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -45,7 +45,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * If @autocreate_list is true, then as an alternative to a normal QList,
>   * list values can be stored as a QString or QDict instead, which will
>   * be interpreted as representing single element lists. This should only
> - * by used if compatibility is required with the OptsVisitor which allowed
> + * be used if compatibility is required with the OptsVisitor which allowed

This typo fix should be hoisted earlier in the series when it was
introduced.

>   * repeated keys, without list indexes, to represent lists. e.g. set this
>   * to true if you have compatibility requirements for
>   *
> @@ -55,6 +55,22 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   *
>   *   -arg foo.0=hello,foo.1=world
>   *
> + * If @autocreate_struct_levels is non-zero, then when visiting structs,
> + * if the corresponding QDict is not found, it will automatically create
> + * a QDict containing all remaining unvisited options. This should only
> + * be used if compatibility is required with the OptsVisitor which flatten
> + * structs so that all keys were at the same level. e.g. set this to a
> + * non-zero number if you compatibility requirements for
> + *
> + *   -arg type=person,surname=blogs,forename=fred
> + *
> + * to be treated as equivalent to the perferred syntax

s/perferred/preferred/


> +static void test_visitor_in_struct_autocreate(TestInputVisitorData *data,
> +  const void *unused)
> +{
> +Visitor *v;
> +int64_t vlan;
> +char *id = NULL;
> +char *type;
> +int64_t fd;
> +char *script = NULL;
> +
> +v = visitor_input_test_init_internal(
> +data, true, true, false, 3,
> +"{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
> +"'script': 'ifup' }", NULL);
> +

This is where intentionally mixing up the parameter orders might be
worthwhile.

Overall, the idea looks reasonable.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/5] msix_init: assert programming error

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Alex Williamson  writes:
> 
> > On Thu, 29 Sep 2016 15:11:27 +0200
> > Markus Armbruster  wrote:
> >
> >> Alex Williamson  writes:
> >> 
> >> > On Tue, 13 Sep 2016 08:16:20 +0200
> >> > Markus Armbruster  wrote:
> >> >  
> >> >> Cc: Alex for device assignment expertise.
> >> >> 
> >> >> Cao jin  writes:
> >> >>   
> >> >> > On 09/12/2016 09:29 PM, Markus Armbruster wrote:
> >> >> >> Cao jin  writes:
> >> >> >>
> >> >> >>> The input parameters is used for creating the msix capable device, 
> >> >> >>> so
> >> >> >>> they must obey the PCI spec, or else, it should be programming 
> >> >> >>> error.
> >> >> >>
> >> >> >> True when the the parameters come from a device model attempting to
> >> >> >> define a PCI device violating the spec.  But what if the parameters 
> >> >> >> come
> >> >> >> from an actual PCI device violating the spec, via device assignment? 
> >> >> >>
> >> >> >
> >> >> > Before the patch, on invalid param, the vfio behaviour is:
> >> >> >   error_report("vfio: msix_init failed");
> >> >> >   then, device create fail.
> >> >> >
> >> >> > After the patch, its behaviour is:
> >> >> >   asserted.
> >> >> >
> >> >> > Do you mean we should still report some useful info to user on invalid
> >> >> > params?
> >> >> 
> >> >> In the normal case, asking msix_init() to create MSI-X that are out of
> >> >> spec is a programming error: the code that does it is broken and needs
> >> >> fixing.
> >> >> 
> >> >> Device assignment might be the exception: there, the parameters for
> >> >> msix_init() come from the assigned device, not the program.  If they
> >> >> violate the spec, the device is broken.  This wouldn't be a programming
> >> >> error.  Alex, can this happen?
> >> >> 
> >> >> If yes, we may want to handle it by failing device assignment.  
> >> >
> >> >
> >> > Generally, I think the entire premise of these sorts of patches is
> >> > flawed.  We take a working error path that allows a driver to robustly
> >> > abort on unexpected date and turn it into a time bomb.  Often the
> >> > excuse for this is that "error handling is hard".  Tough.  Now a
> >> > hot-add of a device that triggers this changes from a simple failure to
> >> > a denial of service event.  Furthermore, we base that time bomb on our
> >> > interpretation of the spec, which we can only validate against in-tree
> >> > devices.
> >> >
> >> > We have actually had assigned devices that fail the sanity test here,
> >> > there's a quirk in vfio_msix_early_setup() for a Chelsio device with
> >> > this bug.  Do we really want user experiencing aborts when a simple
> >> > device initialization failure is sufficient?
> >> >
> >> > Generally abort code paths like this cause me to do my own sanity
> >> > testing, which is really poor practice since we should have that sanity
> >> > testing in the common code.  Thanks,  
> >> 
> >> I prefer to assert on programming error, because 1. it does double duty
> >> as documentation, 2. error handling of impossible conditions is commonly
> >> wrong, and 3. assertion failures have a much better chance to get the
> >> program fixed.  Even when presence of a working error path kills 2., the
> >> other two make me stick to assertions.
> >
> > So we're looking at:
> >
> >> -if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> >> -return -EINVAL;
> >> -}
> >
> > vs
> >
> >> +assert(nentries >= 1 && nentries <= PCI_MSIX_FLAGS_QSIZE + 1);
> >
> > How do you argue that one of these provides better self documentation
> > than the other?
> 
> The first one says "this can happen, and when it does, the function
> fails cleanly."  For a genuine programming error, this is in part
> misleading.
> 
> The second one says "I assert this can't happen.  We'd be toast if I was
> wrong."
> 
> > The assert may have a better chance of getting fixed, but it's because
> > the existence of the assert itself exposes a vulnerability in the code.
> > Which would you rather have in production, a VMM that crashes on the
> > slightest deviance from the input it expects or one that simply errors
> > the faulting code path and continues?
> 
> Invalid input to a program should never be treated as programming error.
> 
> > Error handling is hard, which is why we need to look at it as a
> > collection of smaller problems.  We return an error at a leaf function
> > and let callers of that function decide how to handle it.  If some of
> > those callers don't want to deal with error handling, abort there, we
> > can come back to them later, but let the code paths that do want proper
> > error handling to continue.  If we add aborts into the leaf function,
> > then any calling path that wants to be robust against an error needs to
> > fully sanitize the input itself, at which point we have different
> > 

Re: [Qemu-devel] [PATCH] usb: Change *_exitfn return type from void to int

[Jonathan Neuschäfer]

On Fri, Sep 30, 2016 at 11:21:35PM +0530, Akanksha Srivastava wrote:
> Subject: [Qemu-devel] [PATCH] usb: Change *_exitfn return type from void to 
> int

This should read "from int to void", I guess.

> The *_exitfn functions cannot fail and should not be
> returning int.
> Suggested as a Bite-sized task
> Signed-off-by: Akanksha Srivastava 
> ---
>  hw/usb/ccid-card-emulated.c | 4 ++--
>  hw/usb/ccid-card-passthru.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
[...]
> -static int passthru_exitfn(CCIDCardState *base)
> +static void passthru_exitfn(CCIDCardState *base)
>  {
> -return 0;
> +return;
>  }

Have you compile-tested this change? I think you'll have to adjust the
definition of exitfn in CCIDCardClass, and ccid_card_exitfn (in
hw/usb/dev-smartcard-reader.c) as well.


Regards,
Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v14 08/21] qapi: allow QObjectInputVisitor to be created with QemuOpts

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> Instead of requiring all callers to go through the mutli-step
> process of turning QemuOpts into a suitable QObject for visiting,
> add a new constructor that encapsulates this logic. This will
> allow QObjectInputVisitor to be a drop-in replacement for the
> existing OptsVisitor with minimal code changes for callers.
> 
> NB, at this point it is only supporting opts syntax which
> explicitly matches the QAPI schema structure, so is not yet
> a true drop-in replacement for OptsVisitor. The patches that
> follow will add the special cases requird for full backwards

s/requird/required/

> compatibility with OptsVisitor.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

> +++ b/include/qemu/option.h
> @@ -125,7 +125,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const 
> char *params,
>  int permit_abbrev);
>  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> Error **errp);
> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict);
>  void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);

Should the const-correctness of this parameter be hoisted to any earlier
patch?  But here is fine, as the first place where the compiler requires it.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> When converting QemuOpts to a QObject, there is no information
> about compound types available, so when visiting a list, the
> corresponding QObject is not guaranteed to be a QList. We
> therefore need to be able to auto-create a single element QList
> from whatever type we find.
> 
> This mode should only be enabled if you have compatibility
> requirements for
> 
>-arg foo=hello,foo=world
> 
> to be treated as equivalent to the preferred syntax:
> 
>-arg foo.0=hello,foo.1=world
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h | 20 +++-
>  qapi/qobject-input-visitor.c | 27 +--
>  tests/test-qobject-input-visitor.c   | 88 
> +++-
>  3 files changed, 117 insertions(+), 18 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> Currently the QObjectInputVisitor assumes that all scalar
> values are directly represented as the final types declared
> by the thing being visited. ie it assumes an 'int' is using
> QInt, and a 'bool' is using QBool, etc.  This is good when
> QObjectInputVisitor is fed a QObject that came from a JSON
> document on the QMP monitor, as it will strictly validate
> correctness.
> 
> To allow QObjectInputVisitor to be reused for visiting
> a QObject originating from QemuOpts, an alternative mode
> is needed where all the scalars types are represented as
> QString and converted on the fly to the final desired
> type.
> 
> Reviewed-by: Kevin Wolf 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h |  32 +-
>  qapi/qobject-input-visitor.c | 132 
>  tests/test-qobject-input-visitor.c   | 194 
> ++-
>  3 files changed, 350 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] usb: Change *_exitfn return type from void to int

[Akanksha Srivastava]

The *_exitfn functions cannot fail and should not be
returning int.
Suggested as a Bite-sized task
Signed-off-by: Akanksha Srivastava 
---
 hw/usb/ccid-card-emulated.c | 4 ++--
 hw/usb/ccid-card-passthru.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 3213f9f..abb2773 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -547,7 +547,7 @@ static int emulated_initfn(CCIDCardState *base)
 return 0;
 }
 
-static int emulated_exitfn(CCIDCardState *base)
+static void emulated_exitfn(CCIDCardState *base)
 {
 EmulatedState *card = EMULATED_CCID_CARD(base);
 VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
@@ -564,7 +564,7 @@ static int emulated_exitfn(CCIDCardState *base)
 qemu_mutex_destroy(>handle_apdu_mutex);
 qemu_mutex_destroy(>vreader_mutex);
 qemu_mutex_destroy(>event_list_mutex);
-return 0;
+return;
 }
 
 static Property emulated_card_properties[] = {
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 2eacea7..2bd9cb2 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -364,9 +364,9 @@ static int passthru_initfn(CCIDCardState *base)
 return 0;
 }
 
-static int passthru_exitfn(CCIDCardState *base)
+static void passthru_exitfn(CCIDCardState *base)
 {
-return 0;
+return;
 }
 
 static VMStateDescription passthru_vmstate = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v14 06/21] qapi: don't pass two copies of TestInputVisitorData to tests

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> The input_visitor_test_add() method was accepting an instance
> of 'TestInputVisitorData' and passing it as the 'user_data'
> parameter to test functions. The main 'TestInputVisitorData'
> instance that was actually used, was meanwhile being allocated
> automatically by the test framework fixture setup.
> 
> The 'user_data' parameter is going to be needed for tests
> added in later patches, so getting rid of the current mistaken
> usage now allows this.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  tests/test-qobject-input-visitor.c | 76 
> --
>  1 file changed, 32 insertions(+), 44 deletions(-)

Reviewed-by: Eric Blake 

The improved commit message makes the difference :)

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 01/12] block/nbd: Drop trailing "." in error messages

[Eric Blake]

On 09/28/2016 03:55 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c   | 4 ++--
>  tests/qemu-iotests/051.out| 4 ++--
>  tests/qemu-iotests/051.pc.out | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v1 1/1] target-arm: A64: Fix decoding of iss_sf in disas_ld_lit

[Peter Maydell]

On 30 September 2016 at 03:49, Edgar E. Iglesias
 wrote:
> On Fri, Sep 30, 2016 at 12:42:27PM +0200, Thomas Huth wrote:
>> On 30.09.2016 12:19, Edgar E. Iglesias wrote:
>> > From: "Edgar E. Iglesias" 
>> >
>> > Fix the decoding of iss_sf in disas_ld_lit.
>> > The SF (Sixty-Four) field in the ISS (Instruction Specific Syndrome)
>> > is a bit that specifies the width of the register that the
>> > instruction loads to.
>> >
>> > If cleared it specifies 32 bits.
>> > If set it specifies 64 bits.
>> >
>> > Signed-off-by: Edgar E. Iglesias 
>> > ---
>> >  target-arm/translate-a64.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> > index ddf52f5..eae25c3 100644
>> > --- a/target-arm/translate-a64.c
>> > +++ b/target-arm/translate-a64.c
>> > @@ -2025,7 +2025,7 @@ static void disas_ld_lit(DisasContext *s, uint32_t 
>> > insn)
>> >  do_fp_ld(s, rt, tcg_addr, size);
>> >  } else {
>> >  /* Only unsigned 32bit loads target 32bit registers.  */
>> > -bool iss_sf = opc == 0 ? 32 : 64;
>> > +bool iss_sf = opc == 0 ? false : true;
>>
>> You could simplify that to:
>>
>>   bool iss_sf = !(opc == 0);
>
> I don't really see how that is simpler/clearer.
>
> I considered:
> bool iss_sf = opc != 0;
>
> but felt the expanded one was clearer in particular as a patch for review.

I would probably personally have written it without the ?:, but
I don't think it's significant enough to bother respinning.

thanks
-- PMM