[Qemu-devel] [PATCH] iotests: 153: Fix dead code

[Fam Zheng]

This step was left behind my mistake. As suggested by the echoed text,
the intention was to test two devices with the same image, with
different options. The behavior should be the same as two QEMU
processes. Complete it.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/153 |  2 ++
 tests/qemu-iotests/153.out | 25 +
 2 files changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index 673813cd39..0daeb1b005 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -162,6 +162,7 @@ for opts1 in "" "read-only=on" 
"read-only=on,force-share=on"; do
 _cleanup_qemu
 done
 
+test_opts="read-only=off read-only=on read-only=on,force-share=on"
 for opt1 in $test_opts; do
 for opt2 in $test_opts; do
 echo
@@ -170,6 +171,7 @@ for opt1 in $test_opts; do
 done
 done
 
+echo
 echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
 (
 $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 3492ba7a29..93eaf10486 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -369,6 +369,31 @@ _qemu_img_wrapper bench -U -w -c 1 TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': force-share=on can only be used 
with read-only images
 
 Round done
+
+== Two devices with the same image (read-only=off - read-only=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=off: Failed to get 
"write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=off - read-only=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=on: Failed to get 
shared "write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=off - 
read-only=on,force-share=on) ==
+
+== Two devices with the same image (read-only=on - read-only=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,read-only=off: Failed to get 
"write" lock
+Is another process using the image?
+
+== Two devices with the same image (read-only=on - read-only=on) ==
+
+== Two devices with the same image (read-only=on - 
read-only=on,force-share=on) ==
+
+== Two devices with the same image (read-only=on,force-share=on - 
read-only=off) ==
+
+== Two devices with the same image (read-only=on,force-share=on - 
read-only=on) ==
+
+== Two devices with the same image (read-only=on,force-share=on - 
read-only=on,force-share=on) ==
+
 == Creating TEST_DIR/t.qcow2.[abc] ==
 Formatting 'TEST_DIR/t.IMGFMT.a', fmt=IMGFMT size=33554432 
backing_file=TEST_DIR/t.IMGFMT
 Formatting 'TEST_DIR/t.IMGFMT.b', fmt=IMGFMT size=33554432 
backing_file=TEST_DIR/t.IMGFMT
-- 
2.17.1

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 10.07.2018 08:50, Peter Maydell wrote:
> On 9 July 2018 at 23:03, Thomas Huth  wrote:
>> On 09.07.2018 23:42, Peter Maydell wrote:
>>> On 9 July 2018 at 22:03, Thomas Huth  wrote:
 When trying to "device_add bcm2837" on a machine that is not suitable for
 this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":

 echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " 
 \
  "'arguments': {'command-line': 'info qtree'}}" | \
  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp 
 stdio

 {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
  "package": "build-all"}, "capabilities": []}}
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
  hotplugged on this machine"}}
 Segmentation fault (core dumped)

 The problem is that qdev_set_parent_bus() from instance_init adds a link
 to the child devices which is not valid anymore after the device init
 failed. Thus the qdev_set_parent_bus() must rather be done in the realize
 function instead.
>>>
>>> Yuck. The real problem here is that we're still requiring the
>>> code that creates these QOM devices to manually set the parent
>>> in the first place. It's not surprising that we don't get it right
>>> (either parenting in the wrong place or not at all). I'd much
>>> rather see us fix that properly than keep papering over places
>>> where we get it wrong.
>>
>> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
>> you exactly recommend to do instead?
> 
> I'm not clear either, but I don't think that what we're
> currently doing can be right.

Hm, ok, so how to continue here now? Shall we at least mark the
bcm2836/7 devices with user_creatable=false, so that users can not crash
their QEMU so easily with device_add? The problem with introspection via
device-list-properties would still continue to exist, but I think that's
less likely used in practice... otherwise we could still move the
qdev_set_parent_bus() calls to the realize() function instead, and just
add a big fat FIXME comment in front of the code block, so that we
remember to clean that up one day...

 Thomas

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Cornelia Huck]

On Wed, 11 Jul 2018 08:53:20 +0200
Thomas Huth  wrote:

> On 10.07.2018 17:24, Peter Krempa wrote:
> > On Tue, Jul 10, 2018 at 17:01:22 +0200, Cornelia Huck wrote:  

> >> So, from that I gather that a hard failure would be the easiest for
> >> libvirt to detect (and everything else would become complicated really
> >> quickly), right?  
> > 
> > People start complaining only when stuff breaks. If anything is optional
> > people will usually not enable it. That makes any non-mandatory option
> > not work in most cases.  
> 
> So would it help if we "invert" the logic, i.e. deprecated_report()
> would do exit(1) by default? Then, if the (human) users still want to
> continue with the deprecated option, they have to add a
> "--ignore-deprecation" command line switch to make QEMU start
> successfully...

That is sure to get the attention of even 'normal' users, but we'd have
to make it really, really obvious (1) how to get it working again and
(2) what other things we'd like them to do (like 'if you are using a
management tool, please let them know about it'). I'm fearing a lot of
'I followed that ancient guide, and it does not work' type of reports,
though.

[Qemu-devel] [Bug 1476183] Re: can not create 4 serial port on window (guest os)

[Thomas Huth]

Looking through old bug tickets... can you still reproduce this issue
with the latest version of QEMU and Windows? Or could we close this
ticket nowadays?

** Changed in: qemu
   Status: New => Incomplete

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

Title:
  can not create 4 serial port on window (guest os)

Status in QEMU:
  Incomplete

Bug description:
  qemu ver: 2.1.2-Latest

  guest os: window 7 64bit with 2 cpu

  problem: when qemu start with 4 serial port, on linux(rhel 7) guest
  os, /dev/ttyS0-4 is work fine.  but on window 7 guest os, only show
  com1,com2 in device manager, how to get com3 & com4 ?

  qemu cmd:
   -chardev spiceport,id=charserial0,name=org.qemu.console.serial.0
   -device isa-serial,chardev=charserial0,id=serial0
   -chardev spiceport,id=charserial1,name=org.qemu.console.serial.1
   -device isa-serial,chardev=charserial1,id=serial1
   -chardev spiceport,id=charserial2,name=org.qemu.console.serial.2
   -device isa-serial,chardev=charserial2,id=serial2
   -chardev spiceport,id=charserial3,name=org.qemu.console.serial.3
   -device isa-serial,chardev=charserial3,id=serial3

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

[Qemu-devel] [Bug 1476800] Re: Instant runtime error (Host: Windows 8.1 VM: WinXP ISO)

[Thomas Huth]

Looking through old bug tickets... which version of QEMU did you use
here? Can you still reproduce this issue with the latest version of
QEMU? Or could we close this ticket nowadays?


** Changed in: qemu
   Status: New => Incomplete

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

Title:
  Instant runtime error (Host: Windows 8.1 VM: WinXP ISO)

Status in QEMU:
  Incomplete

Bug description:
  I have Qemu Manager on my Windows 8.1 laptop and have a WXP iso and a
  blank disk image (from here
  http://www.mediafire.com/download/rtec86bwwmee00s/Blank_Disk.zip ) and
  as soon as I try to open it I get a Runtime Error (
  http://i.gyazo.com/bfebf7e1e7a670f8e52cc95c5923a67e.png )

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

[Qemu-devel] [Bug 1474263] Re: "Image format was not specified" warning should be suppressed for the vvfat (and probably nbd) driver

[Thomas Huth]

Looking through old bug tickets...  can you still reproduce this bug
with the latest version of QEMU? At least for vvfat, the warning message
does not seem to occur anymore.

** Changed in: qemu
   Status: New => Incomplete

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

Title:
  "Image format was not specified" warning should be suppressed for the
  vvfat (and probably nbd) driver

Status in QEMU:
  Incomplete

Bug description:
  Running

  qemu -drive file.driver=vvfat,file.dir=.

  displays

  WARNING: Image format was not specified for 'json:{"dir": ".", "driver": 
"vvfat"}' and probing guessed raw.
   Automatically detecting the format is dangerous for raw images, 
write operations on block 0 will be restricted.
   Specify the 'raw' format explicitly to remove the restrictions.

  However, since "images" provided by the vvfat driver are always raw
  (and the first sector isn't writeable in any case), this warning is
  superfluous and should not be displayed.

  A similar warning is displayed for NBD devices; I suspect it should be
  also disabled for similar reasons, but I'm not sure if serving non-raw
  images is actually a violation of the protocol. qemu-nbd translates
  them to raw images, for what it's worth, even if it may be suppressed
  with -f raw.

  Noticed on 2.3.0; the code that causes this behaviour is still
  apparently present in today's git master
  (f3a1b5068cea303a55e2a21a97e66d057eaae638). Speaking of versions: you
  may want to update the copyright notice that qemu -version displays.

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

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[stephane duverger]

To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
> entered gdb_vm_state_change() with and use CPUState *cpu =
> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
> which then deref crap->singlestep_enabled.
>
> So I think this is not the correct fix.
>

I think you are wrong. You can enter gdb_vm_state_change() only if it has
been registered through qemu_add_vm_change_state_handler(). And this
happens in gdbserver_start() which is called only when you start the
gdbserver stub.

This is exactly what we don't want to do here: use qemu breakpoints and
debug events forwarding without the need to enable a gdb stub.

There might be an historical reason that vCPU debug events are always
forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
should not be mandatory.

One could consider a check to the gdbserver state right before:

if (gdbserver_enabled())
gdb_set_stop_cpu(cpu);

As found in other part of qemu code: kvm_enabled, hax_enabled, ...


> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
> reach this.
>

Correct if you previously add the gdbserver_enabled() check. Else this
would abort the qemu instance each time a debug event is triggered
and forwarded to a vm_change_state handler.

>>>  void gdb_set_stop_cpu(CPUState *cpu)
> >>>  {
> >>> +if (!gdbserver_state) {
> >>> +return;
> >>> +}
> >>>  gdbserver_state->c_cpu = cpu;
> >>>  gdbserver_state->g_cpu = cpu;
>
> I find it safer the opposite way, if (s) { s-> ... }
>

Sincerely, this argument is subjective. If it's part of Qemu coding
standard,
i would agree. Again there is a lot of situations in the Qemu code where
exit conditions are checked first and then lead to a return, preventing an
unneeded level of indentation. Seriously, we are talking about a 2 lines
function.

stephane

Re: [Qemu-devel] [PATCH] docker: Update debootstrap script after Debian migration from Alioth to Salsa

[Fam Zheng]

On Tue, 07/03 12:35, Philippe Mathieu-Daudé wrote:
> This silents the following warning:
> 
>   Cloning into './debootstrap.git'...
>   warning: redirecting to 
> https://salsa.debian.org/installer-team/debootstrap.git/
> 
> See https://lists.debian.org/debian-devel-announce/2018/01/msg4.html
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/docker/dockerfiles/debian-bootstrap.pre | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre 
> b/tests/docker/dockerfiles/debian-bootstrap.pre
> index 7c76dce663..44b18db7a2 100755
> --- a/tests/docker/dockerfiles/debian-bootstrap.pre
> +++ b/tests/docker/dockerfiles/debian-bootstrap.pre
> @@ -53,7 +53,7 @@ if [ -z $DEBOOTSTRAP_DIR ]; then
>  NEED_DEBOOTSTRAP=true
>  fi
>  if $NEED_DEBOOTSTRAP; then
> -DEBOOTSTRAP_SOURCE=https://anonscm.debian.org/git/d-i/debootstrap.git
> +
> DEBOOTSTRAP_SOURCE=https://salsa.debian.org/installer-team/debootstrap.git
>  git clone ${DEBOOTSTRAP_SOURCE} ./debootstrap.git
>  export DEBOOTSTRAP_DIR=./debootstrap.git
>  DEBOOTSTRAP=./debootstrap.git/debootstrap
> -- 
> 2.18.0
> 

Queued, thanks.

Fam

Re: [Qemu-devel] [PATCH 07/12] migration: hold the lock only if it is really needed

[Peter Xu]

On Thu, Jun 28, 2018 at 05:33:58PM +0800, Xiao Guangrong wrote:
> 
> 
> On 06/19/2018 03:36 PM, Peter Xu wrote:
> > On Mon, Jun 04, 2018 at 05:55:15PM +0800, guangrong.x...@gmail.com wrote:
> > > From: Xiao Guangrong 
> > > 
> > > Try to hold src_page_req_mutex only if the queue is not
> > > empty
> > 
> > Pure question: how much this patch would help?  Basically if you are
> > running compression tests then I think it means you are with precopy
> > (since postcopy cannot work with compression yet), then here the lock
> > has no contention at all.
> 
> Yes, you are right, however we can observe it is in the top functions
> (after revert this patch):
> 
> Samples: 29K of event 'cycles', Event count (approx.): 22263412260
> +   7.99%  kqemu  qemu-system-x86_64   [.] ram_bytes_total
> +   6.95%  kqemu  [kernel.kallsyms][k] copy_user_enhanced_fast_string
> +   6.23%  kqemu  qemu-system-x86_64   [.] qemu_put_qemu_file
> +   6.20%  kqemu  qemu-system-x86_64   [.] qemu_event_set
> +   5.80%  kqemu  qemu-system-x86_64   [.] __ring_put
> +   4.82%  kqemu  qemu-system-x86_64   [.] compress_thread_data_done
> +   4.11%  kqemu  qemu-system-x86_64   [.] ring_is_full
> +   3.07%  kqemu  qemu-system-x86_64   [.] threads_submit_request_prepare
> +   2.83%  kqemu  qemu-system-x86_64   [.] ring_mp_get
> +   2.71%  kqemu  qemu-system-x86_64   [.] __ring_is_full
> +   2.46%  kqemu  qemu-system-x86_64   [.] buffer_zero_sse2
> +   2.40%  kqemu  qemu-system-x86_64   [.] add_to_iovec
> +   2.21%  kqemu  qemu-system-x86_64   [.] ring_get
> +   1.96%  kqemu  [kernel.kallsyms][k] __lock_acquire
> +   1.90%  kqemu  libc-2.12.so [.] memcpy
> +   1.55%  kqemu  qemu-system-x86_64   [.] ring_len
> +   1.12%  kqemu  libpthread-2.12.so   [.] pthread_mutex_unlock
> +   1.11%  kqemu  qemu-system-x86_64   [.] ram_find_and_save_block
> +   1.07%  kqemu  qemu-system-x86_64   [.] ram_save_host_page
> +   1.04%  kqemu  qemu-system-x86_64   [.] qemu_put_buffer
> +   0.97%  kqemu  qemu-system-x86_64   [.] compress_page_with_multi_thread
> +   0.96%  kqemu  qemu-system-x86_64   [.] ram_save_target_page
> +   0.93%  kqemu  libpthread-2.12.so   [.] pthread_mutex_lock

(sorry to respond late; I was busy with other stuff for the
 release...)

I am trying to find out anything related to unqueue_page() but I
failed.  Did I miss anything obvious there?

> 
> I guess its atomic operations cost CPU resource and check-before-lock is
> a common tech, i think it shouldn't have side effect, right? :)

Yeah it makes sense to me. :)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH V5] qemu-img: align result of is_allocated_sectors

[Kevin Wolf]

Am 10.07.2018 um 22:16 hat Peter Lieven geschrieben:
> 
> 
> > Am 10.07.2018 um 17:31 schrieb Kevin Wolf :
> > 
> > Am 10.07.2018 um 17:05 hat Peter Lieven geschrieben:
> >> We currently don't enforce that the sparse segments we detect during 
> >> convert are
> >> aligned. This leads to unnecessary and costly read-modify-write cycles 
> >> either
> >> internally in Qemu or in the background on the storage device as nearly all
> >> modern filesystems or hardware have a 4k alignment internally.
> >> 
> >> This patch modifies is_allocated_sectors so that its *pnum result will 
> >> always
> >> end at an alignment boundary. This way all requests will end at an 
> >> alignment
> >> boundary. The start of all requests will also be aligned as long as the 
> >> results
> >> of get_block_status do not lead to an unaligned offset.
> >> 
> >> The number of RMW cycles when converting an example image [1] to a raw 
> >> device that
> >> has 4k sector size is about 4600 4k read requests to perform a total of 
> >> about 15000
> >> write requests. With this path the additional 4600 read requests are 
> >> eliminated while
> >> the number of total write requests stays constant.
> >> 
> >> [1] 
> >> https://cloud-images.ubuntu.com/releases/16.04/release/ubuntu-16.04-server-cloudimg-amd64-disk1.vmdk
> >> 
> >> Signed-off-by: Peter Lieven 
> > 
> > It looked convincing, but I'm afraid this is still not correct.
> > qemu-iotests 122 fails for me with this patch.
> 
> I will have a look, where and why exactly it fails, but the allocation
> pattern might be slightly different due to the alignment. What counts
> is that the output is byte identical or not?

Right, I noticed only after sending this email that it's qemu-img map
output that changes and this might actually be okay. I didn't check,
however, if the exact changes are what is expected and whether we need
to add more test cases to cover what the test originally wanted to
cover.

So after all, there's a good chance that all that's missing is just an
update to the test case.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

Gentle ping. CCing Paolo Bonzini.

Regards, Dima.

On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
> Ping.
> 
> Regards, Dima.
> 
> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
> > In the memory_region_do_invalidate_mmio_ptr() routine the section
> > variable is intialized by the memory_region_find() call. The section.mr
> > field can be set to NULL.
> > 
> > Add the check for NULL before trying to drop a section.
> > 
> > Signed-off-by: Dima Stepanov 
> > ---
> >  memory.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 3212acc..bb45248 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -2712,7 +2712,7 @@ static void 
> > memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
> >  /* Reset dirty so this doesn't happen later. */
> >  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> >  
> > -if (section.mr != mr) {
> > +if (section.mr && (section.mr != mr)) {
> >  /* memory_region_find add a ref on section.mr */
> >  memory_region_unref(section.mr);
> >  if (MMIO_INTERFACE(section.mr->owner)) {
> > -- 
> > 2.7.4
> > 
> > 

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue

[Kevin Wolf]

Am 10.07.2018 um 16:02 hat Marc-André Lureau geschrieben:
> Hi
> 
> On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf  wrote:
> > Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
> >> handle_qmp_command() reports JSON syntax errors right away.  This is
> >> wrong when OOB is enabled, because the errors can "jump the queue"
> >> then.
> >>
> >> The previous commit fixed the same bug for semantic errors, by
> >> delaying the checking until dispatch.  We can't delay the checking, so
> >> delay the reporting.
> >>
> >> Signed-off-by: Markus Armbruster 
> >> Reviewed-by: Eric Blake 
> >> Message-Id: <20180703085358.13941-19-arm...@redhat.com>
> >
> > I'm observing a qemu crash in qemu-iotests 153 (which does however not
> > seem to make the test case fail). git bisect points me to this patch.
> >
> > I'm getting output like this:
> >
> > *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): 
> > invalid pointer: 0x555f7870f7e0 ***
> > === Backtrace: =
> > /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
> > /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
> > /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
> > /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
> > /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
> > /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
> >
> > Interestingly, this doesn't want to produce a core dump for me, so no
> > backtrace with usable function names here. But I assume that you can
> > easily reproduce this yourself.
> >
> 
> Looks like the double-free regression, you could try: "[PATCH]
> monitor: fix double-free of request error"

Thanks, that does fix it. Looks like it missed -rc0, though?

Kevin

Re: [Qemu-devel] [PATCH v3 15/20] kvm: arm/arm64: Allow tuning the physical address size for VM

[Suzuki K Poulose]

On 10/07/18 18:03, Dave Martin wrote:

On Tue, Jul 10, 2018 at 05:38:39PM +0100, Suzuki K Poulose wrote:

On 09/07/18 14:37, Dave Martin wrote:

On Mon, Jul 09, 2018 at 01:29:42PM +0100, Marc Zyngier wrote:

On 09/07/18 12:23, Dave Martin wrote:


[...]


Wedging arguments into a few bits in the type argument feels awkward,
and may be regretted later if we run out of bits, or something can't be
represented in the chosen encoding.


I think that's a pretty convincing argument for a "better" CREATE_VM,
one that would have a clearly defined, structured (and potentially
extensible) argument.

I've quickly hacked the following:

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index b6270a3b38e9..3e76214034c2 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -735,6 +735,20 @@ struct kvm_ppc_resize_hpt {
__u32 pad;
  };

+struct kvm_create_vm2 {
+   __u64   version;/* Or maybe not */
+   union {
+   struct {
+#define KVM_ARM_SVE_CAPABLE(1 << 0)
+#define KVM_ARM_SELECT_IPA {1 << 1)
+   __u64   capabilities;
+   __u16   sve_vlen;
+   __u8ipa_size;
+   } arm64;
+   __u64   dummy[15];
+   };
+};
+
  #define KVMIO 0xAE

  /* machine type bits, to be used as argument to KVM_CREATE_VM */

Other architectures could fill in their own bits if they need to.

Thoughts?


This kind of thing should work, but it may still get messy when we
add additional fields.



Marc, Dave,

I like Dave's approach. Some comments below.



It we want this to work cross-arch, would it make sense to go
for a more generic approach, say

struct kvm_create_vm_attr_any {
 __u32   type;
};

#define KVM_CREATE_VM_ATTR_ARCH_CAPABILITIES 1
struct kvm_create_vm_attr_arch_capabilities {
 __u32   type;
 __u16   size; /* support future expansion of capabilities[] */
 __u16   reserved;
 __u64   capabilities[1];
};


We also need to advertise which attributes are supported by the host,
so that the user can tune the available ones. That would make a bit mask
like the above trickier, unless we return the supported values back
in the argument ptr for the "probe" call. And this scheme in general
can be useful for passing back a non-boolean result specific to the
attribute, without having a per-attribute ioctl. (e.g, maximum limit
for IPA).


Maybe, but this could quickly become bloated.  (My approach already
feels a bit bloated...)

I'm not sure that arbitrarily complex negotiation will really be
needed, but userspace might want to change its mind if setting a
particular propertiy fails.

An alternative might be to have a bunch of per-VM ioctls to configure
different things, like x86 has.  There's at least precedent for that.
For arm, we currently only have a few.  That allows for easy extension,
at the cost of adding ioctls.


As you know, one of the major problems with the per-VM ioctls is
the ordering of different operations and tracking to make sure that
the userspace follows the expected order. e.g, the first approach for
IPA series was based on this and it made things complex enough to drop
it.



There may be some ioctls we can reuse, like KVM_ENABLE_CAP for per-
vm capability flags.


May be we could switch to KVM_VM_CAPS and pass a list of capabilities
to be enabled at creation time ? The kvm_enable_cap can pass in additional
arguments for each cap. That way we don't have to rely on a new set of
attributes and probing becomes straight forward.

Suzuki

Re: [Qemu-devel] [PATCH] ppc/xics: split ICP into icp-base and icp class

[Greg Kurz]

On Wed, 11 Jul 2018 11:28:02 +1000
David Gibson  wrote:

> On Tue, Jul 10, 2018 at 05:55:14PM +0200, Greg Kurz wrote:
> > Recent cleanup in commit a028dd423ee6 causes QEMU to crash during CPU
> > hotplug:
> > 
> > (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> > Segmentation fault (core dumped)
> > 
> > When the hotplug path tries to reset the ICP device, we end
> > up calling:
> > 
> > static void icp_kvm_reset(DeviceState *dev)
> > {
> > ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > 
> > icpc->parent_reset(dev);
> > 
> > but icpc->parent_reset is NULL.  
> 
> This seems terribly complex to address this symptom.  Couldn't we just
> do a
> 
>   if (icpc->parent_reset)
>   icpc->parent_reset(dev);
> 
> ?
> 

This would certainly avoid the SEGV but this would mean that
we skip icp_reset() for hot plugged ICPs... and it doesn't
address the fact that icp_kvm_reset() is never called for cold
plugged ones.

Commit a028dd423ee6 inverted the control: icp_reset() no
longer calls per-ICP type reset function. It is up to each
type to call icp_reset().

Maybe I can split the patch to ease review. But if you're really
seeking simplicity, the best call is probably to simply revert
a028dd423ee6 at this point.

> > Indeed, the TYPE_ICP class doesn't implement DeviceClass::reset, but
> > rather directly registers a reset handler with qemu_register_reset().
> > This is done this way because ICPs aren't SysBus devices but we want
> > machine reset to reset them anyway (commit 7ea6e0671754).
> > 
> > The crash also proves that ipc_kvm_reset() is obviously not called for
> > cold plugged devices, ie, TYPE_ICP_KVM should register its own handler
> > with qemu_register_reset().
> > 
> > The motivation of commit a028dd423ee6 to have cleaner object patterns
> > is good, but it means that all specialized ICP types should register
> > their own reset handler to be called at machine reset.
> > 
> > So this patchs converts the current TYPE_ICP class into an abstract
> > TYPE_ICP_BASE class that implements DeviceClass::reset instead of
> > calling qemu_register_reset().
> > 
> > The _new_ TYPE_ICP class is derived from TYPE_ICP_BASE to cover the
> > the pseries.kernel-irqchip=off case. It merely registers a reset
> > handler with qemu_register_reset(). Its device class functions
> > are renamed as icp_simple_* to avoid confusion with the base class.
> > 
> > All other specialized ICP types are also converted to register their
> > own reset handler as well.
> > 
> > This matches what we already have with ICS.
> > 
> > Since we support CPU hot unplug with spapr, unrealize functions
> > are added to TYPE_ICP and TYPE_ICP_KVM so that they can call
> > qemu_unregister_reset().
> > 
> > Reported-by: Satheesh Rajendran 
> > Fixes: a028dd423ee6dfd091a8c63028240832bf10f671
> > Signed-off-by: Greg Kurz 
> > ---
> > 
> > I've successfully tested the following:
> > - pseries with in-kernel XICS
> > - pseries with emulated XICS
> > - powernv
> > - migration of pseries, including migration to QEMU 2.12 and back
> > ---
> >  hw/intc/xics.c|   97 
> > -
> >  hw/intc/xics_kvm.c|   27 +++---
> >  hw/intc/xics_pnv.c|   27 +++---
> >  include/hw/ppc/xics.h |   12 --
> >  4 files changed, 129 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index b9f1a3c97214..b478b9120062 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -40,7 +40,7 @@
> >  
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon)
> >  {
> > -ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > +ICPStateClass *icpc = ICP_BASE_GET_CLASS(icp);
> >  int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> >  
> >  if (!icp->output) {
> > @@ -255,7 +255,7 @@ static void icp_irq(ICSState *ics, int server, int nr, 
> > uint8_t priority)
> >  static int icp_dispatch_pre_save(void *opaque)
> >  {
> >  ICPState *icp = opaque;
> > -ICPStateClass *info = ICP_GET_CLASS(icp);
> > +ICPStateClass *info = ICP_BASE_GET_CLASS(icp);
> >  
> >  if (info->pre_save) {
> >  info->pre_save(icp);
> > @@ -267,7 +267,7 @@ static int icp_dispatch_pre_save(void *opaque)
> >  static int icp_dispatch_post_load(void *opaque, int version_id)
> >  {
> >  ICPState *icp = opaque;
> > -ICPStateClass *info = ICP_GET_CLASS(icp);
> > +ICPStateClass *info = ICP_BASE_GET_CLASS(icp);
> >  
> >  if (info->post_load) {
> >  return info->post_load(icp, version_id);
> > @@ -291,9 +291,9 @@ static const VMStateDescription vmstate_icp_server = {
> >  },
> >  };
> >  
> > -static void icp_reset(void *dev)
> > +static void icp_base_reset(DeviceState *dev)
> >  {
> > -ICPState *icp = ICP(dev);
> > +ICPState *icp = ICP_BASE(dev);
> >  
> >  icp->xirr = 0;
> >  icp->pending_priority = 0xff;
> > @@ -303,9 +303,9 @@ static void icp_reset(void *dev)
> >  qemu_set_irq(icp->output, 0);
> >  }
> >  

Re: [Qemu-devel] [PATCH] iotests: 153: Fix dead code

[Kevin Wolf]

Am 11.07.2018 um 09:05 hat Fam Zheng geschrieben:
> This step was left behind my mistake. As suggested by the echoed text,
> the intention was to test two devices with the same image, with
> different options. The behavior should be the same as two QEMU
> processes. Complete it.
> 
> Signed-off-by: Fam Zheng 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [virtio-dev] Re: [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...

[Cornelia Huck]

On Tue, 10 Jul 2018 17:07:37 -0700
Siwei Liu  wrote:

> On Mon, Jul 9, 2018 at 6:54 PM, Michael S. Tsirkin  wrote:
> > On Mon, Jul 09, 2018 at 06:11:53PM -0700, si-wei liu wrote:  
> >> The plan is to enable group ID based matching in the first place rather 
> >> than
> >> match by MAC, the latter of which is fragile and problematic.  
> >
> > It isn't all that fragile - hyperv used same for a while, so if someone
> > posts working patches with QEMU support but before this grouping stuff,
> > I'll happily apply them.  
> 
> I wouldn't box the solution to very limited scenario just because of
> matching by MAC, the benefit of having generic group ID in the first
> place is that we save the effort of maintaining legacy MAC based
> pairing that just adds complexity anyway. Currently the VF's MAC
> address cannot be changed by either PF or by the guest user is a
> severe limitation due to this. The other use case is that PT device
> than VF would generally have different MAC than the standby virtio. We
> shouldn't limit itself to VF specific scenario from the very
> beginning.

So, this brings me to a different concern: the semantics of
VIRTIO_NET_F_STANDBY.

* The currently sole user seems to be the virtio-net Linux driver.
* The commit messages, code comments and Documentation/ all talk about
  matching by MAC.
* I could not find any proposed update to the virtio spec. (If there
  had been an older proposal with a different feature name, it is not
  discoverable.)

VIRTIO_NET_F_STANDBY is a host <-> guest interface. As there's no
official spec, you can only go by the Linux implementation, and by that
its semantics seem to be 'match by MAC', not 'match by other criteria'.

How is this supposed to work in the long run?

Re: [Qemu-devel] [PATCH v2] iotests: nbd: Stop qemu-nbd before remaking image

[Kevin Wolf]

Am 11.07.2018 um 08:40 hat Fam Zheng geschrieben:
> 197 is one example where _make_test_img is used twice without stopping
> the NBD server in between. An error will occur like this:
> 
> @@ -26,9 +26,13 @@
> 
>  === Partial final cluster ===
> 
> +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "resize" lock
> +Is another process using the image?
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024
> +Failed to find an available port: Address already in use
>  read 1024/1024 bytes at offset 0
> 
> Patch _make_test_img to stop the old qemu-nbd before starting a new one,
> which fixes this problem, and similarly 215.
> 
> Signed-off-by: Fam Zheng 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: Document copy offloading implications with -S and -c

[Kevin Wolf]

Am 11.07.2018 um 03:22 hat Fam Zheng geschrieben:
> Explicitly enabling zero detection or compression suppresses copy
> offloading during convert. Document it.
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Fam Zheng 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [qemu-s390x] [RFC 01/15] s390 vfio-ccw: Add bootindex property and IPLB data

[Thomas Huth]

On 05.07.2018 19:25, Jason J. Herne wrote:
> From: "Jason J. Herne" 

You might want to do a --reset-author of this patch to get rid of this
"From:" line...

> Add bootindex property and iplb data for vfio-ccw devices. This allows us to
> forward boot information into the bios for vfio-ccw devices.
> 
> Signed-off-by: Jason J. Herne 

... and remove the superfluous S-o-b line, too?

> Acked-by: Halil Pasic 
> Signed-off-by: Jason J. Herne 

 Thomas

Re: [Qemu-devel] [PATCH v3 15/20] kvm: arm/arm64: Allow tuning the physical address size for VM

[Dave Martin]

On Wed, Jul 11, 2018 at 10:05:50AM +0100, Suzuki K Poulose wrote:
> On 10/07/18 18:03, Dave Martin wrote:
> >On Tue, Jul 10, 2018 at 05:38:39PM +0100, Suzuki K Poulose wrote:
> >>On 09/07/18 14:37, Dave Martin wrote:
> >>>On Mon, Jul 09, 2018 at 01:29:42PM +0100, Marc Zyngier wrote:
> On 09/07/18 12:23, Dave Martin wrote:
> >
> >[...]
> >
> >Wedging arguments into a few bits in the type argument feels awkward,
> >and may be regretted later if we run out of bits, or something can't be
> >represented in the chosen encoding.
> 
> I think that's a pretty convincing argument for a "better" CREATE_VM,
> one that would have a clearly defined, structured (and potentially
> extensible) argument.
> 
> I've quickly hacked the following:
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index b6270a3b38e9..3e76214034c2 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -735,6 +735,20 @@ struct kvm_ppc_resize_hpt {
>   __u32 pad;
>   };
> 
> +struct kvm_create_vm2 {
> + __u64   version;/* Or maybe not */
> + union {
> + struct {
> +#define KVM_ARM_SVE_CAPABLE  (1 << 0)
> +#define KVM_ARM_SELECT_IPA   {1 << 1)
> + __u64   capabilities;
> + __u16   sve_vlen;
> + __u8ipa_size;
> + } arm64;
> + __u64   dummy[15];
> + };
> +};
> +
>   #define KVMIO 0xAE
> 
>   /* machine type bits, to be used as argument to KVM_CREATE_VM */
> 
> Other architectures could fill in their own bits if they need to.
> 
> Thoughts?
> >>>
> >>>This kind of thing should work, but it may still get messy when we
> >>>add additional fields.
> >>
> >>
> >>Marc, Dave,
> >>
> >>I like Dave's approach. Some comments below.
> >>
> >>>
> >>>It we want this to work cross-arch, would it make sense to go
> >>>for a more generic approach, say
> >>>
> >>>struct kvm_create_vm_attr_any {
> >>> __u32   type;
> >>>};
> >>>
> >>>#define KVM_CREATE_VM_ATTR_ARCH_CAPABILITIES 1
> >>>struct kvm_create_vm_attr_arch_capabilities {
> >>> __u32   type;
> >>> __u16   size; /* support future expansion of capabilities[] */
> >>> __u16   reserved;
> >>> __u64   capabilities[1];
> >>>};
> >>
> >>We also need to advertise which attributes are supported by the host,
> >>so that the user can tune the available ones. That would make a bit mask
> >>like the above trickier, unless we return the supported values back
> >>in the argument ptr for the "probe" call. And this scheme in general
> >>can be useful for passing back a non-boolean result specific to the
> >>attribute, without having a per-attribute ioctl. (e.g, maximum limit
> >>for IPA).
> >
> >Maybe, but this could quickly become bloated.  (My approach already
> >feels a bit bloated...)
> >
> >I'm not sure that arbitrarily complex negotiation will really be
> >needed, but userspace might want to change its mind if setting a
> >particular propertiy fails.
> >
> >An alternative might be to have a bunch of per-VM ioctls to configure
> >different things, like x86 has.  There's at least precedent for that.
> >For arm, we currently only have a few.  That allows for easy extension,
> >at the cost of adding ioctls.
> 
> As you know, one of the major problems with the per-VM ioctls is
> the ordering of different operations and tracking to make sure that
> the userspace follows the expected order. e.g, the first approach for
> IPA series was based on this and it made things complex enough to drop
> it.

I'm aware of that, but if we are adding a new KVM_CREATE_VM, we could
perhaps give it different semantics: i.e., we create a half-created VM
that only accepts configuration ioctls and a "finish creation" ioctl
that finalises everything before you're allowed to create devices,
vcpus etc.

This is the sort of thing I was moving torwards for SVE (but for
vcpus there).

I'm not saying we should drop the existing KVM_CREATE_VM2 ideas,
but that we should take a step back if it starts to accrue complexity.

> >
> >There may be some ioctls we can reuse, like KVM_ENABLE_CAP for per-
> >vm capability flags.
> 
> May be we could switch to KVM_VM_CAPS and pass a list of capabilities
> to be enabled at creation time ? The kvm_enable_cap can pass in additional
> arguments for each cap. That way we don't have to rely on a new set of
> attributes and probing becomes straight forward.

That's a possibility.  I guess we'd need to understand how exactly x86
uses this.

Cheers
---Dave

[Qemu-devel] [PATCH for-3.0] target/arm: Fix LD1W and LDFF1W (scalar plus vector)

[Richard Henderson]

'I' was being double-incremented; correctly within the inner loop
and incorrectly within the outer loop.

Signed-off-by: Richard Henderson 
---

Fixes a SIGSEGV within one of these generated helpers,
exposed by an armclang vectorized code sample.


r~

---
 target/arm/sve_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index cec0d3ee54..ddc592ff79 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -4855,7 +4855,7 @@ void HELPER(NAME)(CPUARMState *env, void *vd, void *vg, 
void *vm,   \
 intptr_t i, oprsz = simd_oprsz(desc);   \
 unsigned scale = simd_data(desc);   \
 uintptr_t ra = GETPC(); \
-for (i = 0; i < oprsz; i++) {   \
+for (i = 0; i < oprsz; ) {  \
 uint16_t pg = *(uint16_t *)(vg + H1_2(i >> 3)); \
 do {\
 TYPEM m = 0;\
@@ -4936,7 +4936,7 @@ void HELPER(NAME)(CPUARMState *env, void *vd, void *vg, 
void *vm,   \
 uintptr_t ra = GETPC(); \
 bool first = true;  \
 mmap_lock();\
-for (i = 0; i < oprsz; i++) {   \
+for (i = 0; i < oprsz; ) {  \
 uint16_t pg = *(uint16_t *)(vg + H1_2(i >> 3)); \
 do {\
 TYPEM m = 0;\
-- 
2.17.1

Re: [Qemu-devel] [qemu-s390x] [RFC 03/15] s390-bios: decouple common boot logic from virtio

[Thomas Huth]

On 05.07.2018 19:25, Jason J. Herne wrote:
> From: "Jason J. Herne" 

git commit --amend --reset-author ?

> Create a boot_setup function to handle getting boot information from
> the machine/hypervisor. This decouples common boot logic from the
> virtio code path and allows us to make use of it for the real dasd boot
> scenario.
> 
> Signed-off-by: Jason J. Herne 
> Acked-by: Halil Pasic 
> Reviewed-by: Collin Walling  Signed-off-by: Jason J. Herne 
> ---
>  pc-bios/s390-ccw/main.c | 28 
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 63117d1..8ecab93 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -14,16 +14,17 @@
>  
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  static SubChannelId blk_schid = { .one = 1 };
> -IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>  static char loadparm_str[LOADPARM_LEN + 1] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>  QemuIplParameters qipl;
> +IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> +static bool have_iplb;
>  
>  #define LOADPARM_PROMPT "PROMPT  "
>  #define LOADPARM_EMPTY  ""
>  #define BOOT_MENU_FLAG_MASK (QIPL_FLAG_BM_OPTS_CMD | QIPL_FLAG_BM_OPTS_ZIPL)
>  
>  /*
> - * Priniciples of Operations (SA22-7832-09) chapter 17 requires that
> + * Principles of Operations (SA22-7832-09) chapter 17 requires that
>   * a subsystem-identification is at 184-187 and bytes 188-191 are zero
>   * after list-directed-IPL and ccw-IPL.
>   */
> @@ -111,23 +112,33 @@ static void cio_setup(void)
>  enable_mss_facility();
>  }
>  
> +/*
> + * Collect various pieces of information from the hypervisor/hardware that
> + * we'll use to determine exactly how we'll boot.
> + */
> +static void boot_setup(void)
> +{
> +char lpmsg[] = "LOADPARM=[]\n";
> +
> +sclp_get_loadparm_ascii(loadparm_str);
> +memcpy(lpmsg + 10, loadparm_str, 8);
> +sclp_print(lpmsg);
> +
> +have_iplb = store_iplb(&iplb);
> +}
> +
>  static void virtio_setup(void)
>  {
>  Schib schib;
>  int ssid;
>  bool found = false;
>  uint16_t dev_no;
> -char ldp[] = "LOADPARM=[]\n";
>  VDev *vdev = virtio_get_device();
>  QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
>  
> -sclp_get_loadparm_ascii(loadparm_str);
> -memcpy(ldp + 10, loadparm_str, LOADPARM_LEN);
> -sclp_print(ldp);
> -
>  memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
>  
> -if (store_iplb(&iplb)) {
> +if (have_iplb) {
>  switch (iplb.pbt) {
>  case S390_IPL_TYPE_CCW:
>  dev_no = iplb.ccw.devno;
> @@ -174,6 +185,7 @@ int main(void)
>  {
>  sclp_setup();
>  cio_setup();
> +boot_setup();

Maybe rather name it bootparm_setup? Just "boot" is IMHO a little bit
too generic... Anyway:

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [GIT PULL for qemu-pseries] pseries: Update SLOF firmware image

[Alexey Kardashevskiy]

On Wed, 11 Jul 2018 16:26:19 +1000
David Gibson  wrote:

> On Tue, Jul 10, 2018 at 05:14:53PM +1000, Alexey Kardashevskiy wrote:
> > On Tue, 10 Jul 2018 16:42:48 +1000
> > David Gibson  wrote:
> >   
> > > On Tue, Jul 10, 2018 at 12:22:44PM +1000, Alexey Kardashevskiy wrote:  
> > > > On Mon,  2 Jul 2018 16:26:44 +1000
> > > > Alexey Kardashevskiy  wrote:
> > > > 
> > > > > The following changes since commit 
> > > > > edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> > > > > 
> > > > >   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> > > > > 
> > > > > are available in the git repository at:
> > > > > 
> > > > >   g...@github.com:aik/qemu.git tags/qemu-slof-20180702
> > > > > 
> > > > > for you to fetch changes up to 
> > > > > edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> > > > > 
> > > > >   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> > > > > 
> > > > > 
> > > > > 
> > > > > I just missed a couple of gcc 8.1 fixes and thought I can still 
> > > > > squeeze
> > > > > them before the 3.0  soft freeze.
> > > > > 
> > > > > 
> > > > > *** Note: this is not for master, this is for pseries
> > > > 
> > > > 
> > > > Not good for 3.1 either?
> > > 
> > > Ugh.. somehow I missed this SLOF update request, and now we're in 3.0
> > > hard freeze.
> > > 
> > > How bad are the bugs this SLOF update fixes?  
> > 
> > These only fix gcc 8.1 warnings and runtime errors => not really
> > bugfixes so it is not a big deal if they miss 3.0.  
> 
> Ok, well, if you want to resend the update, I'll queue it for 3.1.


It is still on github, why resend?



--
Alexey


pgpGjsNZGblL0.pgp
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] linux-user: ppc64: use the correct values for F_*LK64s

[Shivaprasad G Bhat]

Qemu includes the glibc headers for the host defines and target headers are
part of the qemu source themselves. The glibc has the F_GETLK64, F_SETLK64
and F_SETLKW64 defined to 12, 13 and 14 for all archs(generic) in
sysdeps/unix/sysv/linux/bits/fcntl-linux.h. The linux kernel generic
definition for F_*LK is 5, 6 & 7 and F_*LK64* is 12,13, and 14 as seen in
include/uapi/asm-generic/fcntl.h. On 64bit machine, by default the kernel
assumes all F_*LK to 64bit calls and doesnt support use of F_*LK64* as
can be seen in include/linux/fcntl.h in linux source.

On x86_64 host, the values for F_*LK64* are set to 5, 6 and 7
explicitly in /usr/include/x86_64-linux-gnu/bits/fcntl.h by the glibc.
Whereas, a PPC64 host doesn't have such a definition in
/usr/include/powerpc64le-linux-gnu/bits/fcntl.h by the glibc. So,
the sources on PPC64 host sees the default value of F_*LK64*
as 12, 13 & 14(fcntl-linux.h).

Since the 64bit kernel doesnt support 12, 13 & 14; the glibc fcntl syscall
implementation(__libc_fcntl*(), __fcntl64_nocancel) does the F_*LK64* value
convertion back to F_*LK* values on PPC64 as seen in
sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h with FCNTL_ADJUST_CMD()
macro. Whereas on x86_64 host the values for F_*LK64* are set to 5, 6 and 7
and no adjustments are needed.

Since qemu doesnt use the glibc fcntl, but makes the safe_syscall* on its
own, the PPC64 qemu is calling the syscall with 12, 13, and 14(without
adjustment) and they all fail. The fcntl calls to F_GETLK/F_SETLK|W all
fail by all pplications run on PPC64 host user emulation.

The fix here could be to see why on PPC64 the glibc is still keeping
F_*LK64* different from F_*LK and why adjusting them to 5, 6 and 7 before
the syscall for PPC only. See if we can make the
/usr/include/powerpc64le-linux-gnu/bits/fcntl.h to have the values
5, 6 & 7 just like x86_64 and remove the adjustment code in glibc. That way,
qemu sources see the kernel supported values in glibc headers.

OR

On PPC64 host, qemu sources see both F_LK* & F_LK64* as same and set to
12, 13 and 14 because __USE_FILE_OFFSET64 is defined in qemu
sources(also refer sysdeps/unix/sysv/linux/bits/fcntl-linux.h).
Since F_*LK and F_*LK64 are same, the value adjument like done by glibc in
qemu sources is difficult. So, Overwrite the glibc defaults with the actual
supported values in Qemu. The current patch is doing this.

Signed-off-by: Shivaprasad G Bhat 
---
 linux-user/syscall.c |   14 ++
 1 file changed, 14 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 7b9ac3b408..1693e69ce0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -250,6 +250,20 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 
arg4,type5 arg5,  \
 #define TARGET_NR__llseek TARGET_NR_llseek
 #endif
 
+/* glibc headers has these defined to 12, 13 and 14 and is not supported
+ * by kernel. The glibc fcntl call actually adjusts them back to 5, 6 and 7
+ * before making the syscall(). Since we make the syscall directly,
+ * overwite/adjust to what is supported by the kernel.
+ */
+#if defined(__linux__) && defined(__powerpc64__)
+#undef F_GETLK64
+#define F_GETLK64  5   /* Get record locking info.  */
+#undef F_SETLK64
+#define F_SETLK64  6   /* Set record locking info (non-blocking).  */
+#undef F_SETLKW64
+#define F_SETLKW64 7   /* Set record locking info (blocking).  */
+#endif
+
 #ifdef __NR_gettid
 _syscall0(int, gettid)
 #else

Re: [Qemu-devel] [PATCH v6 0/2] file-posix: specify expected filetypes

[Kevin Wolf]

Am 10.07.2018 um 19:00 hat John Snow geschrieben:
> Tighten which types of files we'll allow you to specify for the various
> file drivers (file, host_device, host_cdrom).

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Paolo Bonzini]

On 10/07/2018 17:50, Stefan Hajnoczi wrote:
> The ->pre_plug() callback is invoked before the device is realized.  The
> ->plug() callback is invoked when the device is being realized but
> before it is reset.
> 
> This patch adds a ->post_plug() callback which is invoked after the
> device has been reset.  This callback is needed by HotplugHandlers that
> need to wait until after ->reset().
> 
> Signed-off-by: Stefan Hajnoczi 

Thanks Stefan.  I looked a bit at the code last week, and it seemed to
me that pretty much all "plug" callbacks should be split into a
"pre_plug" (check) and a "post_plug" (commit).  So the patch is okay,
but please remove the Error** argument of post_plug, because all the
checks should have happened already (there is none in the case of
virtio-scsi).

Thanks,

Paolo

> ---
>  include/hw/hotplug.h | 12 
>  hw/core/hotplug.c| 11 +++
>  hw/core/qdev.c   |  7 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 1a0516a479..1e8b1053b6 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @parent: Opaque parent interface.
>   * @pre_plug: pre plug callback called at start of device.realize(true)
>   * @plug: plug callback called at end of device.realize(true).
> + * @post_plug: post plug callback called after device.realize(true) and 
> device
> + * reset
>   * @unplug_request: unplug request callback.
>   *  Used as a means to initiate device unplug for devices 
> that
>   *  require asynchronous unplug handling.
> @@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass {
>  /*  */
>  hotplug_fn pre_plug;
>  hotplug_fn plug;
> +hotplug_fn post_plug;
>  hotplug_fn unplug_request;
>  hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -83,6 +86,15 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>DeviceState *plugged_dev,
>Error **errp);
>  
> +/**
> + * hotplug_handler_post_plug:
> + *
> + * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
> + */
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp);
> +
>  /**
>   * hotplug_handler_unplug_request:
>   *
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..ab34c19461 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>  }
>  }
>  
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp)
> +{
> +HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +if (hdc->post_plug) {
> +hdc->post_plug(plug_handler, plugged_dev, errp);
> +}
> +}
> +
>  void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>  DeviceState *plugged_dev,
>  Error **errp)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cf0db4b6da..78c0e031ff 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  }
>  if (dev->hotplugged) {
>  device_reset(dev);
> +
> +if (hotplug_ctrl) {
> +hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
> +if (local_err != NULL) {
> +goto child_realize_fail;
> +}
> +}
>  }
>  dev->pending_deleted_event = false;
>  } else if (!value && dev->realized) {
> 

Re: [Qemu-devel] [PATCH for-3.0] target/arm: Fix LD1W and LDFF1W (scalar plus vector)

[Laurent Desnogues]

Hello,

On Wed, Jul 11, 2018 at 12:39 PM, Richard Henderson
 wrote:
> 'I' was being double-incremented; correctly within the inner loop
> and incorrectly within the outer loop.
>
> Signed-off-by: Richard Henderson 

I didn't try to apply the patch but line numbers look wrong.

I guess this should apply to DO_LD1_ZPZ_S and DO_LDFF1_ZPZ macros, in
which case you get my review:

Reviewed-by: Laurent Desnogues 

Thanks,

Laurent

> ---
>
> Fixes a SIGSEGV within one of these generated helpers,
> exposed by an armclang vectorized code sample.
>
>
> r~
>
> ---
>  target/arm/sve_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index cec0d3ee54..ddc592ff79 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4855,7 +4855,7 @@ void HELPER(NAME)(CPUARMState *env, void *vd, void *vg, 
> void *vm,   \
>  intptr_t i, oprsz = simd_oprsz(desc);   \
>  unsigned scale = simd_data(desc);   \
>  uintptr_t ra = GETPC(); \
> -for (i = 0; i < oprsz; i++) {   \
> +for (i = 0; i < oprsz; ) {  \
>  uint16_t pg = *(uint16_t *)(vg + H1_2(i >> 3)); \
>  do {\
>  TYPEM m = 0;\
> @@ -4936,7 +4936,7 @@ void HELPER(NAME)(CPUARMState *env, void *vd, void *vg, 
> void *vm,   \
>  uintptr_t ra = GETPC(); \
>  bool first = true;  \
>  mmap_lock();\
> -for (i = 0; i < oprsz; i++) {   \
> +for (i = 0; i < oprsz; ) {  \
>  uint16_t pg = *(uint16_t *)(vg + H1_2(i >> 3)); \
>  do {\
>  TYPEM m = 0;\
> --
> 2.17.1
>
>

[Qemu-devel] [Bug 1474263] Re: "Image format was not specified" warning should be suppressed for the vvfat (and probably nbd) driver

[Max Reitz]

Yes, it does appear, you just need to make vvfat rw:

$ qemu-system-x86_64 -drive file.driver=vvfat,file.dir=foo,file.rw=on
vvfat foo chs 1024,16,63
WARNING: Image format was not specified for 'json:{"dir": "foo", "driver": 
"vvfat", "rw": "on"}' and probing guessed raw.
 Automatically detecting the format is dangerous for raw images, write 
operations on block 0 will be restricted.
 Specify the 'raw' format explicitly to remove the restrictions.

(The warning is not shown with R/O devices because they don’t have the
security issue.)

My point hasn’t changed, though,  I fundamentally agree with the points
in this report, but I don‘t think “fixing” this is worth it.

For NBD, “fixing” it would mean potentially breaking existing use cases
(which I still don’t see the point of, but there’s no point in breaking
them just to make a warning disappear).

For vvfat, there are three things: First of all, I don’t like it very much, so 
as I said, I’d rather deprecate it altogether (though this BZ shows we 
shouldn’t do that).
Secondly, in order for the warning to disappear, a protocol driver would need 
to enforce a certain format on top when probing.  That would require a bit of 
new infrastructure, although I have to admit it wouldn’t be impossible.
Thirdly, I suppose most people use vvfat with block device options like done in 
the bug report?  In that case, it is trivial to add a format=raw (or 
driver=raw), exactly like the warning suggests.

(Though you can use vvfat with a plain filename, too:

$  qemu-system-x86_64 fat:32:rw:foo
qemu-system-x86_64: warning: FAT32 has not been tested. You are welcome to do 
so!
vvfat foo chs 1024,16,63
WARNING: Image format was not specified for 'json:{"fat-type": 32, "dir": 
"foo", "driver": "vvfat", "floppy": false, "rw": true}' and probing guessed raw.
 Automatically detecting the format is dangerous for raw images, write 
operations on block 0 will be restricted.
 Specify the 'raw' format explicitly to remove the restrictions.

)

So all in all I think this is indeed kind of a bug (at least it’s a
nuisance that could be better), fixing it would not be impossible, but
it’s just very low on at least my priority list (probably somewhere
around “implement bdrv_refresh_filename() for vvfat so you don't get the
json:{} filenames you can see above”).

Max

** Changed in: qemu
   Status: Incomplete => Opinion

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

Title:
  "Image format was not specified" warning should be suppressed for the
  vvfat (and probably nbd) driver

Status in QEMU:
  Opinion

Bug description:
  Running

  qemu -drive file.driver=vvfat,file.dir=.

  displays

  WARNING: Image format was not specified for 'json:{"dir": ".", "driver": 
"vvfat"}' and probing guessed raw.
   Automatically detecting the format is dangerous for raw images, 
write operations on block 0 will be restricted.
   Specify the 'raw' format explicitly to remove the restrictions.

  However, since "images" provided by the vvfat driver are always raw
  (and the first sector isn't writeable in any case), this warning is
  superfluous and should not be displayed.

  A similar warning is displayed for NBD devices; I suspect it should be
  also disabled for similar reasons, but I'm not sure if serving non-raw
  images is actually a violation of the protocol. qemu-nbd translates
  them to raw images, for what it's worth, even if it may be suppressed
  with -f raw.

  Noticed on 2.3.0; the code that causes this behaviour is still
  apparently present in today's git master
  (f3a1b5068cea303a55e2a21a97e66d057eaae638). Speaking of versions: you
  may want to update the copyright notice that qemu -version displays.

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

Re: [Qemu-devel] [Qemu-block] [PATCH] throttle-groups: fix hang when group member leaves

[Stefan Hajnoczi]

On Wed, Jul 04, 2018 at 03:54:10PM +0100, Stefan Hajnoczi wrote:

Sorry you weren't CCed originally, Berto.  This one is for you! :)

> Throttle groups consist of members sharing one throttling state
> (including bps/iops limits).  Round-robin scheduling is used to ensure
> fairness.  If a group member already has a timer pending then other
> groups members do not schedule their own timers.  The next group member
> will have its turn when the existing timer expires.
> 
> A hang may occur when a group member leaves while it had a timer
> scheduled.  Although the code carefully removes the group member from
> the round-robin list, it does not schedule the next member.  Therefore
> remaining members continue to wait for the removed member's timer to
> expire.
> 
> This patch schedules the next request if a timer is pending.
> Unfortunately the actual bug is a race condition that I've been unable
> to capture in a test case.
> 
> Sometimes drive2 hangs when drive1 is removed from the throttling group:
> 
>   $ qemu ... -drive 
> if=none,id=drive1,cache=none,format=qcow2,file=data1.qcow2,iops=100,group=foo 
> \
>  -device virtio-blk-pci,id=virtio-blk-pci0,drive=drive1 \
>  -drive 
> if=none,id=drive2,cache=none,format=qcow2,file=data2.qcow2,iops=10,group=foo \
>  -device virtio-blk-pci,id=virtio-blk-pci1,drive=drive2
>   (guest-console1)# fio -filename /dev/vda 4k-seq-read.job
>   (guest-console2)# fio -filename /dev/vdb 4k-seq-read.job
>   (qmp) {"execute": "block_set_io_throttle", "arguments": {"device": 
> "drive1","bps": 0,"bps_rd": 0,"bps_wr": 0,"iops": 0,"iops_rd": 0,"iops_wr": 
> 0}}
> 
> Reported-by: Nini Gu 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1535914
> Cc: Alberto Garcia 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/throttle-groups.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 36cc0430c3..e297b04e17 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -564,6 +564,10 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
> *tgm)
>  
>  qemu_mutex_lock(&tg->lock);
>  for (i = 0; i < 2; i++) {
> +if (timer_pending(tgm->throttle_timers.timers[i])) {
> +tg->any_timer_armed[i] = false;
> +schedule_next_request(tgm, i);
> +}
>  if (tg->tokens[i] == tgm) {
>  token = throttle_group_next_tgm(tgm);
>  /* Take care of the case where this is the last tgm in the group 
> */
> -- 
> 2.17.1
> 
> 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [BUG] checkpatch.pl hangs on target/mips/msa_helper.c

[Stefan Hajnoczi]

On Wed, Jul 04, 2018 at 03:35:18PM +, Aleksandar Markovic wrote:
> If  checkpatch.pl is applied (using switch "-f") on file 
> target/mips/msa_helper.c, it will hang.
> 
> There is a workaround for this particular file:
> 
> These lines in msa_helper.c:
> 
> uint## BITS ##_t S = _S, T = _T;\
> uint## BITS ##_t as, at, xs, xt, xd;\
> 
> should be replaced with:
> 
> uint## BITS ## _t S = _S, T = _T;   \
> uint## BITS ## _t as, at, xs, xt, xd;   \
> 
> (a space is added after the second "##" in each line)
> 
> The workaround is found by partial deleting and undeleting of the code in 
> msa_helper.c in binary search fashion.
> 
> This workaround will soon be submitted by me as a patch within a series on 
> misc MIPS issues.
> 
> I took a look at checkpatch.pl code, and it looks it is fairly complicated to 
> fix the issue, since it happens in the code segment involving intricate logic 
> conditions.

Thanks for figuring this out, Aleksandar.  Not sure if anyone else has
the apetite to fix checkpatch.pl.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3] accel: forbid early use of kvm_enabled() and friends

[Stefan Hajnoczi]

On Thu, Jul 05, 2018 at 09:54:30AM +0200, Greg Kurz wrote:
> It is unsafe to rely on *_enabled() helpers before the accelerator has
> been initialized, ie, accel_init_machine() has succeeded, because they
> always return false. But it is still possible to end up calling them
> indirectly by inadvertance, and cause QEMU to misbehave.
> 
> This patch causes QEMU to abort if we try to check for any accelerator
> before accel_init_machine() was called. This will help to catch bugs
> earlier.
> 
> Signed-off-by: Greg Kurz 
> ---
> 
> Current master (SHA1 2a018f6e9878) has such a kvm_enabled() misuse that
> triggers the assert:
> 
> https://travis-ci.org/gkurz/qemu/jobs/400059821#L8709
> 
> The fix for the misuse is available, waiting to be merged:
> 
> https://patchwork.ozlabs.org/patch/938077/
> 
> v3: - use an inline function
> - convert all accelerators
> - dropped David's R-b
> 
> v2: - dropped change in qom/cpu.c (useless header inclusion)
> - only #include "sysemu/kvm.h" if we actually need it
> - added David's R-b from v1 because changes in v2 are minor
> ---
>  accel/accel.c |4 
>  include/hw/xen/xen.h  |3 ++-
>  include/qemu-common.h |3 ++-
>  include/sysemu/accel.h|8 
>  include/sysemu/hvf.h  |3 ++-
>  include/sysemu/kvm.h  |3 ++-
>  include/sysemu/qtest.h|3 ++-
>  stubs/Makefile.objs   |1 +
>  stubs/accel.c |   12 
>  target/i386/hax-all.c |2 +-
>  target/i386/whpx-all.c|2 +-
>  tests/ptimer-test-stubs.c |2 ++
>  12 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 stubs/accel.c

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-discuss] IRQ per CPU

[Stefan Hajnoczi]

On Thu, Jul 05, 2018 at 03:16:44PM +0800, Dongli Zhang wrote:
> 
> 
> On 07/05/2018 12:12 PM, Probir Roy wrote:
> >> Does 'per CPU basis' indicates irq per cpu, or irq per device queue?
> > 
> > IRQ per CPU core, meaning that IRQ will be raised at and served by
> > that CPU. Does IRQ per queue mean the same thing?
> > 
> 
> About 'IRQ per queue', the device may create multiple queue in the OS driver.
> The number of queues are always proportional to the number of CPU core/thread
> (although this is also always configurable by OS driver).
> 
> Usually the per-queue irq/vector is bound to each CPU. As the number of
> queue/irq is always the same as the number of CPU, it is sort of 'per CPU
> basis', as each CPU will be serving irq for its own queue.

Some more background on devices with multiple irqs:

It's the guest kernel that configures interrupt routing on modern
systems where the interrupt controller hardware supports that.

With a PCI device you would need multiple Message-Signalled Interrupt
vectors.  Then the guest driver can set the irq affinity so that the
vcpu of your choice receives each of these interrupts.

Interrupts are a limited resource, even when using Message-Signalled
Interrupts, so you'll have to choose a maximum number of interrupts that
the device supports.

https://en.wikipedia.org/wiki/Message_Signaled_Interrupts#MSI-X

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 1/3] hw/arm: extract ARM M Profile base class from ARMv7-M

[Stefan Hajnoczi]

On Thu, Jul 05, 2018 at 04:49:22PM +0100, Peter Maydell wrote:
> On 5 July 2018 at 16:45, Peter Maydell  wrote:
> > On 30 June 2018 at 10:13, Stefan Hajnoczi  wrote:
> >> The ARMv7-M code is largely similar to what other M Profile CPUs need.
> >> Extract the common M Profile aspects into the ARMMProfileState base
> >> class.  ARMv6-M will inherit from this class in the following patch.
> >>
> >> It might be possible to make ARMv6-M the base class of ARMv7-M, but it
> >> seems cleaner to have an M Profile base class instead of saying an
> >> "ARMv7-M is an ARMv6-M".
> >>
> >> Signed-off-by: Stefan Hajnoczi 
> >
> > This makes sense, I guess (though it currently leaves us in the
> > odd position that we have separate a object for v6m, but the v7m
> > object handles both v7m and v8m...)
> 
> ...though I guess the counter-argument is that the only thing that
> the v7m object is doing that v6m doesn't want is creating
> the bitbanding device, and in fact bitbanding is optional in v7m
> (you can configure a Cortex-M3 without it). So maybe we should
> instead just have a QOM property to let you turn off the
> bitbanding ?

Okay, we can do that.  So how about a single ARMMProfileState class for
v6, v7, and v8?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC PATCH 5/6] virt-acpi-build: add PPTT table

[Andrew Jones]

On Wed, Jul 04, 2018 at 02:49:22PM +0200, Andrew Jones wrote:
> The ACPI PPTT table supports topology descriptions for ACPI
> guests. Note, while a DT boot Linux guest with a non-flat CPU
> topology will see socket and core IDs being sequential integers
> starting from zero, e.g. with -smp 4,sockets=2,cores=2,threads=1
> 
> a DT boot produces
> 
>  cpu:  0 package_id:  0 core_id:  0
>  cpu:  1 package_id:  0 core_id:  1
>  cpu:  2 package_id:  1 core_id:  0
>  cpu:  3 package_id:  1 core_id:  1
> 
> an ACPI boot produces
> 
>  cpu:  0 package_id: 36 core_id:  0
>  cpu:  1 package_id: 36 core_id:  1
>  cpu:  2 package_id: 96 core_id:  2
>  cpu:  3 package_id: 96 core_id:  3
> 
> This is due to several reasons:
> 
>  1) DT cpu nodes do not have an equivalent field to what the PPTT
> ACPI Processor ID must be, i.e. something equal to the MADT CPU
> UID or equal to the UID of an ACPI processor container. In both
> ACPI cases those are platform dependant IDs assigned by the
> vendor.
> 
>  2) While QEMU is the vendor for a guest, if the topology specifies
> SMT (> 1 thread), then, with ACPI, it is impossible to assign a
> core-id the same value as a package-id, thus it is not possible
> to have package-id=0 and core-id=0. This is because package and
> core containers must be in the same ACPI namespace and therefore
> must have unique UIDs.
> 
>  3) ACPI processor containers are not required for PPTT tables to
> be used and, due to the limitations of which IDs are selected
> described above in (2), they are not helpful for QEMU, so we
> don't build them with this patch. In the absence of them, Linux
> assigns its own unique IDs. The maintainers have chosen not to use
> counters from zero, but rather ACPI table offsets, which explains
> why the numbers are so much larger than with DT.
> 
>  4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
> match the logical CPU IDs, because these IDs must be equal to the
> MADT CPU UID (as no processor containers are present), and QEMU
> uses the logical CPU ID for these MADT IDs.
> 
> Cc: Igor Mammedov 
> Cc: Shannon Zhao 
> Cc: "Michael S. Tsirkin" 
> Signed-off-by: Andrew Jones 
> ---
>  hw/acpi/aml-build.c | 50 +
>  hw/arm/virt-acpi-build.c|  5 
>  include/hw/acpi/aml-build.h |  2 ++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736de9..37e8f5182ae9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1679,6 +1680,55 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
>   table_data->len - slit_start, 1, NULL, NULL);
>  }
>  
> +/*
> + * ACPI 6.2 Processor Properties Topology Table (PPTT)
> + */
> +static void build_cpu_hierarchy(GArray *tbl, uint32_t flags,
> +uint32_t parent, uint32_t id)
> +{
> +build_append_byte(tbl, 0);  /* Type 0 - processor */
> +build_append_byte(tbl, 20); /* Length, no private resources */
> +build_append_int_noprefix(tbl, 0, 2);   /* Reserved */
> +build_append_int_noprefix(tbl, flags, 4);
> +build_append_int_noprefix(tbl, parent, 4);
> +build_append_int_noprefix(tbl, id, 4);
> +build_append_int_noprefix(tbl, 0, 4); /* Num private resources */
> +}
> +
> +void build_pptt(GArray *table_data, BIOSLinker *linker, int possible_cpus)
> +{
> +int pptt_start = table_data->len;
> +int uid = 0, cpus = 0, socket;
> +
> +acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +for (socket = 0; cpus < possible_cpus; socket++) {
> +uint32_t socket_offset = table_data->len - pptt_start;
> +int core;
> +
> +build_cpu_hierarchy(table_data, 1, 0, socket);
> +
> +for (core = 0; core < smp_cores; core++) {
> +uint32_t core_offset = table_data->len - pptt_start;
> +int thread;
> +
> +if (smp_threads > 1) {
> +build_cpu_hierarchy(table_data, 0, socket_offset, core);
> +for (thread = 0; thread < smp_threads; thread++) {
> +build_cpu_hierarchy(table_data, 2, core_offset, uid++);
> +}
> + } else {
> +build_cpu_hierarchy(table_data, 2, socket_offset, uid++);

It just occurred to me that the uid++'s above should be something like
uid[i++]'s, where uid[] is an argument to the function instead. Otherwise
this general aml builder function isn't written generally enough.

> + }
> +}
> +cpus += smp_cores * smp_threads;
> +}
> +
> +build_header(linker, table_data,
> + (void *)(table_data->data + pptt_start), "PPTT",
> +

Re: [Qemu-devel] [RFC v3 2/2] tests: Add ARMv6-M reserved register test

[Stefan Hajnoczi]

On Fri, Jul 06, 2018 at 03:55:14PM +0100, Peter Maydell wrote:
> On 5 July 2018 at 23:21, Julia Suvorova  wrote:
> > Check that reserved SCS registers return 0 at read,
> > and writes are ignored.
> >
> > Based-on: <20180627143815.1829-1-j...@jms.id.au>
> > Based-on: <20180630091343.14391-1-stefa...@redhat.com>
> >
> > Signed-off-by: Julia Suvorova 
> > ---
> > Test will work if Joel's patches will use ARMv6-M.
> >
> >  tests/Makefile.include|  2 ++
> >  tests/tcg/arm/test-reserved-reg.c | 60 +++
> >  2 files changed, 62 insertions(+)
> >  create mode 100644 tests/tcg/arm/test-reserved-reg.c
> 
> Is this in the wrong place? It doesn't look like a
> tests/tcg test -- those directories are for test
> programs which need to be compiled with a cross compiler
> for the guest CPU and then run on it.

Yes, this test should go in tests/ along with the other qtests.

> 
> I tried running 'make check' and I got this error:
> 
>   CC  tests/tcg/arm/test-reserved-reg.o
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/tcg/arm/test-reserved-reg.c:60:1:
> fatal error: opening dependency file
> tests/tcg/arm/test-reserved-reg.d: No such file or directory
>  }
>  ^
> 
> which is a bit weird.
> 
> thanks
> -- PMM
> 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] linux-user: ppc64: use the correct values for F_*LK64s

[Laurent Vivier]

Le 11/07/2018 à 12:55, Shivaprasad G Bhat a écrit :
> Qemu includes the glibc headers for the host defines and target headers are
> part of the qemu source themselves. The glibc has the F_GETLK64, F_SETLK64
> and F_SETLKW64 defined to 12, 13 and 14 for all archs(generic) in
> sysdeps/unix/sysv/linux/bits/fcntl-linux.h. The linux kernel generic
> definition for F_*LK is 5, 6 & 7 and F_*LK64* is 12,13, and 14 as seen in
> include/uapi/asm-generic/fcntl.h. On 64bit machine, by default the kernel
> assumes all F_*LK to 64bit calls and doesnt support use of F_*LK64* as
> can be seen in include/linux/fcntl.h in linux source.
> 
> On x86_64 host, the values for F_*LK64* are set to 5, 6 and 7
> explicitly in /usr/include/x86_64-linux-gnu/bits/fcntl.h by the glibc.
> Whereas, a PPC64 host doesn't have such a definition in
> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h by the glibc. So,
> the sources on PPC64 host sees the default value of F_*LK64*
> as 12, 13 & 14(fcntl-linux.h).
> 
> Since the 64bit kernel doesnt support 12, 13 & 14; the glibc fcntl syscall
> implementation(__libc_fcntl*(), __fcntl64_nocancel) does the F_*LK64* value
> convertion back to F_*LK* values on PPC64 as seen in
> sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h with FCNTL_ADJUST_CMD()
> macro. Whereas on x86_64 host the values for F_*LK64* are set to 5, 6 and 7
> and no adjustments are needed.
> 
> Since qemu doesnt use the glibc fcntl, but makes the safe_syscall* on its
> own, the PPC64 qemu is calling the syscall with 12, 13, and 14(without
> adjustment) and they all fail. The fcntl calls to F_GETLK/F_SETLK|W all
> fail by all pplications run on PPC64 host user emulation.
> 
> The fix here could be to see why on PPC64 the glibc is still keeping
> F_*LK64* different from F_*LK and why adjusting them to 5, 6 and 7 before
> the syscall for PPC only. See if we can make the
> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h to have the values
> 5, 6 & 7 just like x86_64 and remove the adjustment code in glibc. That way,
> qemu sources see the kernel supported values in glibc headers.
> 
> OR
> 
> On PPC64 host, qemu sources see both F_LK* & F_LK64* as same and set to
> 12, 13 and 14 because __USE_FILE_OFFSET64 is defined in qemu
> sources(also refer sysdeps/unix/sysv/linux/bits/fcntl-linux.h).
> Since F_*LK and F_*LK64 are same, the value adjument like done by glibc in
> qemu sources is difficult. So, Overwrite the glibc defaults with the actual
> supported values in Qemu. The current patch is doing this.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  linux-user/syscall.c |   14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 7b9ac3b408..1693e69ce0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -250,6 +250,20 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 
> arg4,type5 arg5,\
>  #define TARGET_NR__llseek TARGET_NR_llseek
>  #endif
>  
> +/* glibc headers has these defined to 12, 13 and 14 and is not supported
> + * by kernel. The glibc fcntl call actually adjusts them back to 5, 6 and 7
> + * before making the syscall(). Since we make the syscall directly,
> + * overwite/adjust to what is supported by the kernel.
> + */
> +#if defined(__linux__) && defined(__powerpc64__)
> +#undef F_GETLK64
> +#define F_GETLK64  5   /* Get record locking info.  */
> +#undef F_SETLK64
> +#define F_SETLK64  6   /* Set record locking info (non-blocking).  */
> +#undef F_SETLKW64
> +#define F_SETLKW64 7   /* Set record locking info (blocking).  */
> +#endif
> +
>  #ifdef __NR_gettid
>  _syscall0(int, gettid)
>  #else
> 

These macros are used in target_to_host_fcntl_cmd(), and this function
is used with safe_fcntl() and fcntl().

So I think it would be cleaner to do the change after
target_to_host_fcntl_cmd() in do_fcntl() as it is done in glibc instead
of redefining system values. Something like:

--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6782,6 +6782,12 @@ static abi_long do_fcntl(int fd, int cmd,
abi_ulong arg)
 if (host_cmd == -TARGET_EINVAL)
return host_cmd;

+#if defined(__linux__) && defined(__powerpc64__)
+if (host_cmd >= F_GETLK64 && host_cmd <= F_SETLKW64) {
+host_cmd -= F_GETLK64 - F_GETLK;
+}
+#endif
+
 switch(cmd) {
 case TARGET_F_GETLK:
 ret = copy_from_user_flock(&fl64, arg);

Thanks,
Laurent

Re: [Qemu-devel] qemu-nbd vs 'simple' trace backend vs iotest 147

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 03:45:49PM +0200, Cornelia Huck wrote:
> Hi,
> 
> I recently noticed that iotest 147 was hanging on my laptop, but worked
> fine on my s390x LPAR. Turned out that the architecture was a red
> herring; on both platforms, things fail with the 'simple' trace backend
> and work with e.g. the 'log' trace backend. Some details on the
> failures with the 'simple' backend:
> 
> - The first run of 147 passes. However, there are two processes hanging
>   around, one using a unix socket and one using an inet socket:
> 
> cohuck   22912  0.0  0.0 156580  3836 ?Ss   14:32   0:00 
> /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f qcow2 
> /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -p 10811
> cohuck   22925  0.0  0.0 156580  3840 ?Ss   14:32   0:00 
> /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f qcow2 
> /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -k 
> /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/nbd.socket
> 
> Attaching a gdb shows that we seem to be waiting on flushing:
> 
> (gdb) bt
> #0  0x7f461c078b99 in syscall () from /lib64/libc.so.6
> #1  0x7f461d13650f in g_cond_wait () from /lib64/libglib-2.0.so.0
> #2  0x560cf3a1caf2 in flush_trace_file (wait=255)
> at /home/cohuck/git/qemu/trace/simple.c:139
> #3  st_flush_trace_buffer () at /home/cohuck/git/qemu/trace/simple.c:374
> #4  0x7f461bfc01d8 in __run_exit_handlers () from /lib64/libc.so.6
> #5  0x7f461bfc022a in exit () from /lib64/libc.so.6
> #6  0x560cf392eb7e in main (argc=, argv=)
> at /home/cohuck/git/qemu/qemu-nbd.c:1076
> 
> (for both processes)

Please also print backtraces for the other threads:

  (gdb) thread apply all bt

There should be another thread in writeout_thread() so I'm surprised
that flush_trace_file() is getting stuck in g_cond_wait().

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] linux-user: ppc64: use the correct values for F_*LK64s

[Laurent Vivier]

Le 11/07/2018 à 15:04, Laurent Vivier a écrit :
> Le 11/07/2018 à 12:55, Shivaprasad G Bhat a écrit :
>> Qemu includes the glibc headers for the host defines and target headers are
>> part of the qemu source themselves. The glibc has the F_GETLK64, F_SETLK64
>> and F_SETLKW64 defined to 12, 13 and 14 for all archs(generic) in
>> sysdeps/unix/sysv/linux/bits/fcntl-linux.h. The linux kernel generic
>> definition for F_*LK is 5, 6 & 7 and F_*LK64* is 12,13, and 14 as seen in
>> include/uapi/asm-generic/fcntl.h. On 64bit machine, by default the kernel
>> assumes all F_*LK to 64bit calls and doesnt support use of F_*LK64* as
>> can be seen in include/linux/fcntl.h in linux source.
>>
>> On x86_64 host, the values for F_*LK64* are set to 5, 6 and 7
>> explicitly in /usr/include/x86_64-linux-gnu/bits/fcntl.h by the glibc.
>> Whereas, a PPC64 host doesn't have such a definition in
>> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h by the glibc. So,
>> the sources on PPC64 host sees the default value of F_*LK64*
>> as 12, 13 & 14(fcntl-linux.h).
>>
>> Since the 64bit kernel doesnt support 12, 13 & 14; the glibc fcntl syscall
>> implementation(__libc_fcntl*(), __fcntl64_nocancel) does the F_*LK64* value
>> convertion back to F_*LK* values on PPC64 as seen in
>> sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h with FCNTL_ADJUST_CMD()
>> macro. Whereas on x86_64 host the values for F_*LK64* are set to 5, 6 and 7
>> and no adjustments are needed.
>>
>> Since qemu doesnt use the glibc fcntl, but makes the safe_syscall* on its
>> own, the PPC64 qemu is calling the syscall with 12, 13, and 14(without
>> adjustment) and they all fail. The fcntl calls to F_GETLK/F_SETLK|W all
>> fail by all pplications run on PPC64 host user emulation.
>>
>> The fix here could be to see why on PPC64 the glibc is still keeping
>> F_*LK64* different from F_*LK and why adjusting them to 5, 6 and 7 before
>> the syscall for PPC only. See if we can make the
>> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h to have the values
>> 5, 6 & 7 just like x86_64 and remove the adjustment code in glibc. That way,
>> qemu sources see the kernel supported values in glibc headers.
>>
>> OR
>>
>> On PPC64 host, qemu sources see both F_LK* & F_LK64* as same and set to
>> 12, 13 and 14 because __USE_FILE_OFFSET64 is defined in qemu
>> sources(also refer sysdeps/unix/sysv/linux/bits/fcntl-linux.h).
>> Since F_*LK and F_*LK64 are same, the value adjument like done by glibc in
>> qemu sources is difficult. So, Overwrite the glibc defaults with the actual
>> supported values in Qemu. The current patch is doing this.
>>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>  linux-user/syscall.c |   14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 7b9ac3b408..1693e69ce0 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -250,6 +250,20 @@ static type name (type1 arg1,type2 arg2,type3 
>> arg3,type4 arg4,type5 arg5,   \
>>  #define TARGET_NR__llseek TARGET_NR_llseek
>>  #endif
>>  
>> +/* glibc headers has these defined to 12, 13 and 14 and is not supported
>> + * by kernel. The glibc fcntl call actually adjusts them back to 5, 6 and 7
>> + * before making the syscall(). Since we make the syscall directly,
>> + * overwite/adjust to what is supported by the kernel.
>> + */
>> +#if defined(__linux__) && defined(__powerpc64__)
>> +#undef F_GETLK64
>> +#define F_GETLK64  5   /* Get record locking info.  */
>> +#undef F_SETLK64
>> +#define F_SETLK64  6   /* Set record locking info (non-blocking).  
>> */
>> +#undef F_SETLKW64
>> +#define F_SETLKW64 7   /* Set record locking info (blocking).  */
>> +#endif
>> +
>>  #ifdef __NR_gettid
>>  _syscall0(int, gettid)
>>  #else
>>
> 
> These macros are used in target_to_host_fcntl_cmd(), and this function
> is used with safe_fcntl() and fcntl().
> 
> So I think it would be cleaner to do the change after
> target_to_host_fcntl_cmd() in do_fcntl() as it is done in glibc instead
> of redefining system values. Something like:
> 
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6782,6 +6782,12 @@ static abi_long do_fcntl(int fd, int cmd,
> abi_ulong arg)
>  if (host_cmd == -TARGET_EINVAL)
> return host_cmd;
> 
> +#if defined(__linux__) && defined(__powerpc64__)

Moreover, I think the "defined(__linux__)" is not needed as
linux-user/syscall.c is compiled only on linux.

Thanks,
Laurent

Re: [Qemu-devel] qemu-nbd vs 'simple' trace backend vs iotest 147

[Cornelia Huck]

On Wed, 11 Jul 2018 14:06:17 +0100
Stefan Hajnoczi  wrote:

> On Mon, Jul 09, 2018 at 03:45:49PM +0200, Cornelia Huck wrote:
> > Hi,
> > 
> > I recently noticed that iotest 147 was hanging on my laptop, but worked
> > fine on my s390x LPAR. Turned out that the architecture was a red
> > herring; on both platforms, things fail with the 'simple' trace backend
> > and work with e.g. the 'log' trace backend. Some details on the
> > failures with the 'simple' backend:
> > 
> > - The first run of 147 passes. However, there are two processes hanging
> >   around, one using a unix socket and one using an inet socket:
> > 
> > cohuck   22912  0.0  0.0 156580  3836 ?Ss   14:32   0:00 
> > /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f 
> > qcow2 /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -p 
> > 10811
> > cohuck   22925  0.0  0.0 156580  3840 ?Ss   14:32   0:00 
> > /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f 
> > qcow2 /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -k 
> > /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/nbd.socket
> > 
> > Attaching a gdb shows that we seem to be waiting on flushing:
> > 
> > (gdb) bt
> > #0  0x7f461c078b99 in syscall () from /lib64/libc.so.6
> > #1  0x7f461d13650f in g_cond_wait () from /lib64/libglib-2.0.so.0
> > #2  0x560cf3a1caf2 in flush_trace_file (wait=255)
> > at /home/cohuck/git/qemu/trace/simple.c:139
> > #3  st_flush_trace_buffer () at /home/cohuck/git/qemu/trace/simple.c:374
> > #4  0x7f461bfc01d8 in __run_exit_handlers () from /lib64/libc.so.6
> > #5  0x7f461bfc022a in exit () from /lib64/libc.so.6
> > #6  0x560cf392eb7e in main (argc=, argv=)
> > at /home/cohuck/git/qemu/qemu-nbd.c:1076
> > 
> > (for both processes)  
> 
> Please also print backtraces for the other threads:
> 
>   (gdb) thread apply all bt
> 
> There should be another thread in writeout_thread() so I'm surprised
> that flush_trace_file() is getting stuck in g_cond_wait().

I'll re-run to check, but there was only one thread in the process in
question.

Re: [Qemu-devel] [RFC v3 06/15] hw/arm/virt: Allocate device_memory

[Igor Mammedov]

On Thu, 5 Jul 2018 16:27:05 +0200
Auger Eric  wrote:

> Hi Shameer,
> 
> On 07/05/2018 03:19 PM, Shameerali Kolothum Thodi wrote:
> >   
> >> -Original Message-
> >> From: Auger Eric [mailto:eric.au...@redhat.com]
> >> Sent: 05 July 2018 13:18
> >> To: David Hildenbrand ; eric.auger@gmail.com;
> >> qemu-devel@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
> >> Shameerali Kolothum Thodi ;
> >> imamm...@redhat.com
> >> Cc: w...@redhat.com; drjo...@redhat.com; da...@gibson.dropbear.id.au;
> >> dgilb...@redhat.com; ag...@suse.de
> >> Subject: Re: [Qemu-devel] [RFC v3 06/15] hw/arm/virt: Allocate
> >> device_memory
> >>
> >> Hi David,
> >>
> >> On 07/05/2018 02:09 PM, David Hildenbrand wrote:  
> >>> On 05.07.2018 14:00, Auger Eric wrote:  
>  Hi David,
> 
>  On 07/05/2018 01:54 PM, David Hildenbrand wrote:  
> > On 05.07.2018 13:42, Auger Eric wrote:  
> >> Hi David,
> >>
> >> On 07/04/2018 02:05 PM, David Hildenbrand wrote:  
> >>> On 03.07.2018 21:27, Auger Eric wrote:  
>  Hi David,
>  On 07/03/2018 08:25 PM, David Hildenbrand wrote:  
> > On 03.07.2018 09:19, Eric Auger wrote:  
> >> We define a new hotpluggable RAM region (aka. device memory).
> >> Its base is 2TB GPA. This obviously requires 42b IPA support
> >> in KVM/ARM, FW and guest kernel. At the moment the device
> >> memory region is max 2TB.  
> >
> > Maybe a stupid question, but why exactly does it have to start at 
> > 2TB
> > (and not e.g. at 1TB)?  
>  not a stupid question. See tentative answer below.  
> >  
> >>
> >> This is largely inspired of device memory initialization in
> >> pc machine code.
> >>
> >> Signed-off-by: Eric Auger 
> >> Signed-off-by: Kwangwoo Lee 
> >> ---
> >>  hw/arm/virt.c | 104  
> >> --  
> >>  include/hw/arm/arm.h  |   2 +
> >>  include/hw/arm/virt.h |   1 +
> >>  3 files changed, 79 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 5a4d0bf..6fefb78 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -59,6 +59,7 @@
> >>  #include "qapi/visitor.h"
> >>  #include "standard-headers/linux/input.h"
> >>  #include "hw/arm/smmuv3.h"
> >> +#include "hw/acpi/acpi.h"
> >>
> >>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >>  static void virt_##major##_##minor##_class_init(ObjectClass 
> >> *oc,  
> >> \  
> >> @@ -94,34 +95,25 @@
> >>
> >>  #define PLATFORM_BUS_NUM_IRQS 64
> >>
> >> -/* RAM limit in GB. Since VIRT_MEM starts at the 1GB mark, this  
> >> means  
> >> - * RAM can go up to the 256GB mark, leaving 256GB of the physical
> >> - * address space unallocated and free for future use between 256G 
> >>  
> >> and 512G.  
> >> - * If we need to provide more RAM to VMs in the future then we  
> >> need to:  
> >> - *  * allocate a second bank of RAM starting at 2TB and working 
> >> up  
>  I acknowledge this comment was the main justification. Now if you 
>  look  
> >> at  
> 
>  Principles of ARM Memory Maps
>   
> >> http://infocenter.arm.com/help/topic/com.arm.doc.den0001c/DEN0001C_princ
> >> iples_of_arm_memory_maps.pdf  
>  chapter 2.3 you will find that when adding PA bits, you always leave
>  space for reserved space and mapped IO.  
> >>>
> >>> Thanks for the pointer!
> >>>
> >>> So ... we can fit
> >>>
> >>> a) 2GB at 2GB
> >>> b) 32GB at 32GB
> >>> c) 512GB at 512GB
> >>> d) 8TB at 8TB
> >>> e) 128TB at 128TB
> >>>
> >>> (this is a nice rule of thumb if I understand it correctly :) )
> >>>
> >>> We should strive for device memory (maxram_size - ram_size) to fit
> >>> exactly into one of these slots (otherwise things get nasty).
> >>>
> >>> Depending on the ram_size, we might have simpler setups and can  
> >> support  
> >>> more configurations, no?
> >>>
> >>> E.g. ram_size <= 34GB, device_memory <= 512GB  
> >>> -> move ram into a) and b)
> >>> -> move device memory into c)  
> >>
> >> The issue is machvirt doesn't comply with that document.
> >> At the moment we have
> >> 0 -> 1GB MMIO
> >> 1GB -> 256GB RAM
> >> 256GB -> 512GB is theoretically reserved for IO but most is free.
> >> 512GB -> 1T is reserved for ECAM MMIO range. This is the top of our
> >> existing 40b GPA space.
> >>
> >> We don't want to change this address map due to legacy reasons.
> >>  
> >
> > Thanks, good to know!
> >  
> >> Another question! do you know if it w

[Qemu-devel] [PATCH] kvmclock: run KVM_KVMCLOCK_CTRL ioctl in vcpu thread

[Yongji Xie]

According to KVM API Documentation, we should only
run vcpu ioctls from the same thread that was used
to create the vcpu. This patch makes KVM_KVMCLOCK_CTRL
ioctl consistent with the Documentation.

No functional change.

Signed-off-by: Yongji Xie 
Signed-off-by: Chai Wen 
---
 hw/i386/kvm/clock.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 0bf1c60..25ea783 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -147,6 +147,15 @@ static void kvm_update_clock(KVMClockState *s)
 s->clock_is_reliable = kvm_has_adjust_clock_stable();
 }
 
+static void do_kvmclock_ctrl(CPUState *cpu, run_on_cpu_data data)
+{
+int ret = kvm_vcpu_ioctl(cpu, KVM_KVMCLOCK_CTRL, 0);
+
+if (ret && ret != -EINVAL) {
+fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
+}
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
  RunState state)
 {
@@ -183,13 +192,7 @@ static void kvmclock_vm_state_change(void *opaque, int 
running,
 return;
 }
 CPU_FOREACH(cpu) {
-ret = kvm_vcpu_ioctl(cpu, KVM_KVMCLOCK_CTRL, 0);
-if (ret) {
-if (ret != -EINVAL) {
-fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
-}
-return;
-}
+run_on_cpu(cpu, do_kvmclock_ctrl, RUN_ON_CPU_NULL);
 }
 } else {
 
-- 
1.7.9.5

[Qemu-devel] [PATCH] kvmclock: run KVM_KVMCLOCK_CTRL ioctl in vcpu thread

[Yongji Xie]

According to KVM API Documentation, we should only
run vcpu ioctls from the same thread that was used
to create the vcpu. This patch makes KVM_KVMCLOCK_CTRL
ioctl consistent with the Documentation.

No functional change.

Signed-off-by: Yongji Xie 
Signed-off-by: Chai Wen 
---
 hw/i386/kvm/clock.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 0bf1c60..25ea783 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -147,6 +147,15 @@ static void kvm_update_clock(KVMClockState *s)
 s->clock_is_reliable = kvm_has_adjust_clock_stable();
 }
 
+static void do_kvmclock_ctrl(CPUState *cpu, run_on_cpu_data data)
+{
+int ret = kvm_vcpu_ioctl(cpu, KVM_KVMCLOCK_CTRL, 0);
+
+if (ret && ret != -EINVAL) {
+fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
+}
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
  RunState state)
 {
@@ -183,13 +192,7 @@ static void kvmclock_vm_state_change(void *opaque, int 
running,
 return;
 }
 CPU_FOREACH(cpu) {
-ret = kvm_vcpu_ioctl(cpu, KVM_KVMCLOCK_CTRL, 0);
-if (ret) {
-if (ret != -EINVAL) {
-fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
-}
-return;
-}
+run_on_cpu(cpu, do_kvmclock_ctrl, RUN_ON_CPU_NULL);
 }
 } else {
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Stefan Hajnoczi]

On Tue, Jul 10, 2018 at 06:33:35PM +0300, Julia Suvorova via Qemu-devel wrote:
> The differences from ARMv7-M NVIC are:
>   * ARMv6-M only supports up to 32 external interrupts
>(configurable feature already). The ICTR is reserved.
>   * Active Bit Register is reserved.
>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
> 
> Signed-off-by: Julia Suvorova 
> ---
>  hw/intc/armv7m_nvic.c | 29 +
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 38aaf3dc8e..8545c87caa 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool 
> secure, uint8_t prio)
>  assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>  assert(irq < s->num_irq);
>  
> +if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {

ARMv8-M also has the NVIC:
https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf

Should arm_feature(&s->cpu->env, ARM_FEATURE_V6) be used instead of
!arm_feature(&s->cpu->env, ARM_FEATURE_V7)?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: Document copy offloading implications with -S and -c

[Stefan Hajnoczi]

On Wed, Jul 11, 2018 at 09:22:27AM +0800, Fam Zheng wrote:
> Explicitly enabling zero detection or compression suppresses copy
> offloading during convert. Document it.
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img.texi | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Stefan Hajnoczi]

On Tue, Jul 10, 2018 at 04:50:36PM +0100, Stefan Hajnoczi wrote:
> The ->pre_plug() callback is invoked before the device is realized.  The
> ->plug() callback is invoked when the device is being realized but
> before it is reset.
> 
> This patch adds a ->post_plug() callback which is invoked after the
> device has been reset.  This callback is needed by HotplugHandlers that
> need to wait until after ->reset().
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/hotplug.h | 12 
>  hw/core/hotplug.c| 11 +++
>  hw/core/qdev.c   |  7 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 1a0516a479..1e8b1053b6 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @parent: Opaque parent interface.
>   * @pre_plug: pre plug callback called at start of device.realize(true)
>   * @plug: plug callback called at end of device.realize(true).
> + * @post_plug: post plug callback called after device.realize(true) and 
> device
> + * reset
>   * @unplug_request: unplug request callback.
>   *  Used as a means to initiate device unplug for devices 
> that
>   *  require asynchronous unplug handling.
> @@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass {
>  /*  */
>  hotplug_fn pre_plug;
>  hotplug_fn plug;
> +hotplug_fn post_plug;
>  hotplug_fn unplug_request;
>  hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -83,6 +86,15 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>DeviceState *plugged_dev,
>Error **errp);
>  
> +/**
> + * hotplug_handler_post_plug:
> + *
> + * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
> + */
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp);
> +
>  /**
>   * hotplug_handler_unplug_request:
>   *
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..ab34c19461 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>  }
>  }
>  
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp)
> +{
> +HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +if (hdc->post_plug) {
> +hdc->post_plug(plug_handler, plugged_dev, errp);
> +}
> +}
> +
>  void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>  DeviceState *plugged_dev,
>  Error **errp)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cf0db4b6da..78c0e031ff 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -865,6 +865,13 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  }
>  if (dev->hotplugged) {
>  device_reset(dev);
> +
> +if (hotplug_ctrl) {

In the final patch I will move this out of if (dev->hotplugged) since
the other HotplugHandler callbacks are also invoked unconditionally.

> +hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
> +if (local_err != NULL) {
> +goto child_realize_fail;
> +}
> +}
>  }
>  dev->pending_deleted_event = false;
>  } else if (!value && dev->realized) {
> -- 
> 2.17.1
> 
> 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3] accel: forbid early use of kvm_enabled() and friends

[Philippe Mathieu-Daudé]

On 07/05/2018 04:54 AM, Greg Kurz wrote:
> It is unsafe to rely on *_enabled() helpers before the accelerator has
> been initialized, ie, accel_init_machine() has succeeded, because they
> always return false. But it is still possible to end up calling them
> indirectly by inadvertance, and cause QEMU to misbehave.
> 
> This patch causes QEMU to abort if we try to check for any accelerator
> before accel_init_machine() was called. This will help to catch bugs
> earlier.
> 
> Signed-off-by: Greg Kurz 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
> 
> Current master (SHA1 2a018f6e9878) has such a kvm_enabled() misuse that
> triggers the assert:
> 
> https://travis-ci.org/gkurz/qemu/jobs/400059821#L8709
> 
> The fix for the misuse is available, waiting to be merged:
> 
> https://patchwork.ozlabs.org/patch/938077/
> 
> v3: - use an inline function
> - convert all accelerators
> - dropped David's R-b
> 
> v2: - dropped change in qom/cpu.c (useless header inclusion)
> - only #include "sysemu/kvm.h" if we actually need it
> - added David's R-b from v1 because changes in v2 are minor
> ---
>  accel/accel.c |4 
>  include/hw/xen/xen.h  |3 ++-
>  include/qemu-common.h |3 ++-
>  include/sysemu/accel.h|8 
>  include/sysemu/hvf.h  |3 ++-
>  include/sysemu/kvm.h  |3 ++-
>  include/sysemu/qtest.h|3 ++-
>  stubs/Makefile.objs   |1 +
>  stubs/accel.c |   12 
>  target/i386/hax-all.c |2 +-
>  target/i386/whpx-all.c|2 +-
>  tests/ptimer-test-stubs.c |2 ++
>  12 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 stubs/accel.c
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 966b2d8f536c..bbd890abd7ae 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -68,6 +68,8 @@ static int accel_init_machine(AccelClass *acc, MachineState 
> *ms)
>  return ret;
>  }
>  
> +AccelState *current_accelerator;
> +
>  void configure_accelerator(MachineState *ms)
>  {
>  const char *accel;
> @@ -116,6 +118,8 @@ void configure_accelerator(MachineState *ms)
>  if (init_failed) {
>  error_report("Back to %s accelerator", acc->name);
>  }
> +
> +current_accelerator = ms->accelerator;
>  }
>  
>  void accel_register_compat_props(AccelState *accel)
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 7efcdaa8fe92..5a1664f4948f 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -25,9 +25,10 @@ extern bool xen_domid_restrict;
>  
>  extern bool xen_allowed;
>  
> +#include "sysemu/accel.h"
>  static inline bool xen_enabled(void)
>  {
> -return xen_allowed;
> +return assert_accelerator_initialized(xen_allowed);
>  }
>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 85f4749aefb7..01d5e4d97dbf 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -82,7 +82,8 @@ int qemu_openpty_raw(int *aslave, char *pty_name);
>  extern bool tcg_allowed;
>  void tcg_exec_init(unsigned long tb_size);
>  #ifdef CONFIG_TCG
> -#define tcg_enabled() (tcg_allowed)
> +#include "sysemu/accel.h"
> +#define tcg_enabled() (assert_accelerator_initialized(tcg_allowed))
>  #else
>  #define tcg_enabled() 0
>  #endif
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 637358f43014..843334ae5229 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -72,4 +72,12 @@ void accel_register_compat_props(AccelState *accel);
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
>  
> +extern AccelState *current_accelerator;
> +
> +static inline bool assert_accelerator_initialized(bool allowed)
> +{
> +assert(current_accelerator != NULL);
> +return allowed;
> +}
> +
>  #endif
> diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
> index 241118845c52..62bdc9ff9b3d 100644
> --- a/include/sysemu/hvf.h
> +++ b/include/sysemu/hvf.h
> @@ -26,7 +26,8 @@ extern int hvf_disabled;
>  #include "hw/hw.h"
>  uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>   int reg);
> -#define hvf_enabled() !hvf_disabled
> +#include "sysemu/accel.h"
> +#define hvf_enabled() assert_accelerator_initialized(!hvf_disabled)
>  #else
>  #define hvf_enabled() 0
>  #define hvf_get_supported_cpuid(func, idx, reg) 0
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0b64b8e06786..5a2e59e99128 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -46,7 +46,8 @@ extern bool kvm_direct_msi_allowed;
>  extern bool kvm_ioeventfd_any_length_allowed;
>  extern bool kvm_msi_use_devid;
>  
> -#define kvm_enabled()   (kvm_allowed)
> +#include "sysemu/accel.h"
> +#define kvm_enabled()   (assert_accelerator_initialized(kvm_allowed))
>  /**
>   * kvm_irqchip_in_kernel:
>   *
> diff --git a/include/sysem

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Paolo Bonzini]

On 11/07/2018 15:29, Stefan Hajnoczi wrote:
>>  if (dev->hotplugged) {
>>  device_reset(dev);
>> +
>> +if (hotplug_ctrl) {
> In the final patch I will move this out of if (dev->hotplugged) since
> the other HotplugHandler callbacks are also invoked unconditionally.

I'm not even sure why the reset is needed (removing it would also fix
the bug!), and why it's only done on hotplug; however, it is probably
there because it updates some bus-level state, so it's dangerous to
remove it.

Paolo

>> +hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
>> +if (local_err != NULL) {
>> +goto child_realize_fail;
>> +}
>> +}
>>  }
>>  dev->pending_deleted_event = false;

Re: [Qemu-devel] [PATCH v2 1/3] hw/arm: extract ARM M Profile base class from ARMv7-M

[Peter Maydell]

On 11 July 2018 at 13:51, Stefan Hajnoczi  wrote:
> On Thu, Jul 05, 2018 at 04:49:22PM +0100, Peter Maydell wrote:
>> On 5 July 2018 at 16:45, Peter Maydell  wrote:
>> > On 30 June 2018 at 10:13, Stefan Hajnoczi  wrote:
>> >> The ARMv7-M code is largely similar to what other M Profile CPUs need.
>> >> Extract the common M Profile aspects into the ARMMProfileState base
>> >> class.  ARMv6-M will inherit from this class in the following patch.
>> >>
>> >> It might be possible to make ARMv6-M the base class of ARMv7-M, but it
>> >> seems cleaner to have an M Profile base class instead of saying an
>> >> "ARMv7-M is an ARMv6-M".
>> >>
>> >> Signed-off-by: Stefan Hajnoczi 
>> >
>> > This makes sense, I guess (though it currently leaves us in the
>> > odd position that we have separate a object for v6m, but the v7m
>> > object handles both v7m and v8m...)
>>
>> ...though I guess the counter-argument is that the only thing that
>> the v7m object is doing that v6m doesn't want is creating
>> the bitbanding device, and in fact bitbanding is optional in v7m
>> (you can configure a Cortex-M3 without it). So maybe we should
>> instead just have a QOM property to let you turn off the
>> bitbanding ?
>
> Okay, we can do that.  So how about a single ARMMProfileState class for
> v6, v7, and v8?

Yes, I think that makes sense. (A lot of our code says 'v7m' when
it really means 'm profile in general' for historical reasons. I'm
not too worried about trying to tidy that up.)

thanks
-- PMM

Re: [Qemu-devel] qemu-nbd vs 'simple' trace backend vs iotest 147

[Cornelia Huck]

On Wed, 11 Jul 2018 15:15:45 +0200
Cornelia Huck  wrote:

> On Wed, 11 Jul 2018 14:06:17 +0100
> Stefan Hajnoczi  wrote:
> 
> > On Mon, Jul 09, 2018 at 03:45:49PM +0200, Cornelia Huck wrote:  
> > > Hi,
> > > 
> > > I recently noticed that iotest 147 was hanging on my laptop, but worked
> > > fine on my s390x LPAR. Turned out that the architecture was a red
> > > herring; on both platforms, things fail with the 'simple' trace backend
> > > and work with e.g. the 'log' trace backend. Some details on the
> > > failures with the 'simple' backend:
> > > 
> > > - The first run of 147 passes. However, there are two processes hanging
> > >   around, one using a unix socket and one using an inet socket:
> > > 
> > > cohuck   22912  0.0  0.0 156580  3836 ?Ss   14:32   0:00 
> > > /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f 
> > > qcow2 /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -p 
> > > 10811
> > > cohuck   22925  0.0  0.0 156580  3840 ?Ss   14:32   0:00 
> > > /home/cohuck/git/qemu/build/tests/qemu-iotests/../../qemu-nbd --fork -f 
> > > qcow2 /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/test.img -k 
> > > /home/cohuck/git/qemu/build/tests/qemu-iotests/scratch/nbd.socket
> > > 
> > > Attaching a gdb shows that we seem to be waiting on flushing:
> > > 
> > > (gdb) bt
> > > #0  0x7f461c078b99 in syscall () from /lib64/libc.so.6
> > > #1  0x7f461d13650f in g_cond_wait () from /lib64/libglib-2.0.so.0
> > > #2  0x560cf3a1caf2 in flush_trace_file (wait=255)
> > > at /home/cohuck/git/qemu/trace/simple.c:139
> > > #3  st_flush_trace_buffer () at /home/cohuck/git/qemu/trace/simple.c:374
> > > #4  0x7f461bfc01d8 in __run_exit_handlers () from /lib64/libc.so.6
> > > #5  0x7f461bfc022a in exit () from /lib64/libc.so.6
> > > #6  0x560cf392eb7e in main (argc=, argv= > > out>)
> > > at /home/cohuck/git/qemu/qemu-nbd.c:1076
> > > 
> > > (for both processes)
> > 
> > Please also print backtraces for the other threads:
> > 
> >   (gdb) thread apply all bt
> > 
> > There should be another thread in writeout_thread() so I'm surprised
> > that flush_trace_file() is getting stuck in g_cond_wait().  
> 
> I'll re-run to check, but there was only one thread in the process in
> question.

OK, I have two threads for one of the qemu-nbds using inet created on
the second run (when it fails with the 'port already in use' message):

(gdb) thread apply all bt

Thread 2 (Thread 0x7f639be49700 (LWP 3091)):
#0  0x7f639d549b99 in syscall () from /lib64/libc.so.6
#1  0x7f639e60750f in g_cond_wait () from /lib64/libglib-2.0.so.0
#2  0x5619516d298f in wait_for_trace_records_available ()
at /home/cohuck/git/qemu/trace/simple.c:150
#3  writeout_thread (opaque=)
at /home/cohuck/git/qemu/trace/simple.c:169
#4  0x7f639e5e9486 in g_thread_proxy () from /lib64/libglib-2.0.so.0
#5  0x7f639d81750b in start_thread () from /lib64/libpthread.so.0
#6  0x7f639d54f16f in clone () from /lib64/libc.so.6

Thread 1 (Thread 0x7f63a05bba40 (LWP 3090)):
#0  0x7f639d820d68 in read () from /lib64/libpthread.so.0
#1  0x5619515ec61c in main (argc=, argv=0x7ffc1220bfb8)
at /home/cohuck/git/qemu/qemu-nbd.c:881

That one goes away after I Ctrl-C out of the hanging iotest (together
with the zombie qemu-ndb).

The other qemu-nbds (the inet and the unix socket ones from the first
run, the second inet one from the second run) have a single thread with
the same backtrace I posted above.

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Peter Maydell]

On 11 July 2018 at 14:25, Stefan Hajnoczi  wrote:
> On Tue, Jul 10, 2018 at 06:33:35PM +0300, Julia Suvorova via Qemu-devel wrote:
>> The differences from ARMv7-M NVIC are:
>>   * ARMv6-M only supports up to 32 external interrupts
>>(configurable feature already). The ICTR is reserved.
>>   * Active Bit Register is reserved.
>>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
>>
>> Signed-off-by: Julia Suvorova 
>> ---
>>  hw/intc/armv7m_nvic.c | 29 +
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 38aaf3dc8e..8545c87caa 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool 
>> secure, uint8_t prio)
>>  assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>>  assert(irq < s->num_irq);
>>
>> +if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
>
> ARMv8-M also has the NVIC:
> https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf
>
> Should arm_feature(&s->cpu->env, ARM_FEATURE_V6) be used instead of
> !arm_feature(&s->cpu->env, ARM_FEATURE_V7)?

All of v6M, v7M and v8M cores will set ARM_FEATURE_V6, so
testing on it doesn't distinguish them.

thanks
-- PMM

Re: [Qemu-devel] [BUG] checkpatch.pl hangs on target/mips/msa_helper.c

[Philippe Mathieu-Daudé]

On 07/11/2018 09:36 AM, Stefan Hajnoczi wrote:
> On Wed, Jul 04, 2018 at 03:35:18PM +, Aleksandar Markovic wrote:
>> If  checkpatch.pl is applied (using switch "-f") on file 
>> target/mips/msa_helper.c, it will hang.
>>
>> There is a workaround for this particular file:
>>
>> These lines in msa_helper.c:
>>
>> uint## BITS ##_t S = _S, T = _T;\
>> uint## BITS ##_t as, at, xs, xt, xd;\
>>
>> should be replaced with:
>>
>> uint## BITS ## _t S = _S, T = _T;   \
>> uint## BITS ## _t as, at, xs, xt, xd;   \
>>
>> (a space is added after the second "##" in each line)
>>
>> The workaround is found by partial deleting and undeleting of the code in 
>> msa_helper.c in binary search fashion.
>>
>> This workaround will soon be submitted by me as a patch within a series on 
>> misc MIPS issues.
>>
>> I took a look at checkpatch.pl code, and it looks it is fairly complicated 
>> to fix the issue, since it happens in the code segment involving intricate 
>> logic conditions.
> 
> Thanks for figuring this out, Aleksandar.  Not sure if anyone else has
> the apetite to fix checkpatch.pl.

Anyone else but Paolo ;P

http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg01250.html



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Philippe Mathieu-Daudé]

Hi Dima,

On 07/11/2018 05:34 AM, Dima Stepanov wrote:
> Gentle ping. CCing Paolo Bonzini.
> 
> Regards, Dima.
> 
> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
>> Ping.
>>
>> Regards, Dima.
>>
>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
>>> In the memory_region_do_invalidate_mmio_ptr() routine the section
>>> variable is intialized by the memory_region_find() call. The section.mr
>>> field can be set to NULL.
>>>
>>> Add the check for NULL before trying to drop a section.
>>>
>>> Signed-off-by: Dima Stepanov 
>>> ---
>>>  memory.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 3212acc..bb45248 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -2712,7 +2712,7 @@ static void 
>>> memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
>>>  /* Reset dirty so this doesn't happen later. */
>>>  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
>>>  
>>> -if (section.mr != mr) {
>>> +if (section.mr && (section.mr != mr)) {

section.mr can't be NULL here.

You can give the static analyzer a hint using "assert(section.mr);"

>>>  /* memory_region_find add a ref on section.mr */
>>>  memory_region_unref(section.mr);
>>>  if (MMIO_INTERFACE(section.mr->owner)) {
>>> -- 
>>> 2.7.4
>>>

Regards,

Phil.



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Kashyap Chamarthy]

On Tue, Jul 10, 2018 at 05:01:22PM +0200, Cornelia Huck wrote:

[...]

> Who is, in general, testing which libvirt version? I can think of:
> - libvirt developers, which will probably run libvirt current git, but
>   more likely a released QEMU?
> - QEMU (and other related tools) developers, who will probably use QEMU
>   current git, but a released libvirt
> - normal (technical) users and (integration) testers, who will probably
>   use released versions of libvirt and QEMU

There is another kind of integration tester -- e.g. FWIW, in Nova I've
been advocating to create a CI job that would do the following:

- QEMU from Git
- libvirt from Git
- Nova from Git

Along with:

- Latest QEMU release
- Latest libvirt release
- Nova from Git

With the aim of seeing what explodes in Nova and file bugs (for the
relevant low-leve components) and coordinate with relevant upstream
accordingly.

* * *

Further, Nova's libvirt driver defines several version constants.  The
following two are set to a version that is available across a set of
"Linux distributions that matter"[*]

MIN_LIBVIRT_VERSION = (1, 3, 1)
MIN_QEMU_VERSION = (2, 5, 0)

(The above are the minimum required versions _today_.)

And at the start of each development cycle (lasts 6 months), we evaluate
what versions are available and update the version matrix[*]:

NEXT_MIN_LIBVIRT_VERSION = (3, 0, 0)
NEXT_MIN_QEMU_VERSION = (2, 8, 0)

(The above will be the versions for the _upcoming_ development cycle --
sometime end of this year.)

https://wiki.openstack.org/wiki/LibvirtDistroSupportMatrix


-- 
/kashyap

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

On further investigation of my instance, there appeared to be no high
logging volume to the console, nor anything using the /dev/ttyS0 other
than agetty.  Switching from the generic kernel to the AWS kernel seems
to have stabilised it.

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

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

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

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

I'm seeing this on AWS EC2 when there's (apparently) high logging volume
to the console, very similarly to
https://www.reddit.com/r/sysadmin/comments/6zuqad/mongodb_aws_ec2_serial8250_too_much_work_for_irq4/

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

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

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

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

** Tags added: canonical-is

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

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

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

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Philippe Mathieu-Daudé]

Hi Stephane,

On 07/11/2018 04:52 AM, stephane duverger wrote:
> To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
>> entered gdb_vm_state_change() with and use CPUState *cpu =
>> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
>> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
>> which then deref crap->singlestep_enabled.
>>
>> So I think this is not the correct fix.
>>
> 
> I think you are wrong. You can enter gdb_vm_state_change() only if it has
> been registered through qemu_add_vm_change_state_handler(). And this
> happens in gdbserver_start() which is called only when you start the
> gdbserver stub.
> 
> This is exactly what we don't want to do here: use qemu breakpoints and
> debug events forwarding without the need to enable a gdb stub.

Well, at least we agree the gdb stub code is not straightforward.

And apparently without seeing the bigger picture about how you are using
this, I am missing something.

> There might be an historical reason that vCPU debug events are always
> forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
> should not be mandatory.
> 
> One could consider a check to the gdbserver state right before:
> 
> if (gdbserver_enabled())
> gdb_set_stop_cpu(cpu);
> 
> As found in other part of qemu code: kvm_enabled, hax_enabled, ...
> 
> 
>> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
>> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
>> reach this.
>>
> 
> Correct if you previously add the gdbserver_enabled() check. Else this
> would abort the qemu instance each time a debug event is triggered
> and forwarded to a vm_change_state handler.
> 
  void gdb_set_stop_cpu(CPUState *cpu)
>  {
> +if (!gdbserver_state) {
> +return;
> +}
>  gdbserver_state->c_cpu = cpu;
>  gdbserver_state->g_cpu = cpu;
>>
>> I find it safer the opposite way, if (s) { s-> ... }
>>
> 
> Sincerely, this argument is subjective. If it's part of Qemu coding
> standard,
> i would agree. Again there is a lot of situations in the Qemu code where
> exit conditions are checked first and then lead to a return, preventing an
> unneeded level of indentation. Seriously, we are talking about a 2 lines
> function.

This is indeed a personal subjective suggestion, not part of QEMU Coding
standard. Rational is, if in the future someone needs to modify
gdb_set_stop_cpu(), it would be easier/safer for him.

No worries ;)

Regards,

Phil.



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1781211] [NEW] HAXM acceleration does not work at all.

[Dmitriy via Qemu-devel]

Public bug reported:

I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does not 
works when i turn on hax acceleration. Permanent kernel panics, black screen 
freezing and other crashes happens when i run qemu.
Qemu crashed with hax - when i ran it from iso. It crashed on already installed 
system - it's not matters. 

Versions:
archlinux-2018.07.01-x86_64
ubuntu-18.04-live-server-amd64.iso

I run qemu-system-x86_64.exe binary.

My CPU:
core i7 2600k

See screenshot

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: hax haxm windows

** Attachment added: "2018-07-11_15-49-15.png"
   
https://bugs.launchpad.net/bugs/1781211/+attachment/5162388/+files/2018-07-11_15-49-15.png

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

Title:
  HAXM acceleration does not work at all.

Status in QEMU:
  New

Bug description:
  I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does 
not works when i turn on hax acceleration. Permanent kernel panics, black 
screen freezing and other crashes happens when i run qemu.
  Qemu crashed with hax - when i ran it from iso. It crashed on already 
installed system - it's not matters. 

  Versions:
  archlinux-2018.07.01-x86_64
  ubuntu-18.04-live-server-amd64.iso

  I run qemu-system-x86_64.exe binary.

  My CPU:
  core i7 2600k

  See screenshot

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

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> Basic framework steps are the following:
> - All nodes and edges are created in their respective machine/driver/test 
> files
> - The framework starts QEMU and asks for a list of available drivers
>   and machines

QEMU doesn't know about qtest drivers.  Does "drivers" mean "devices"
(i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
drivers?

The concept of qgraph makes sense.  Ease of use and documentation will
be critical to gain adoption.  The extra complexity makes it hard for
people writing and running tests, so it's important that it's clear
which tests will get run and why (or why not).


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] docker: Install more packages in centos7

[Philippe Mathieu-Daudé]

On 07/11/2018 03:58 AM, Fam Zheng wrote:
> This makes test-block work.
> 
> Signed-off-by: Fam Zheng 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tests/docker/dockerfiles/centos7.docker | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/docker/dockerfiles/centos7.docker 
> b/tests/docker/dockerfiles/centos7.docker
> index 575de29a0a..83462b7205 100644
> --- a/tests/docker/dockerfiles/centos7.docker
> +++ b/tests/docker/dockerfiles/centos7.docker
> @@ -3,6 +3,7 @@ RUN yum install -y epel-release centos-release-xen
>  RUN yum -y update
>  ENV PACKAGES \
>  bison \
> +bzip2 \
>  bzip2-devel \
>  ccache \
>  csnappy-devel \
> @@ -12,10 +13,12 @@ ENV PACKAGES \
>  gettext \
>  git \
>  glib2-devel \
> +libaio-devel \
>  libepoxy-devel \
>  libfdt-devel \
>  librdmacm-devel \
>  lzo-devel \
> +nettle-devel \
>  make \
>  mesa-libEGL-devel \
>  mesa-libgbm-devel \
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> The io_readx() function needs to know whether the load it is
> doing is an MMU_DATA_LOAD or an MMU_INST_FETCH, so that it
> can pass the right value to the cpu_transaction_failed()
> function. Plumb this information through from the softmmu
> code.
> 
> This is currently not often going to give the wrong answer,
> because usually instruction fetches go via get_page_addr_code().
> However once we switch over to handling execution from non-RAM by
> creating single-insn TBs, the path for an insn fetch to generate
> a bus error will be through cpu_ld*_code() and io_readx(),
> so without this change we will generate a d-side fault when we
> should generate an i-side fault.
> 
> We also have to pass the access type via a CPU struct global
> down to unassigned_mem_read(), for the benefit of the targets
> which still use the cpu_unassigned_access() hook (m68k, mips,
> sparc, xtensa).
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  accel/tcg/softmmu_template.h | 11 +++
>  include/qom/cpu.h|  6 ++
>  accel/tcg/cputlb.c   |  5 +++--
>  memory.c |  3 ++-
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
> index badbf148803..f060a693d41 100644
> --- a/accel/tcg/softmmu_template.h
> +++ b/accel/tcg/softmmu_template.h
> @@ -99,11 +99,12 @@ static inline DATA_TYPE glue(io_read, 
> SUFFIX)(CPUArchState *env,
>size_t mmu_idx, size_t index,
>target_ulong addr,
>uintptr_t retaddr,
> -  bool recheck)
> +  bool recheck,
> +  MMUAccessType access_type)
>  {
>  CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
>  return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck,
> -DATA_SIZE);
> +access_type, DATA_SIZE);
>  }
>  #endif
>  
> @@ -140,7 +141,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, 
> target_ulong addr,
>  /* ??? Note that the io helpers always read data in the target
> byte ordering.  We should push the LE/BE request down into io.  */
>  res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
> -tlb_addr & TLB_RECHECK);
> +tlb_addr & TLB_RECHECK,
> +READ_ACCESS_TYPE);
>  res = TGT_LE(res);
>  return res;
>  }
> @@ -207,7 +209,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, 
> target_ulong addr,
>  /* ??? Note that the io helpers always read data in the target
> byte ordering.  We should push the LE/BE request down into io.  */
>  res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
> -tlb_addr & TLB_RECHECK);
> +tlb_addr & TLB_RECHECK,
> +READ_ACCESS_TYPE);
>  res = TGT_BE(res);
>  return res;
>  }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index bd796579ee4..ecf6ed556a9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -386,6 +386,12 @@ struct CPUState {
>   */
>  uintptr_t mem_io_pc;
>  vaddr mem_io_vaddr;
> +/*
> + * This is only needed for the legacy cpu_unassigned_access() hook;
> + * when all targets using it have been converted to use
> + * cpu_transaction_failed() instead it can be removed.
> + */
> +MMUAccessType mem_io_access_type;
>  
>  int kvm_fd;
>  struct KVMState *kvm_state;
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 20c147d6554..c491703f15f 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -789,7 +789,7 @@ static inline ram_addr_t 
> qemu_ram_addr_from_host_nofail(void *ptr)
>  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>   int mmu_idx,
>   target_ulong addr, uintptr_t retaddr,
> - bool recheck, int size)
> + bool recheck, MMUAccessType access_type, int size)
>  {
>  CPUState *cpu = ENV_GET_CPU(env);
>  hwaddr mr_offset;
> @@ -831,6 +831,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
>  }
>  
>  cpu->mem_io_vaddr = addr;
> +cpu->mem_io_access_type = access_type;
>  
>  if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>  qemu_mutex_lock_iothread();
> @@ -843,7 +844,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
>  section->offset_within_address_space -
>  section->offset_

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Peter Maydell]

On 11 July 2018 at 14:47, Philippe Mathieu-Daudé  wrote:
> Hi Dima,
>
> On 07/11/2018 05:34 AM, Dima Stepanov wrote:
>> Gentle ping. CCing Paolo Bonzini.
>>
>> Regards, Dima.
>>
>> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
>>> Ping.
>>>
>>> Regards, Dima.
>>>
>>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
 In the memory_region_do_invalidate_mmio_ptr() routine the section
 variable is intialized by the memory_region_find() call. The section.mr
 field can be set to NULL.

 Add the check for NULL before trying to drop a section.

 Signed-off-by: Dima Stepanov 
 ---
  memory.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/memory.c b/memory.c
 index 3212acc..bb45248 100644
 --- a/memory.c
 +++ b/memory.c
 @@ -2712,7 +2712,7 @@ static void 
 memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
  /* Reset dirty so this doesn't happen later. */
  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);

 -if (section.mr != mr) {
 +if (section.mr && (section.mr != mr)) {
>
> section.mr can't be NULL here.
>
> You can give the static analyzer a hint using "assert(section.mr);"

Not in my view much point in messing with this code, though:
(a) it's broken and unusable as it stands (race conditions)
(b) it's obsoleted by the execute-from-mmio patchset
http://patchwork.ozlabs.org/cover/942090/ and if/when that
goes in it will all just get deleted.

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue

[Markus Armbruster]

Kevin Wolf  writes:

> Am 10.07.2018 um 16:02 hat Marc-André Lureau geschrieben:
>> Hi
>> 
>> On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf  wrote:
>> > Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
>> >> handle_qmp_command() reports JSON syntax errors right away.  This is
>> >> wrong when OOB is enabled, because the errors can "jump the queue"
>> >> then.
>> >>
>> >> The previous commit fixed the same bug for semantic errors, by
>> >> delaying the checking until dispatch.  We can't delay the checking, so
>> >> delay the reporting.
>> >>
>> >> Signed-off-by: Markus Armbruster 
>> >> Reviewed-by: Eric Blake 
>> >> Message-Id: <20180703085358.13941-19-arm...@redhat.com>
>> >
>> > I'm observing a qemu crash in qemu-iotests 153 (which does however not
>> > seem to make the test case fail). git bisect points me to this patch.
>> >
>> > I'm getting output like this:
>> >
>> > *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): 
>> > invalid pointer: 0x555f7870f7e0 ***
>> > === Backtrace: =
>> > /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
>> > /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
>> > /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
>> > /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
>> > /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
>> > /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
>> >
>> > Interestingly, this doesn't want to produce a core dump for me, so no
>> > backtrace with usable function names here. But I assume that you can
>> > easily reproduce this yourself.
>> >
>> 
>> Looks like the double-free regression, you could try: "[PATCH]
>> monitor: fix double-free of request error"
>
> Thanks, that does fix it. Looks like it missed -rc0, though?

Yes.  I'll work on a pull request for -rc1.

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Igor Mammedov]

On Wed, 11 Jul 2018 15:32:12 +0200
Paolo Bonzini  wrote:

> On 11/07/2018 15:29, Stefan Hajnoczi wrote:
> >>  if (dev->hotplugged) {
> >>  device_reset(dev);
> >> +
> >> +if (hotplug_ctrl) {  
> > In the final patch I will move this out of if (dev->hotplugged) since
> > the other HotplugHandler callbacks are also invoked unconditionally.  
> 
> I'm not even sure why the reset is needed (removing it would also fix
> the bug!), and why it's only done on hotplug; however, it is probably
> there because it updates some bus-level state, so it's dangerous to
> remove it.
it might be also so that each device won't have to call reset manually from
their realize (5ab28c834) to initialize device into initial state.

> Paolo
> 
> >> +hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
> >> +if (local_err != NULL) {
> >> +goto child_realize_fail;
> >> +}
> >> +}
> >>  }
> >>  dev->pending_deleted_event = false;  
> 

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Emanuele]

On 07/11/2018 04:00 PM, Stefan Hajnoczi wrote:


On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:

Basic framework steps are the following:
- All nodes and edges are created in their respective machine/driver/test files
- The framework starts QEMU and asks for a list of available drivers
   and machines

QEMU doesn't know about qtest drivers.  Does "drivers" mean "devices"
(i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
drivers?

Yes, they are QEMU devices, it was a typo.
Not all qtest drivers are mapped with the QEMU devices, though. In fact, 
for now all drivers "contained" or "produced" by other nodes are not 
QEMU devices.


The others (machines and nodes that "consume"), are mapped to QEMU using 
the

{ "execute": "qom-list-types", "arguments" : { "implements" : "device" } }
for "consume" and
{ "execute": "query-machines" }
for machines.

With "consume" I mean in situation like "node X" ---> consumes --> "node 
Y",

"X" should be in the output of qom-list-types query.

The concept of qgraph makes sense.  Ease of use and documentation will
be critical to gain adoption.  The extra complexity makes it hard for
people writing and running tests, so it's important that it's clear
which tests will get run and why (or why not).

I am trying to document and makes it as easy and clean as possible :)

[Qemu-devel] [PATCH v2 1/4] tests: Add an option for snapshot (default: off)

[Fam Zheng]

Not using snapshot has the benefit of automatically persisting useful
test harnesses, such as docker images and ccache database. Although it
will lose some cleanness, it is imaginably useful for patchew.

Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 3643117816..e5d6a328d5 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -216,6 +216,8 @@ def parse_args(vm_name):
   help="build QEMU from source in guest")
 parser.add_option("--interactive", "-I", action="store_true",
   help="Interactively run command")
+parser.add_option("--snapshot", "-s", action="store_true",
+  help="run tests with a snapshot")
 parser.disable_interspersed_args()
 return parser.parse_args()
 
@@ -241,7 +243,10 @@ def main(vmcls):
jobs=args.jobs)]
 else:
 cmd = argv
-vm.boot(args.image + ",snapshot=on")
+img = args.image
+if args.snapshot:
+img += ",snapshot=on"
+vm.boot(img)
 vm.wait_ssh()
 except Exception as e:
 if isinstance(e, SystemExit) and e.code == 0:
-- 
2.17.1

[Qemu-devel] [PATCH v2 0/4] Add a CentOS test image to run docker tests

[Fam Zheng]

v2: Drop archive-source.sh changes.
The new test depends on the iotests nbd fix I posted today to pass.

Docker testing on patchew has long suffered from 'make check' hangings. The
cleanness of VM testing is the cure. Now let's add a CentOS 7 image to run the
tests.  It's purely ad-hoc, but hopefully still easy to understand and use for
everyone.

The first patch makes passing source code from host to the container in VM
working, and is a nice clean up.

The second patch makes caches work, to speed up repetitive runs like on
patchew.

The last patch adds the new image that does the job. Two out of three docker
tests running on patchew.org are added to the image. I'll wait for Peter to fix
the 'docker-test-quick@centos6' hanging (oob test) before adding it too.

Fam

Fam Zheng (4):
  tests: Add an option for snapshot (default: off)
  tests/vm: Pass verbose flag into VM make commands
  tests: Allow overriding archive path with SRC_ARCHIVE
  tests: Add centos VM testing

 tests/docker/Makefile.include |  7 ++-
 tests/vm/Makefile.include |  2 +-
 tests/vm/basevm.py| 10 -
 tests/vm/centos   | 84 +++
 tests/vm/freebsd  |  4 +-
 tests/vm/netbsd   |  4 +-
 tests/vm/openbsd  |  4 +-
 tests/vm/ubuntu.i386  |  4 +-
 8 files changed, 106 insertions(+), 13 deletions(-)
 create mode 100755 tests/vm/centos

-- 
2.17.1

[Qemu-devel] [PATCH v2 3/4] tests: Allow overriding archive path with SRC_ARCHIVE

[Fam Zheng]

In VM based tests, the source archive is created in host, we don't have
to run archive-source.sh again, as it complicates the Makefile and
scripts.

Signed-off-by: Fam Zheng 
---
 tests/docker/Makefile.include | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index b2a7e761cc..f0525b91a7 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -27,8 +27,11 @@ DOCKER_SRC_COPY := $(BUILD_DIR)/docker-src.$(CUR_TIME)
 
 $(DOCKER_SRC_COPY):
@mkdir $@
-   $(call quiet-command, cd $(SRC_PATH) && scripts/archive-source.sh 
$@/qemu.tar, \
-   "GEN", "$@/qemu.tar")
+   $(if $(SRC_ARCHIVE), \
+   $(call quiet-command, cp "$(SRC_ARCHIVE)" $@/qemu.tar, \
+   "CP", "$@/qemu.tar"), \
+   $(call quiet-command, cd $(SRC_PATH) && 
scripts/archive-source.sh $@/qemu.tar, \
+   "GEN", "$@/qemu.tar"))
$(call quiet-command, cp $(SRC_PATH)/tests/docker/run $@/run, \
"COPY","RUNNER")
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/4] tests/vm: Pass verbose flag into VM make commands

[Fam Zheng]

Our Makefile has:

vm-build-%: tests/vm/%.img
$(call quiet-command, \
$(SRC_PATH)/tests/vm/$* \
$(if $(V)$(DEBUG), --debug) \
$(if $(DEBUG), --interactive) \

the intention of which is to let the make command in VM have V=1 if
V=1 is set. We pass this to the BUILD_SCRIPT format.

Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py   | 3 ++-
 tests/vm/freebsd | 4 ++--
 tests/vm/netbsd  | 4 ++--
 tests/vm/openbsd | 4 ++--
 tests/vm/ubuntu.i386 | 4 ++--
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index e5d6a328d5..25361c6b7d 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -240,7 +240,8 @@ def main(vmcls):
 vm.add_source_dir(args.build_qemu)
 cmd = [vm.BUILD_SCRIPT.format(
configure_opts = " ".join(argv),
-   jobs=args.jobs)]
+   jobs=args.jobs,
+   verbose="1" if args.debug else "")]
 else:
 cmd = argv
 img = args.image
diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 039dad8f69..b20612a6c9 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -23,8 +23,8 @@ class FreeBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/vtbd1;
 ./configure {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index 3972d8b45c..3f9d34a208 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -23,8 +23,8 @@ class NetBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rld1a;
 ./configure --python=python2.7 {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/openbsd b/tests/vm/openbsd
index 6ae16d97fd..3285c1abde 100755
--- a/tests/vm/openbsd
+++ b/tests/vm/openbsd
@@ -23,9 +23,9 @@ class OpenBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rsd1c;
 ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 
--python=python2.7 {configure_opts};
-gmake -j{jobs};
+gmake -j{jobs} V={verbose};
 # XXX: "gmake check" seems to always hang or fail
-#gmake check;
+#gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
index fc319e0e6e..b4eaa0dedc 100755
--- a/tests/vm/ubuntu.i386
+++ b/tests/vm/ubuntu.i386
@@ -25,8 +25,8 @@ class UbuntuX86VM(basevm.BaseVM):
 sudo chmod a+r /dev/vdb;
 tar -xf /dev/vdb;
 ./configure {configure_opts};
-make -j{jobs};
-make check;
+make -j{jobs} V={verbose};
+make check V={verbose};
 """
 
 def _gen_cloud_init_iso(self):
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/4] tests: Add centos VM testing

[Fam Zheng]

This one does docker testing in the VM. It is intended to replace the
native docker testing on patchew testers.

Signed-off-by: Fam Zheng 
---
 tests/vm/Makefile.include |  2 +-
 tests/vm/centos   | 84 +++
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100755 tests/vm/centos

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 5daa2a3b73..e492cd73e5 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -2,7 +2,7 @@
 
 .PHONY: vm-build-all
 
-IMAGES := ubuntu.i386 freebsd netbsd openbsd
+IMAGES := ubuntu.i386 freebsd netbsd openbsd centos
 IMAGE_FILES := $(patsubst %, tests/vm/%.img, $(IMAGES))
 
 .PRECIOUS: $(IMAGE_FILES)
diff --git a/tests/vm/centos b/tests/vm/centos
new file mode 100755
index 00..afd560c564
--- /dev/null
+++ b/tests/vm/centos
@@ -0,0 +1,84 @@
+#!/usr/bin/env python
+#
+# CentOS image
+#
+# Copyright 2018 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng 
+#
+# This code is licensed under the GPL version 2 or later.  See
+# the COPYING file in the top-level directory.
+#
+
+import os
+import sys
+import subprocess
+import basevm
+import time
+
+class CentosVM(basevm.BaseVM):
+name = "centos"
+BUILD_SCRIPT = """
+set -e;
+cd $(mktemp -d);
+export SRC_ARCHIVE=/dev/vdb;
+sudo chmod a+r $SRC_ARCHIVE;
+tar -xf $SRC_ARCHIVE;
+make docker-test-block@centos7 V={verbose} J={jobs};
+make docker-test-quick@centos7 V={verbose} J={jobs};
+make docker-test-mingw@fedora V={verbose} J={jobs};
+"""
+
+def _gen_cloud_init_iso(self):
+cidir = self._tmpdir
+mdata = open(os.path.join(cidir, "meta-data"), "w")
+mdata.writelines(["instance-id: centos-vm-0\n",
+  "local-hostname: centos-guest\n"])
+mdata.close()
+udata = open(os.path.join(cidir, "user-data"), "w")
+udata.writelines(["#cloud-config\n",
+  "chpasswd:\n",
+  "  list: |\n",
+  "root:%s\n" % self.ROOT_PASS,
+  "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS),
+  "  expire: False\n",
+  "users:\n",
+  "  - name: %s\n" % self.GUEST_USER,
+  "sudo: ALL=(ALL) NOPASSWD:ALL\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "  - name: root\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "locale: en_US.UTF-8\n"])
+udata.close()
+subprocess.check_call(["genisoimage", "-output", "cloud-init.iso",
+   "-volid", "cidata", "-joliet", "-rock",
+   "user-data", "meta-data"],
+   cwd=cidir,
+   stdin=self._devnull, stdout=self._stdout,
+   stderr=self._stdout)
+return os.path.join(cidir, "cloud-init.iso")
+
+def build_image(self, img):
+cimg = 
self._download_with_cache("https://cloud.centos.org/centos/7/images/CentOS-7-x86_64-GenericCloud-1802.qcow2.xz";)
+img_tmp = img + ".tmp"
+subprocess.check_call(["cp", "-f", cimg, img_tmp + ".xz"])
+subprocess.check_call(["xz", "-df", img_tmp + ".xz"])
+subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
+self.boot(img_tmp, extra_args = ["-cdrom", self._gen_cloud_init_iso()])
+self.wait_ssh()
+self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
+self.ssh_root_check("yum update -y")
+self.ssh_root_check("yum install -y docker make git")
+self.ssh_root_check("systemctl enable docker")
+self.ssh_root("poweroff")
+self.wait()
+if os.path.exists(img):
+os.remove(img)
+os.rename(img_tmp, img)
+return 0
+
+if __name__ == "__main__":
+sys.exit(basevm.main(CentosVM))
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:30AM +0200, Emanuele Giuseppe Esposito wrote:
> +/* Graph Edge.*/
> +struct QOSGraphEdge {
> +QOSEdgeType type;
> +char *dest;
> +char *arg; /* just for CONTAIS and CONSUMED_BY */

CONTAINS?

> +/**
> + * remove_node(): removes a node @val from the nodes hash table.
> + * Note that node->name is not free'd since it will represent the
> + * hash table key
> + */
> +static void remove_node(void *val)

This is a confusing function name and doc comment.  It does not remove
the node from a hash table, it just frees it.

Please call this destroy_node().  The g_hash_table_new_full() argument
is GDestroyNotify value_destroy_func, so that would be consistent with
glib naming too.

> +{
> +QOSGraphNode *node = (QOSGraphNode *) val;

There is no need to cast void pointers in C.  Just "QOSGraphNode *node =
val;" will do.

> +/**
> + * build_driver_cmd_line(): builds the command line for the driver
> + * @node. The node name must be a valid qemu identifier, since it
> + * will be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * concatenated to the command line.
> + *
> + * For drivers, prepend -device to the driver name.
> + */
> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)

Why is this called "driver" instead of "device"?

> +{
> +if (args) {
> +node->command_line = g_strconcat("-device ", node->name, ",",
> +  args, NULL);
> +} else {
> +node->command_line = g_strconcat("-device ", node->name, NULL);
> +}
> +}
> +
> +/**
> + * build_test_cmd_line(): builds the command line for the test
> + * @node. The node name need not to be a valid qemu identifier, since it
> + * will not be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * used as additional command line.
> + */
> +static void build_test_cmd_line(QOSGraphNode *node, const char *args)
> +{
> +if (args) {
> +node->command_line = g_strdup(args);
> +} else {
> +node->command_line = NULL;
> +}

This is equivalent to:

  node->command_line = g_strdup(args);

https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup

> diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
> new file mode 100644
> index 00..54a1786c1e
> --- /dev/null
> +++ b/tests/libqos/qgraph.h
> @@ -0,0 +1,259 @@
> +/*
> + * libqos driver framework

The per-function doc comment are good.  Please also include higher-level
examples of how to use this.  At first glance all these functions are
overwhelming and it's not clear how to implement new tests, drivers,
interfaces, or machines.

See include/qom/object.h for inspiration.

> +/* edge types*/
> +enum QOSEdgeType {
> +CONTAINS,
> +PRODUCES,
> +CONSUMED_BY
> +};
> +
> +/* node types*/
> +enum QOSNodeType {
> +MACHINE,
> +DRIVER,
> +INTERFACE,
> +TEST
> +};

The QOSEdgeType and QOSNodeType enum constants use commonly-used names
in the global namespace.  Please prefix them since this is a header
file that will be included from many source files.

> +
> +/**
> + * Each driver, test or machine will have this as first field.
> + * Depending on the edge, the node will call the corresponding
> + * function when walking the path.
> + *
> + * QOSGraphObject also provides a destructor, used to deallocate the
> + * after the test has been executed.
> + */
> +struct QOSGraphObject {
> +/* for produces, returns void * */
> +QOSGetDriver get_driver;

Unused?

> +/* for contains, returns a QOSGraphObject * */
> +QOSGetDevice get_device;

Unused?

> diff --git a/tests/libqos/qgraph_extra.h b/tests/libqos/qgraph_extra.h
> new file mode 100644
> index 00..22850c0368
> --- /dev/null
> +++ b/tests/libqos/qgraph_extra.h
> @@ -0,0 +1,155 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#ifndef QGRAPH_EXTRA_H
> +#define QGRAPH_EXTRA_H
> +
> +#include "qgraph.h"
> +#define PRINT_DEBUG 0

I would expect the #ifdef to be in the C file that uses it.  PRINT_DEBUG
is too generic a name for a .h file, it may cause namespace collisions.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] docker: Install more packages in centos7

[Fam Zheng]

On Wed, 07/11 14:58, Fam Zheng wrote:
> This makes test-block work.
> 
> Signed-off-by: Fam Zheng 

Queued, thanks.

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that all the callers can handle get_page_addr_code() returning -1,
> remove all the code which tries to handle execution from MMIO regions
> or small-MMU-region RAM areas. This will mean that we can correctly
> execute from these areas, rather than ending up either aborting QEMU
> or delivering an incorrect guest exception.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

> ---
>  accel/tcg/cputlb.c | 95 +-
>  1 file changed, 10 insertions(+), 85 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c491703f15f..abb0225dc79 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -741,39 +741,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>  prot, mmu_idx, size);
>  }
>  
> -static void report_bad_exec(CPUState *cpu, target_ulong addr)
> -{
> -/* Accidentally executing outside RAM or ROM is quite common for
> - * several user-error situations, so report it in a way that
> - * makes it clear that this isn't a QEMU bug and provide suggestions
> - * about what a user could do to fix things.
> - */
> -error_report("Trying to execute code outside RAM or ROM at 0x"
> - TARGET_FMT_lx, addr);
> -error_printf("This usually means one of the following happened:\n\n"
> - "(1) You told QEMU to execute a kernel for the wrong 
> machine "
> - "type, and it crashed on startup (eg trying to run a "
> - "raspberry pi kernel on a versatilepb QEMU machine)\n"
> - "(2) You didn't give QEMU a kernel or BIOS filename at all, 
> "
> - "and QEMU executed a ROM full of no-op instructions until "
> - "it fell off the end\n"
> - "(3) Your guest kernel has a bug and crashed by jumping "
> - "off into nowhere\n\n"
> - "This is almost always one of the first two, so check your "
> - "command line and that you are using the right type of 
> kernel "
> - "for this machine.\n"
> - "If you think option (3) is likely then you can try 
> debugging "
> - "your guest with the -d debug options; in particular "
> - "-d guest_errors will cause the log to include a dump of 
> the "
> - "guest register state at this point.\n\n"
> - "Execution cannot continue; stopping here.\n\n");
> -
> -/* Report also to the logs, with more detail including register dump */
> -qemu_log_mask(LOG_GUEST_ERROR, "qemu: fatal: Trying to execute code "
> -  "outside RAM or ROM at 0x" TARGET_FMT_lx "\n", addr);
> -log_cpu_state_mask(LOG_GUEST_ERROR, cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> -}
> -
>  static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>  {
>  ram_addr_t ram_addr;
> @@ -963,7 +930,6 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, 
> target_ulong addr)
>  MemoryRegionSection *section;
>  CPUState *cpu = ENV_GET_CPU(env);
>  CPUIOTLBEntry *iotlbentry;
> -hwaddr physaddr, mr_offset;
>  
>  index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>  mmu_idx = cpu_mmu_index(env, true);
> @@ -977,65 +943,24 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, 
> target_ulong addr)
>(TLB_RECHECK | TLB_INVALID_MASK)) == TLB_RECHECK)) {
>  /*
>   * This is a TLB_RECHECK access, where the MMU protection
> - * covers a smaller range than a target page, and we must
> - * repeat the MMU check here. This tlb_fill() call might
> - * longjump out if this access should cause a guest exception.
> - */
> -int index;
> -target_ulong tlb_addr;
> -
> -tlb_fill(cpu, addr, 0, MMU_INST_FETCH, mmu_idx, 0);
> -
> -index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -tlb_addr = env->tlb_table[mmu_idx][index].addr_code;
> -if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> -/* RAM access. We can't handle this, so for now just stop */
> -cpu_abort(cpu, "Unable to handle guest executing from RAM within 
> "
> -  "a small MPU region at 0x" TARGET_FMT_lx, addr);
> -}
> -/*
> - * Fall through to handle IO accesses (which will almost certainly
> - * also result in failure)
> + * covers a smaller range than a target page. Return -1 to
> + * indicate that we cannot simply execute from RAM here;
> + * we will perform the necessary repeat of the MMU check
> + * when the "execute a single insn" code performs the
> + * load of the guest insn.
>   */
> +return -1;
>  }
>  
>  iotlbentry = &env->iotlb[mmu_idx][index];
>  section = iotlb_

Re: [Qemu-devel] [PATCH] seccomp: allow sched_setscheduler() with SCHED_IDLE policy

[Eduardo Otubo]

On 10/07/2018 - 16:55:57, Marc-André Lureau wrote:
> Current and upcoming mesa releases rely on a shader disk cash. It uses
> a thread job queue with low priority, set with
> sched_setscheduler(SCHED_IDLE). However, that syscall is rejected by
> the "resourcecontrol" seccomp qemu filter.
> 
> Since it should be safe to allow lowering thread priority, let's allow
> scheduling thread to idle policy.
> 
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594456
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  qemu-seccomp.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 148e4c6f24..9cd8eb9499 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -34,6 +34,12 @@
>  struct QemuSeccompSyscall {
>  int32_t num;
>  uint8_t set;
> +uint8_t narg;
> +const struct scmp_arg_cmp *arg_cmp;
> +};
> +
> +const struct scmp_arg_cmp sched_setscheduler_arg[] = {
> +SCMP_A1(SCMP_CMP_NE, SCHED_IDLE)
>  };
>  
>  static const struct QemuSeccompSyscall blacklist[] = {
> @@ -92,7 +98,8 @@ static const struct QemuSeccompSyscall blacklist[] = {
>  { SCMP_SYS(setpriority),QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_setparam), QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_getparam), QEMU_SECCOMP_SET_RESOURCECTL },
> -{ SCMP_SYS(sched_setscheduler), QEMU_SECCOMP_SET_RESOURCECTL },
> +{ SCMP_SYS(sched_setscheduler), QEMU_SECCOMP_SET_RESOURCECTL,
> +  ARRAY_SIZE(sched_setscheduler_arg), sched_setscheduler_arg },
>  { SCMP_SYS(sched_getscheduler), QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_setaffinity),  QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_getaffinity),  QEMU_SECCOMP_SET_RESOURCECTL },
> @@ -118,7 +125,8 @@ static int seccomp_start(uint32_t seccomp_opts)
>  continue;
>  }
>  
> -rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, blacklist[i].num, 0);
> +rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
> +blacklist[i].narg, blacklist[i].arg_cmp);
>  if (rc < 0) {
>  goto seccomp_return;
>  }
> -- 
> 2.18.0.129.ge3331758f1
> 

Acked-by: Eduardo Otubo 

Patch looks safe enough for me. If everyone else is OK with this I'll send a
pull-request tomorrow morning.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
>  {
> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +QPCIBusPC *qpci = obj;
> +if (!g_strcmp0(interface, "pci-bus")) {
> +return &qpci->bus;
> +}
> +printf("%s not present in pci-bus-pc", interface);
> +abort();
> +}

At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance.  qgraph duplicates part of
the object model.

> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> +if (!bus) {
> +return;
> +}

When does this happen and why?

> +dev->bus = bus;
> +dev->devfn = devfn;
> +
> +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> +printf("PCI Device not found\n");
> +abort();
> +}
> +qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)

It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC.  How about inlining this in
qpci_init_pc() instead of keeping a separate function?

> +{
>  assert(qts);
>  
>  ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
> *alloc)
>  ret->bus.mmio_alloc_ptr = 0xE000;
>  ret->bus.mmio_limit = 0x1ULL;
>  
> +ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +qpci_set_pc(ret, qts, alloc);
> +
>  return &ret->bus;
>  }
>  
>  void qpci_free_pc(QPCIBus *bus)
>  {
> +if (!bus) {
> +return;
> +}

Why is this needed now?

> +
>  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>  
>  g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, 
> uint8_t slot)
>  
>  qmp_eventwait("DEVICE_DELETED");
>  }
> +
> +static void qpci_pc(void)
> +{
> +qos_node_create_driver("pci-bus-pc", NULL);
> +qos_node_produces("pci-bus-pc", "pci-bus");

In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.

Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.

I'll think about this more as I read the rest of the patches.

> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>  
>  #include "libqos/pci.h"
>  #include "libqos/malloc.h"
> +#include "qgraph.h"
>  
> +typedef struct QPCIBusPC {
> +QOSGraphObject obj;
> +QPCIBus bus;
> +} QPCIBusPC;

Why does this need to be public?

> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);

Why does this need to be public?

> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);

Why does this need to be public?

>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>  void qpci_free_pc(QPCIBus *bus);
>  
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>  
>  #include "hw/pci/pci_regs.h"
>  #include "qemu/host-utils.h"
> +#include "qgraph.h"
>  
>  void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>   void (*func)(QPCIDevice *dev, int devfn, void 
> *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const 
> char *id,
>  qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>   opts ? ", " : "", opts ? opts : "");
>  }
> +
> +static void qpci(void)
> +{
> +qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);

Why does an interface need to be created?  The drivers declare which
interfaces they support?

I don't think this can be used to detect typoes in the driver's
qos_node_produces() call since there is no explicit control over the
order in which libqos_init() functions are called.  So the driver may
call qos_node_produces() before the qos_node_create_interface() is
called?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework

[Paolo Bonzini]

On 11/07/2018 16:28, Stefan Hajnoczi wrote:
>> + * build_driver_cmd_line(): builds the command line for the driver
>> + * @node. The node name must be a valid qemu identifier, since it
>> + * will be used to build the command line.
>> + *
>> + * It is also possible to pass an optional @args that will be
>> + * concatenated to the command line.
>> + *
>> + * For drivers, prepend -device to the driver name.
>> + */
>> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)
> Why is this called "driver" instead of "device"?
> 

It's the command line that is needed for the driver to work; it can
include also e.g. a -netdev or -blockdev option, though the most common
case is to have just -device.

>> + *
>> + * QOSGraphObject also provides a destructor, used to deallocate the
>> + * after the test has been executed.
>> + */
>> +struct QOSGraphObject {
>> +/* for produces, returns void * */
>> +QOSGetDriver get_driver;
> 
> Unused?
> 
>> +/* for contains, returns a QOSGraphObject * */
>> +QOSGetDevice get_device;
> 
> Unused?

What is unused?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:33AM +0200, Emanuele Giuseppe Esposito wrote:
> Add arm/raspi2 machine to the graph. This machine contains a generic-sdhci, so
> its constructor must take care of setting it properly when called.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/Makefile.include|  3 +-
>  tests/libqos/raspi2-machine.c | 68 +++
>  2 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 tests/libqos/raspi2-machine.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index acbf704a8a..de75a7394e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,7 +770,8 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) 
> $(libqos-pc-obj-y) tests/libqos/usb.o
>  libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
> tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
> tests/libqos/malloc-generic.o
>  
>  libqgraph-obj-y = tests/libqos/qgraph.o
> -libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) 
> tests/libqos/sdhci.o
> +libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
>  
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/raspi2-machine.c b/tests/libqos/raspi2-machine.c
> new file mode 100644
> index 00..47f024076f
> --- /dev/null
> +++ b/tests/libqos/raspi2-machine.c
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +
> +typedef struct QRaspi2Machine QRaspi2Machine;
> +
> +struct QRaspi2Machine {
> +QOSGraphObject obj;
> +QSDHCI_MemoryMapped sdhci;
> +};
> +
> +static void raspi2_destroy(QOSGraphObject *obj)
> +{
> +g_free(obj);
> +}
> +
> +static QOSGraphObject *raspi2_get_device(void *obj, const char *device)
> +{
> +QRaspi2Machine *machine = obj;
> +if (!g_strcmp0(device, "generic-sdhci")) {
> +return &machine->sdhci.obj;
> +}
> +
> +printf("%s not present in arm/raspi2", device);
> +abort();
> +}
> +
> +static void *qos_create_machine_arm_raspi2(void)
> +{
> +QRaspi2Machine *machine = g_new0(QRaspi2Machine, 1);
> +
> +machine->obj.get_device = raspi2_get_device;
> +machine->obj.destructor = raspi2_destroy;
> +qos_create_sdhci_mm(&machine->sdhci, 0x3f30, &(QSDHCIProperties) {
> +.version = 3,
> +.baseclock = 52,
> +.capab.sdma = false,
> +.capab.reg = 0x052134b4
> +});
> +return &machine->obj;
> +}
> +
> +static void raspi2(void)
> +{
> +qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
> +qos_node_contains("arm/raspi2", "generic-sdhci");
> +}
> +
> +libqos_init(raspi2);

Hmm...I didn't realize it would be necessary to manually define each
machine type.  I thought qgraph would use qemu-system-x86_64
-machine/-device/info qtree to introspect QEMU and instantiate the
appropriate drivers.

My concern is that this manual approach of defining machines complicates
qtests.  This file will need to be modified by multiple people - each of
them will be interested in a specific driver and not interested in the
driver that other people have added.

This makes things more complicated where previous qtests simply had an
if (x86) { add_test(test_x86_foo); } else if (arm) {
add_test(test_arm_foo); } in their source file.  It's ugly but it's
obvious.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:35AM +0200, Emanuele Giuseppe Esposito wrote:
> Add main executable that takes care of starting the framework, create the
> nodes, set the available drivers/machines, discover the path and run tests.

This is elegant, I like it.

> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/Makefile.include |   3 +
>  tests/qos-test.c   | 310 +
>  2 files changed, 313 insertions(+)
>  create mode 100644 tests/qos-test.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 706ac39ea5..e2db3e9d09 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -776,6 +776,9 @@ libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o 
> tests/libqos/x86_64_pc-machi
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
>  
> +check-qtest-pci-y += tests/qos-test$(EXESUF)
> +tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pc-obj-y)
> +
>  tests/qmp-test$(EXESUF): tests/qmp-test.o
>  tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
>  tests/rtc-test$(EXESUF): tests/rtc-test.o
> diff --git a/tests/qos-test.c b/tests/qos-test.c
> new file mode 100644
> index 00..f4748c44a4
> --- /dev/null
> +++ b/tests/qos-test.c
> @@ -0,0 +1,310 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qlist.h"
> +#include "libqos/qgraph.h"
> +#include "libqos/qgraph_extra.h"
> +
> +/**
> + * create_machine_name(): appends the architecture to @name if
> + * @is_machine is valid.
> + */
> +static void create_machine_name(const char **name, bool is_machine)
> +{
> +const char *arch;
> +if (!is_machine) {
> +return;
> +}
> +arch = qtest_get_arch();
> +*name = g_strconcat(arch, "/", *name, NULL);
> +}
> +
> +/**
> + * destroy_machine_name(): frees the given @name if
> + * @is_machine is valid.
> + */
> +static void destroy_machine_name(const char *name, bool is_machine)
> +{
> +if (!is_machine) {
> +return;
> +}
> +g_free((char *)name);
> +}
> +
> +/**
> + * apply_to_qlist(): using QMP queries QEMU for a list of
> + * machines and devices available, and sets the respective node
> + * as TRUE. If a node is found, also all its produced and contained
> + * child are marked available.
> + *
> + * See qos_graph_node_set_availability() for more info
> + */
> +static void apply_to_qlist(QList *list, bool is_machine)
> +{
> +const QListEntry *p;
> +const char *name;
> +QDict *minfo;
> +QObject *qobj;
> +QString *qstr;
> +
> +for (p = qlist_first(list); p; p = qlist_next(p)) {
> +minfo = qobject_to(QDict, qlist_entry_obj(p));
> +g_assert(minfo);
> +qobj = qdict_get(minfo, "name");
> +g_assert(qobj);
> +qstr = qobject_to(QString, qobj);
> +g_assert(qstr);
> +name = qstring_get_str(qstr);
> +create_machine_name(&name, is_machine);
> +qos_graph_node_set_availability(name, TRUE);
> +destroy_machine_name(name, is_machine);
> +qobj = qdict_get(minfo, "alias");
> +if (qobj) {
> +g_assert(qobj);
> +qstr = qobject_to(QString, qobj);
> +g_assert(qstr);
> +name = qstring_get_str(qstr);
> +create_machine_name(&name, is_machine);
> +qos_graph_node_set_availability(name, TRUE);
> +destroy_machine_name(name, is_machine);
> +}
> +}
> +}
> +
> +/**
> + * qos_set_machines_devices_available(): sets availability of qgraph
> + * machines and devices.
> + *
> + * This function firstly starts QEMU with "-machine none" option,
> + * and then executes the QMP protocol asking for the list of devices
> + * and machines available.
> + *
> + * for each of these items, it looks up the corresponding qgraph node,
> + * setting it as available. The list currently returns all devices that
> + * are either machines or CONSUMED_BY other nodes.
> + * Therefore, in order to mark all other nodes, it recursively sets
> + * all its CONTAINS and PRODUCES child as available too.
> + */
> +static void qos_set_machines_d

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/6] target/arm: Allow execution from small regions

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that we have full support for small regions, including execution,
> we can remove the workarounds where we marked all small regions as
> non-executable for the M-profile MPU and SAU.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/helper.c | 23 ---
>  1 file changed, 23 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a2ac96084e7..ed96e6c02fb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9784,17 +9784,6 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, 
> uint32_t address,
>  
>  fi->type = ARMFault_Permission;
>  fi->level = 1;
> -/*
> - * Core QEMU code can't handle execution from small pages yet, so
> - * don't try it. This way we'll get an MPU exception, rather than
> - * eventually causing QEMU to exit in get_page_addr_code().
> - */
> -if (*page_size < TARGET_PAGE_SIZE && (*prot & PAGE_EXEC)) {
> -qemu_log_mask(LOG_UNIMP,
> -  "MPU: No support for execution from regions "
> -  "smaller than 1K\n");
> -*prot &= ~PAGE_EXEC;
> -}
>  return !(*prot & (1 << access_type));
>  }
>  
> @@ -10014,18 +10003,6 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, 
> uint32_t address,
>  
>  fi->type = ARMFault_Permission;
>  fi->level = 1;
> -/*
> - * Core QEMU code can't handle execution from small pages yet, so
> - * don't try it. This means any attempted execution will generate
> - * an MPU exception, rather than eventually causing QEMU to exit in
> - * get_page_addr_code().
> - */
> -if (*is_subpage && (*prot & PAGE_EXEC)) {
> -qemu_log_mask(LOG_UNIMP,
> -  "MPU: No support for execution from regions "
> -  "smaller than 1K\n");
> -*prot &= ~PAGE_EXEC;
> -}
>  return !(*prot & (1 << access_type));
>  }
>  
> 

Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:36AM +0200, Emanuele Giuseppe Esposito wrote:
> +/**
> + * Old sdhci_t structure:

Do you intend to delete this comment before this series is merged?  It
seems like a TODO that doesn't need to be kept around.

> +qos_add_test("sdhci-test", "sdhci", test_machine);

How does this work for tests that need access to more than 1 device?
Can they request a driver instance via the API?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 2/4] tests/vm: Pass verbose flag into VM make commands

[Philippe Mathieu-Daudé]

On 07/11/2018 11:18 AM, Fam Zheng wrote:
> Our Makefile has:
> 
> vm-build-%: tests/vm/%.img
>   $(call quiet-command, \
>   $(SRC_PATH)/tests/vm/$* \
>   $(if $(V)$(DEBUG), --debug) \
>   $(if $(DEBUG), --interactive) \
> 
> the intention of which is to let the make command in VM have V=1 if
> V=1 is set. We pass this to the BUILD_SCRIPT format.
> 
> Signed-off-by: Fam Zheng 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tests/vm/basevm.py   | 3 ++-
>  tests/vm/freebsd | 4 ++--
>  tests/vm/netbsd  | 4 ++--
>  tests/vm/openbsd | 4 ++--
>  tests/vm/ubuntu.i386 | 4 ++--
>  5 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index e5d6a328d5..25361c6b7d 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -240,7 +240,8 @@ def main(vmcls):
>  vm.add_source_dir(args.build_qemu)
>  cmd = [vm.BUILD_SCRIPT.format(
> configure_opts = " ".join(argv),
> -   jobs=args.jobs)]
> +   jobs=args.jobs,
> +   verbose="1" if args.debug else "")]
>  else:
>  cmd = argv
>  img = args.image
> diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> index 039dad8f69..b20612a6c9 100755
> --- a/tests/vm/freebsd
> +++ b/tests/vm/freebsd
> @@ -23,8 +23,8 @@ class FreeBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/vtbd1;
>  ./configure {configure_opts};
> -gmake -j{jobs};
> -gmake check;
> +gmake -j{jobs} V={verbose};
> +gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/netbsd b/tests/vm/netbsd
> index 3972d8b45c..3f9d34a208 100755
> --- a/tests/vm/netbsd
> +++ b/tests/vm/netbsd
> @@ -23,8 +23,8 @@ class NetBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/rld1a;
>  ./configure --python=python2.7 {configure_opts};
> -gmake -j{jobs};
> -gmake check;
> +gmake -j{jobs} V={verbose};
> +gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/openbsd b/tests/vm/openbsd
> index 6ae16d97fd..3285c1abde 100755
> --- a/tests/vm/openbsd
> +++ b/tests/vm/openbsd
> @@ -23,9 +23,9 @@ class OpenBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/rsd1c;
>  ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 
> --python=python2.7 {configure_opts};
> -gmake -j{jobs};
> +gmake -j{jobs} V={verbose};
>  # XXX: "gmake check" seems to always hang or fail
> -#gmake check;
> +#gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
> index fc319e0e6e..b4eaa0dedc 100755
> --- a/tests/vm/ubuntu.i386
> +++ b/tests/vm/ubuntu.i386
> @@ -25,8 +25,8 @@ class UbuntuX86VM(basevm.BaseVM):
>  sudo chmod a+r /dev/vdb;
>  tar -xf /dev/vdb;
>  ./configure {configure_opts};
> -make -j{jobs};
> -make check;
> +make -j{jobs} V={verbose};
> +make check V={verbose};
>  """
>  
>  def _gen_cloud_init_iso(self):
> 

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Paolo Bonzini]

On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +static void *qpci_get_driver(void *obj, const char *interface)
>>  {
>> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +QPCIBusPC *qpci = obj;
>> +if (!g_strcmp0(interface, "pci-bus")) {
>> +return &qpci->bus;
>> +}
>> +printf("%s not present in pci-bus-pc", interface);
>> +abort();
>> +}
> 
> At this point I wonder if it makes sense to use the QEMU Object Model
> (QOM), which has interfaces and inheritance.  qgraph duplicates part of
> the object model.

Replying for Emanuele on this point since we didn't really go through
QOM yet; I'll let him answer the comments that are more related to the code.

QOM is much heavier weight than qgraph, and adds a lot more boilerplate:

- QOM properties interact with QAPI and bring in a lot of baggage;
qgraph would only use "child" properties to implement containment.

- QOM objects are long-lived, qgraph objects only last for the duration
of a single test.  qgraph doesn't need reference counting or complex
two-phase initialization like "realize" or "user_complete"

- QOM's hierarchy is shallow, but qgraph doesn't really need _any_
hierarchy.  Because it focuses on interface rather than hierarchy,
everything can be expressed simply through structs contained into one
another.

Consider that qgraph is 1/4th the size of QOM, and a large part of it is
the graph data structure that (as you said) would not be provided by QOM.

There are two things where using QOM would save a little bit of
duplicated concepts, namely the get_driver (get interface, what you
quote above) and get_device (get contained object) callbacks.  However,
it wouldn't simplify the code at all, because it would require the
introduction of separate instance and class structs.  We didn't want to
add all too much boilerplate for people that want to write qtest, as you
pointed out in the review of patch 4.

>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> 
> It's not clear to me what the purpose of this function is - at least the
> name is a bit cryptic since it seems more like an initialization
> function than 'setting pc' on QPCIBusPC.  How about inlining this in
> qpci_init_pc() instead of keeping a separate function?

I agree about the naming.  Perhaps we can rename the existing
qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.

Would you prefer if the split was done in the patch that introduces the
user for qpci_set_pc (patch 5)?  We did it here because this patch
prepares qpci-pc

Thanks,

Paolo

>> +{
>>  assert(qts);
>>  
>>  ret->bus.pio_readb = qpci_pc_pio_readb;
>> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
>> *alloc)
>>  ret->bus.mmio_alloc_ptr = 0xE000;
>>  ret->bus.mmio_limit = 0x1ULL;
>>  
>> +ret->obj.get_driver = qpci_get_driver;
>> +}
>> +
>> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +{
>> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +qpci_set_pc(ret, qts, alloc);
>> +
>>  return &ret->bus;
>>  }
>>  
>>  void qpci_free_pc(QPCIBus *bus)
>>  {
>> +if (!bus) {
>> +return;
>> +}
> 
> Why is this needed now?
> 
>> +
>>  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>>  
>>  g_free(s);
>> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, 
>> uint8_t slot)
>>  
>>  qmp_eventwait("DEVICE_DELETED");
>>  }
>> +
>> +static void qpci_pc(void)
>> +{
>> +qos_node_create_driver("pci-bus-pc", NULL);
>> +qos_node_produces("pci-bus-pc", "pci-bus");
> 
> In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
> a driver perspective it seems QOM can already do what is needed and the
> qgraph infrastructure isn't necessary.
> 
> Obviously the depth-first search *is* unique and not in QOM, although
> QOM does offer a tree namespace which can be used for looking up object
> instances and I guess this could be used to configure tests at runtime.
> 
> I'll think about this more as I read the rest of the patches.
> 
>> +}
>> +
>> +libqos_init(qpci_pc);
>> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
>> index 491eeac756..ee381c5667 100644
>> --- a/tests/libqos/pci-pc.h
>> +++ b/tests/libqos/pci-pc.h
>> @@ -15,7 +15,15 @@
>>  
>>  #include "libqos/pci.h"
>>  #include "libqos/malloc.h"
>> +#include "qgraph.h"
>>  
>> +typedef struct QPCIBusPC {
>> +QOSGraphObject obj;
>> +QPCIBus bus;
>> +} QPCIBusPC;
> 
> Why does this need to be public?
> 
>> +
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
> 
> Why does this need to be public?
> 
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> 
> Why does this need to be public?
> 
>>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>>  void qpci_free_p

[Qemu-devel] [Bug 1772075] Re: Segmentation fault on aarch64 vm at powerdown

[M0Rf30]

In order not to upload a big image I can say that you can generate the image 
with this tool
https://github.com/M0Rf30/simonpi
the initrd used is in the arch linux arm boot partition generated by the 
previous referenced tool.

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

Title:
  Segmentation fault on aarch64 vm at powerdown

Status in QEMU:
  New

Bug description:
  OS Arch Linux
  x86_64
  qemu version: 2.12

  cmdline:
  qemu-system-aarch64 -nographic -cpu cortex-a57 -m 2048 -M virt,gic_version=3 
-machine virtualization=true -bios /usr/share/ovmf/AARCH64/QEMU_EFI.fd -drive 
file=fat:rw:/opt/simonpiemu/kernels/rpi-3,if=none,format=raw,cache=none,id=hd0 
-device virtio-blk-device,drive=hd0 -drive 
file=/home/morfeo/.simonpi/sd-arch-rpi-3-qemu.img,if=none,format=raw,cache=none,id=hd1
 -device virtio-blk-device,drive=hd1 -kernel 
/opt/simonpiemu/kernels/rpi-3/Image -append "root=/dev/vda2 fstab=no 
rootfstype=ext4 rw console=ttyAMA0" -initrd 
/home/morfeo/.simonpi/rpi-3/boot/initramfs-linux.img -device 
virtio-net-device,mac=52:54:26:11:72:9b,netdev=net0 -netdev 
tap,id=net0,ifname=rasp-tap0,script=no,downscript=no

  error:

  qemu-system-aarch64: /build/qemu/src/qemu-2.12.0/block.c:3375:
  bdrv_close_all: Assertion `QTAILQ_EMPTY(&all_bdrv_states)' failed.

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

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Igor Mammedov]

On Tue, 10 Jul 2018 16:50:36 +0100
Stefan Hajnoczi  wrote:

> The ->pre_plug() callback is invoked before the device is realized.  The
> ->plug() callback is invoked when the device is being realized but  
> before it is reset.
> 
> This patch adds a ->post_plug() callback which is invoked after the
> device has been reset.  This callback is needed by HotplugHandlers that
> need to wait until after ->reset().
it seems other plug handlers would be also affected by this issue
if monitor lock wouldn't block them when guest tried to processes
hotplug notification and trapped back on MMIO happily waiting for
device_add to release lock before proceeding.

It also seems wrong to call _plug handler on maybe partially
initialized device so perhaps we should first finish devices/children
realization then do reset and only after that call _plug() handler

something like following should fix the issue:

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf0db4b..a80372d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -830,6 +830,18 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 goto fail;
 }
 
+QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+object_property_set_bool(OBJECT(bus), true, "realized",
+ &local_err);
+if (local_err != NULL) {
+goto child_realize_fail;
+}
+}
+
+if (dev->hotplugged) {
+device_reset(dev);
+}
+
 DEVICE_LISTENER_CALL(realize, Forward, dev);
 
 if (hotplug_ctrl) {
@@ -837,7 +849,7 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 }
 
 if (local_err != NULL) {
-goto post_realize_fail;
+goto child_realize_fail;
 }
 
 /*
@@ -852,20 +864,9 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
dev->instance_id_alias,
dev->alias_required_for_version,
&local_err) < 0) {
-goto post_realize_fail;
-}
-}
-
-QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-object_property_set_bool(OBJECT(bus), true, "realized",
- &local_err);
-if (local_err != NULL) {
 goto child_realize_fail;
 }
 }
-if (dev->hotplugged) {
-device_reset(dev);
-}
 dev->pending_deleted_event = false;
 } else if (!value && dev->realized) {
 Error **local_errp = NULL;
@@ -902,7 +903,6 @@ child_realize_fail:
 vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
 }
 
-post_realize_fail:
 g_free(dev->canonical_path);
 dev->canonical_path = NULL;
 if (dc->unrealize) {


> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/hotplug.h | 12 
>  hw/core/hotplug.c| 11 +++
>  hw/core/qdev.c   |  7 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 1a0516a479..1e8b1053b6 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @parent: Opaque parent interface.
>   * @pre_plug: pre plug callback called at start of device.realize(true)
>   * @plug: plug callback called at end of device.realize(true).
> + * @post_plug: post plug callback called after device.realize(true) and 
> device
> + * reset
>   * @unplug_request: unplug request callback.
>   *  Used as a means to initiate device unplug for devices 
> that
>   *  require asynchronous unplug handling.
> @@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass {
>  /*  */
>  hotplug_fn pre_plug;
>  hotplug_fn plug;
> +hotplug_fn post_plug;
>  hotplug_fn unplug_request;
>  hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -83,6 +86,15 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>DeviceState *plugged_dev,
>Error **errp);
>  
> +/**
> + * hotplug_handler_post_plug:
> + *
> + * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
> + */
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp);
> +
>  /**
>   * hotplug_handler_unplug_request:
>   *
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..ab34c19461 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>  }
>  }
>  
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +

Re: [Qemu-devel] [PATCH v2 4/4] tests: Add centos VM testing

[Philippe Mathieu-Daudé]

Hi Fam,

On 07/11/2018 11:18 AM, Fam Zheng wrote:
> This one does docker testing in the VM. It is intended to replace the
> native docker testing on patchew testers.
> 
> Signed-off-by: Fam Zheng 
> ---
>  tests/vm/Makefile.include |  2 +-
>  tests/vm/centos   | 84 +++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100755 tests/vm/centos
> 
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index 5daa2a3b73..e492cd73e5 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -2,7 +2,7 @@
>  
>  .PHONY: vm-build-all
>  
> -IMAGES := ubuntu.i386 freebsd netbsd openbsd
> +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos

You missed to add an entry to the vm-test rule.

>  IMAGE_FILES := $(patsubst %, tests/vm/%.img, $(IMAGES))
>  
>  .PRECIOUS: $(IMAGE_FILES)
> diff --git a/tests/vm/centos b/tests/vm/centos
> new file mode 100755
> index 00..afd560c564
> --- /dev/null
> +++ b/tests/vm/centos
> @@ -0,0 +1,84 @@
> +#!/usr/bin/env python
> +#
> +# CentOS image
> +#
> +# Copyright 2018 Red Hat Inc.
> +#
> +# Authors:
> +#  Fam Zheng 
> +#
> +# This code is licensed under the GPL version 2 or later.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import os
> +import sys
> +import subprocess
> +import basevm
> +import time
> +
> +class CentosVM(basevm.BaseVM):
> +name = "centos"
> +BUILD_SCRIPT = """
> +set -e;
> +cd $(mktemp -d);
> +export SRC_ARCHIVE=/dev/vdb;
> +sudo chmod a+r $SRC_ARCHIVE;
> +tar -xf $SRC_ARCHIVE;
> +make docker-test-block@centos7 V={verbose} J={jobs};
> +make docker-test-quick@centos7 V={verbose} J={jobs};
> +make docker-test-mingw@fedora V={verbose} J={jobs};
> +"""
> +
> +def _gen_cloud_init_iso(self):
> +cidir = self._tmpdir
> +mdata = open(os.path.join(cidir, "meta-data"), "w")
> +mdata.writelines(["instance-id: centos-vm-0\n",
> +  "local-hostname: centos-guest\n"])
> +mdata.close()
> +udata = open(os.path.join(cidir, "user-data"), "w")
> +udata.writelines(["#cloud-config\n",
> +  "chpasswd:\n",
> +  "  list: |\n",
> +  "root:%s\n" % self.ROOT_PASS,
> +  "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS),
> +  "  expire: False\n",
> +  "users:\n",
> +  "  - name: %s\n" % self.GUEST_USER,
> +  "sudo: ALL=(ALL) NOPASSWD:ALL\n",
> +  "ssh-authorized-keys:\n",
> +  "- %s\n" % basevm.SSH_PUB_KEY,
> +  "  - name: root\n",
> +  "ssh-authorized-keys:\n",
> +  "- %s\n" % basevm.SSH_PUB_KEY,
> +  "locale: en_US.UTF-8\n"])
> +udata.close()
> +subprocess.check_call(["genisoimage", "-output", "cloud-init.iso",
> +   "-volid", "cidata", "-joliet", "-rock",
> +   "user-data", "meta-data"],
> +   cwd=cidir,
> +   stdin=self._devnull, stdout=self._stdout,
> +   stderr=self._stdout)
> +return os.path.join(cidir, "cloud-init.iso")
> +
> +def build_image(self, img):
> +cimg = 
> self._download_with_cache("https://cloud.centos.org/centos/7/images/CentOS-7-x86_64-GenericCloud-1802.qcow2.xz";)
> +img_tmp = img + ".tmp"
> +subprocess.check_call(["cp", "-f", cimg, img_tmp + ".xz"])
> +subprocess.check_call(["xz", "-df", img_tmp + ".xz"])
> +subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
> +self.boot(img_tmp, extra_args = ["-cdrom", 
> self._gen_cloud_init_iso()])
> +self.wait_ssh()
> +self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
> +self.ssh_root_check("yum update -y")
> +self.ssh_root_check("yum install -y docker make git")
> +self.ssh_root_check("systemctl enable docker")
> +self.ssh_root("poweroff")
> +self.wait()
> +if os.path.exists(img):
> +os.remove(img)
> +os.rename(img_tmp, img)
> +return 0
> +
> +if __name__ == "__main__":
> +sys.exit(basevm.main(CentosVM))
> 

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> This work is being done as Google Summer of Code 2018 project for QEMU,
> my mentors are Paolo Bonzini and Laurent Vivier.
> Additional infos on the project can be found at:
> https://wiki.qemu.org/Features/qtest_driver_framework

Thanks, I've finished reviewing this version.  It looks like a good
start!

The main challenge to me seems "how can we make tests simpler?".  The
presence of a new API and object model raises the bar for writing and
running tests.  I hope all qtests will use qgraph but if the complexity
is too high then qgraph may only be used in a few niche cases where
authors think it's worth abstracting machine types and others will
continue to write qtests without qgraph.

What are the next steps and do you have plans for making qgraph more
integral to libqos?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Paolo Bonzini]

On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>> +machine->obj.get_device = raspi2_get_device;
>> +machine->obj.destructor = raspi2_destroy;
>> +qos_create_sdhci_mm(&machine->sdhci, 0x3f30, &(QSDHCIProperties) {
>> +.version = 3,
>> +.baseclock = 52,
>> +.capab.sdma = false,
>> +.capab.reg = 0x052134b4
>> +});
>> +return &machine->obj;
>> +}
>> +
>> +static void raspi2(void)
>> +{
>> +qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
>> +qos_node_contains("arm/raspi2", "generic-sdhci");
>> +}
>> +
>> +libqos_init(raspi2);
> Hmm...I didn't realize it would be necessary to manually define each
> machine type.  I thought qgraph would use qemu-system-x86_64
> -machine/-device/info qtree to introspect QEMU and instantiate the
> appropriate drivers.

That doesn't provide enough information yet.  However, PCI devices are
already discovered automatically and the next step (next year :)) could
be to use device tree or ACPI to discover embedded devices.

Using QOM properties instead of duplicating things like QSDHCIProperties
is a great idea.  Of course the duplication was already there ("old
sdhci_t structure" in patch 7), so I don't think it should be a blocker,
but indeed there's a better way to do it.

> My concern is that this manual approach of defining machines complicates
> qtests.  This file will need to be modified by multiple people - each of
> them will be interested in a specific driver and not interested in the
> driver that other people have added.

This is exactly the opposite problem that we have now: when you write a
test you are interested typically only in one machine, and the
"if(x86)elseif(arm)" gets in the way.  (Also: it's also difficult to
split the ifs across files, i.e. it scales worse; and there's lots of
duplicated code across tests to do "for" loops of g_test_add, because
the ifs don't lend itself to "automatic" generation of test cases via
path walk).

The advantage is that (just like for OS vs. QEMU) the structure of this
file matches closely the board files in hw/arm/, which is something you
should already be familiar with if you're adding a new board to QEMU.

Paolo

Re: [Qemu-devel] [PATCH for-3.0] pc: Use "3.0+" constant as default SMBIOS version

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 10:07:31AM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote:
> > Every time we create new PC machine-types in QEMU, the defaults
> > for SMBIOS fields change unnecessarily because the version field
> > defaults to MachineClass::name.
> > 
> > This can cause unexpected side-effects, like triggering license
> > reactivation on guest software, or changing the VM memory layout
> > because of BIOS table size changes.
> 
> Does that really matter though ? By its very nature the 'Version'
> field in SMBIOS is expected to change if you alter something about
> the hardware. If guests OS don't want to be exposed to changes in
> SMBIOS they would be using a fixed machine type, not the variable
> "pc" type that continually changes.
> 
> We could put padding in the string if we want to avoid BIOS table
> layout changes.
> 
> Having version change though feels like it is working as intended
> for the semantics of these Version: fields in BIOS.

Michael, do you have additional info on the original motivation
for suggesting this change and why do you consider it a bug?
(I don't have any concrete examples to justify the change)

-- 
Eduardo

[Qemu-devel] [Bug 1781211] Re: HAXM acceleration does not work at all.

[Dmitriy via Qemu-devel]

After some time I decided it is haxm bug - so i created the same issue
on haxm project too

https://github.com/intel/haxm/issues/74

** Bug watch added: github.com/intel/haxm/issues #74
   https://github.com/intel/haxm/issues/74

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

Title:
  HAXM acceleration does not work at all.

Status in QEMU:
  New

Bug description:
  I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does 
not works when i turn on hax acceleration. Permanent kernel panics, black 
screen freezing and other crashes happens when i run qemu.
  Qemu crashed with hax - when i ran it from iso. It crashed on already 
installed system - it's not matters. 

  Versions:
  archlinux-2018.07.01-x86_64
  ubuntu-18.04-live-server-amd64.iso

  I run qemu-system-x86_64.exe binary.

  My CPU:
  core i7 2600k

  See screenshot

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

Re: [Qemu-devel] [PATCH v2 2/2] vga: don't pick cirrus by default

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 12:26:52AM +0200, Sebastian Bauer wrote:
> Hi,
> 
> Am 2018-07-09 23:23, schrieb Eduardo Habkost:
> > List of machines with default_display==NULL on those
> > architectures:
> > 
> > alpha:
> > none empty machine
> > 
> > mips:
> > mipssim  MIPS MIPSsim platform
> > none empty machine
> > 
> > ppc*:
> > bamboo   bamboo
> > mpc8544dsmpc8544ds
> > none empty machine
> > powernv  IBM PowerNV (Non-Virtualized)
> > ppce500  generic paravirt e500 platform
> > ref405ep ref405ep
> > sam460ex aCube Sam460ex
> > taihutaihu
> > virtex-ml507 Xilinx Virtex ML507 reference design
> > 
> > x86_64:
> > isapcISA-only PC
> > none empty machine
> > xenfvXen Fully-virtualized PC
> > xenpvXen Para-virtualized PC
> 
> Which of these machines really require the Cirrus? The xen ones look like
> that they can deal with std. The isapc is should probably stay at the
> Cirrus.
> 
> Also the "none" seems to be a false-positive. I suppose they mean "empty",
> i.e., no graphics card at all?

"none" looked like a false positive when I first looked, but now
I think it's not.  Shouldn't it set default_display="none"?

> 
> And at least the ppc ones can be canceled out, they should work with std,
> the new default (expect the sam460ex which goes an own route for now).

If machines prefer "std", they should set default_display="std"
explicitly.

> 
> What is the indented target release for the patch?

I'm not convinced this patch is appropriate for 3.0.  If we have
remaining bugs they should be fixed by setting default_display
explicitly on the affected machines.

> 
> If the patch is applied to 3.1 then I think there is enough time to fix
> issues caused by the patch. Additionally, a warning could be put in the
> ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
> machines define an own default. This is should be enough time for people to
> complain or to fix things.

I don't think we will really make user-visible changes: we can
simply work to keep existing behavior, but the difference is that
this will be implemented by setting default_display explicitly on
all machines.

> 
> If the patch is to be applied to 3.0 then all non-ppc ones need to be
> reconsidered.
> 
> The "important" ppc machines have been fixed already. I can do the remaining
> if this is wanted.

This part worries me: do we have other machines that are broken
right now?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

Hi Phil,

On Wed, Jul 11, 2018 at 10:47:18AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Dima,
> 
> On 07/11/2018 05:34 AM, Dima Stepanov wrote:
> > Gentle ping. CCing Paolo Bonzini.
> > 
> > Regards, Dima.
> > 
> > On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
> >> Ping.
> >>
> >> Regards, Dima.
> >>
> >> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
> >>> In the memory_region_do_invalidate_mmio_ptr() routine the section
> >>> variable is intialized by the memory_region_find() call. The section.mr
> >>> field can be set to NULL.
> >>>
> >>> Add the check for NULL before trying to drop a section.
> >>>
> >>> Signed-off-by: Dima Stepanov 
> >>> ---
> >>>  memory.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/memory.c b/memory.c
> >>> index 3212acc..bb45248 100644
> >>> --- a/memory.c
> >>> +++ b/memory.c
> >>> @@ -2712,7 +2712,7 @@ static void 
> >>> memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
> >>>  /* Reset dirty so this doesn't happen later. */
> >>>  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> >>>  
> >>> -if (section.mr != mr) {
> >>> +if (section.mr && (section.mr != mr)) {
> 
> section.mr can't be NULL here.
> 
> You can give the static analyzer a hint using "assert(section.mr);"

My point was:
  section = memory_region_find(mr, offset, size) ->
ret = memory_region_find_rcu(mr, addr, size); ->
  MemoryRegionSection ret = { .mr = NULL };
  ...
  as = memory_region_to_address_space(root);
  if this call returns NULL, then the ret value will be returned
  (with .mr == NULL)
But maybe it is impossible for this case. Thanks for the reply.

Regards, Dima.

> 
> >>>  /* memory_region_find add a ref on section.mr */
> >>>  memory_region_unref(section.mr);
> >>>  if (MMIO_INTERFACE(section.mr->owner)) {
> >>> -- 
> >>> 2.7.4
> >>>
> 
> Regards,
> 
> Phil.
> 

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 06:21:09PM +0300, Viktor Prutyanov wrote:
> This patch adds field with content of KERNEL_GS_BASE MSR to QEMU note in
> ELF dump.
> 
> On Windows, if all vCPUs are running usermode tasks at the time the dump is
> created, this can be helpful in the discovery of guest system structures
> during conversion ELF dump to MEMORY.DMP dump.
> 
> Signed-off-by: Viktor Prutyanov 
> ---
>  target/i386/arch_dump.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index 35b55fc..a702138 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -237,7 +237,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>   * please count up QEMUCPUSTATE_VERSION if you have changed definition of
>   * QEMUCPUState, and modify the tools using this information accordingly.

Where are the tools using this information, that need to be
updated?  Won't this break existing versions of those tools?

Is the dump format and pointers to available tools documented
somewhere?


>   */
> -#define QEMUCPUSTATE_VERSION (1)
> +#define QEMUCPUSTATE_VERSION (2)
>  
>  struct QEMUCPUSegment {
>  uint32_t selector;
> @@ -258,6 +258,7 @@ struct QEMUCPUState {
>  QEMUCPUSegment cs, ds, es, fs, gs, ss;
>  QEMUCPUSegment ldt, tr, gdt, idt;
>  uint64_t cr[5];
> +uint64_t kernel_gs_base;
>  };
>  
>  typedef struct QEMUCPUState QEMUCPUState;
> @@ -315,6 +316,8 @@ static void qemu_get_cpustate(QEMUCPUState *s, 
> CPUX86State *env)
>  s->cr[2] = env->cr[2];
>  s->cr[3] = env->cr[3];
>  s->cr[4] = env->cr[4];
> +
> +s->kernel_gs_base = env->kernelgsbase;
>  }
>  
>  static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

On Wed, Jul 11, 2018 at 03:09:13PM +0100, Peter Maydell wrote:
> On 11 July 2018 at 14:47, Philippe Mathieu-Daudé  wrote:
> > Hi Dima,
> >
> > On 07/11/2018 05:34 AM, Dima Stepanov wrote:
> >> Gentle ping. CCing Paolo Bonzini.
> >>
> >> Regards, Dima.
> >>
> >> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
> >>> Ping.
> >>>
> >>> Regards, Dima.
> >>>
> >>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
>  In the memory_region_do_invalidate_mmio_ptr() routine the section
>  variable is intialized by the memory_region_find() call. The section.mr
>  field can be set to NULL.
> 
>  Add the check for NULL before trying to drop a section.
> 
>  Signed-off-by: Dima Stepanov 
>  ---
>   memory.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
>  diff --git a/memory.c b/memory.c
>  index 3212acc..bb45248 100644
>  --- a/memory.c
>  +++ b/memory.c
>  @@ -2712,7 +2712,7 @@ static void 
>  memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
>   /* Reset dirty so this doesn't happen later. */
>   cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> 
>  -if (section.mr != mr) {
>  +if (section.mr && (section.mr != mr)) {
> >
> > section.mr can't be NULL here.
> >
> > You can give the static analyzer a hint using "assert(section.mr);"
> 
> Not in my view much point in messing with this code, though:
> (a) it's broken and unusable as it stands (race conditions)
> (b) it's obsoleted by the execute-from-mmio patchset
> http://patchwork.ozlabs.org/cover/942090/ and if/when that
> goes in it will all just get deleted.


Got it.

Thanks, Dima.

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 09:21:48AM +0200, Thomas Huth wrote:
> On 10.07.2018 08:50, Peter Maydell wrote:
> > On 9 July 2018 at 23:03, Thomas Huth  wrote:
> >> On 09.07.2018 23:42, Peter Maydell wrote:
> >>> On 9 July 2018 at 22:03, Thomas Huth  wrote:
>  When trying to "device_add bcm2837" on a machine that is not suitable for
>  this device, you can quickly crash QEMU afterwards, e.g. with "info 
>  qtree":
> 
>  echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>   "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', 
>  " \
>   "'arguments': {'command-line': 'info qtree'}}" | \
>   aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp 
>  stdio
> 
>  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>   "package": "build-all"}, "capabilities": []}}
>  {"return": {}}
>  {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>   hotplugged on this machine"}}
>  Segmentation fault (core dumped)
> 
>  The problem is that qdev_set_parent_bus() from instance_init adds a link
>  to the child devices which is not valid anymore after the device init
>  failed. Thus the qdev_set_parent_bus() must rather be done in the realize
>  function instead.
> >>>
> >>> Yuck. The real problem here is that we're still requiring the
> >>> code that creates these QOM devices to manually set the parent
> >>> in the first place. It's not surprising that we don't get it right
> >>> (either parenting in the wrong place or not at all). I'd much
> >>> rather see us fix that properly than keep papering over places
> >>> where we get it wrong.
> >>
> >> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
> >> you exactly recommend to do instead?
> > 
> > I'm not clear either, but I don't think that what we're
> > currently doing can be right.
> 
> Hm, ok, so how to continue here now? Shall we at least mark the
> bcm2836/7 devices with user_creatable=false, so that users can not crash
> their QEMU so easily with device_add? The problem with introspection via
> device-list-properties would still continue to exist, but I think that's
> less likely used in practice... otherwise we could still move the
> qdev_set_parent_bus() calls to the realize() function instead, and just
> add a big fat FIXME comment in front of the code block, so that we
> remember to clean that up one day...

Crashing device-list-properties should be a blocker bug, IMO.

Moving to realize is not the best solution, but I would prefer to
do that in 3.0 instead of leaving the device-list-properties
crash unfixed.

Another solution is to reintroduce
DeviceClass::cannot_destroy_with_object_finalize_yet (commit
08f00df4f4b8b4e38ad620477cc90cf5f73832d9), and set
cannot_destroy_with_object_finalize_yet=true on bcm2837.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Paolo Bonzini]

On 11/07/2018 18:00, Eduardo Habkost wrote:
>> @@ -237,7 +237,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
>> CPUState *cs,
>>   * please count up QEMUCPUSTATE_VERSION if you have changed definition of
>>   * QEMUCPUState, and modify the tools using this information accordingly.
> Where are the tools using this information, that need to be
> updated?  Won't this break existing versions of those tools?

I think it's okay to _not_ change the version, since the format is
backwards-compatible.  Each QEMUCPUState struct is in a separate ELF
note, and the presence of the new field is visible in both 1) the size
of the note 2) the size field of the struct.

Another possibility is to stash kernel_gs_base in cr[1].  This approach
doesn't scale, but the word is otherwise unused if we want to make it
super safe.  I don't recommend it.

Paolo


> Is the dump format and pointers to available tools documented
> somewhere?
> 
> 

[Qemu-devel] [Bug 1772075] Re: Segmentation fault on aarch64 vm at powerdown

[Peter Maydell]

As I said, I don't want to have to deal with image generation tools and
extracting initrds from disk images. The easiest thing for me is if you
can just provide all the files and the command line I can use to
reproduce.

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

Title:
  Segmentation fault on aarch64 vm at powerdown

Status in QEMU:
  New

Bug description:
  OS Arch Linux
  x86_64
  qemu version: 2.12

  cmdline:
  qemu-system-aarch64 -nographic -cpu cortex-a57 -m 2048 -M virt,gic_version=3 
-machine virtualization=true -bios /usr/share/ovmf/AARCH64/QEMU_EFI.fd -drive 
file=fat:rw:/opt/simonpiemu/kernels/rpi-3,if=none,format=raw,cache=none,id=hd0 
-device virtio-blk-device,drive=hd0 -drive 
file=/home/morfeo/.simonpi/sd-arch-rpi-3-qemu.img,if=none,format=raw,cache=none,id=hd1
 -device virtio-blk-device,drive=hd1 -kernel 
/opt/simonpiemu/kernels/rpi-3/Image -append "root=/dev/vda2 fstab=no 
rootfstype=ext4 rw console=ttyAMA0" -initrd 
/home/morfeo/.simonpi/rpi-3/boot/initramfs-linux.img -device 
virtio-net-device,mac=52:54:26:11:72:9b,netdev=net0 -netdev 
tap,id=net0,ifname=rasp-tap0,script=no,downscript=no

  error:

  qemu-system-aarch64: /build/qemu/src/qemu-2.12.0/block.c:3375:
  bdrv_close_all: Assertion `QTAILQ_EMPTY(&all_bdrv_states)' failed.

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

Re: [Qemu-devel] [PATCH v6 4/4] acpi: build TPM Physical Presence interface

[Igor Mammedov]

On Wed, 4 Jul 2018 18:00:41 +0200
Marc-André Lureau  wrote:

> HI
> 
> On Wed, Jul 4, 2018 at 5:39 PM, Igor Mammedov  wrote:
> > On Thu, 28 Jun 2018 19:26:57 +0200
> > Marc-André Lureau  wrote:
> >  
> >> From: Stefan Berger 
> >>
> >> The TPM Physical Presence interface consists of an ACPI part, a shared
> >> memory part, and code in the firmware. Users can send messages to the
> >> firmware by writing a code into the shared memory through invoking the
> >> ACPI code. When a reboot happens, the firmware looks for the code and
> >> acts on it by sending sequences of commands to the TPM.
> >>
> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't
> >> assume that SMIs are necessary to use. It uses a similar datastructure for
> >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use
> >> of it. I extended the shared memory data structure with an array of 256
> >> bytes, one for each code that could be implemented. The array contains
> >> flags describing the individual codes. This decouples the ACPI 
> >> implementation
> >> from the firmware implementation.
> >>
> >> The underlying TCG specification is accessible from the following page.
> >>
> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
> >>
> >> This patch implements version 1.30.
> >>
> >> Signed-off-by: Stefan Berger 
> >> [ Marc-André - ACPI code improvements and windows fixes ]
> >> Signed-off-by: Marc-André Lureau 
> >>
> >> ---
> >>
> >> v7: (Marc-André)
> >>  - use first spec version/section in code comments
> >>  - use more variables for ASL code building
> >>  - remove unnecessering ToInteger() calls
> >>
> >> v6:
> >>  - more code documentation (Marc-André)
> >>  - use some explicit named variables to ease reading (Marc-André)
> >>  - use fixed size fields/memory regions, remove PPI struct (Marc-André)
> >>  - only add PPI ACPI methods if PPI is enabled (Marc-André)
> >>  - document the qemu/firmware ACPI memory region (Stefan)
> >>
> >> v5 (Marc-André):
> >>  - /struct tpm_ppi/struct TPMPPIData
> >>
> >> v4 (Marc-André):
> >>  - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI
> >> handling.
> >>  - replace 'return Package (..) {} ' with scoped variables, to fix
> >>Windows ACPI handling.
> >>
> >> v3:
> >>  - add support for PPI to CRB
> >>  - split up OperationRegion TPPI into two parts, one containing
> >>the registers (TPP1) and the other one the flags (TPP2); switched
> >>the order of the flags versus registers in the code
> >>  - adapted ACPI code to small changes to the array of flags where
> >>previous flag 0 was removed and now shifting right wasn't always
> >>necessary anymore
> >>
> >> v2:
> >>  - get rid of FAIL variable; function 5 was using it and always
> >>returns 0; the value is related to the ACPI function call not
> >>a possible failure of the TPM function call.
> >>  - extend shared memory data structure with per-opcode entries
> >>holding flags and use those flags to determine what to return
> >>to caller
> >>  - implement interface version 1.3
> >> ---
> >>  include/hw/acpi/tpm.h |   8 +
> >>  hw/i386/acpi-build.c  | 403 +-
> >>  docs/specs/tpm.txt|  79 +
> >>  3 files changed, 487 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index f79d68a77a..e0bd07862e 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -196,4 +196,12 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>  #define TPM_PPI_VERSION_NONE0
> >>  #define TPM_PPI_VERSION_1_301
> >>
> >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */
> >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0)
> >> +#define TPM_PPI_FUNC_BIOS_ONLY   (1 << 0)
> >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0)
> >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0)
> >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0)
> >> +#define TPM_PPI_FUNC_MASK(7 << 0)
> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index ebd64c4818..263677f3e4 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -43,6 +43,7 @@
> >>  #include "hw/acpi/memory_hotplug.h"
> >>  #include "sysemu/tpm.h"
> >>  #include "hw/acpi/tpm.h"
> >> +#include "hw/tpm/tpm_ppi.h"
> >>  #include "hw/acpi/vmgenid.h"
> >>  #include "sysemu/tpm_backend.h"
> >>  #include "hw/timer/mc146818rtc_regs.h"
> >> @@ -1789,6 +1790,396 @@ static Aml *build_q35_osc_method(void)
> >>  return method;
> >>  }
> >>
> >> +static void
> >> +build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >> +{
> >> +Aml *method, *field, *ifctx, *ifctx2, *ifctx3;
> >> +Aml *pak, *tpm2, *tpm3, *pprm, *pprq;
> >> +int i;
> >> +
> >> +if (!object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> >> +return;
> >> +}  
> > I'd prefer th

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Viktor Prutyanov]

On Wed, 11 Jul 2018 13:00:25 -0300
Eduardo Habkost  wrote:

> On Tue, Jul 10, 2018 at 06:21:09PM +0300, Viktor Prutyanov wrote:
> > This patch adds field with content of KERNEL_GS_BASE MSR to QEMU
> > note in ELF dump.
> > 
> > On Windows, if all vCPUs are running usermode tasks at the time the
> > dump is created, this can be helpful in the discovery of guest
> > system structures during conversion ELF dump to MEMORY.DMP dump.
> > 
> > Signed-off-by: Viktor Prutyanov 
> > ---
> >  target/i386/arch_dump.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> > index 35b55fc..a702138 100644
> > --- a/target/i386/arch_dump.c
> > +++ b/target/i386/arch_dump.c
> > @@ -237,7 +237,7 @@ int
> > x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> >   * please count up QEMUCPUSTATE_VERSION if you have changed
> > definition of
> >   * QEMUCPUState, and modify the tools using this information
> > accordingly.  
> 
> Where are the tools using this information, that need to be
> updated?  Won't this break existing versions of those tools?
> 
> Is the dump format and pointers to available tools documented
> somewhere?

I hope that someone from community knows about those tools because I
can't find such tools.

> 
> >   */
> > -#define QEMUCPUSTATE_VERSION (1)
> > +#define QEMUCPUSTATE_VERSION (2)
> >  
> >  struct QEMUCPUSegment {
> >  uint32_t selector;
> > @@ -258,6 +258,7 @@ struct QEMUCPUState {
> >  QEMUCPUSegment cs, ds, es, fs, gs, ss;
> >  QEMUCPUSegment ldt, tr, gdt, idt;
> >  uint64_t cr[5];
> > +uint64_t kernel_gs_base;
> >  };
> >  
> >  typedef struct QEMUCPUState QEMUCPUState;
> > @@ -315,6 +316,8 @@ static void qemu_get_cpustate(QEMUCPUState *s,
> > CPUX86State *env) s->cr[2] = env->cr[2];
> >  s->cr[3] = env->cr[3];
> >  s->cr[4] = env->cr[4];
> > +
> > +s->kernel_gs_base = env->kernelgsbase;
> >  }
> >  
> >  static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Paolo Bonzini]

On 11/07/2018 18:26, Viktor Prutyanov wrote:
>> Where are the tools using this information, that need to be
>> updated?  Won't this break existing versions of those tools?
>>
>> Is the dump format and pointers to available tools documented
>> somewhere?
> I hope that someone from community knows about those tools because I
> can't find such tools.
> 

I checked crash, and it doesn't have any issue with QEMUCPUState that is
bigger than what is currently in QEMU.  It doesn't use anything but CR3
and IDT base.  It also doesn't at all check the version! :O

Paolo

[Qemu-devel] [PATCH] linux-user: fix mmap_find_vma_reserved()

[Laurent Vivier]

The value given by mmap_find_vma_reserved() is used with mmap(),
so it is needed to be aligned with the host page size.

Since commit 18e80c55bb, reserved_va is only aligned to TARGET_PAGE_SIZE,
and it works well if this size is greater or equal to the host page size.

But ppc64 hosts have 64kB page size and when we start a 4kiB page size
guest (like i386), it fails when it tries to mmap the stack:

mmap stack: Invalid argument

Fixes: 18e80c55bb (linux-user: Tidy and enforce reserved_va initialization)
Signed-off-by: Laurent Vivier 
---
 linux-user/main.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 52b5a618fe..a370b89ee6 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -689,6 +689,11 @@ int main(int argc, char **argv, char **envp)
 target_environ = envlist_to_environ(envlist, NULL);
 envlist_free(envlist);
 
+/* reserved_va must be aligned with the host page size
+ * has it is used with mmap()
+ */
+reserved_va &= qemu_host_page_mask;
+
 /*
  * Now that page sizes are configured in tcg_exec_init() we can do
  * proper page alignment for guest_base.
-- 
2.17.1