Re: [PULL v2 00/20] tcg patch queue

[Richard Henderson]

On 9/5/22 22:58, Stefan Hajnoczi wrote:

The tsan (clang) build is broken:
https://gitlab.com/qemu-project/qemu/-/jobs/2982480773

clang-10 -m64 -mcx16 -Ilibqemu-x86_64-linux-user.fa.p -I. -I..
-Itarget/i386 -I../target/i386 -I../common-user/host/x86_64
-I../linux-user/include/host/x86_64 -I../linux-user/include
-Ilinux-user -I../linux-user -Ilinux-user/x86_64
-I../linux-user/x86_64 -Iqapi -Itrace -Iui -Iui/shader
-I/usr/include/capstone -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fcolor-diagnostics -Wall
-Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
/builds/qemu-project/qemu/linux-headers -isystem linux-headers -iquote
. -iquote /builds/qemu-project/qemu -iquote
/builds/qemu-project/qemu/include -iquote
/builds/qemu-project/qemu/tcg/i386 -pthread -fsanitize=thread
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides
-Wno-missing-include-dirs -Wno-shift-negative-value
-Wno-string-plus-int -Wno-typedef-redefinition
-Wno-tautological-type-limit-compare -fstack-protector-strong -fPIE
-isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H
'-DCONFIG_TARGET="x86_64-linux-user-config-target.h"'
'-DCONFIG_DEVICES="x86_64-linux-user-config-devices.h"' -MD -MQ
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o -MF
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o.d -o
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o -c
../linux-user/elfload.c
../linux-user/elfload.c:198:18: error: integer overflow in
preprocessor expression [-Werror]
#if ULONG_MAX >= TARGET_VSYSCALL_PAGE
^~~~
../target/i386/cpu.h:2386:47: note: expanded from macro 'TARGET_VSYSCALL_PAGE'
# define TARGET_VSYSCALL_PAGE (UINT64_C(-10) << 20)
~ ^ ~~


Arg!  This is a compiler bug.


r~

Re: [PATCH v2 00/11] Introduce new acpi/smbios python tests using biosbits

[Ani Sinha]

On Thu, Jul 14, 2022 at 6:54 PM Peter Maydell  wrote:
>
> On Mon, 11 Jul 2022 at 10:34, Michael S. Tsirkin  wrote:
> >
> > On Sun, Jul 10, 2022 at 10:30:03PM +0530, Ani Sinha wrote:
> > > Changelog:
> > > v2:
> > >  - a new class of python based tests introduced that is separate from 
> > > avocado
> > >tests or qtests. Can be run by using "make check-pytest".
> > >  - acpi biosbits tests are the first tests to use pytest environment.
> > >  - bios bits tests now download the bits binary archives from a remote
> > >repository if they are not found locally. The test skips if download
> > >fails.
> > >  - A new environment variable is introduced that can be passed by the 
> > > tester
> > >to specify the location of the bits archives locally. test skips if the
> > >bits binaries are not found in that location.
> > >  - if pip install of python module fails for whatever reaoson, the test 
> > > skips.
> > >  - misc code fixes including spell check of the README doc. README has 
> > > been
> > >updated as well.
> > >  - addition of SPDX license headers to bits test files.
> > >  - update MAINTAINERS to reflect the new pytest test class.
> > >
> > > For biosbits repo:
> > >  - added Dockerfile and build script. Made bios bits build on gcc 11.
> > >https://github.com/ani-sinha/bits/blob/bits-qemu-logging/Dockerfile
> > >
> > > https://github.com/ani-sinha/bits/blob/bits-qemu-logging/build-artifacts.sh
> > >The build script generates the zip archive and tarball used by the 
> > > test.
> >
> > So far so good, I think it's ok for a start. It's probably a good idea
> > to host the source on qemu.org. Peter - any objection to this?
>
> Dan was looking at v1 from the point of view of how we handle the
> guest binary blobs for these tests -- I'd rather defer to him rather
> than taking the time to get up to speed on the issue myself.

Ok let's resurrect this discussion again. What are we going to do with
bios bits? Put it in git.qemu.org then?
I have put a lot of time and effort into this work and I believe this
will add another valuable tool to test acpi stuff, so I am not going
away :-)

Re: [PATCH] tests: unit: add NULL-pointer check

[Paolo Bonzini]

Il mar 6 set 2022, 07:01 Markus Armbruster  ha scritto:

> Next, permit me a few words on writing tests.  For me, a unit test fails
> by crashing.  Crashing with a nice message is optional.  The more likely
> the failure, the more useful is niceness.  Complete niceness is
> impossible --- if we could predict all crashes, we wouldn't need tests.
> Trying to push niceness can be overly verbose.  Thus, judgement calls,
> and matters of taste.
>

I agree; however, *relying* on a crash for correctness of the test is not
great. Part of the test here is checking that an empty qdict_crumple
returns a dictionary and not, say, a list. The newly-added assertion avoids
that two wrongs end up making a right: if qobject_check_type somehow failed
to identify the dictionary and returned (QDict *) obj, qdict_size would not
crash.

Unlikely as it is, it's nicer to spell out the postconditions that the test
is checking.

Paolo



> Wanting to mollify Coverity is a valid argument.
>
>

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Nico Boehr]

Quoting Pierre Morel (2022-09-02 09:55:23)
[...]
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 00..a6ca006ec5
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
[...]
> +void s390_topology_new_cpu(int core_id)
> +{
[...]
> +socket_id = core_id / topo->cores;

The comment below is essential for understanding all of this. Move it before 
this line.

> +
> +bit = core_id;
> +origin = bit / 64;
> +bit %= 64;
> +bit = 63 - bit;
> +
> +/*
> + * At the core level, each CPU is represented by a bit in a 64bit
> + * unsigned long. Set on plug and clear on unplug of a CPU.

cleared  ^

[...]
> + * In that case the origin field, representing the offset of the first 
> CPU
> + * in the CPU container allows to represent up to the maximal number of
> + * CPU inside several CPU containers inside the socket container.

How about:
"In that case the origin variable represents the offset of the first CPU in the
CPU container. More than 64 CPUs per socket are represented in several CPU
containers inside the socket container."

> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b5ca154e2f..15cefd104b 100644
[...]
> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>  /* init memory + setup max page size. Required for the CPU model */
>  s390_memory_init(machine->ram);
>  
> +/* Adding the topology must be done before CPU intialization*/

space  ^

Re: [PATCH] tests: unit: simplify test-visitor-serialization list tests

[Stefan Weil via]

Am 05.09.22 um 13:00 schrieb Paolo Bonzini:

test-visitor-serialization list tests is using an "if" to pick either the first
element of the list or the next one.  This was done presumably to mimic the
code that creates the list, which has to fill in either the head pointer
or the next pointer of the last element.  However, the code in the insert
phase is a pretty standard singly-linked list insertion, while the one
in the visit phase looks weird and even looks at the first item twice:
this is confusing because the test puts in 32 items and finishes with
an assertion that i == 33.

So, move the "else" step in a separate switch statement, and change
the do...while loop to a while, because cur_head has already been
initialized beforehand.

Signed-off-by: Paolo Bonzini 
---
  tests/unit/test-visitor-serialization.c | 157 +++-
  1 file changed, 69 insertions(+), 88 deletions(-)

diff --git a/tests/unit/test-visitor-serialization.c 
b/tests/unit/test-visitor-serialization.c
index 907263d030..667e8fed82 100644
--- a/tests/unit/test-visitor-serialization.c
+++ b/tests/unit/test-visitor-serialization.c
@@ -427,131 +427,117 @@ static void test_primitive_lists(gconstpointer opaque)
  ops->deserialize((void **)&pl_copy_ptr, serialize_data,
   visit_primitive_list, &error_abort);
  
-i = 0;

+
+switch (pl_copy.type) {

[...]> +default:

+g_assert_not_reached();
+}
  
  /* compare our deserialized list of primitives to the original */

-do {
+i = 0;
+while (cur_head) {
  switch (pl_copy.type) {
  case PTYPE_STRING: {

[...]

@@ -578,9 +559,9 @@ static void test_primitive_lists(gconstpointer opaque)
  g_assert_not_reached();


As both switch statements have the same 12 cases plus a default case 
with g_assert_not_reached(), a static code analyzer might complain that 
the 2nd default case will indeed never be reached because the first one 
already raises an assertion. So the code in the 2nd default case could 
be removed.


Regards,
Stefan


  }
  i++;
-} while (cur_head);
+}





OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v3 13/15] virtio-net: support queue reset

[Jason Wang]

On Tue, Sep 6, 2022 at 12:25 AM Kangjie Xu  wrote:
>
>
> 在 2022/9/5 16:30, Jason Wang 写道:
> >
> > 在 2022/8/25 16:08, Kangjie Xu 写道:
> >> From: Xuan Zhuo 
> >>
> >> virtio-net and vhost-kernel implement queue reset.
> >> Queued packets in the corresponding queue pair are flushed
> >> or purged.
> >>
> >> Signed-off-by: Xuan Zhuo 
> >> Signed-off-by: Kangjie Xu 
> >> ---
> >>   hw/net/virtio-net.c | 18 ++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 27b59c0ad6..d774a3e652 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -540,6 +540,23 @@ static RxFilterInfo
> >> *virtio_net_query_rxfilter(NetClientState *nc)
> >>   return info;
> >>   }
> >>   +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
> >> queue_index)
> >> +{
> >> +VirtIONet *n = VIRTIO_NET(vdev);
> >> +NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >> +
> >> +if (!nc->peer) {
> >> +return;
> >> +}
> >> +
> >> +if (get_vhost_net(nc->peer) &&
> >> +nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
> >> +vhost_net_virtqueue_reset(vdev, nc, queue_index);
> >> +}
> >> +
> >> +flush_or_purge_queued_packets(nc);
> >
> >
> > But the codes doesn't prevent the usersapce datapath from being used?
> > (e.g vhost=off)
> >
> > E.g vhost_net_start_one() had:
> >
> > if (net->nc->info->poll) {
> > net->nc->info->poll(net->nc, false);
> > }
> I review this part in vhost_net_start/stop_one().
>
> I didn't take the usersapce datapath into consideration.

Because it's used for vhost-net. So I think maybe we should first
implement the rest in the userspace datapath first then try to
implement them in the vhost?

> If we don't
> prevent it, the userspace datapath may access it and causes some
> problems. From this point, we should disable it.
>
> But if we add net->nc->info->poll in vq reset, it can only operate at
> queue-pair level, which obeys "per-virtqueue".
>
> What's your opinion on this point? I think it's ok to add it in vq reset.

See above, I'd suggest to implement the vq reset in qemu usersapce
datapath first. Then we can do vhost part on top.

>
> >
> > And I will wonder if it's better to consider to:
> >
> > 1) factor out the per virtqueue start/stop from
> > vhost_net_start/stop_one()
> >
> > 2) simply use the helper factored out via step 1)
> >
> > Thanks
> >
> I have a different idea about it, vhost_virtqueue_start/stop()(in
> hw/virtio/vhost.c) are already good abstractions of per virtqueue
> start/stop.
>
> These two functions are used in vhost_dev_start. It's just because
> vhost-net needs some configuration, so we add net->nc->info->poll and
> set_backend for it. But for other devices using vhost_dev_start,
> set_backend and net->nc->info->poll may be not necessary.
>
> I think your apporach to abstract per virtqueue start/stop from
> vhost_net_start/stop_one() will break the generality of
> vhost_dev_start(), which is a common interface for different devices.
>
> To conclude my opinions
>
> 1. I think we need to add net->nc->info->poll in our
> vhost_net_virtqueue_reset() and vhost_net_virtqueue_restart()
>
> 2. We still need vhost_net_virtqueue_reset() and
> vhost_net_virtqueue_restart().
>
>  a. vhost_net_virtqueue_reset() is a wrapper for vhost_virtqueue_stop().
>
>  b. vhost_net_virtqueue_restart() is a wrapper for
> vhost_virtqueue_start().

Sounds good, let's do and see.

Thanks

>
> Thanks
>
> >
> >> +}
> >> +
> >>   static void virtio_net_reset(VirtIODevice *vdev)
> >>   {
> >>   VirtIONet *n = VIRTIO_NET(vdev);
> >> @@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass
> >> *klass, void *data)
> >>   vdc->set_features = virtio_net_set_features;
> >>   vdc->bad_features = virtio_net_bad_features;
> >>   vdc->reset = virtio_net_reset;
> >> +vdc->queue_reset = virtio_net_queue_reset;
> >>   vdc->set_status = virtio_net_set_status;
> >>   vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >>   vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>

Re: [PATCH v3 10/15] vhost-net: vhost-kernel: introduce vhost_net_virtqueue_reset()

[Jason Wang]

On Mon, Sep 5, 2022 at 6:15 PM Kangjie Xu  wrote:
>
>
> 在 2022/9/5 16:03, Jason Wang 写道:
> >
> > 在 2022/8/25 16:08, Kangjie Xu 写道:
> >> Introduce vhost_virtqueue_reset(), which can reset the specific
> >> virtqueue in the device. Then it will unmap vrings and the desc
> >> of the virtqueue.
> >>
> >> Here we do not reuse the vhost_net_stop_one() or vhost_dev_stop(),
> >> because they work at queue pair level. We do not use
> >> vhost_virtqueue_stop() because it may stop the device in the
> >> backend.
> >
> >
> > So I think this is not true at least for vhost-net kernel baceknd.
> >
> >
> But vhost-user(OVS-DPDK) will stop the device.

Yes, what I meant is that, considering this series only deal with
vhost-net. We can leave any "fix" or "workaround" until e.g the
vhost-user support is posted?

>
> When DPDK vhost received VHOST_USER_GET_VRING_BASE message, it will call
> vhost_destroy_device_notify() to destroy the device.

Right, actually this trick is used even in some hardware.

>
> It seems like it is a inconsistency error in DPDK. Maybe I should submit
> a patch to DPDK. We can stop the device only when all the virtqueues in
> one device are destroyed.

That would be fine.

>
> >>
> >> This patch only considers the case of vhost-kernel, when
> >> NetClientDriver is NET_CLIENT_DRIVER_TAP.
> >>
> >> Signed-off-by: Kangjie Xu 
> >> Signed-off-by: Xuan Zhuo 
> >> ---
> >>   hw/net/vhost_net.c  | 22 ++
> >>   include/net/vhost_net.h |  2 ++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index ccac5b7a64..be51be98b3 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -514,3 +514,25 @@ int vhost_net_set_mtu(struct vhost_net *net,
> >> uint16_t mtu)
> >> return vhost_ops->vhost_net_set_mtu(&net->dev, mtu);
> >>   }
> >> +
> >> +void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
> >> +   int vq_index)
> >> +{
> >> +VHostNetState *net = get_vhost_net(nc->peer);
> >> +const VhostOps *vhost_ops = net->dev.vhost_ops;
> >> +struct vhost_vring_file file = { .fd = -1 };
> >> +int idx;
> >> +
> >> +/* should only be called after backend is connected */
> >> +assert(vhost_ops);
> >> +
> >> +idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> >> +
> >> +if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> >> +file.index = idx;
> >> +int r = vhost_net_set_backend(&net->dev, &file);
> >> +assert(r >= 0);
> >> +}
> >
> >
> > Do we need to reset e.g last_avail_idx here?
> >
> > Thanks
> >
> I did not reset it because we will re-configure them when we restart
> virtqueue.

Is last_avail_idx reset to 0 in this case?

Thanks

>
> Thanks
>
> >
> >> +
> >> +vhost_virtqueue_unmap(&net->dev, vdev, net->dev.vqs + idx, idx);
> >> +}
> >> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> >> index 387e913e4e..85d85a4957 100644
> >> --- a/include/net/vhost_net.h
> >> +++ b/include/net/vhost_net.h
> >> @@ -48,4 +48,6 @@ uint64_t vhost_net_get_acked_features(VHostNetState
> >> *net);
> >> int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
> >>   +void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState
> >> *nc,
> >> +   int vq_index);
> >>   #endif
>

Re: [PATCH] tests: unit: simplify test-visitor-serialization list tests

[Markus Armbruster]

Paolo Bonzini  writes:

> test-visitor-serialization list tests is using an "if" to pick either the 
> first
> element of the list or the next one.  This was done presumably to mimic the
> code that creates the list, which has to fill in either the head pointer
> or the next pointer of the last element.  However, the code in the insert
> phase is a pretty standard singly-linked list insertion, while the one
> in the visit phase looks weird and even looks at the first item twice:
> this is confusing because the test puts in 32 items and finishes with
> an assertion that i == 33.
>
> So, move the "else" step in a separate switch statement, and change
> the do...while loop to a while, because cur_head has already been
> initialized beforehand.
>
> Signed-off-by: Paolo Bonzini 

Still a rather smelly bowl of copy-pasta, but this is incremental
improvement, so:
Reviewed-by: Markus Armbruster 

Re: [PATCH] tests: unit: add NULL-pointer check

[Markus Armbruster]

Paolo Bonzini  writes:

> In CID 1432593, Coverity complains that the result of qdict_crumple()
> might leak if it is not a dictionary.  This is not a practical concern
> since the test would fail immediately with a NULL pointer dereference
> in qdict_size().
>
> However, it is not nice to depend on qdict_size() crashing, so add an
> explicit assertion that that the crumpled object was indeed a dictionary.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  tests/unit/check-block-qdict.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/unit/check-block-qdict.c b/tests/unit/check-block-qdict.c
> index 5a25825093..751c58e737 100644
> --- a/tests/unit/check-block-qdict.c
> +++ b/tests/unit/check-block-qdict.c
> @@ -504,7 +504,7 @@ static void qdict_crumple_test_empty(void)
>  src = qdict_new();
>  
>  dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
> -
> +g_assert(dst);
>  g_assert_cmpint(qdict_size(dst), ==, 0);
>  
>  qobject_unref(src);

First, I'm fine with the patch, so
Reviewed-by: Markus Armbruster 

Next, permit me a few words on writing tests.  For me, a unit test fails
by crashing.  Crashing with a nice message is optional.  The more likely
the failure, the more useful is niceness.  Complete niceness is
impossible --- if we could predict all crashes, we wouldn't need tests.
Trying to push niceness can be overly verbose.  Thus, judgement calls,
and matters of taste.

Wanting to mollify Coverity is a valid argument.

qemu-system-aach64: A deadloop in aio_poll

[wang...@chinatelecom.cn]

Hi:
We found a deadloop in qemu-system-aarch64, the backtrace is:
(gdb)

  Id   Target Id Frame
* 1Thread 0xb4bee010 (LWP 1616556) "qemu-system-aar" 0xb5d58d64 
in __lll_lock_wait () from target:/lib64/libpthread.so.0
  2Thread 0xb4bea950 (LWP 1616625) "qemu-system-aar" 0xb5ca1310 
in syscall () from target:/lib64/libc.so.6
  3Thread 0xafffe950 (LWP 1616631) "qemu-system-aar" 0xb5c069a0 
in sigtimedwait () from target:/lib64/libc.so.6
  4Thread 0xae91c950 (LWP 1616637) "IO mon_iothread" 0xb5c9b668 
in poll () from target:/lib64/libc.so.6
  5Thread 0xade18950 (LWP 1616690) "CPU 0/KVM"   0xb5c9b774 
in ppoll () from target:/lib64/libc.so.6
  6Thread 0x579fe950 (LWP 1616731) "vnc_worker"  0xb5d557f0 
in pthread_cond_wait@@GLIBC_2.17 () from target:/lib64/libpthread.so.0
(gdb) thread app all bt

(gdb) bt
#0  0xb5d58d64 in __lll_lock_wait () from target:/lib64/libpthread.so.0
#1  0xb5d51db8 in pthread_mutex_lock () from 
target:/lib64/libpthread.so.0
#2  0xb4d0d2b4 in qemu_mutex_lock_impl (mutex=0xb528b5c0 
, file=0xb4e907d8 "../util/main-loop.c", line=242) at 
../util/qemu-thread-posix.c:79
#3  0xb4badf68 in qemu_mutex_lock_iothread_impl 
(file=file@entry=0xb4e907d8 "../util/main-loop.c", line=line@entry=242) at 
../softmmu/cpus.c:485
#4  0xb4d225b4 in os_host_main_loop_wait (timeout=0) at 
../util/main-loop.c:242
#5  main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:520
#6  0xb4b08b78 in qemu_main_loop () at ../softmmu/vl.c:1678
#7  0xb4745db0 in main (argc=, argv=, 
envp=) at ../softmmu/main.c:50
(gdb) thread app all bt

Thread 6 (Thread 0x579fe950 (LWP 1616731)):
#0  0xb5d557f0 in pthread_cond_wait@@GLIBC_2.17 () from 
target:/lib64/libpthread.so.0
#1  0xb4d0d60c in qemu_cond_wait_impl (cond=, 
mutex=0xe73a7a78, file=0xb4da2ea0 "../ui/vnc-jobs.c", line=215) at 
../util/qemu-thread-posix.c:174
#2  0xb4848b94 in vnc_worker_thread_loop 
(queue=queue@entry=0xe73a7a40) at ../ui/vnc-jobs.c:215
#3  0xb4849110 in vnc_worker_thread (arg=0xe73a7a40) at 
../ui/vnc-jobs.c:325
#4  0xb4d0d17c in qemu_thread_start (args=0xe73a7ae0) at 
../util/qemu-thread-posix.c:521
#5  0xb5d4f718 in start_thread () from target:/lib64/libpthread.so.0
#6  0xb5ca507c in thread_start () from target:/lib64/libc.so.6

Thread 5 (Thread 0xade18950 (LWP 1616690)):
#0  0xb5c9b774 in ppoll () from target:/lib64/libc.so.6
#1  0xb4d0f86c in ppoll (__ss=0x0, __timeout=0xade179c8, 
__nfds=, __fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, 
timeout=timeout@entry=59132090) at ../util/qemu-timer.c:349
#3  0xb4d26630 in fdmon_poll_wait (ctx=0xe2c7b0a0, 
ready_list=0xade17ac0, timeout=59132090) at ../util/fdmon-poll.c:80
#4  0xb4d16050 in aio_poll (ctx=0xe2c7b0a0, 
blocking=blocking@entry=true) at ../util/aio-posix.c:607
#5  0xb4c42308 in bdrv_drain_all_begin () at ../block/io.c:629
#6  bdrv_drain_all_begin () at ../block/io.c:594
#7  0xb4c423ec in bdrv_drain_all () at ../block/io.c:680
#8  0xb4c25668 in qmp_transaction (dev_list=0x504f2490, 
has_props=false, props=0x50891f50, errp=errp@entry=0xade17bd8) at 
../blockdev.c:2361
#9  0xb4ca7348 in qmp_marshal_transaction (args=, 
ret=, errp=0xb43eaec8) at qapi/qapi-commands-transaction.c:43
#10 0xb4d17b24 in do_qmp_dispatch_bh (opaque=0xb43eaed8) at 
../qapi/qmp-dispatch.c:110
#11 0xb4d13050 in aio_bh_poll (ctx=ctx@entry=0xe2c7b0a0) at 
../util/async.c:164
#12 0xb4d15bb8 in aio_poll (ctx=ctx@entry=0xe2c7b0a0, 
blocking=blocking@entry=true) at ../util/aio-posix.c:659
#13 0xb4c6fc14 in blk_prw (blk=0xe2ce95f0, 
offset=offset@entry=10240, buf=, bytes=bytes@entry=512, 
co_entry=co_entry@entry=0xb4c70530 , flags=flags@entry=0)
at ../block/block-backend.c:1335
#14 0xb4c6fda4 in blk_pwrite (blk=, 
offset=offset@entry=10240, buf=, count=count@entry=512, 
flags=flags@entry=0) at ../block/block-backend.c:1501
#15 0xb4936044 in pflash_update (offset=10240, offset@entry=10744, 
size=size@entry=4, pfl=, pfl=) at 
../hw/block/pflash_cfi01.c:398
#16 0xb4936b64 in pflash_write (be=, width=4, 
value=, offset=10744, pfl=0xe2c7def0) at 
../hw/block/pflash_cfi01.c:527
#17 pflash_mem_write_with_attrs (opaque=0xe2c7def0, addr=10744, 
value=, len=4, attrs=...) at ../hw/block/pflash_cfi01.c:685
#18 0xb4ba65fc in access_with_adjusted_size (addr=addr@entry=10744, 
value=value@entry=0xade17ef8, size=4, access_size_min=, 
access_size_max=,
access_fn=access_fn@entry=0xb4ba6990 
, mr=0xe2c7e2a0, attrs=...) at 
../softmmu/memory.c:552
#19 0xb4ba99a4 in memory_region_dispatch_wri

[PATCH V3 3/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

To boot S-mode firmware payload like EDK2 from persistent
flash storage, qemu needs to pass the flash address as the
next_addr in fw_dynamic_info to the opensbi.

When both -kernel and -pflash options are provided in command line,
the kernel (and initrd if -initrd) will be copied to fw_cfg table.
The S-mode FW will load the kernel/initrd from fw_cfg table.

If only pflash is given but not -kernel, then it is the job of
of the S-mode firmware to locate and load the kernel.

In either case, update the kernel_entry with the flash address
so that the opensbi can jump to the entry point of the S-mode
firmware.

Signed-off-by: Sunil V L 
---
 hw/riscv/boot.c | 28 
 hw/riscv/virt.c | 17 -
 include/hw/riscv/boot.h |  1 +
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 1ae7596873..39436b8d56 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -338,3 +338,31 @@ void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr 
fdt_addr)
 riscv_cpu->env.fdt_addr = fdt_addr;
 }
 }
+
+void riscv_setup_firmware_boot(MachineState *machine)
+{
+if (machine->kernel_filename) {
+FWCfgState *fw_cfg;
+fw_cfg = fw_cfg_find();
+
+assert(fw_cfg);
+/*
+ * Expose the kernel, the command line, and the initrd in fw_cfg.
+ * We don't process them here at all, it's all left to the
+ * firmware.
+ */
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+ machine->kernel_filename,
+ true);
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+ machine->initrd_filename, false);
+if (machine->kernel_cmdline) {
+fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+   strlen(machine->kernel_cmdline) + 1);
+fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+  machine->kernel_cmdline);
+}
+}
+}
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index b6bbf03f61..b985df9b16 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1258,7 +1258,22 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 s->fw_cfg = create_fw_cfg(machine);
 rom_set_fw(s->fw_cfg);
 
-if (machine->kernel_filename) {
+if (drive_get(IF_PFLASH, 0, 1)) {
+/*
+ * S-mode FW like EDk2 will be kept in second plash (unit 1). When
+ * both -kernel and -pflash options are provided in command line,
+ * the kernel, initrd will be copied to fw_cfg table and opensbi
+ * will jump to flash address which is the entry point of S-mode FW.
+ * It is the job of the S-mode FW to load the kernel/initrd and launch.
+ *
+ * If only pflash is given but not -kernel, then it is the job of
+ * of the S-mode firmware to locate and load the kernel.
+ * In either case, the next_addr for opensbi will be the flash address.
+ */
+riscv_setup_firmware_boot(machine);
+kernel_entry = virt_memmap[VIRT_FLASH].base +
+ virt_memmap[VIRT_FLASH].size / 2;
+} else if (machine->kernel_filename) {
 kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
  firmware_end_addr);
 
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index a36f7618f5..93e5f8760d 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -57,5 +57,6 @@ void riscv_rom_copy_firmware_info(MachineState *machine, 
hwaddr rom_base,
   uint32_t reset_vec_size,
   uint64_t kernel_entry);
 void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr fdt_addr);
+void riscv_setup_firmware_boot(MachineState *machine);
 
 #endif /* RISCV_BOOT_H */
-- 
2.25.1

回复: qemu-system-aach64: A deadloop in aio_poll

[wang...@chinatelecom.cn]

# virsh version
Compiled against library: libvirt 7.0.0
Using library: libvirt 7.0.0
Using API: QEMU 7.0.0
Running hypervisor: QEMU 5.2.0
# cat /etc/redhat-release 
CentOS Linux release 8.3.2011
# uname -a
Linux HS-22 5.10.106-4 #4 SMP Sat Apr 2 11:27:09 UTC 2022 aarch64 aarch64 
aarch64 GNU/Linux






wang...@chinatelecom.cn
 
发件人： wang...@chinatelecom.cn
发送时间： 2022-09-06 11:18
收件人： qemu-devel
抄送： ChangLimin; 陈相如; 李亚磊
主题： qemu-system-aach64: A deadloop in aio_poll
Hi:
We found a deadloop in qemu-system-aarch64, the backtrace is:
(gdb)

  Id   Target Id Frame
* 1Thread 0xb4bee010 (LWP 1616556) "qemu-system-aar" 0xb5d58d64 
in __lll_lock_wait () from target:/lib64/libpthread.so.0
  2Thread 0xb4bea950 (LWP 1616625) "qemu-system-aar" 0xb5ca1310 
in syscall () from target:/lib64/libc.so.6
  3Thread 0xafffe950 (LWP 1616631) "qemu-system-aar" 0xb5c069a0 
in sigtimedwait () from target:/lib64/libc.so.6
  4Thread 0xae91c950 (LWP 1616637) "IO mon_iothread" 0xb5c9b668 
in poll () from target:/lib64/libc.so.6
  5Thread 0xade18950 (LWP 1616690) "CPU 0/KVM"   0xb5c9b774 
in ppoll () from target:/lib64/libc.so.6
  6Thread 0x579fe950 (LWP 1616731) "vnc_worker"  0xb5d557f0 
in pthread_cond_wait@@GLIBC_2.17 () from target:/lib64/libpthread.so.0
(gdb) thread app all bt

(gdb) bt
#0  0xb5d58d64 in __lll_lock_wait () from target:/lib64/libpthread.so.0
#1  0xb5d51db8 in pthread_mutex_lock () from 
target:/lib64/libpthread.so.0
#2  0xb4d0d2b4 in qemu_mutex_lock_impl (mutex=0xb528b5c0 
, file=0xb4e907d8 "../util/main-loop.c", line=242) at 
../util/qemu-thread-posix.c:79
#3  0xb4badf68 in qemu_mutex_lock_iothread_impl 
(file=file@entry=0xb4e907d8 "../util/main-loop.c", line=line@entry=242) at 
../softmmu/cpus.c:485
#4  0xb4d225b4 in os_host_main_loop_wait (timeout=0) at 
../util/main-loop.c:242
#5  main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:520
#6  0xb4b08b78 in qemu_main_loop () at ../softmmu/vl.c:1678
#7  0xb4745db0 in main (argc=, argv=, 
envp=) at ../softmmu/main.c:50
(gdb) thread app all bt

Thread 6 (Thread 0x579fe950 (LWP 1616731)):
#0  0xb5d557f0 in pthread_cond_wait@@GLIBC_2.17 () from 
target:/lib64/libpthread.so.0
#1  0xb4d0d60c in qemu_cond_wait_impl (cond=, 
mutex=0xe73a7a78, file=0xb4da2ea0 "../ui/vnc-jobs.c", line=215) at 
../util/qemu-thread-posix.c:174
#2  0xb4848b94 in vnc_worker_thread_loop 
(queue=queue@entry=0xe73a7a40) at ../ui/vnc-jobs.c:215
#3  0xb4849110 in vnc_worker_thread (arg=0xe73a7a40) at 
../ui/vnc-jobs.c:325
#4  0xb4d0d17c in qemu_thread_start (args=0xe73a7ae0) at 
../util/qemu-thread-posix.c:521
#5  0xb5d4f718 in start_thread () from target:/lib64/libpthread.so.0
#6  0xb5ca507c in thread_start () from target:/lib64/libc.so.6

Thread 5 (Thread 0xade18950 (LWP 1616690)):
#0  0xb5c9b774 in ppoll () from target:/lib64/libc.so.6
#1  0xb4d0f86c in ppoll (__ss=0x0, __timeout=0xade179c8, 
__nfds=, __fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, 
timeout=timeout@entry=59132090) at ../util/qemu-timer.c:349
#3  0xb4d26630 in fdmon_poll_wait (ctx=0xe2c7b0a0, 
ready_list=0xade17ac0, timeout=59132090) at ../util/fdmon-poll.c:80
#4  0xb4d16050 in aio_poll (ctx=0xe2c7b0a0, 
blocking=blocking@entry=true) at ../util/aio-posix.c:607
#5  0xb4c42308 in bdrv_drain_all_begin () at ../block/io.c:629
#6  bdrv_drain_all_begin () at ../block/io.c:594
#7  0xb4c423ec in bdrv_drain_all () at ../block/io.c:680
#8  0xb4c25668 in qmp_transaction (dev_list=0x504f2490, 
has_props=false, props=0x50891f50, errp=errp@entry=0xade17bd8) at 
../blockdev.c:2361
#9  0xb4ca7348 in qmp_marshal_transaction (args=, 
ret=, errp=0xb43eaec8) at qapi/qapi-commands-transaction.c:43
#10 0xb4d17b24 in do_qmp_dispatch_bh (opaque=0xb43eaed8) at 
../qapi/qmp-dispatch.c:110
#11 0xb4d13050 in aio_bh_poll (ctx=ctx@entry=0xe2c7b0a0) at 
../util/async.c:164
#12 0xb4d15bb8 in aio_poll (ctx=ctx@entry=0xe2c7b0a0, 
blocking=blocking@entry=true) at ../util/aio-posix.c:659
#13 0xb4c6fc14 in blk_prw (blk=0xe2ce95f0, 
offset=offset@entry=10240, buf=, bytes=bytes@entry=512, 
co_entry=co_entry@entry=0xb4c70530 , flags=flags@entry=0)
at ../block/block-backend.c:1335
#14 0xb4c6fda4 in blk_pwrite (blk=, 
offset=offset@entry=10240, buf=, count=count@entry=512, 
flags=flags@entry=0) at ../block/block-backend.c:1501
#15 0xb4936044 in pflash_update (offset=10240, offset@entry=10744, 
size=size@entry=4, pfl=, pfl=) at 
../hw/block/pflash_cfi01.c:398
#16 0xb4936b64 in pflash_write (be=, width=4, 
value=, offset=10744, pfl=0xe2c7def0) at 
../

[PATCH V3 2/3] hw/riscv: virt: Move create_fw_cfg() prior to loading kernel

[Sunil V L]

To enable both -kernel and -pflash options, the fw_cfg needs to be
created prior to loading the kernel.

Signed-off-by: Sunil V L 
---
 hw/riscv/virt.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ff8c0df5cd..b6bbf03f61 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1251,6 +1251,13 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 RISCV64_BIOS_BIN, start_addr, NULL);
 }
 
+/*
+ * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
+ * tree cannot be altered and we get FDT_ERR_NOSPACE.
+ */
+s->fw_cfg = create_fw_cfg(machine);
+rom_set_fw(s->fw_cfg);
+
 if (machine->kernel_filename) {
 kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
  firmware_end_addr);
@@ -1284,13 +1291,6 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 start_addr = virt_memmap[VIRT_FLASH].base;
 }
 
-/*
- * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
- * tree cannot be altered and we get FDT_ERR_NOSPACE.
- */
-s->fw_cfg = create_fw_cfg(machine);
-rom_set_fw(s->fw_cfg);
-
 /* Compute the fdt load address in dram */
 fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
machine->ram_size, machine->fdt);
-- 
2.25.1

[PATCH V3 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

This series adds the support to boot S-mode FW like EDK2 from the flash. The
S-mode firmware should be kept in pflash unit 1.

When -kernel (and -initrd) option is also provided along with the flash,
the kernel (and initrd) will be loaded into fw_cfg table and opensbi will
branch to the flash address which will be the entry point of the S-mode
firmware. The S-mode FW then loads and launches the kernel.

When only -pflash option is provided in the command line, the kernel
will be located and loaded in the usual way by the S-mode firmware.

These patches are available in below branch.
https://github.com/vlsunil/qemu/tree/pflash_v2

The first two patches in this series are refactor patches.

These changes are tested with a WIP EDK2 port for virt machine. Below
are the instructions to build and test this feature.

1) Get EDK2 sources from below branches.
https://github.com/vlsunil/edk2/tree/virt_refactor_smode_v1
https://github.com/vlsunil/edk2-platforms/tree/virt_refactor_smode_v1

2) Build EDK2 for RISC-V
export WORKSPACE=`pwd`
export GCC5_RISCV64_PREFIX=riscv64-linux-gnu-
export PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms
export EDK_TOOLS_PATH=$WORKSPACE/edk2/BaseTools
source edk2/edksetup.sh
make -C edk2/BaseTools clean
make -C edk2/BaseTools
make -C edk2/BaseTools/Source/C
source edk2/edksetup.sh BaseTools
build -a RISCV64  -p Platform/Qemu/RiscVVirt/RiscVVirt.dsc -t GCC5

3)Make the EDK2 image size to match with what qemu flash expects
truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd

4) Run
a) Boot to EFI shell (no -kernel / -initrd option)
qemu-system-riscv64  -nographic   -drive 
file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  
-machine virt -M 2G

b) With -kernel, -initrd and -pflash
qemu-system-riscv64  -nographic   -drive 
file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  
-machine virt -M 2G -kernel arch/riscv/boot/Image.gz -initrd rootfs.cpio 


Changes since V2:
1) Moved the doc comment to .h file

Changes since V1:
1) Modified code to support the use case when both -kernel and -pflash 
are configured.
2) Refactor patches added to help (1) above.
3) Cover letter added with test instructions.

Sunil V L (3):
  hw/arm,loongarch: Move load_image_to_fw_cfg() to common location
  hw/riscv: virt: Move create_fw_cfg() prior to loading kernel
  hw/riscv: virt: Enable booting S-mode firmware from pflash

 hw/arm/boot.c | 49 ---
 hw/loongarch/virt.c   | 33 --
 hw/nvram/fw_cfg.c | 32 +
 hw/riscv/boot.c   | 28 ++
 hw/riscv/virt.c   | 31 ++---
 include/hw/nvram/fw_cfg.h | 21 +
 include/hw/riscv/boot.h   |  1 +
 7 files changed, 105 insertions(+), 90 deletions(-)

-- 
2.25.1

[PATCH V3 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
function will be required by riscv too. So, it's time to refactor and
move this function to a common path.

Signed-off-by: Sunil V L 
---
 hw/arm/boot.c | 49 ---
 hw/loongarch/virt.c   | 33 --
 hw/nvram/fw_cfg.c | 32 +
 include/hw/nvram/fw_cfg.h | 21 +
 4 files changed, 53 insertions(+), 82 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..704f368d9c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
 }
 }
 
-/**
- * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
- *  by key.
- * @fw_cfg: The firmware config instance to store the data in.
- * @size_key:   The firmware config key to store the size of the loaded
- *  data under, with fw_cfg_add_i32().
- * @data_key:   The firmware config key to store the loaded data under,
- *  with fw_cfg_add_bytes().
- * @image_name: The name of the image file to load. If it is NULL, the
- *  function returns without doing anything.
- * @try_decompress: Whether the image should be decompressed (gunzipped) before
- *  adding it to fw_cfg. If decompression fails, the image is
- *  loaded as-is.
- *
- * In case of failure, the function prints an error message to stderr and the
- * process exits with status 1.
- */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
- uint16_t data_key, const char *image_name,
- bool try_decompress)
-{
-size_t size = -1;
-uint8_t *data;
-
-if (image_name == NULL) {
-return;
-}
-
-if (try_decompress) {
-size = load_image_gzipped_buffer(image_name,
- LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-}
-
-if (size == (size_t)-1) {
-gchar *contents;
-gsize length;
-
-if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-error_report("failed to load \"%s\"", image_name);
-exit(1);
-}
-size = length;
-data = (uint8_t *)contents;
-}
-
-fw_cfg_add_i32(fw_cfg, size_key, size);
-fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static int do_arm_linux_init(Object *obj, void *opaque)
 {
 if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 5cc0b05538..eee2c193c0 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
 }
 }
 
-/* Load an image file into an fw_cfg entry identified by key. */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
- uint16_t data_key, const char *image_name,
- bool try_decompress)
-{
-size_t size = -1;
-uint8_t *data;
-
-if (image_name == NULL) {
-return;
-}
-
-if (try_decompress) {
-size = load_image_gzipped_buffer(image_name,
- LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-}
-
-if (size == (size_t)-1) {
-gchar *contents;
-gsize length;
-
-if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-error_report("failed to load \"%s\"", image_name);
-exit(1);
-}
-size = length;
-data = (uint8_t *)contents;
-}
-
-fw_cfg_add_i32(fw_cfg, size_key, size);
-fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
 {
 /*
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d605f3f45a..371a45dfe2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -41,6 +41,7 @@
 #include "qapi/error.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/loader.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
@@ -1221,6 +1222,37 @@ FWCfgState *fw_cfg_find(void)
 return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
+  uint16_t data_key, const char *image_name,
+  bool try_decompress)
+{
+size_t size = -1;
+uint8_t *data;
+
+if (image_name == NULL) {
+return;
+}
+
+if (try_decompress) {
+size = load_image_gzipped_buffer(image_name,
+ LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
+}
+
+if (size == (size_t)-1) {
+gchar *contents;
+gsize length;
+
+if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
+error_report("failed to load \"%s\"", image_name);
+   

Re: [PATCH V2 1/3] hw/arm,loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

On Mon, Sep 05, 2022 at 10:20:40PM +0100, Peter Maydell wrote:
> On Mon, 5 Sept 2022 at 19:23, Sunil V L  wrote:
> >
> > load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> > function will be required by riscv too. So, it's time to refactor and
> > move this function to a common path.
> >
> > Signed-off-by: Sunil V L 
> > ---
> >  hw/arm/boot.c | 49 ---
> >  hw/loongarch/virt.c   | 33 --
> >  hw/nvram/fw_cfg.c | 49 +++
> >  include/hw/nvram/fw_cfg.h |  3 +++
> >  4 files changed, 52 insertions(+), 82 deletions(-)
> 
> 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index d605f3f45a..3f68dae5d2 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -41,6 +41,7 @@
> >  #include "qapi/error.h"
> >  #include "hw/acpi/aml-build.h"
> >  #include "hw/pci/pci_bus.h"
> > +#include "hw/loader.h"
> >
> >  #define FW_CFG_FILE_SLOTS_DFLT 0x20
> >
> > @@ -1221,6 +1222,54 @@ FWCfgState *fw_cfg_find(void)
> >  return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
> >  }
> >
> > +/**
> > + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry 
> > identified
> > + *  by key.
> 
> For functions declared in public header files, put the doc comment
> in the .h file, not the .c file, please.

Thanks! Peter. Will send V3 with this change.

Thanks
Sunil
> 
> thanks
> -- PMM

Re: [PATCH v3 13/15] virtio-net: support queue reset

[Kangjie Xu]

在 2022/9/5 16:30, Jason Wang 写道:


在 2022/8/25 16:08, Kangjie Xu 写道:

From: Xuan Zhuo 

virtio-net and vhost-kernel implement queue reset.
Queued packets in the corresponding queue pair are flushed
or purged.

Signed-off-by: Xuan Zhuo 
Signed-off-by: Kangjie Xu 
---
  hw/net/virtio-net.c | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27b59c0ad6..d774a3e652 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -540,6 +540,23 @@ static RxFilterInfo 
*virtio_net_query_rxfilter(NetClientState *nc)

  return info;
  }
  +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t 
queue_index)

+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
+    if (!nc->peer) {
+    return;
+    }
+
+    if (get_vhost_net(nc->peer) &&
+    nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+    vhost_net_virtqueue_reset(vdev, nc, queue_index);
+    }
+
+    flush_or_purge_queued_packets(nc);



But the codes doesn't prevent the usersapce datapath from being used? 
(e.g vhost=off)


I think we do not need to prevent it for vhost=off, because the 
virtio-net device is in control of the tap device.


After we reset the vq, the virtio-net send and recv will not use the 
userspace datapath. (virtio_net_flush_tx() and virtio_net_receive() will 
early returns because vq->vring.avail == 0)


So even if we don't prevent it using net->nc->info->poll, virtio-net 
device will prevent it. And the logic here is similar to virtio_reset(), 
I think it will not cause problems.


Thanks.



E.g vhost_net_start_one() had:

    if (net->nc->info->poll) {
    net->nc->info->poll(net->nc, false);
    }

And I will wonder if it's better to consider to:

1) factor out the per virtqueue start/stop from 
vhost_net_start/stop_one()


2) simply use the helper factored out via step 1)

Thanks



+}
+
  static void virtio_net_reset(VirtIODevice *vdev)
  {
  VirtIONet *n = VIRTIO_NET(vdev);
@@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass 
*klass, void *data)

  vdc->set_features = virtio_net_set_features;
  vdc->bad_features = virtio_net_bad_features;
  vdc->reset = virtio_net_reset;
+    vdc->queue_reset = virtio_net_queue_reset;
  vdc->set_status = virtio_net_set_status;
  vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
  vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH v2 0/4] hw/arm/virt: Improve address assignment for high memory regions

[Gavin Shan]

Hi Eric,

On 8/24/22 6:06 PM, Eric Auger wrote:

On 8/24/22 05:29, Gavin Shan wrote:

On 8/15/22 4:29 PM, Gavin Shan wrote:

There are three high memory regions, which are VIRT_HIGH_REDIST2,
VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
are floating on highest RAM address. However, they can be disabled
in several cases.
  (1) One specific high memory region is disabled by developer by
  toggling vms->highmem_{redists, ecam, mmio}.
  (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
  'virt-2.12' or ealier than it.
  (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
  on 32-bits system.
  (4) One specific high memory region is disabled when it breaks the
  PA space limit.
  The current implementation of virt_set_memmap() isn't comprehensive
because the space for one specific high memory region is always
reserved from the PA space for case (1), (2) and (3). In the code,
'base' and 'vms->highest_gpa' are always increased for those three
cases. It's unnecessary since the assigned space of the disabled
high memory region won't be used afterwards.

The series intends to improve the address assignment for these
high memory regions:

PATCH[1] and PATCH[2] are cleanup and preparatory works.
PATCH[3] improves address assignment for these high memory regions
PATCH[4] moves the address assignment logic into standalone helper

History
===
v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html

Changelog
=
v2:
    * Split the patches for easier review    (Gavin)
    * Improved changelog (Marc)
    * Use 'bool fits' in virt_set_high_memmap()  (Eric)


You did not really convince me about migration compat wrt the high MMIO
region. Aren't the PCI BARs saved/restored meaning the device driver is
expecting to find data at the same GPA. But what if your high MMIO
region was relocated in the dest QEMU with a possibly smaller VM IPA?
Don't you have MMIO regions now allocated outside of the dest MMIO
region? How does the PCI host bridge route accesses to those regions?
What do I miss?



[...]

Sorry for the delay because I was offline last week. I was intending
to explain the migration on virtio-net device and spent some time to
go through the code. I found it's still complicated for an example
because PCI and virtio device models are involved. So lets still use
e1000e.c as an example here.

There are lots of registers residing in MMIO region, including MSIx
table. The MSIx table is backed by PCIDevice::msix_table, which is
a buffer. The access to MSIx table is read from or write to the buffer.
The corresponding handler is hw/msix.c::msix_table_mmio_ops. msix_init()
is called by e1000e.c to setup the MSIx table, which is associated with
memory BAR#3. As the registers in MSIx table is totally emulated by
QEMU, the BAR's base address isn't a concern.

  struct PCIDevice {
 :
 uint8_t *msix_table;
 :
 MemoryRegion msix_table_mmio;
 :
  };

  /* @table_bar is registered as memory BAR#3 in e1000e_pci_realize() */
  int msix_init(struct PCIDevice *dev, unsigned short nentries,
MemoryRegion *table_bar, uint8_t table_bar_nr,
unsigned table_offset, MemoryRegion *pba_bar,
uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
Error **errp)
  {
   :
memory_region_init_io(&dev->msix_table_mmio, OBJECT(dev), 
&msix_table_mmio_ops, dev,
  "msix-table", table_size);
memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
   :
  }

As we concerned, the BAR's base addresses for MSIx tables are different on 
source
and destination VMs. It's still not a problem because the registers in MSIx 
table
are migrated, saved on source VM and restored on destination VM one by one. It's
to say, not the whole buffer (PCIDevice::msix_table) is saved and restored at 
once.
Further more, the unique ID string, instead the corresponding BAR's base 
address,
is used to identify the MSIx table. For this particular case, the ID string is
something like "e1000e_dev_id/pci-:05:00.0/msix state". With this ID string
is received on the destination VM, the object and PCI device is located and the
forth-coming data is saved to PCIDevice::msix_table.

  static const VMStateDescription e1000e_vmstate = {
.name = "e1000e",
.version_id = 1,
.minimum_version_id = 1,
.pre_save = e1000e_pre_save,
.post_load = e1000e_post_load,
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, E1000EState),
VMSTATE_MSIX(parent_obj, E1000EState),
:
}
  };

  #define VMSTATE_MSIX_TEST(_field, _state, _test) { \
.name = (stringify(_field)), \
.size = sizeof(PCIDevice),   \
.vmsd = &vmstate_msi

[PATCH] tests/vm: update NetBSD to 9.3

[Brad Smith]

tests/vm: update NetBSD to 9.3

Signed-off-by: Brad Smith 
---
 tests/vm/netbsd | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index da6773ff59..aa54338dfa 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -22,8 +22,8 @@ class NetBSDVM(basevm.BaseVM):
 name = "netbsd"
 arch = "x86_64"
 
-link = 
"https://cdn.netbsd.org/pub/NetBSD/NetBSD-9.2/images/NetBSD-9.2-amd64.iso";
-csum = 
"5ee0ea101f73386b9b424f5d1041e371db3c42fdd6f4e4518dc79c4a08f31d43091ebe93425c9f0dcaaed2b51131836fe6774f33f89030b58d64709b35fda72f"
+link = 
"https://cdn.netbsd.org/pub/NetBSD/NetBSD-9.3/images/NetBSD-9.3-amd64.iso";
+csum = 
"2bfce544f762a579f61478e7106c436fc48731ff25cf6f79b392ba5752e6f5ec130364286f7471716290a5f033637cf56aacee7fedb91095face59adf36300c3"
 size = "20G"
 pkgs = [
 # tools
-- 
2.37.2

[PATCH v3 5/5] tests/tcg/linux-test: Add linux-madvise test

[Ilya Leoshkevich]

Add a test that checks madvise(MADV_DONTNEED) behavior with anonymous
and file mappings in order to prevent regressions.

Signed-off-by: Ilya Leoshkevich 
---
 tests/tcg/multiarch/linux/linux-madvise.c | 70 +++
 1 file changed, 70 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-madvise.c

diff --git a/tests/tcg/multiarch/linux/linux-madvise.c 
b/tests/tcg/multiarch/linux/linux-madvise.c
new file mode 100644
index 00..29d0997e68
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-madvise.c
@@ -0,0 +1,70 @@
+#include 
+#include 
+#include 
+#include 
+
+static void test_anonymous(void)
+{
+int pagesize = getpagesize();
+char *page;
+int ret;
+
+page = mmap(NULL, pagesize, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+assert(page != MAP_FAILED);
+
+/* Check that mprotect() does not interfere with MADV_DONTNEED. */
+ret = mprotect(page, pagesize, PROT_READ | PROT_WRITE);
+assert(ret == 0);
+
+/* Check that MADV_DONTNEED clears the page. */
+*page = 42;
+ret = madvise(page, pagesize, MADV_DONTNEED);
+assert(ret == 0);
+assert(*page == 0);
+
+ret = munmap(page, pagesize);
+assert(ret == 0);
+}
+
+static void test_file(void)
+{
+char tempname[] = "/tmp/.cmadviseXX";
+int pagesize = getpagesize();
+ssize_t written;
+char c = 42;
+char *page;
+int ret;
+int fd;
+
+fd = mkstemp(tempname);
+assert(fd != -1);
+ret = unlink(tempname);
+assert(ret == 0);
+written = write(fd, &c, sizeof(c));
+assert(written == sizeof(c));
+page = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE, fd, 0);
+assert(page != MAP_FAILED);
+
+/* Check that mprotect() does not interfere with MADV_DONTNEED. */
+ret = mprotect(page, pagesize, PROT_READ | PROT_WRITE);
+assert(ret == 0);
+
+/* Check that MADV_DONTNEED resets the page. */
+*page = 0;
+ret = madvise(page, pagesize, MADV_DONTNEED);
+assert(ret == 0);
+assert(*page == c);
+
+ret = munmap(page, pagesize);
+assert(ret == 0);
+ret = close(fd);
+assert(ret == 0);
+}
+
+int main(void)
+{
+test_anonymous();
+test_file();
+
+return EXIT_SUCCESS;
+}
-- 
2.37.2

[PATCH v3 3/5] linux-user: Implement stracing madvise()

[Ilya Leoshkevich]

The default implementation has several problems: the first argument is
not displayed as a pointer, making it harder to grep; the third
argument is not symbolized; and there are several extra unused
arguments.

Signed-off-by: Ilya Leoshkevich 
---
 linux-user/strace.c| 41 +
 linux-user/strace.list |  2 +-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 7d882526da..c262c0c9b6 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -17,6 +17,7 @@
 #include "qemu.h"
 #include "user-internals.h"
 #include "strace.h"
+#include "target_mman.h"
 
 struct syscallname {
 int nr;
@@ -2969,6 +2970,46 @@ print_stat(CPUArchState *cpu_env, const struct 
syscallname *name,
 #define print_lstat64   print_stat
 #endif
 
+#if defined(TARGET_NR_madvise)
+static struct enums madvise_advice[] = {
+ENUM_TARGET(MADV_NORMAL),
+ENUM_TARGET(MADV_RANDOM),
+ENUM_TARGET(MADV_SEQUENTIAL),
+ENUM_TARGET(MADV_WILLNEED),
+ENUM_TARGET(MADV_DONTNEED),
+ENUM_TARGET(MADV_FREE),
+ENUM_TARGET(MADV_REMOVE),
+ENUM_TARGET(MADV_DONTFORK),
+ENUM_TARGET(MADV_DOFORK),
+ENUM_TARGET(MADV_MERGEABLE),
+ENUM_TARGET(MADV_UNMERGEABLE),
+ENUM_TARGET(MADV_HUGEPAGE),
+ENUM_TARGET(MADV_NOHUGEPAGE),
+ENUM_TARGET(MADV_DONTDUMP),
+ENUM_TARGET(MADV_DODUMP),
+ENUM_TARGET(MADV_WIPEONFORK),
+ENUM_TARGET(MADV_KEEPONFORK),
+ENUM_TARGET(MADV_COLD),
+ENUM_TARGET(MADV_PAGEOUT),
+ENUM_TARGET(MADV_POPULATE_READ),
+ENUM_TARGET(MADV_POPULATE_WRITE),
+ENUM_TARGET(MADV_DONTNEED_LOCKED),
+ENUM_END,
+};
+
+static void
+print_madvise(CPUArchState *cpu_env, const struct syscallname *name,
+  abi_long arg0, abi_long arg1, abi_long arg2,
+  abi_long arg3, abi_long arg4, abi_long arg5)
+{
+print_syscall_prologue(name);
+print_pointer(arg0, 0);
+print_raw_param("%d", arg1, 0);
+print_enums(madvise_advice, arg2, 1);
+print_syscall_epilogue(name);
+}
+#endif
+
 #if defined(TARGET_NR_fstat) || defined(TARGET_NR_fstat64)
 static void
 print_fstat(CPUArchState *cpu_env, const struct syscallname *name,
diff --git a/linux-user/strace.list b/linux-user/strace.list
index 72e17b1acf..c93effdbc8 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -541,7 +541,7 @@
 { TARGET_NR_lstat64, "lstat64" , NULL, print_lstat64, NULL },
 #endif
 #ifdef TARGET_NR_madvise
-{ TARGET_NR_madvise, "madvise" , NULL, NULL, NULL },
+{ TARGET_NR_madvise, "madvise" , NULL, print_madvise, NULL },
 #endif
 #ifdef TARGET_NR_madvise1
 { TARGET_NR_madvise1, "madvise1" , NULL, NULL, NULL },
-- 
2.37.2

[PATCH v3 2/5] linux-user: Fix madvise(MADV_DONTNEED) on alpha

[Ilya Leoshkevich]

MADV_DONTNEED has a different value on alpha, compared to all the other
architectures. Fix by using TARGET_MADV_DONTNEED instead of
MADV_DONTNEED.

Fixes: 892a4f6a750a ("linux-user: Add partial support for MADV_DONTNEED")
Signed-off-by: Ilya Leoshkevich 
---
 linux-user/mmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 048c4135af..a5f1ab129c 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -22,6 +22,7 @@
 #include "qemu.h"
 #include "user-internals.h"
 #include "user-mmap.h"
+#include "target_mman.h"
 
 static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
 static __thread int mmap_lock_count;
@@ -891,7 +892,7 @@ abi_long target_madvise(abi_ulong start, abi_ulong len_in, 
int advice)
  * anonymous mappings. In this case passthrough is safe, so do it.
  */
 mmap_lock();
-if (advice == MADV_DONTNEED &&
+if (advice == TARGET_MADV_DONTNEED &&
 can_passthrough_madv_dontneed(start, end)) {
 ret = get_errno(madvise(g2h_untagged(start), len, MADV_DONTNEED));
 if (ret == 0) {
-- 
2.37.2

[PATCH v3 4/5] linux-user: Passthrough MADV_DONTNEED for certain file mappings

[Ilya Leoshkevich]

This is a follow-up for commit 892a4f6a750a ("linux-user: Add partial
support for MADV_DONTNEED"), which added passthrough for anonymous
mappings. File mappings can be handled in a similar manner.

In order to do that, mark pages, for which mmap() was passed through,
with PAGE_PASSTHROUGH, and then allow madvise() passthrough for these
pages. Drop the explicit PAGE_ANON check, since anonymous mappings are
expected to have PAGE_PASSTHROUGH anyway.

Add PAGE_PASSTHROUGH to PAGE_STICKY in order to keep it on mprotect().

Signed-off-by: Ilya Leoshkevich 
Message-Id: <20220725125043.43048-1-...@linux.ibm.com>
---
 accel/tcg/translate-all.c |  2 +-
 include/exec/cpu-all.h|  6 ++
 linux-user/mmap.c | 27 ++-
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index b83161a081..a47cf38e38 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2263,7 +2263,7 @@ int page_get_flags(target_ulong address)
 #ifndef PAGE_TARGET_STICKY
 #define PAGE_TARGET_STICKY  0
 #endif
-#define PAGE_STICKY  (PAGE_ANON | PAGE_TARGET_STICKY)
+#define PAGE_STICKY  (PAGE_ANON | PAGE_PASSTHROUGH | PAGE_TARGET_STICKY)
 
 /* Modify the flags of a page and invalidate the code if necessary.
The flag PAGE_WRITE_ORG is positioned automatically depending
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 491629b9ba..16b7df41bf 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -262,6 +262,12 @@ extern const TargetPageBits target_page;
 #define PAGE_TARGET_1  0x0200
 #define PAGE_TARGET_2  0x0400
 
+/*
+ * For linux-user, indicates that the page is mapped with the same semantics
+ * in both guest and host.
+ */
+#define PAGE_PASSTHROUGH 0x0800
+
 #if defined(CONFIG_USER_ONLY)
 void page_dump(FILE *f);
 
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index a5f1ab129c..3a0f67619a 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -425,7 +425,8 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, 
abi_ulong align)
 abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
  int flags, int fd, abi_ulong offset)
 {
-abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
+abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len,
+  passthrough_start = -1, passthrough_end = -1;
 int page_flags, host_prot;
 
 mmap_lock();
@@ -538,6 +539,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
target_prot,
 host_start += offset - host_offset;
 }
 start = h2g(host_start);
+passthrough_start = start;
+passthrough_end = start + len;
 } else {
 if (start & ~TARGET_PAGE_MASK) {
 errno = EINVAL;
@@ -620,6 +623,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
target_prot,
  host_prot, flags, fd, offset1);
 if (p == MAP_FAILED)
 goto fail;
+passthrough_start = real_start;
+passthrough_end = real_end;
 }
 }
  the_end1:
@@ -627,7 +632,18 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
target_prot,
 page_flags |= PAGE_ANON;
 }
 page_flags |= PAGE_RESET;
-page_set_flags(start, start + len, page_flags);
+if (passthrough_start == passthrough_end) {
+page_set_flags(start, start + len, page_flags);
+} else {
+if (start < passthrough_start) {
+page_set_flags(start, passthrough_start, page_flags);
+}
+page_set_flags(passthrough_start, passthrough_end,
+   page_flags | PAGE_PASSTHROUGH);
+if (passthrough_end < start + len) {
+page_set_flags(passthrough_end, start + len, page_flags);
+}
+}
  the_end:
 trace_target_mmap_complete(start);
 if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
@@ -846,7 +862,7 @@ static bool can_passthrough_madv_dontneed(abi_ulong start, 
abi_ulong end)
 }
 
 for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
-if (!(page_get_flags(addr) & PAGE_ANON)) {
+if (!(page_get_flags(addr) & PAGE_PASSTHROUGH)) {
 return false;
 }
 }
@@ -888,8 +904,9 @@ abi_long target_madvise(abi_ulong start, abi_ulong len_in, 
int advice)
  * This is a hint, so ignoring and returning success is ok.
  *
  * This breaks MADV_DONTNEED, completely implementing which is quite
- * complicated. However, there is one low-hanging fruit: host-page-aligned
- * anonymous mappings. In this case passthrough is safe, so do it.
+ * complicated. However, there is one low-hanging fruit: mappings that are
+ * known to have the same semantics in the host and the guest. In this case
+ * passthrough is safe, so do it.
  */
 mmap_lock();
 if (advice == TARGET_MADV_DONTNEED &&
-- 
2.37.2

[PATCH v3 1/5] linux-user: Provide MADV_* definitions

[Ilya Leoshkevich]

Provide MADV_* definitions using target_mman.h header, similar to what
kernel does. Most architectures use the same values, with the exception
of alpha and hppa.

Signed-off-by: Ilya Leoshkevich 
---
 linux-user/aarch64/target_mman.h |  1 +
 linux-user/alpha/target_mman.h   |  8 +++
 linux-user/arm/target_mman.h |  1 +
 linux-user/cris/target_mman.h|  1 +
 linux-user/generic/target_mman.h | 92 
 linux-user/hexagon/target_mman.h |  1 +
 linux-user/hppa/target_mman.h| 15 +
 linux-user/i386/target_mman.h|  1 +
 linux-user/loongarch64/target_mman.h |  1 +
 linux-user/m68k/target_mman.h|  1 +
 linux-user/microblaze/target_mman.h  |  1 +
 linux-user/mips/target_mman.h|  1 +
 linux-user/mips64/target_mman.h  |  1 +
 linux-user/nios2/target_mman.h   |  1 +
 linux-user/openrisc/target_mman.h|  1 +
 linux-user/ppc/target_mman.h |  1 +
 linux-user/riscv/target_mman.h   |  1 +
 linux-user/s390x/target_mman.h   |  1 +
 linux-user/sh4/target_mman.h |  1 +
 linux-user/sparc/target_mman.h   |  1 +
 linux-user/x86_64/target_mman.h  |  1 +
 linux-user/xtensa/target_mman.h  |  1 +
 22 files changed, 134 insertions(+)
 create mode 100644 linux-user/aarch64/target_mman.h
 create mode 100644 linux-user/alpha/target_mman.h
 create mode 100644 linux-user/arm/target_mman.h
 create mode 100644 linux-user/cris/target_mman.h
 create mode 100644 linux-user/generic/target_mman.h
 create mode 100644 linux-user/hexagon/target_mman.h
 create mode 100644 linux-user/hppa/target_mman.h
 create mode 100644 linux-user/i386/target_mman.h
 create mode 100644 linux-user/loongarch64/target_mman.h
 create mode 100644 linux-user/m68k/target_mman.h
 create mode 100644 linux-user/microblaze/target_mman.h
 create mode 100644 linux-user/mips/target_mman.h
 create mode 100644 linux-user/mips64/target_mman.h
 create mode 100644 linux-user/nios2/target_mman.h
 create mode 100644 linux-user/openrisc/target_mman.h
 create mode 100644 linux-user/ppc/target_mman.h
 create mode 100644 linux-user/riscv/target_mman.h
 create mode 100644 linux-user/s390x/target_mman.h
 create mode 100644 linux-user/sh4/target_mman.h
 create mode 100644 linux-user/sparc/target_mman.h
 create mode 100644 linux-user/x86_64/target_mman.h
 create mode 100644 linux-user/xtensa/target_mman.h

diff --git a/linux-user/aarch64/target_mman.h b/linux-user/aarch64/target_mman.h
new file mode 100644
index 00..e7ba6070fe
--- /dev/null
+++ b/linux-user/aarch64/target_mman.h
@@ -0,0 +1 @@
+#include "../generic/target_mman.h"
diff --git a/linux-user/alpha/target_mman.h b/linux-user/alpha/target_mman.h
new file mode 100644
index 00..cd6e3d70a6
--- /dev/null
+++ b/linux-user/alpha/target_mman.h
@@ -0,0 +1,8 @@
+#ifndef ALPHA_TARGET_MMAN_H
+#define ALPHA_TARGET_MMAN_H
+
+#define TARGET_MADV_DONTNEED 6
+
+#include "../generic/target_mman.h"
+
+#endif
diff --git a/linux-user/arm/target_mman.h b/linux-user/arm/target_mman.h
new file mode 100644
index 00..e7ba6070fe
--- /dev/null
+++ b/linux-user/arm/target_mman.h
@@ -0,0 +1 @@
+#include "../generic/target_mman.h"
diff --git a/linux-user/cris/target_mman.h b/linux-user/cris/target_mman.h
new file mode 100644
index 00..e7ba6070fe
--- /dev/null
+++ b/linux-user/cris/target_mman.h
@@ -0,0 +1 @@
+#include "../generic/target_mman.h"
diff --git a/linux-user/generic/target_mman.h b/linux-user/generic/target_mman.h
new file mode 100644
index 00..1436a3c543
--- /dev/null
+++ b/linux-user/generic/target_mman.h
@@ -0,0 +1,92 @@
+#ifndef LINUX_USER_TARGET_MMAN_H
+#define LINUX_USER_TARGET_MMAN_H
+
+#ifndef TARGET_MADV_NORMAL
+#define TARGET_MADV_NORMAL 0
+#endif
+
+#ifndef TARGET_MADV_RANDOM
+#define TARGET_MADV_RANDOM 1
+#endif
+
+#ifndef TARGET_MADV_SEQUENTIAL
+#define TARGET_MADV_SEQUENTIAL 2
+#endif
+
+#ifndef TARGET_MADV_WILLNEED
+#define TARGET_MADV_WILLNEED 3
+#endif
+
+#ifndef TARGET_MADV_DONTNEED
+#define TARGET_MADV_DONTNEED 4
+#endif
+
+#ifndef TARGET_MADV_FREE
+#define TARGET_MADV_FREE 8
+#endif
+
+#ifndef TARGET_MADV_REMOVE
+#define TARGET_MADV_REMOVE 9
+#endif
+
+#ifndef TARGET_MADV_DONTFORK
+#define TARGET_MADV_DONTFORK 10
+#endif
+
+#ifndef TARGET_MADV_DOFORK
+#define TARGET_MADV_DOFORK 11
+#endif
+
+#ifndef TARGET_MADV_MERGEABLE
+#define TARGET_MADV_MERGEABLE 12
+#endif
+
+#ifndef TARGET_MADV_UNMERGEABLE
+#define TARGET_MADV_UNMERGEABLE 13
+#endif
+
+#ifndef TARGET_MADV_HUGEPAGE
+#define TARGET_MADV_HUGEPAGE 14
+#endif
+
+#ifndef TARGET_MADV_NOHUGEPAGE
+#define TARGET_MADV_NOHUGEPAGE 15
+#endif
+
+#ifndef TARGET_MADV_DONTDUMP
+#define TARGET_MADV_DONTDUMP 16
+#endif
+
+#ifndef TARGET_MADV_DODUMP
+#define TARGET_MADV_DODUMP 17
+#endif
+
+#ifndef TARGET_MADV_WIPEONFORK
+#define TARGET_MADV_WIPEONFORK 18
+#endif
+
+#ifndef TARGET_MADV_KEEPONFORK
+#define TARGET_MADV_KEEPONFORK 19
+#endif
+
+#ifndef TARGET_MADV_COLD
+#define TARGET_MADV_COLD 20
+#endif
+
+#ifndef TARGET_MAD

[PATCH v3 0/5] linux-user: Passthrough MADV_DONTNEED for certain file mappings

[Ilya Leoshkevich]

Hi,

This series is made of patches from [1]. I've added a test and noticed
that madvise(MADV_DONTNEED) was broken on alpha, fixing which required
adding per-arch MADV_* definitions. This in turn affected the strace
patch, so it made sense to make a series out of the results.

Patch 1 adds MADV_* constants for all architectures.
Patch 2 fixes the alpha bug.
Patch 3 adds madvise() support to strace.
Patch 4 adds MADV_DONTNEED support for file mappings.
Patch 5 adds a test.

Best regards,
Ilya

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00710.html

Ilya Leoshkevich (5):
  linux-user: Provide MADV_* definitions
  linux-user: Fix madvise(MADV_DONTNEED) on alpha
  linux-user: Implement stracing madvise()
  linux-user: Passthrough MADV_DONTNEED for certain file mappings
  tests/tcg/linux-test: Add linux-madvise test

 accel/tcg/translate-all.c |  2 +-
 include/exec/cpu-all.h|  6 ++
 linux-user/aarch64/target_mman.h  |  1 +
 linux-user/alpha/target_mman.h|  8 ++
 linux-user/arm/target_mman.h  |  1 +
 linux-user/cris/target_mman.h |  1 +
 linux-user/generic/target_mman.h  | 92 +++
 linux-user/hexagon/target_mman.h  |  1 +
 linux-user/hppa/target_mman.h | 15 
 linux-user/i386/target_mman.h |  1 +
 linux-user/loongarch64/target_mman.h  |  1 +
 linux-user/m68k/target_mman.h |  1 +
 linux-user/microblaze/target_mman.h   |  1 +
 linux-user/mips/target_mman.h |  1 +
 linux-user/mips64/target_mman.h   |  1 +
 linux-user/mmap.c | 30 ++--
 linux-user/nios2/target_mman.h|  1 +
 linux-user/openrisc/target_mman.h |  1 +
 linux-user/ppc/target_mman.h  |  1 +
 linux-user/riscv/target_mman.h|  1 +
 linux-user/s390x/target_mman.h|  1 +
 linux-user/sh4/target_mman.h  |  1 +
 linux-user/sparc/target_mman.h|  1 +
 linux-user/strace.c   | 41 ++
 linux-user/strace.list|  2 +-
 linux-user/x86_64/target_mman.h   |  1 +
 linux-user/xtensa/target_mman.h   |  1 +
 tests/tcg/multiarch/linux/linux-madvise.c | 70 +
 28 files changed, 277 insertions(+), 8 deletions(-)
 create mode 100644 linux-user/aarch64/target_mman.h
 create mode 100644 linux-user/alpha/target_mman.h
 create mode 100644 linux-user/arm/target_mman.h
 create mode 100644 linux-user/cris/target_mman.h
 create mode 100644 linux-user/generic/target_mman.h
 create mode 100644 linux-user/hexagon/target_mman.h
 create mode 100644 linux-user/hppa/target_mman.h
 create mode 100644 linux-user/i386/target_mman.h
 create mode 100644 linux-user/loongarch64/target_mman.h
 create mode 100644 linux-user/m68k/target_mman.h
 create mode 100644 linux-user/microblaze/target_mman.h
 create mode 100644 linux-user/mips/target_mman.h
 create mode 100644 linux-user/mips64/target_mman.h
 create mode 100644 linux-user/nios2/target_mman.h
 create mode 100644 linux-user/openrisc/target_mman.h
 create mode 100644 linux-user/ppc/target_mman.h
 create mode 100644 linux-user/riscv/target_mman.h
 create mode 100644 linux-user/s390x/target_mman.h
 create mode 100644 linux-user/sh4/target_mman.h
 create mode 100644 linux-user/sparc/target_mman.h
 create mode 100644 linux-user/x86_64/target_mman.h
 create mode 100644 linux-user/xtensa/target_mman.h
 create mode 100644 tests/tcg/multiarch/linux/linux-madvise.c

-- 
2.37.2

Re: [PULL 00/11] OpenRISC updates for 7.2.0

[Stefan Hajnoczi]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any 
user-visible changes.


signature.asc
Description: PGP signature

Re: [PATCH v2] linux-user: Implement stracing madvise()

[Ilya Leoshkevich]

On Mon, 2022-09-05 at 23:40 +0200, Ilya Leoshkevich wrote:
> The default implementation has several problems: the first argument
> is
> not displayed as a pointer, making it harder to grep; the third
> argument is not symbolized; and there are several extra unused
> arguments.
> 
> Signed-off-by: Ilya Leoshkevich 
> ---
> 
> v1:
> https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg04275.html
> v1 -> v2: Add all enum values (Richard).
> 
>  linux-user/strace.c    | 67
> ++
>  linux-user/strace.list |  2 +-
>  2 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 7d882526da..da2ff27512 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -2969,6 +2969,73 @@ print_stat(CPUArchState *cpu_env, const struct
> syscallname *name,
>  #define print_lstat64   print_stat
>  #endif
>  
> +#if defined(TARGET_NR_madvise)
> +#define TARGET_MADV_NORMAL 0
> +#define TARGET_MADV_RANDOM 1
> +#define TARGET_MADV_SEQUENTIAL 2
> +#define TARGET_MADV_WILLNEED 3
> +#define TARGET_MADV_DONTNEED 4
> +#define TARGET_MADV_FREE 8
> +#define TARGET_MADV_REMOVE 9
> +#define TARGET_MADV_DONTFORK 10
> +#define TARGET_MADV_DOFORK 11
> +#define TARGET_MADV_HWPOISON 100
> +#define TARGET_MADV_SOFT_OFFLINE 101
> +#define TARGET_MADV_MERGEABLE 12
> +#define TARGET_MADV_UNMERGEABLE 13
> +#define TARGET_MADV_HUGEPAGE 14
> +#define TARGET_MADV_NOHUGEPAGE 15
> +#define TARGET_MADV_DONTDUMP 16
> +#define TARGET_MADV_DODUMP 17
> +#define TARGET_MADV_WIPEONFORK 18
> +#define TARGET_MADV_KEEPONFORK 19
> +#define TARGET_MADV_COLD 20
> +#define TARGET_MADV_PAGEOUT 21
> +#define TARGET_MADV_POPULATE_READ 22
> +#define TARGET_MADV_POPULATE_WRITE 23
> +#define TARGET_MADV_DONTNEED_LOCKED 24
> +
> +static struct enums madvise_advice[] = {
> +    ENUM_TARGET(MADV_NORMAL),
> +    ENUM_TARGET(MADV_RANDOM),
> +    ENUM_TARGET(MADV_SEQUENTIAL),
> +    ENUM_TARGET(MADV_WILLNEED),
> +    ENUM_TARGET(MADV_DONTNEED),
> +    ENUM_TARGET(MADV_FREE),
> +    ENUM_TARGET(MADV_REMOVE),
> +    ENUM_TARGET(MADV_DONTFORK),
> +    ENUM_TARGET(MADV_DOFORK),
> +    ENUM_TARGET(MADV_HWPOISON),
> +    ENUM_TARGET(MADV_SOFT_OFFLINE),
> +    ENUM_TARGET(MADV_MERGEABLE),
> +    ENUM_TARGET(MADV_UNMERGEABLE),
> +    ENUM_TARGET(MADV_HUGEPAGE),
> +    ENUM_TARGET(MADV_NOHUGEPAGE),
> +    ENUM_TARGET(MADV_DONTDUMP),
> +    ENUM_TARGET(MADV_DODUMP),
> +    ENUM_TARGET(MADV_WIPEONFORK),
> +    ENUM_TARGET(MADV_KEEPONFORK),
> +    ENUM_TARGET(MADV_COLD),
> +    ENUM_TARGET(MADV_PAGEOUT),
> +    ENUM_TARGET(MADV_POPULATE_READ),
> +    ENUM_TARGET(MADV_POPULATE_WRITE),
> +    ENUM_TARGET(MADV_DONTNEED_LOCKED),
> +    ENUM_END,
> +};
> +
> +static void
> +print_madvise(CPUArchState *cpu_env, const struct syscallname *name,
> +  abi_long arg0, abi_long arg1, abi_long arg2,
> +  abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_prologue(name);
> +    print_pointer(arg0, 0);
> +    print_raw_param("%d", arg1, 0);
> +    print_enums(madvise_advice, arg2, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  #if defined(TARGET_NR_fstat) || defined(TARGET_NR_fstat64)
>  static void
>  print_fstat(CPUArchState *cpu_env, const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index 72e17b1acf..c93effdbc8 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -541,7 +541,7 @@
>  { TARGET_NR_lstat64, "lstat64" , NULL, print_lstat64, NULL },
>  #endif
>  #ifdef TARGET_NR_madvise
> -{ TARGET_NR_madvise, "madvise" , NULL, NULL, NULL },
> +{ TARGET_NR_madvise, "madvise" , NULL, print_madvise, NULL },
>  #endif
>  #ifdef TARGET_NR_madvise1
>  { TARGET_NR_madvise1, "madvise1" , NULL, NULL, NULL },

Please disregard this patch.

While fixing a test failure on alpha, I noticed that it uses different
values for MADV_* enum.

I will need to provide per-arch MADV_* definitions and also fix
target_madvise() to use TARGET_MADV_DONTNEED instead of MADV_DONTNEED.

Best regards,
Ilya

[PULL 1/1] usb-braille: Better explain that one also has to create a chardev backend

[Samuel Thibault]

Users have reported not to understand the documentation. This completes
it to give an explicit example how one is supposed to set up a virtual
braille USB device.

Signed-off-by: Samuel Thibault 
---
 docs/system/devices/usb.rst | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/docs/system/devices/usb.rst b/docs/system/devices/usb.rst
index f39a88f080..37cb9b33ae 100644
--- a/docs/system/devices/usb.rst
+++ b/docs/system/devices/usb.rst
@@ -178,8 +178,20 @@ option or the ``device_add`` monitor command. Available 
devices are:
host character device id.
 
 ``usb-braille,chardev=id``
-   Braille device. This will use BrlAPI to display the braille output on
-   a real or fake device referenced by id.
+   Braille device. This emulates a Baum Braille device USB port. id has to
+   specify a character device defined with ``-chardev ???,id=id``.  One will
+   normally use BrlAPI to display the braille output on a BRLTTY-supported
+   device with
+
+   .. parsed-literal::
+
+  |qemu_system| [...] -chardev braille,id=brl -device 
usb-braille,chardev=brl
+
+   or alternatively, use the following equivalent shortcut:
+
+   .. parsed-literal::
+
+  |qemu_system| [...] -usbdevice braille
 
 ``usb-net[,netdev=id]``
Network adapter that supports CDC ethernet and RNDIS protocols. id
-- 
2.35.1

[PULL 0/1] baum: better document usb-braille configuration

[Samuel Thibault]

The following changes since commit 3e01455edd5fce06c14e2926b6ef408d9a94c9fb:

  usb-braille: Better explain that one also has to create a chardev backend 
(2022-09-06 00:09:50 +0200)

are available in the Git repository at:

  https://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 3e01455edd5fce06c14e2926b6ef408d9a94c9fb:

  usb-braille: Better explain that one also has to create a chardev backend 
(2022-09-06 00:09:50 +0200)


baum: better document usb-braille configuration

Samuel Thibault (1):
  usb-braille: Better explain that one also has to create a chardev
backend

Re: [PULL v2 00/20] tcg patch queue

[Stefan Hajnoczi]

The tsan (clang) build is broken:
https://gitlab.com/qemu-project/qemu/-/jobs/2982480773

clang-10 -m64 -mcx16 -Ilibqemu-x86_64-linux-user.fa.p -I. -I..
-Itarget/i386 -I../target/i386 -I../common-user/host/x86_64
-I../linux-user/include/host/x86_64 -I../linux-user/include
-Ilinux-user -I../linux-user -Ilinux-user/x86_64
-I../linux-user/x86_64 -Iqapi -Itrace -Iui -Iui/shader
-I/usr/include/capstone -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fcolor-diagnostics -Wall
-Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
/builds/qemu-project/qemu/linux-headers -isystem linux-headers -iquote
. -iquote /builds/qemu-project/qemu -iquote
/builds/qemu-project/qemu/include -iquote
/builds/qemu-project/qemu/tcg/i386 -pthread -fsanitize=thread
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides
-Wno-missing-include-dirs -Wno-shift-negative-value
-Wno-string-plus-int -Wno-typedef-redefinition
-Wno-tautological-type-limit-compare -fstack-protector-strong -fPIE
-isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H
'-DCONFIG_TARGET="x86_64-linux-user-config-target.h"'
'-DCONFIG_DEVICES="x86_64-linux-user-config-devices.h"' -MD -MQ
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o -MF
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o.d -o
libqemu-x86_64-linux-user.fa.p/linux-user_elfload.c.o -c
../linux-user/elfload.c
../linux-user/elfload.c:198:18: error: integer overflow in
preprocessor expression [-Werror]
#if ULONG_MAX >= TARGET_VSYSCALL_PAGE
^~~~
../target/i386/cpu.h:2386:47: note: expanded from macro 'TARGET_VSYSCALL_PAGE'
# define TARGET_VSYSCALL_PAGE (UINT64_C(-10) << 20)
~ ^ ~~

[PATCH v2] linux-user: Implement stracing madvise()

[Ilya Leoshkevich]

The default implementation has several problems: the first argument is
not displayed as a pointer, making it harder to grep; the third
argument is not symbolized; and there are several extra unused
arguments.

Signed-off-by: Ilya Leoshkevich 
---

v1: https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg04275.html
v1 -> v2: Add all enum values (Richard).

 linux-user/strace.c| 67 ++
 linux-user/strace.list |  2 +-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 7d882526da..da2ff27512 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -2969,6 +2969,73 @@ print_stat(CPUArchState *cpu_env, const struct 
syscallname *name,
 #define print_lstat64   print_stat
 #endif
 
+#if defined(TARGET_NR_madvise)
+#define TARGET_MADV_NORMAL 0
+#define TARGET_MADV_RANDOM 1
+#define TARGET_MADV_SEQUENTIAL 2
+#define TARGET_MADV_WILLNEED 3
+#define TARGET_MADV_DONTNEED 4
+#define TARGET_MADV_FREE 8
+#define TARGET_MADV_REMOVE 9
+#define TARGET_MADV_DONTFORK 10
+#define TARGET_MADV_DOFORK 11
+#define TARGET_MADV_HWPOISON 100
+#define TARGET_MADV_SOFT_OFFLINE 101
+#define TARGET_MADV_MERGEABLE 12
+#define TARGET_MADV_UNMERGEABLE 13
+#define TARGET_MADV_HUGEPAGE 14
+#define TARGET_MADV_NOHUGEPAGE 15
+#define TARGET_MADV_DONTDUMP 16
+#define TARGET_MADV_DODUMP 17
+#define TARGET_MADV_WIPEONFORK 18
+#define TARGET_MADV_KEEPONFORK 19
+#define TARGET_MADV_COLD 20
+#define TARGET_MADV_PAGEOUT 21
+#define TARGET_MADV_POPULATE_READ 22
+#define TARGET_MADV_POPULATE_WRITE 23
+#define TARGET_MADV_DONTNEED_LOCKED 24
+
+static struct enums madvise_advice[] = {
+ENUM_TARGET(MADV_NORMAL),
+ENUM_TARGET(MADV_RANDOM),
+ENUM_TARGET(MADV_SEQUENTIAL),
+ENUM_TARGET(MADV_WILLNEED),
+ENUM_TARGET(MADV_DONTNEED),
+ENUM_TARGET(MADV_FREE),
+ENUM_TARGET(MADV_REMOVE),
+ENUM_TARGET(MADV_DONTFORK),
+ENUM_TARGET(MADV_DOFORK),
+ENUM_TARGET(MADV_HWPOISON),
+ENUM_TARGET(MADV_SOFT_OFFLINE),
+ENUM_TARGET(MADV_MERGEABLE),
+ENUM_TARGET(MADV_UNMERGEABLE),
+ENUM_TARGET(MADV_HUGEPAGE),
+ENUM_TARGET(MADV_NOHUGEPAGE),
+ENUM_TARGET(MADV_DONTDUMP),
+ENUM_TARGET(MADV_DODUMP),
+ENUM_TARGET(MADV_WIPEONFORK),
+ENUM_TARGET(MADV_KEEPONFORK),
+ENUM_TARGET(MADV_COLD),
+ENUM_TARGET(MADV_PAGEOUT),
+ENUM_TARGET(MADV_POPULATE_READ),
+ENUM_TARGET(MADV_POPULATE_WRITE),
+ENUM_TARGET(MADV_DONTNEED_LOCKED),
+ENUM_END,
+};
+
+static void
+print_madvise(CPUArchState *cpu_env, const struct syscallname *name,
+  abi_long arg0, abi_long arg1, abi_long arg2,
+  abi_long arg3, abi_long arg4, abi_long arg5)
+{
+print_syscall_prologue(name);
+print_pointer(arg0, 0);
+print_raw_param("%d", arg1, 0);
+print_enums(madvise_advice, arg2, 1);
+print_syscall_epilogue(name);
+}
+#endif
+
 #if defined(TARGET_NR_fstat) || defined(TARGET_NR_fstat64)
 static void
 print_fstat(CPUArchState *cpu_env, const struct syscallname *name,
diff --git a/linux-user/strace.list b/linux-user/strace.list
index 72e17b1acf..c93effdbc8 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -541,7 +541,7 @@
 { TARGET_NR_lstat64, "lstat64" , NULL, print_lstat64, NULL },
 #endif
 #ifdef TARGET_NR_madvise
-{ TARGET_NR_madvise, "madvise" , NULL, NULL, NULL },
+{ TARGET_NR_madvise, "madvise" , NULL, print_madvise, NULL },
 #endif
 #ifdef TARGET_NR_madvise1
 { TARGET_NR_madvise1, "madvise1" , NULL, NULL, NULL },
-- 
2.37.2

Re: [PATCH v2] kvm: fix memory leak on failure to read stats descriptors

[Philippe Mathieu-Daudé via]

On Mon, Sep 5, 2022 at 3:28 PM Paolo Bonzini  wrote:
>
> Reported by Coverity as CID 1490142.  Since the size is constant and the
> lifetime is the same as the StatsDescriptors struct, embed the struct
> directly instead of using a separate allocation.
>
> Suggested-by: Richard Henderson 
> Signed-off-by: Paolo Bonzini 
> ---
>  accel/kvm/kvm-all.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2] smbios: sanitize type from external type before checking have_fields_bitmap

[Philippe Mathieu-Daudé via]

On Mon, Sep 5, 2022 at 10:44 PM Paolo Bonzini  wrote:
>
> test_bit uses header->type as an offset; if the file incorrectly specifies a
> type greater than 127, smbios_entry_add will read and write garbage.
>
> To fix this, just pass the smbios data through, assuming the user knows what
> to do.  Reported by Coverity as CID 1487255.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/smbios/smbios.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 6/6] include/exec: Introduce TARGET_PAGE_ENTRY_EXTRA

[Philippe Mathieu-Daudé via]

On 5/9/22 22:22, Richard Henderson wrote:

Allow the target to cache items from the guest page tables.

Signed-off-by: Richard Henderson 
---
  include/exec/cpu-defs.h | 9 +
  1 file changed, 9 insertions(+)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 5e12cc1854..67239b4e5e 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -163,6 +163,15 @@ typedef struct CPUTLBEntryFull {
  
  /* @lg_page_size contains the log2 of the page size. */

  uint8_t lg_page_size;
+
+/*
+ * Allow target-specific additions to this structure.
+ * This may be used to cache items from the guest cpu
+ * page tables for later use by the implementation.
+ */
+#ifdef TARGET_PAGE_ENTRY_EXTRA
+TARGET_PAGE_ENTRY_EXTRA
+#endif
  } CPUTLBEntryFull;


Alternatively declare a per-target structure in cpu-param.h
and here:

typedef struct CPUTLBEntryTarget CPUTLBEntryTarget;

#ifndef TARGET_HAS_PAGE_ENTRY_EXTRA_STRUCT
struct CPUTLBEntryTarget { }
#endif


typedef struct CPUTLBEntryFull {
  ...
  CPUTLBEntryTarget target_extra;
} CPUTLBEntryFull;

Meanwhile:
Reviewed-by: Philippe Mathieu-Daudé 

Re: sphinx-build is really slow, any way to improve that?

[Peter Maydell]

On Mon, 5 Sept 2022 at 20:51, Claudio Fontana  wrote:
> when I build qemu, there is a lot of time spent at the end of the build where 
> one cpu goes 100% on sphinx-build.
>
> Is there some way to parallelize that? It seems it is the current bottleneck 
> for rebuilds for me..

It's a big fat python program, so I suspect not, but
maybe I'm wrong.

You can always configure --disable-docs if you don't care
about the docs and want to make builds faster.

thanks
-- PMM

Re: [PATCH v3 5/6] accel/tcg: Introduce tlb_set_page_full

[Philippe Mathieu-Daudé via]

On 5/9/22 22:22, Richard Henderson wrote:

Now that we have collected all of the page data into
CPUTLBEntryFull, provide an interface to record that
all in one go, instead of using 4 arguments.  This interface
allows CPUTLBEntryFull to be extended without having to
change the number of arguments.

Signed-off-by: Richard Henderson 
---
  include/exec/cpu-defs.h | 14 ++
  include/exec/exec-all.h | 22 +++
  accel/tcg/cputlb.c  | 62 -
  3 files changed, 78 insertions(+), 20 deletions(-)



@@ -1117,35 +1117,36 @@ void tlb_set_page_with_attrs(CPUState *cpu, 
target_ulong vaddr,
  CPUTLBEntry *te, tn;
  hwaddr iotlb, xlat, sz, paddr_page;
  target_ulong vaddr_page;
-int asidx = cpu_asidx_from_attrs(cpu, attrs);
-int wp_flags;
+int asidx, wp_flags, prot;
  bool is_ram, is_romd;
  
  assert_cpu_is_self(cpu);
  
-if (size <= TARGET_PAGE_SIZE) {

+if (full->lg_page_size <= TARGET_PAGE_BITS) {
  sz = TARGET_PAGE_SIZE;
  } else {
-tlb_add_large_page(env, mmu_idx, vaddr, size);
-sz = size;
+sz = (hwaddr)1 << full->lg_page_size;


Could use BIT_ULL() here.

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH V2 1/3] hw/arm,loongarch: Move load_image_to_fw_cfg() to common location

[Peter Maydell]

On Mon, 5 Sept 2022 at 19:23, Sunil V L  wrote:
>
> load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> function will be required by riscv too. So, it's time to refactor and
> move this function to a common path.
>
> Signed-off-by: Sunil V L 
> ---
>  hw/arm/boot.c | 49 ---
>  hw/loongarch/virt.c   | 33 --
>  hw/nvram/fw_cfg.c | 49 +++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  4 files changed, 52 insertions(+), 82 deletions(-)


> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d605f3f45a..3f68dae5d2 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -41,6 +41,7 @@
>  #include "qapi/error.h"
>  #include "hw/acpi/aml-build.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/loader.h"
>
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>
> @@ -1221,6 +1222,54 @@ FWCfgState *fw_cfg_find(void)
>  return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>  }
>
> +/**
> + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry 
> identified
> + *  by key.

For functions declared in public header files, put the doc comment
in the .h file, not the .c file, please.

thanks
-- PMM

Re: [PATCH v3 4/6] accel/tcg: Introduce probe_access_full

[Philippe Mathieu-Daudé via]

On 5/9/22 22:22, Richard Henderson wrote:

Add an interface to return the CPUTLBEntryFull struct
that goes with the lookup.  The result is not intended
to be valid across multiple lookups, so the user must
use the results immediately.

Signed-off-by: Richard Henderson 
---
  include/exec/exec-all.h | 11 ++
  accel/tcg/cputlb.c  | 47 +
  2 files changed, 40 insertions(+), 18 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 2/6] accel/tcg: Drop addr member from SavedIOTLB

[Philippe Mathieu-Daudé via]

On 5/9/22 22:22, Richard Henderson wrote:

This field is only written, not read; remove it.

Signed-off-by: Richard Henderson 
---
  include/hw/core/cpu.h | 1 -
  accel/tcg/cputlb.c| 7 +++
  2 files changed, 3 insertions(+), 5 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 1/6] accel/tcg: Rename CPUIOTLBEntry to CPUTLBEntryFull

[Philippe Mathieu-Daudé via]

On 5/9/22 22:22, Richard Henderson wrote:

This structure will shortly contain more than just
data for accessing MMIO.  Rename the 'addr' member
to 'xlat_section' to more clearly indicate its purpose.

Signed-off-by: Richard Henderson 
---
  include/exec/cpu-defs.h|  22 
  accel/tcg/cputlb.c | 102 +++--
  target/arm/mte_helper.c|  14 ++---
  target/arm/sve_helper.c|   4 +-
  target/arm/translate-a64.c |   2 +-
  5 files changed, 73 insertions(+), 71 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] build: Regenerate meson-buildoptions.sh

[Stefan Hajnoczi]

On Mon, 5 Sept 2022 at 16:41, Richard Henderson
 wrote:
>
> Regeneration was missed by the previous update.
>
> Fixes: 2a2359b84407 ("vduse-blk: Implement vduse-blk export")
> Signed-off-by: Richard Henderson 
> ---
>  scripts/meson-buildoptions.sh | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 359b04e0e6..300ba8b212 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -42,12 +42,13 @@ meson_options_help() {
>printf "%s\n" '  --enable-trace-backends=CHOICES'
>printf "%s\n" '   Set available tracing backends 
> [log] (choices:'
>printf "%s\n" '   
> dtrace/ftrace/log/nop/simple/syslog/ust)'
> -  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
> [share/qemu-firmware]'
> +  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
> [share/qemu-'
> +  printf "%s\n" '   firmware]'
>printf "%s\n" '  --iasl=VALUE Path to ACPI disassembler'
>printf "%s\n" '  --includedir=VALUE   Header file directory [include]'
>printf "%s\n" '  --interp-prefix=VALUEwhere to find shared libraries 
> etc., use %M for'
>printf "%s\n" '   cpu name [/usr/gnemul/qemu-%M]'
> -  printf "%s\n" '  --libdir=VALUE   Library directory [lib64]'
> +  printf "%s\n" '  --libdir=VALUE   Library directory 
> [lib/x86_64-linux-gnu]'

Uh oh, this is distro-specific. Does this mean meson-buildoptions.sh
output is unstable?

Stefan

Re: [PATCH] build: Regenerate meson-buildoptions.sh

[Philippe Mathieu-Daudé via]

On 5/9/22 22:41, Richard Henderson wrote:

Regeneration was missed by the previous update.

Fixes: 2a2359b84407 ("vduse-blk: Implement vduse-blk export")
Signed-off-by: Richard Henderson 
---
  scripts/meson-buildoptions.sh | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 359b04e0e6..300ba8b212 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -42,12 +42,13 @@ meson_options_help() {
printf "%s\n" '  --enable-trace-backends=CHOICES'
printf "%s\n" '   Set available tracing backends 
[log] (choices:'
printf "%s\n" '   
dtrace/ftrace/log/nop/simple/syslog/ust)'
-  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
[share/qemu-firmware]'
+  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
[share/qemu-'
+  printf "%s\n" '   firmware]'


This path split is not very handy to copy/paste.


printf "%s\n" '  --iasl=VALUE Path to ACPI disassembler'
printf "%s\n" '  --includedir=VALUE   Header file directory [include]'
printf "%s\n" '  --interp-prefix=VALUEwhere to find shared libraries 
etc., use %M for'
printf "%s\n" '   cpu name [/usr/gnemul/qemu-%M]'
-  printf "%s\n" '  --libdir=VALUE   Library directory [lib64]'
+  printf "%s\n" '  --libdir=VALUE   Library directory 
[lib/x86_64-linux-gnu]'


This patch seems distro-specific (previous to this patch).

Anyhow:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] linux-user: fix bug about missing signum convert of sigqueue

[Philippe Mathieu-Daudé via]

On 31/8/22 06:10, fa...@mail.ustc.edu.cn wrote:

 From 4ebe8a67ed7c4b1220957b2b67a62ba60e0e80ec Mon Sep 17 00:00:00 2001
From: fanwenjie 
Date: Wed, 31 Aug 2022 11:55:25 +0800
Subject: [PATCH] linux-user: fix bug about missing signum convert of 
sigqueue


Signed-off-by: fanwenjie 
---
  linux-user/syscall.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f409121202..3e5ab4f0b2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9690,7 +9690,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, 
int num, abi_long arg1,

              }
              target_to_host_siginfo(&uinfo, p);
              unlock_user(p, arg3, 0);
-            ret = get_errno(sys_rt_sigqueueinfo(arg1, arg2, &uinfo));
+            ret = get_errno(sys_rt_sigqueueinfo(arg1, 
target_to_host_signal(arg2), &uinfo));


Fixes: 66fb9763af ("basic signal handling")

Date:   Sun Mar 23 01:06:05 2003 +

!@#% ALMOST 20 YEARS %#!@#$

Cc'ing Alex for an entry in the oldest bug fixed table.


      case TARGET_NR_rt_tgsigqueueinfo:
@@ -9703,7 +9703,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, 
int num, abi_long arg1,

              }
              target_to_host_siginfo(&uinfo, p);
              unlock_user(p, arg4, 0);
-            ret = get_errno(sys_rt_tgsigqueueinfo(arg1, arg2, arg3, 
&uinfo));
+            ret = get_errno(sys_rt_tgsigqueueinfo(arg1, arg2, 
target_to_host_signal(arg3), &uinfo));


Fixes: cf8b8bfc50 ("linux-user: add support for rt_tgsigqueueinfo() 
system call")



          }
          return ret;
  #ifdef TARGET_NR_sigreturn
--



Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC] module: removed unused function argument "mayfail"

[Philippe Mathieu-Daudé via]

On 5/9/22 17:55, Claudio Fontana wrote:

mayfail is always passed as false for every invocation throughout the program.
It controls whether to printf or not to printf an error on
g_module_open failure.

Remove this unused argument.

Signed-off-by: Claudio Fontana 
---
  include/qemu/module.h |  8 
  softmmu/qtest.c   |  2 +-
  util/module.c | 20 +---
  3 files changed, 14 insertions(+), 16 deletions(-)


Why RFC?

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC v4 11/11] virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint

[Stefan Hajnoczi]

On Fri, Sep 02, 2022 at 10:06:45AM +0200, David Hildenbrand wrote:
> On 30.08.22 22:16, Stefan Hajnoczi wrote:
> > On Thu, Aug 25, 2022 at 09:43:16AM +0200, David Hildenbrand wrote:
> >> On 23.08.22 21:22, Stefan Hajnoczi wrote:
> >>> On Tue, Aug 23, 2022 at 10:01:59AM +0200, David Hildenbrand wrote:
>  On 23.08.22 00:24, Stefan Hajnoczi wrote:
> > Register guest RAM using BlockRAMRegistrar and set the
> > BDRV_REQ_REGISTERED_BUF flag so block drivers can optimize memory
> > accesses in I/O requests.
> >
> > This is for vdpa-blk, vhost-user-blk, and other I/O interfaces that rely
> > on DMA mapping/unmapping.
> 
>  Can you explain why we're monitoring RAMRegistrar to hook into "guest
>  RAM" and not go the usual path of the MemoryListener?
> >>>
> >>> The requirements are similar to VFIO, which uses RAMBlockNotifier. We
> >>
> >> Only VFIO NVME uses RAMBlockNotifier. Ordinary VFIO uses the 
> >> MemoryListener.
> >>
> >> Maybe the difference is that ordinary VFIO has to replicate the actual
> >> guest physical memory layout, and VFIO NVME is only interested in
> >> possible guest RAM inside guest physical memory.
> >>
> >>> need to learn about all guest RAM because that's where I/O buffers are
> >>> located.
> >>>
> >>> Do you think RAMBlockNotifier should be avoided?
> >>
> >> I assume it depends on the use case. For saying "this might be used for
> >> I/O" it might be good enough I guess.
> >>
> >>>
>  What will BDRV_REQ_REGISTERED_BUF actually do? Pin all guest memory in
>  the worst case such as io_uring fixed buffers would do ( I hope not ).
> >>>
> >>> BLK_REQ_REGISTERED_BUF is a hint that no bounce buffer is necessary
> >>> because the I/O buffer is located in memory that was previously
> >>> registered with bdrv_registered_buf().
> >>>
> >>> The RAMBlockNotifier calls bdrv_register_buf() to let the libblkio
> >>> driver know about RAM. Some libblkio drivers ignore this hint, io_uring
> >>> may use the fixed buffers feature, vhost-user sends the shared memory
> >>> file descriptors to the vhost device server, and VFIO/vhost may pin
> >>> pages.
> >>>
> >>> So the blkio block driver doesn't add anything new, it's the union of
> >>> VFIO/vhost/vhost-user/etc memory requirements.
> >>
> >> The issue is if that backend pins memory inside any of these regions.
> >> Then, you're instantly incompatible to anything the relies on sparse
> >> RAMBlocks, such as memory ballooning or virtio-mem, and have to properly
> >> fence it.
> >>
> >> In that case, you'd have to successfully trigger
> >> ram_block_discard_disable(true) first, before pinning. Who would do that
> >> now conditionally, just like e.g., VFIO does?
> >>
> >> io_uring fixed buffers would be one such example that pins memory and is
> >> problematic. vfio (unless on s390x) is another example, as you point out.
> > 
> > Okay, I think libblkio needs to expose a bool property called
> > "mem-regions-pinned" so QEMU whether or not the registered buffers will
> > be pinned.
> > 
> > Then the QEMU BlockDriver can do:
> > 
> >   if (mem_regions_pinned) {
> >   if (ram_block_discard_disable(true) < 0) {
> >   ...fail to open block device...
> >   }
> >   }
> > 
> > Does that sound right?
> 
> Yes, I think so.
> 
> > 
> > Is "pinned" the best word to describe this or is there a more general
> > characteristic we are looking for?
> 
> pinning should be the right term. We want to express that all user page
> tables will immediately get populated and that a kernel subsystem will
> take longterm references on mapped page that will go out of sync as soon
> as we discard memory e.g., using madvise(MADV_DONTEED).
> 
> We just should not confuse it with memlock / locking into memory, which
> are yet different semantics (e.g., don't swap it out).
> 
> > 
> >>
> >> This has to be treated with care. Another thing to consider is that
> >> different backends might only support a limited number of such regions.
> >> I assume there is a way for QEMU to query this limit upfront? It might
> >> be required for memory hot(un)plug to figure out how many memory slots
> >> we actually have (for ordinary DIMMs, and if we ever want to make this
> >> compatible to virtio-mem, it might be required as well when the backend
> >> pins memory).
> > 
> > Yes, libblkio reports the maximum number of blkio_mem_regions supported
> > by the device. The property is called "max-mem-regions".
> > 
> > The QEMU BlockDriver currently doesn't use this information. Are there
> > any QEMU APIs that should be called to propagate this value?
> 
> I assume we have to do exactly the same thing as e.g.,
> vhost_has_free_slot()/kvm_has_free_slot() does.
> 
> Especially, hw/mem/memory-device.c needs care and
> slots_limit/used_memslots handling in hw/virtio/vhost.c might be
> relevant as well.
> 
> 
> Note that I have some patches pending that extend that handling, by also
> providing how many used+free slots there are, such as:
> 
> https://l

[PATCH v2] smbios: sanitize type from external type before checking have_fields_bitmap

[Paolo Bonzini]

test_bit uses header->type as an offset; if the file incorrectly specifies a
type greater than 127, smbios_entry_add will read and write garbage.

To fix this, just pass the smbios data through, assuming the user knows what
to do.  Reported by Coverity as CID 1487255.

Signed-off-by: Paolo Bonzini 
---
 hw/smbios/smbios.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 60349ee402..4c9f664830 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -1205,13 +1205,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
 return;
 }
 
-if (test_bit(header->type, have_fields_bitmap)) {
-error_setg(errp,
-   "can't load type %d struct, fields already specified!",
-   header->type);
-return;
+if (header->type <= SMBIOS_MAX_TYPE) {
+if (test_bit(header->type, have_fields_bitmap)) {
+error_setg(errp,
+   "can't load type %d struct, fields already 
specified!",
+   header->type);
+return;
+}
+set_bit(header->type, have_binfile_bitmap);
 }
-set_bit(header->type, have_binfile_bitmap);
 
 if (header->type == 4) {
 smbios_type4_count++;
-- 
2.37.2

[PATCH] build: Regenerate meson-buildoptions.sh

[Richard Henderson]

Regeneration was missed by the previous update.

Fixes: 2a2359b84407 ("vduse-blk: Implement vduse-blk export")
Signed-off-by: Richard Henderson 
---
 scripts/meson-buildoptions.sh | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 359b04e0e6..300ba8b212 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -42,12 +42,13 @@ meson_options_help() {
   printf "%s\n" '  --enable-trace-backends=CHOICES'
   printf "%s\n" '   Set available tracing backends 
[log] (choices:'
   printf "%s\n" '   
dtrace/ftrace/log/nop/simple/syslog/ust)'
-  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
[share/qemu-firmware]'
+  printf "%s\n" '  --firmwarepath=VALUESsearch PATH for firmware files 
[share/qemu-'
+  printf "%s\n" '   firmware]'
   printf "%s\n" '  --iasl=VALUE Path to ACPI disassembler'
   printf "%s\n" '  --includedir=VALUE   Header file directory [include]'
   printf "%s\n" '  --interp-prefix=VALUEwhere to find shared libraries 
etc., use %M for'
   printf "%s\n" '   cpu name [/usr/gnemul/qemu-%M]'
-  printf "%s\n" '  --libdir=VALUE   Library directory [lib64]'
+  printf "%s\n" '  --libdir=VALUE   Library directory 
[lib/x86_64-linux-gnu]'
   printf "%s\n" '  --libexecdir=VALUE   Library executable directory 
[libexec]'
   printf "%s\n" '  --localedir=VALUELocale data directory 
[share/locale]'
   printf "%s\n" '  --localstatedir=VALUELocalstate data directory 
[/var/local]'
@@ -154,6 +155,8 @@ meson_options_help() {
   printf "%s\n" '  usb-redir   libusbredir support'
   printf "%s\n" '  vde vde network backend support'
   printf "%s\n" '  vdi vdi image format support'
+  printf "%s\n" '  vduse-blk-export'
+  printf "%s\n" '  VDUSE block export support'
   printf "%s\n" '  vfio-user-server'
   printf "%s\n" '  vfio-user server support'
   printf "%s\n" '  vhost-cryptovhost-user crypto backend support'
@@ -162,8 +165,6 @@ meson_options_help() {
   printf "%s\n" '  vhost-user  vhost-user backend support'
   printf "%s\n" '  vhost-user-blk-server'
   printf "%s\n" '  build vhost-user-blk server'
-  printf "%s\n" '  vduse-blk-export'
-  printf "%s\n" '  VDUSE block export support'
   printf "%s\n" '  vhost-vdpa  vhost-vdpa kernel backend support'
   printf "%s\n" '  virglrenderer   virgl rendering support'
   printf "%s\n" '  virtfs  virtio-9p support'
@@ -422,6 +423,8 @@ _meson_option_parse() {
 --disable-vde) printf "%s" -Dvde=disabled ;;
 --enable-vdi) printf "%s" -Dvdi=enabled ;;
 --disable-vdi) printf "%s" -Dvdi=disabled ;;
+--enable-vduse-blk-export) printf "%s" -Dvduse_blk_export=enabled ;;
+--disable-vduse-blk-export) printf "%s" -Dvduse_blk_export=disabled ;;
 --enable-vfio-user-server) printf "%s" -Dvfio_user_server=enabled ;;
 --disable-vfio-user-server) printf "%s" -Dvfio_user_server=disabled ;;
 --enable-vhost-crypto) printf "%s" -Dvhost_crypto=enabled ;;
@@ -434,8 +437,6 @@ _meson_option_parse() {
 --disable-vhost-user) printf "%s" -Dvhost_user=disabled ;;
 --enable-vhost-user-blk-server) printf "%s" 
-Dvhost_user_blk_server=enabled ;;
 --disable-vhost-user-blk-server) printf "%s" 
-Dvhost_user_blk_server=disabled ;;
---enable-vduse-blk-export) printf "%s" -Dvduse_blk_export=enabled ;;
---disable-vduse-blk-export) printf "%s" -Dvduse_blk_export=disabled ;;
 --enable-vhost-vdpa) printf "%s" -Dvhost_vdpa=enabled ;;
 --disable-vhost-vdpa) printf "%s" -Dvhost_vdpa=disabled ;;
 --enable-virglrenderer) printf "%s" -Dvirglrenderer=enabled ;;
-- 
2.34.1

[PATCH v3 4/6] accel/tcg: Introduce probe_access_full

[Richard Henderson]

Add an interface to return the CPUTLBEntryFull struct
that goes with the lookup.  The result is not intended
to be valid across multiple lookups, so the user must
use the results immediately.

Signed-off-by: Richard Henderson 
---
 include/exec/exec-all.h | 11 ++
 accel/tcg/cputlb.c  | 47 +
 2 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index bcad607c4e..758cf6bcc7 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -434,6 +434,17 @@ int probe_access_flags(CPUArchState *env, target_ulong 
addr,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t retaddr);
 
+#ifndef CONFIG_USER_ONLY
+/**
+ * probe_access_full:
+ * Like probe_access_flags, except also return into @pfull.
+ */
+int probe_access_full(CPUArchState *env, target_ulong addr,
+  MMUAccessType access_type, int mmu_idx,
+  bool nonfault, void **phost,
+  CPUTLBEntryFull **pfull, uintptr_t retaddr);
+#endif
+
 #define CODE_GEN_ALIGN   16 /* must be >= of the size of a icache line 
*/
 
 /* Estimated block size for TB allocation.  */
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 91f2b53142..62159549f6 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1512,7 +1512,8 @@ static void notdirty_write(CPUState *cpu, vaddr 
mem_vaddr, unsigned size,
 static int probe_access_internal(CPUArchState *env, target_ulong addr,
  int fault_size, MMUAccessType access_type,
  int mmu_idx, bool nonfault,
- void **phost, uintptr_t retaddr)
+ void **phost, CPUTLBEntryFull **pfull,
+ uintptr_t retaddr)
 {
 uintptr_t index = tlb_index(env, mmu_idx, addr);
 CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
@@ -1546,10 +1547,12 @@ static int probe_access_internal(CPUArchState *env, 
target_ulong addr,
mmu_idx, nonfault, retaddr)) {
 /* Non-faulting page table read failed.  */
 *phost = NULL;
+*pfull = NULL;
 return TLB_INVALID_MASK;
 }
 
 /* TLB resize via tlb_fill may have moved the entry.  */
+index = tlb_index(env, mmu_idx, addr);
 entry = tlb_entry(env, mmu_idx, addr);
 
 /*
@@ -1563,6 +1566,8 @@ static int probe_access_internal(CPUArchState *env, 
target_ulong addr,
 }
 flags &= tlb_addr;
 
+*pfull = &env_tlb(env)->d[mmu_idx].fulltlb[index];
+
 /* Fold all "mmio-like" bits into TLB_MMIO.  This is not RAM.  */
 if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) {
 *phost = NULL;
@@ -1574,37 +1579,44 @@ static int probe_access_internal(CPUArchState *env, 
target_ulong addr,
 return flags;
 }
 
-int probe_access_flags(CPUArchState *env, target_ulong addr,
-   MMUAccessType access_type, int mmu_idx,
-   bool nonfault, void **phost, uintptr_t retaddr)
+int probe_access_full(CPUArchState *env, target_ulong addr,
+  MMUAccessType access_type, int mmu_idx,
+  bool nonfault, void **phost, CPUTLBEntryFull **pfull,
+  uintptr_t retaddr)
 {
-int flags;
-
-flags = probe_access_internal(env, addr, 0, access_type, mmu_idx,
-  nonfault, phost, retaddr);
+int flags = probe_access_internal(env, addr, 0, access_type, mmu_idx,
+  nonfault, phost, pfull, retaddr);
 
 /* Handle clean RAM pages.  */
 if (unlikely(flags & TLB_NOTDIRTY)) {
-uintptr_t index = tlb_index(env, mmu_idx, addr);
-CPUTLBEntryFull *full = &env_tlb(env)->d[mmu_idx].fulltlb[index];
-
-notdirty_write(env_cpu(env), addr, 1, full, retaddr);
+notdirty_write(env_cpu(env), addr, 1, *pfull, retaddr);
 flags &= ~TLB_NOTDIRTY;
 }
 
 return flags;
 }
 
+int probe_access_flags(CPUArchState *env, target_ulong addr,
+   MMUAccessType access_type, int mmu_idx,
+   bool nonfault, void **phost, uintptr_t retaddr)
+{
+CPUTLBEntryFull *full;
+
+return probe_access_full(env, addr, access_type, mmu_idx,
+ nonfault, phost, &full, retaddr);
+}
+
 void *probe_access(CPUArchState *env, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
 {
+CPUTLBEntryFull *full;
 void *host;
 int flags;
 
 g_assert(-(addr | TARGET_PAGE_MASK) >= size);
 
 flags = probe_access_internal(env, addr, size, access_type, mmu_idx,
-  false, &host, retaddr);
+  fal

[PATCH v3 5/6] accel/tcg: Introduce tlb_set_page_full

[Richard Henderson]

Now that we have collected all of the page data into
CPUTLBEntryFull, provide an interface to record that
all in one go, instead of using 4 arguments.  This interface
allows CPUTLBEntryFull to be extended without having to
change the number of arguments.

Signed-off-by: Richard Henderson 
---
 include/exec/cpu-defs.h | 14 ++
 include/exec/exec-all.h | 22 +++
 accel/tcg/cputlb.c  | 62 -
 3 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index f70f54d850..5e12cc1854 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -148,7 +148,21 @@ typedef struct CPUTLBEntryFull {
  * + the offset within the target MemoryRegion (otherwise)
  */
 hwaddr xlat_section;
+
+/*
+ * @phys_addr contains the physical address in the address space
+ * given by cpu_asidx_from_attrs(cpu, @attrs).
+ */
+hwaddr phys_addr;
+
+/* @attrs contains the memory transaction attributes for the page. */
 MemTxAttrs attrs;
+
+/* @prot contains the complete protections for the page. */
+uint8_t prot;
+
+/* @lg_page_size contains the log2 of the page size. */
+uint8_t lg_page_size;
 } CPUTLBEntryFull;
 
 /*
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 758cf6bcc7..1a30c857f4 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -257,6 +257,28 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState 
*cpu,
uint16_t idxmap,
unsigned bits);
 
+/**
+ * tlb_set_page_full:
+ * @cpu: CPU context
+ * @mmu_idx: mmu index of the tlb to modify
+ * @vaddr: virtual address of the entry to add
+ * @full: the details of the tlb entry
+ *
+ * Add an entry to @cpu tlb index @mmu_idx.  All of the fields of
+ * @full must be filled, except for xlat_section, and constitute
+ * the complete description of the translated page.
+ *
+ * This is generally called by the target tlb_fill function after
+ * having performed a successful page table walk to find the physical
+ * address and attributes for the translation.
+ *
+ * At most one entry for a given virtual address is permitted. Only a
+ * single TARGET_PAGE_SIZE region is mapped; @full->ld_page_size is only
+ * used by tlb_flush_page.
+ */
+void tlb_set_page_full(CPUState *cpu, int mmu_idx, target_ulong vaddr,
+   CPUTLBEntryFull *full);
+
 /**
  * tlb_set_page_with_attrs:
  * @cpu: CPU to add this TLB entry for
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 62159549f6..3a3549ad4a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1095,16 +1095,16 @@ static void tlb_add_large_page(CPUArchState *env, int 
mmu_idx,
 env_tlb(env)->d[mmu_idx].large_page_mask = lp_mask;
 }
 
-/* Add a new TLB entry. At most one entry for a given virtual address
+/*
+ * Add a new TLB entry. At most one entry for a given virtual address
  * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the
  * supplied size is only used by tlb_flush_page.
  *
  * Called from TCG-generated code, which is under an RCU read-side
  * critical section.
  */
-void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
- hwaddr paddr, MemTxAttrs attrs, int prot,
- int mmu_idx, target_ulong size)
+void tlb_set_page_full(CPUState *cpu, int mmu_idx,
+   target_ulong vaddr, CPUTLBEntryFull *full)
 {
 CPUArchState *env = cpu->env_ptr;
 CPUTLB *tlb = env_tlb(env);
@@ -1117,35 +1117,36 @@ void tlb_set_page_with_attrs(CPUState *cpu, 
target_ulong vaddr,
 CPUTLBEntry *te, tn;
 hwaddr iotlb, xlat, sz, paddr_page;
 target_ulong vaddr_page;
-int asidx = cpu_asidx_from_attrs(cpu, attrs);
-int wp_flags;
+int asidx, wp_flags, prot;
 bool is_ram, is_romd;
 
 assert_cpu_is_self(cpu);
 
-if (size <= TARGET_PAGE_SIZE) {
+if (full->lg_page_size <= TARGET_PAGE_BITS) {
 sz = TARGET_PAGE_SIZE;
 } else {
-tlb_add_large_page(env, mmu_idx, vaddr, size);
-sz = size;
+sz = (hwaddr)1 << full->lg_page_size;
+tlb_add_large_page(env, mmu_idx, vaddr, sz);
 }
 vaddr_page = vaddr & TARGET_PAGE_MASK;
-paddr_page = paddr & TARGET_PAGE_MASK;
+paddr_page = full->phys_addr & TARGET_PAGE_MASK;
 
+prot = full->prot;
+asidx = cpu_asidx_from_attrs(cpu, full->attrs);
 section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
-&xlat, &sz, attrs, &prot);
+&xlat, &sz, full->attrs, 
&prot);
 assert(sz >= TARGET_PAGE_SIZE);
 
 tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
   " prot=%x idx=%d\n",
-  vaddr, paddr, prot, mmu_idx);
+  vaddr, full->phys_addr, prot

[PATCH v3 2/6] accel/tcg: Drop addr member from SavedIOTLB

[Richard Henderson]

This field is only written, not read; remove it.

Signed-off-by: Richard Henderson 
---
 include/hw/core/cpu.h | 1 -
 accel/tcg/cputlb.c| 7 +++
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 500503da13..9e47184513 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -218,7 +218,6 @@ struct CPUWatchpoint {
  * the memory regions get moved around  by io_writex.
  */
 typedef struct SavedIOTLB {
-hwaddr addr;
 MemoryRegionSection *section;
 hwaddr mr_offset;
 } SavedIOTLB;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 4585d7c015..03395e725d 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1374,12 +1374,11 @@ static uint64_t io_readx(CPUArchState *env, 
CPUTLBEntryFull *full,
  * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
  * because of the side effect of io_writex changing memory layout.
  */
-static void save_iotlb_data(CPUState *cs, hwaddr addr,
-MemoryRegionSection *section, hwaddr mr_offset)
+static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
+hwaddr mr_offset)
 {
 #ifdef CONFIG_PLUGIN
 SavedIOTLB *saved = &cs->saved_iotlb;
-saved->addr = addr;
 saved->section = section;
 saved->mr_offset = mr_offset;
 #endif
@@ -1408,7 +1407,7 @@ static void io_writex(CPUArchState *env, CPUTLBEntryFull 
*full,
  * The memory_region_dispatch may trigger a flush/resize
  * so for plugins we save the iotlb_data just in case.
  */
-save_iotlb_data(cpu, full->xlat_section, section, mr_offset);
+save_iotlb_data(cpu, section, mr_offset);
 
 if (!qemu_mutex_iothread_locked()) {
 qemu_mutex_lock_iothread();
-- 
2.34.1

[PATCH v3 3/6] accel/tcg: Suppress auto-invalidate in probe_access_internal

[Richard Henderson]

When PAGE_WRITE_INV is set when calling tlb_set_page,
we immediately set TLB_INVALID_MASK in order to force
tlb_fill to be called on the next lookup.  Here in
probe_access_internal, we have just called tlb_fill
and eliminated true misses, thus the lookup must be valid.

This allows us to remove a warning comment from s390x.
There doesn't seem to be a reason to change the code though.

Cc: David Hildenbrand 
Signed-off-by: Richard Henderson 
---
 accel/tcg/cputlb.c| 10 +-
 target/s390x/tcg/mem_helper.c |  4 
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 03395e725d..91f2b53142 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1535,6 +1535,7 @@ static int probe_access_internal(CPUArchState *env, 
target_ulong addr,
 }
 tlb_addr = tlb_read_ofs(entry, elt_ofs);
 
+flags = TLB_FLAGS_MASK;
 page_addr = addr & TARGET_PAGE_MASK;
 if (!tlb_hit_page(tlb_addr, page_addr)) {
 if (!victim_tlb_hit(env, mmu_idx, index, elt_ofs, page_addr)) {
@@ -1550,10 +1551,17 @@ static int probe_access_internal(CPUArchState *env, 
target_ulong addr,
 
 /* TLB resize via tlb_fill may have moved the entry.  */
 entry = tlb_entry(env, mmu_idx, addr);
+
+/*
+ * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
+ * to force the next access through tlb_fill.  We've just
+ * called tlb_fill, so we know that this entry *is* valid.
+ */
+flags &= ~TLB_INVALID_MASK;
 }
 tlb_addr = tlb_read_ofs(entry, elt_ofs);
 }
-flags = tlb_addr & TLB_FLAGS_MASK;
+flags &= tlb_addr;
 
 /* Fold all "mmio-like" bits into TLB_MMIO.  This is not RAM.  */
 if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) {
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index fc52aa128b..3758b9e688 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -148,10 +148,6 @@ static int s390_probe_access(CPUArchState *env, 
target_ulong addr, int size,
 #else
 int flags;
 
-/*
- * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
- * to detect if there was an exception during tlb_fill().
- */
 env->tlb_fill_exc = 0;
 flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, 
phost,
ra);
-- 
2.34.1

[PATCH v3 0/6] tcg: Introduce CPUTLBEntryFull

[Richard Henderson]

This is split out of two patch sets that I have in flight
that allow atomic updates of guest page tables.

v3 fixes some trivial conflicts with the current tcg-next PR:
https://patchew.org/QEMU/20220904002317.60158-1-richard.hender...@linaro.org/


r~


Richard Henderson (6):
  accel/tcg: Rename CPUIOTLBEntry to CPUTLBEntryFull
  accel/tcg: Drop addr member from SavedIOTLB
  accel/tcg: Suppress auto-invalidate in probe_access_internal
  accel/tcg: Introduce probe_access_full
  accel/tcg: Introduce tlb_set_page_full
  include/exec: Introduce TARGET_PAGE_ENTRY_EXTRA

 include/exec/cpu-defs.h   |  45 +--
 include/exec/exec-all.h   |  33 +
 include/hw/core/cpu.h |   1 -
 accel/tcg/cputlb.c| 218 --
 target/arm/mte_helper.c   |  14 +--
 target/arm/sve_helper.c   |   4 +-
 target/arm/translate-a64.c|   2 +-
 target/s390x/tcg/mem_helper.c |   4 -
 8 files changed, 207 insertions(+), 114 deletions(-)

-- 
2.34.1

[PATCH v3 6/6] include/exec: Introduce TARGET_PAGE_ENTRY_EXTRA

[Richard Henderson]

Allow the target to cache items from the guest page tables.

Signed-off-by: Richard Henderson 
---
 include/exec/cpu-defs.h | 9 +
 1 file changed, 9 insertions(+)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 5e12cc1854..67239b4e5e 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -163,6 +163,15 @@ typedef struct CPUTLBEntryFull {
 
 /* @lg_page_size contains the log2 of the page size. */
 uint8_t lg_page_size;
+
+/*
+ * Allow target-specific additions to this structure.
+ * This may be used to cache items from the guest cpu
+ * page tables for later use by the implementation.
+ */
+#ifdef TARGET_PAGE_ENTRY_EXTRA
+TARGET_PAGE_ENTRY_EXTRA
+#endif
 } CPUTLBEntryFull;
 
 /*
-- 
2.34.1

[PATCH v3 1/6] accel/tcg: Rename CPUIOTLBEntry to CPUTLBEntryFull

[Richard Henderson]

This structure will shortly contain more than just
data for accessing MMIO.  Rename the 'addr' member
to 'xlat_section' to more clearly indicate its purpose.

Signed-off-by: Richard Henderson 
---
 include/exec/cpu-defs.h|  22 
 accel/tcg/cputlb.c | 102 +++--
 target/arm/mte_helper.c|  14 ++---
 target/arm/sve_helper.c|   4 +-
 target/arm/translate-a64.c |   2 +-
 5 files changed, 73 insertions(+), 71 deletions(-)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index ba3cd32a1e..f70f54d850 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -108,6 +108,7 @@ typedef uint64_t target_ulong;
 #  endif
 # endif
 
+/* Minimalized TLB entry for use by TCG fast path. */
 typedef struct CPUTLBEntry {
 /* bit TARGET_LONG_BITS to TARGET_PAGE_BITS : virtual address
bit TARGET_PAGE_BITS-1..4  : Nonzero for accesses that should not
@@ -131,14 +132,14 @@ typedef struct CPUTLBEntry {
 
 QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
 
-/* The IOTLB is not accessed directly inline by generated TCG code,
- * so the CPUIOTLBEntry layout is not as critical as that of the
- * CPUTLBEntry. (This is also why we don't want to combine the two
- * structs into one.)
+/*
+ * The full TLB entry, which is not accessed by generated TCG code,
+ * so the layout is not as critical as that of CPUTLBEntry. This is
+ * also why we don't want to combine the two structs.
  */
-typedef struct CPUIOTLBEntry {
+typedef struct CPUTLBEntryFull {
 /*
- * @addr contains:
+ * @xlat_section contains:
  *  - in the lower TARGET_PAGE_BITS, a physical section number
  *  - with the lower TARGET_PAGE_BITS masked off, an offset which
  *must be added to the virtual address to obtain:
@@ -146,9 +147,9 @@ typedef struct CPUIOTLBEntry {
  *   number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM)
  * + the offset within the target MemoryRegion (otherwise)
  */
-hwaddr addr;
+hwaddr xlat_section;
 MemTxAttrs attrs;
-} CPUIOTLBEntry;
+} CPUTLBEntryFull;
 
 /*
  * Data elements that are per MMU mode, minus the bits accessed by
@@ -172,9 +173,8 @@ typedef struct CPUTLBDesc {
 size_t vindex;
 /* The tlb victim table, in two parts.  */
 CPUTLBEntry vtable[CPU_VTLB_SIZE];
-CPUIOTLBEntry viotlb[CPU_VTLB_SIZE];
-/* The iotlb.  */
-CPUIOTLBEntry *iotlb;
+CPUTLBEntryFull vfulltlb[CPU_VTLB_SIZE];
+CPUTLBEntryFull *fulltlb;
 } CPUTLBDesc;
 
 /*
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 8fad2d9b83..4585d7c015 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -200,13 +200,13 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, 
CPUTLBDescFast *fast,
 }
 
 g_free(fast->table);
-g_free(desc->iotlb);
+g_free(desc->fulltlb);
 
 tlb_window_reset(desc, now, 0);
 /* desc->n_used_entries is cleared by the caller */
 fast->mask = (new_size - 1) << CPU_TLB_ENTRY_BITS;
 fast->table = g_try_new(CPUTLBEntry, new_size);
-desc->iotlb = g_try_new(CPUIOTLBEntry, new_size);
+desc->fulltlb = g_try_new(CPUTLBEntryFull, new_size);
 
 /*
  * If the allocations fail, try smaller sizes. We just freed some
@@ -215,7 +215,7 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, 
CPUTLBDescFast *fast,
  * allocations to fail though, so we progressively reduce the allocation
  * size, aborting if we cannot even allocate the smallest TLB we support.
  */
-while (fast->table == NULL || desc->iotlb == NULL) {
+while (fast->table == NULL || desc->fulltlb == NULL) {
 if (new_size == (1 << CPU_TLB_DYN_MIN_BITS)) {
 error_report("%s: %s", __func__, strerror(errno));
 abort();
@@ -224,9 +224,9 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, 
CPUTLBDescFast *fast,
 fast->mask = (new_size - 1) << CPU_TLB_ENTRY_BITS;
 
 g_free(fast->table);
-g_free(desc->iotlb);
+g_free(desc->fulltlb);
 fast->table = g_try_new(CPUTLBEntry, new_size);
-desc->iotlb = g_try_new(CPUIOTLBEntry, new_size);
+desc->fulltlb = g_try_new(CPUTLBEntryFull, new_size);
 }
 }
 
@@ -258,7 +258,7 @@ static void tlb_mmu_init(CPUTLBDesc *desc, CPUTLBDescFast 
*fast, int64_t now)
 desc->n_used_entries = 0;
 fast->mask = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
 fast->table = g_new(CPUTLBEntry, n_entries);
-desc->iotlb = g_new(CPUIOTLBEntry, n_entries);
+desc->fulltlb = g_new(CPUTLBEntryFull, n_entries);
 tlb_mmu_flush_locked(desc, fast);
 }
 
@@ -299,7 +299,7 @@ void tlb_destroy(CPUState *cpu)
 CPUTLBDescFast *fast = &env_tlb(env)->f[i];
 
 g_free(fast->table);
-g_free(desc->iotlb);
+g_free(desc->fulltlb);
 }
 }
 
@@ -1219,7 +1219,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong 
vaddr,
 
 /* Evict the old entry into the victim tlb.  */
 copy_tlb_helper_locke

sphinx-build is really slow, any way to improve that?

[Claudio Fontana]

Hi all,

when I build qemu, there is a lot of time spent at the end of the build where 
one cpu goes 100% on sphinx-build.

Is there some way to parallelize that? It seems it is the current bottleneck 
for rebuilds for me..

Thanks,

Claudio

Re: [PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP

[Víctor Colombo]

On 05/09/2022 15:41, Daniel Henrique Barboza wrote:

On 9/1/22 10:17, Víctor Colombo wrote:

Signed-off-by: Víctor Colombo 
---


What I mentioned in patch 10 also applies to all patches from 11 to 18
it seems. All changes made in patches 09-18 are based on the explanation
gave in patch 08.

The problem with this is that it'll be annoying if/when something goes
wrong. Let's say that the change made in patch 15 caused a side-effect.
Bisect will point it to patch 15, which doesn't have an explanation of
why you made the change, and then one will need to trace it back to the
mailing list to understand it. It's not a given that one will look at
all the recent changes and understand that the logic used in patch 08
are also being used in the subsequent patches.

I don't mind if you just copy/paste the commit message from patch 08 and
just change the instruction name being fixed. What's important is to
provide some context for each individual change.


Thanks,


Daniel



Hello Daniel. Thank you very much for the reviews.

I'll take your recommendation and make the necessary changes.

Best regards,

--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

Signed-off-by: Víctor Colombo 
---


What I mentioned in patch 10 also applies to all patches from 11 to 18
it seems. All changes made in patches 09-18 are based on the explanation
gave in patch 08.

The problem with this is that it'll be annoying if/when something goes
wrong. Let's say that the change made in patch 15 caused a side-effect.
Bisect will point it to patch 15, which doesn't have an explanation of
why you made the change, and then one will need to trace it back to the
mailing list to understand it. It's not a given that one will look at
all the recent changes and understand that the logic used in patch 08
are also being used in the subsequent patches.

I don't mind if you just copy/paste the commit message from patch 08 and
just change the instruction name being fixed. What's important is to
provide some context for each individual change.


Thanks,


Daniel



  target/ppc/fpu_helper.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 5f7f52ab5b..fd3a966371 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2639,6 +2639,8 @@ uint32_t helper_##op(CPUPPCState *env, ppc_vsr_t *xt, 
\
  int all_true = 1; \
  int all_false = 1;\
\
+helper_reset_fpstatus(env);   \
+  \
  for (i = 0; i < nels; i++) {  \
  if (unlikely(tp##_is_any_nan(xa->fld) ||  \
   tp##_is_any_nan(xb->fld))) { \

Re: [PATCH 08/19] target/ppc: Clear fpstatus flags on VSX_CVT_INT_TO_FP_VECTOR

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

Fix xscvsdqp incorrectly not clearing the FI bit.
Power ISA states that xscvsdqp should always set FPSCR.FI=0
Right now, QEMU sometimes is incorrectly setting the flag to 1.

Signed-off-by: Víctor Colombo 
---


Reviewed-by: Daniel Henrique Barboza 


  target/ppc/fpu_helper.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index da79c64eca..94029883c7 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -3126,6 +3126,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,   
  \
  {   \
  ppc_vsr_t t = *xt;  \
  \
+helper_reset_fpstatus(env); \
  t.tfld = stp##_to_##ttp(xb->sfld, &env->fp_status); \
  helper_compute_fprf_##ttp(env, t.tfld); \
  \

Re: [PATCH 10/19] target/ppc: Clear fpstatus flags on VSX_CVT_FP_TO_FP

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

Signed-off-by: Víctor Colombo 
---


IIUC all the changes from patches 8-10 are based on the logic explained
in the commit message of patch 08. Problem is that patches 9 and 10 are
lacking context per themselves.

I'd rather either have patches 9 and 10 squashed into patch 8, or patches
9 and 10 need more meat in their commit messages.


Thanks,


Daniel


  target/ppc/fpu_helper.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index ceb70ed36e..8a20413a78 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2692,6 +2692,8 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, 
ppc_vsr_t *xb)   \
  ppc_vsr_t t = { }; \
  int i; \
 \
+helper_reset_fpstatus(env);\
+   \
  for (i = 0; i < nels; i++) {   \
  t.tfld = stp##_to_##ttp(xb->sfld, &env->fp_status);\
  if (unlikely(stp##_is_signaling_nan(xb->sfld,  \

Re: [PATCH 06/19] target/ppc: Set OV32 when OV is set

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

According to PowerISA: "OV32 is set whenever OV is implicitly set, and
is set to the same value that OV is defined to be set to in 32-bit
mode".

This patch changes helper_update_ov_legacy to set/clear ov32 when
applicable.

Signed-off-by: Víctor Colombo 
---



Reviewed-by: Daniel Henrique Barboza 


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

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index d905f07d02..696096100b 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -37,9 +37,9 @@
  static inline void helper_update_ov_legacy(CPUPPCState *env, int ov)
  {
  if (unlikely(ov)) {
-env->so = env->ov = 1;
+env->so = env->ov = env->ov32 = 1;
  } else {
-env->ov = 0;
+env->ov = env->ov32 = 0;
  }
  }
  

[PATCH V2 3/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

To boot S-mode firmware payload like EDK2 from persistent
flash storage, qemu needs to pass the flash address as the
next_addr in fw_dynamic_info to the opensbi.

When both -kernel and -pflash options are provided in command line,
the kernel (and initrd if -initrd) will be copied to fw_cfg table.
The S-mode FW will load the kernel/initrd from fw_cfg table.

If only pflash is given but not -kernel, then it is the job of
of the S-mode firmware to locate and load the kernel.

In either case, update the kernel_entry with the flash address
so that the opensbi can jump to the entry point of the S-mode
firmware.

Signed-off-by: Sunil V L 
---
 hw/riscv/boot.c | 28 
 hw/riscv/virt.c | 17 -
 include/hw/riscv/boot.h |  1 +
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 1ae7596873..39436b8d56 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -338,3 +338,31 @@ void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr 
fdt_addr)
 riscv_cpu->env.fdt_addr = fdt_addr;
 }
 }
+
+void riscv_setup_firmware_boot(MachineState *machine)
+{
+if (machine->kernel_filename) {
+FWCfgState *fw_cfg;
+fw_cfg = fw_cfg_find();
+
+assert(fw_cfg);
+/*
+ * Expose the kernel, the command line, and the initrd in fw_cfg.
+ * We don't process them here at all, it's all left to the
+ * firmware.
+ */
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+ machine->kernel_filename,
+ true);
+load_image_to_fw_cfg(fw_cfg,
+ FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+ machine->initrd_filename, false);
+if (machine->kernel_cmdline) {
+fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+   strlen(machine->kernel_cmdline) + 1);
+fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+  machine->kernel_cmdline);
+}
+}
+}
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index b6bbf03f61..b985df9b16 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1258,7 +1258,22 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 s->fw_cfg = create_fw_cfg(machine);
 rom_set_fw(s->fw_cfg);
 
-if (machine->kernel_filename) {
+if (drive_get(IF_PFLASH, 0, 1)) {
+/*
+ * S-mode FW like EDk2 will be kept in second plash (unit 1). When
+ * both -kernel and -pflash options are provided in command line,
+ * the kernel, initrd will be copied to fw_cfg table and opensbi
+ * will jump to flash address which is the entry point of S-mode FW.
+ * It is the job of the S-mode FW to load the kernel/initrd and launch.
+ *
+ * If only pflash is given but not -kernel, then it is the job of
+ * of the S-mode firmware to locate and load the kernel.
+ * In either case, the next_addr for opensbi will be the flash address.
+ */
+riscv_setup_firmware_boot(machine);
+kernel_entry = virt_memmap[VIRT_FLASH].base +
+ virt_memmap[VIRT_FLASH].size / 2;
+} else if (machine->kernel_filename) {
 kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
  firmware_end_addr);
 
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index a36f7618f5..93e5f8760d 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -57,5 +57,6 @@ void riscv_rom_copy_firmware_info(MachineState *machine, 
hwaddr rom_base,
   uint32_t reset_vec_size,
   uint64_t kernel_entry);
 void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr fdt_addr);
+void riscv_setup_firmware_boot(MachineState *machine);
 
 #endif /* RISCV_BOOT_H */
-- 
2.25.1

Re: [PATCH 19/19] target/ppc: Clear fpstatus flags on VSX_ROUND

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

Fix xsrdpic and other instructions not clearing the flags and saving
incorrect values to FPSCR.

Signed-off-by: Víctor Colombo 
---


Reviewed-by: Daniel Henrique Barboza 





  target/ppc/fpu_helper.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index fd3a966371..be47d73b14 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -3172,6 +3172,8 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, 
ppc_vsr_t *xb)   \
  int i; \
  FloatRoundMode curr_rounding_mode; \
 \
+helper_reset_fpstatus(env);\
+   \
  if (rmode != FLOAT_ROUND_CURRENT) {\
  curr_rounding_mode = get_float_rounding_mode(&env->fp_status); \
  set_float_rounding_mode(rmode, &env->fp_status);   \

Re: [PATCH 05/19] target/ppc: Zero second doubleword for VSX madd instructions

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

In 205eb5a89e we updated most VSX instructions to zero the
second doubleword, as is requested by PowerISA since v3.1.
However, VSX_MADD helper was left behind unchanged, while it
is also affected and should be fixed as well.

This patch applies the fix for MADD instructions.

Signed-off-by: Víctor Colombo 
---


In this case it's good to add a Fixes tag:


Fixes: 205eb5a89e ("target/ppc: Change VSX instructions behavior to fill with 
zeros")




  target/ppc/fpu_helper.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Daniel Henrique Barboza 



diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 7ab6beadad..da79c64eca 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2178,7 +2178,7 @@ VSX_TSQRT(xvtsqrtsp, 4, float32, VsrW(i), -126, 23)
  void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, 
\
   ppc_vsr_t *s1, ppc_vsr_t *s2, ppc_vsr_t *s3) 
\
  { 
\
-ppc_vsr_t t = *xt;\
+ppc_vsr_t t = { };\
  int i;
\

\
  helper_reset_fpstatus(env);   
\

Re: [PATCH 07/19] target/ppc: Zero second doubleword of VSR registers for FPR insns

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

FPR register are mapped to the first doubleword of the VSR registers.
Since PowerISA v3.1, the second doubleword of the target register
must be zeroed for FP instructions.

This patch does it by writting 0 to the second dw everytime the
first dw is being written using set_fpr.

Signed-off-by: Víctor Colombo 
---


Reviewed-by: Daniel Henrique Barboza 


  target/ppc/translate.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 388337f81b..a0fa419a1f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6443,6 +6443,14 @@ static inline void get_fpr(TCGv_i64 dst, int regno)
  static inline void set_fpr(int regno, TCGv_i64 src)
  {
  tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
+/*
+ * Before PowerISA v3.1 the result of doubleword 1 of the VSR
+ * corresponding to the target FPR was undefined. However,
+ * most (if not all) real hardware were setting the result to 0.
+ * Starting at ISA v3.1, the result for doubleword 1 is now defined
+ * to be 0.
+ */
+tcg_gen_st_i64(tcg_constant_i64(0), cpu_env, vsr64_offset(regno, false));
  }
  
  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)

[PATCH V2 2/3] hw/riscv: virt: Move create_fw_cfg() prior to loading kernel

[Sunil V L]

To enable both -kernel and -pflash options, the fw_cfg needs to be
created prior to loading the kernel.

Signed-off-by: Sunil V L 
---
 hw/riscv/virt.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ff8c0df5cd..b6bbf03f61 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1251,6 +1251,13 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 RISCV64_BIOS_BIN, start_addr, NULL);
 }
 
+/*
+ * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
+ * tree cannot be altered and we get FDT_ERR_NOSPACE.
+ */
+s->fw_cfg = create_fw_cfg(machine);
+rom_set_fw(s->fw_cfg);
+
 if (machine->kernel_filename) {
 kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
  firmware_end_addr);
@@ -1284,13 +1291,6 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 start_addr = virt_memmap[VIRT_FLASH].base;
 }
 
-/*
- * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
- * tree cannot be altered and we get FDT_ERR_NOSPACE.
- */
-s->fw_cfg = create_fw_cfg(machine);
-rom_set_fw(s->fw_cfg);
-
 /* Compute the fdt load address in dram */
 fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
machine->ram_size, machine->fdt);
-- 
2.25.1

[PATCH V2 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
function will be required by riscv too. So, it's time to refactor and
move this function to a common path.

Signed-off-by: Sunil V L 
---
 hw/arm/boot.c | 49 ---
 hw/loongarch/virt.c   | 33 --
 hw/nvram/fw_cfg.c | 49 +++
 include/hw/nvram/fw_cfg.h |  3 +++
 4 files changed, 52 insertions(+), 82 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..704f368d9c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
 }
 }
 
-/**
- * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
- *  by key.
- * @fw_cfg: The firmware config instance to store the data in.
- * @size_key:   The firmware config key to store the size of the loaded
- *  data under, with fw_cfg_add_i32().
- * @data_key:   The firmware config key to store the loaded data under,
- *  with fw_cfg_add_bytes().
- * @image_name: The name of the image file to load. If it is NULL, the
- *  function returns without doing anything.
- * @try_decompress: Whether the image should be decompressed (gunzipped) before
- *  adding it to fw_cfg. If decompression fails, the image is
- *  loaded as-is.
- *
- * In case of failure, the function prints an error message to stderr and the
- * process exits with status 1.
- */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
- uint16_t data_key, const char *image_name,
- bool try_decompress)
-{
-size_t size = -1;
-uint8_t *data;
-
-if (image_name == NULL) {
-return;
-}
-
-if (try_decompress) {
-size = load_image_gzipped_buffer(image_name,
- LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-}
-
-if (size == (size_t)-1) {
-gchar *contents;
-gsize length;
-
-if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-error_report("failed to load \"%s\"", image_name);
-exit(1);
-}
-size = length;
-data = (uint8_t *)contents;
-}
-
-fw_cfg_add_i32(fw_cfg, size_key, size);
-fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static int do_arm_linux_init(Object *obj, void *opaque)
 {
 if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 5cc0b05538..eee2c193c0 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
 }
 }
 
-/* Load an image file into an fw_cfg entry identified by key. */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
- uint16_t data_key, const char *image_name,
- bool try_decompress)
-{
-size_t size = -1;
-uint8_t *data;
-
-if (image_name == NULL) {
-return;
-}
-
-if (try_decompress) {
-size = load_image_gzipped_buffer(image_name,
- LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-}
-
-if (size == (size_t)-1) {
-gchar *contents;
-gsize length;
-
-if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-error_report("failed to load \"%s\"", image_name);
-exit(1);
-}
-size = length;
-data = (uint8_t *)contents;
-}
-
-fw_cfg_add_i32(fw_cfg, size_key, size);
-fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
 {
 /*
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d605f3f45a..3f68dae5d2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -41,6 +41,7 @@
 #include "qapi/error.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/loader.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
@@ -1221,6 +1222,54 @@ FWCfgState *fw_cfg_find(void)
 return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+/**
+ * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
+ *  by key.
+ * @fw_cfg: The firmware config instance to store the data in.
+ * @size_key:   The firmware config key to store the size of the loaded
+ *  data under, with fw_cfg_add_i32().
+ * @data_key:   The firmware config key to store the loaded data under,
+ *  with fw_cfg_add_bytes().
+ * @image_name: The name of the image file to load. If it is NULL, the
+ *  function returns without doing anything.
+ * @try_decompress: Whether the image should be decompressed (gunzipped) before
+ *   

Re: [PATCH 04/19] target/ppc: Set result to QNaN for DENBCD when VXCVI occurs

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

According to the ISA, for instruction DENBCD:
"If an invalid BCD digit or sign code is detected in the source
operand, an invalid-operation exception (VXCVI) occurs."

In the Invalid Operation Exception section, there is the situation:
"When Invalid Operation Exception is disabled (VE=0) and Invalid
Operation occurs (...) If the operation is an (...) or format the
target FPR is set to a Quiet NaN". This was not being done in
QEMU.

This patch sets the result to QNaN when the instruction DENBCD causes
an Invalid Operation Exception.

Signed-off-by: Víctor Colombo 
---


Reviewed-by: Daniel Henrique Barboza 


  target/ppc/dfp_helper.c | 26 --
  1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
index be7aa5357a..cc024316d5 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -1147,6 +1147,26 @@ static inline uint8_t dfp_get_bcd_digit_128(ppc_vsr_t 
*t, unsigned n)
  return t->VsrD((n & 0x10) ? 0 : 1) >> ((n << 2) & 63) & 15;
  }
  
+static inline void dfp_invalid_op_vxcvi_64(struct PPC_DFP *dfp)

+{
+/* TODO: fpscr is incorrectly not being saved to env */
+dfp_set_FPSCR_flag(dfp, FP_VX | FP_VXCVI, FPSCR_VE);
+if ((dfp->env->fpscr & FP_VE) == 0) {
+dfp->vt.VsrD(1) = 0x7c00; /* QNaN */
+}
+}
+
+
+static inline void dfp_invalid_op_vxcvi_128(struct PPC_DFP *dfp)
+{
+/* TODO: fpscr is incorrectly not being saved to env */
+dfp_set_FPSCR_flag(dfp, FP_VX | FP_VXCVI, FPSCR_VE);
+if ((dfp->env->fpscr & FP_VE) == 0) {
+dfp->vt.VsrD(0) = 0x7c00; /* QNaN */
+dfp->vt.VsrD(1) = 0x0;
+}
+}
+
  #define DFP_HELPER_ENBCD(op, size)   \
  void helper_##op(CPUPPCState *env, ppc_fprp_t *t, ppc_fprp_t *b, \
   uint32_t s) \
@@ -1173,7 +1193,8 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t, 
ppc_fprp_t *b, \
  sgn = 0; \
  break;   \
  default: \
-dfp_set_FPSCR_flag(&dfp, FP_VX | FP_VXCVI, FPSCR_VE);\
+dfp_invalid_op_vxcvi_##size(&dfp);   \
+set_dfp##size(t, &dfp.vt);   \
  return;  \
  }\
  }\
@@ -1183,7 +1204,8 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t, 
ppc_fprp_t *b, \
  digits[(size) / 4 - n] = dfp_get_bcd_digit_##size(&dfp.vb,   \
offset++); \
  if (digits[(size) / 4 - n] > 10) {   \
-dfp_set_FPSCR_flag(&dfp, FP_VX | FP_VXCVI, FPSCR_VE);\
+dfp_invalid_op_vxcvi_##size(&dfp);   \
+set_dfp##size(t, &dfp.vt);   \
  return;  \
  } else { \
  nonzero |= (digits[(size) / 4 - n] > 0); \

[PATCH V2 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

This series adds the support to boot S-mode FW like EDK2 from the flash. The
S-mode firmware should be kept in pflash unit 1.

When -kernel (and -initrd) option is also provided along with the flash,
the kernel (and initrd) will be loaded into fw_cfg table and opensbi will
branch to the flash address which will be the entry point of the S-mode
firmware. The S-mode FW then loads and launches the kernel.

When only -pflash option is provided in the command line, the kernel
will be located and loaded in the usual way by the S-mode firmware.

These patches are available in below branch.
https://github.com/vlsunil/qemu/tree/pflash_v2

The first two patches in this series are refactor patches.

These changes are tested with a WIP EDK2 port for virt machine. Below
are the instructions to build and test this feature.

1) Get EDK2 sources from below branches.
https://github.com/vlsunil/edk2/tree/virt_refactor_smode_v1
https://github.com/vlsunil/edk2-platforms/tree/virt_refactor_smode_v1

2) Build EDK2 for RISC-V
export WORKSPACE=`pwd`
export GCC5_RISCV64_PREFIX=riscv64-linux-gnu-
export PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms
export EDK_TOOLS_PATH=$WORKSPACE/edk2/BaseTools
source edk2/edksetup.sh
make -C edk2/BaseTools clean
make -C edk2/BaseTools
make -C edk2/BaseTools/Source/C
source edk2/edksetup.sh BaseTools
build -a RISCV64  -p Platform/Qemu/RiscVVirt/RiscVVirt.dsc -t GCC5

3)Make the EDK2 image size to match with what qemu flash expects
truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd

4) Run
a) Boot to EFI shell (no -kernel / -initrd option)
qemu-system-riscv64  -nographic   -drive 
file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  
-machine virt -M 2G

b) With -kernel, -initrd and -pflash
qemu-system-riscv64  -nographic   -drive 
file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  
-machine virt -M 2G -kernel arch/riscv/boot/Image.gz -initrd rootfs.cpio 


Changes since V1:
1) Modified code to support the use case when both -kernel and -pflash 
are configured.
2) Refactor patches added to help (1) above.
3) Cover letter added with test instructions.

Sunil V L (3):
  hw/arm,loongarch: Move load_image_to_fw_cfg() to common location
  hw/riscv: virt: Move create_fw_cfg() prior to loading kernel
  hw/riscv: virt: Enable booting S-mode firmware from pflash

 hw/arm/boot.c | 49 ---
 hw/loongarch/virt.c   | 33 --
 hw/nvram/fw_cfg.c | 49 +++
 hw/riscv/boot.c   | 28 ++
 hw/riscv/virt.c   | 31 ++---
 include/hw/nvram/fw_cfg.h |  3 +++
 include/hw/riscv/boot.h   |  1 +
 7 files changed, 104 insertions(+), 90 deletions(-)

-- 
2.25.1

Re: [PATCH 03/19] target/ppc: Zero second doubleword in DFP instructions

[Daniel Henrique Barboza]

On 9/1/22 10:17, Víctor Colombo wrote:

Starting at PowerISA v3.1, the second doubleword of the registers
used to store results in DFP instructions are supposed to be zeroed.

 From the ISA, chapter 7.2.1.1 Floating-Point Registers:
"""
Chapter 4. Floating-Point Facility provides 32 64-bit
FPRs. Chapter 5. Decimal Floating-Point also employs
FPRs in decimal floating-point (DFP) operations. When
VSX is implemented, the 32 FPRs are mapped to
doubleword 0 of VSRs 0-31. (...)
All instructions that operate on an FPR are redefined
to operate on doubleword element 0 of the
corresponding VSR. (...)
and the contents of doubleword element 1 of the
VSR corresponding to the target FPR or FPR pair for these
instructions are set to 0.
"""

Before, the result stored at doubleword 1 was said to be undefined.

With that, this patch changes the DFP facility to zero doubleword 1
when using set_dfp64 and set_dfp128. This fixes the behavior for ISA
3.1 while keeping the behavior correct for previous ones.

Signed-off-by: Víctor Colombo 
---


Reviewed-by: Daniel Henrique Barboza 


  target/ppc/dfp_helper.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
index 5ba74b2124..be7aa5357a 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -42,13 +42,16 @@ static void get_dfp128(ppc_vsr_t *dst, ppc_fprp_t *dfp)
  
  static void set_dfp64(ppc_fprp_t *dfp, ppc_vsr_t *src)

  {
-dfp->VsrD(0) = src->VsrD(1);
+dfp[0].VsrD(0) = src->VsrD(1);
+dfp[0].VsrD(1) = 0ULL;
  }
  
  static void set_dfp128(ppc_fprp_t *dfp, ppc_vsr_t *src)

  {
  dfp[0].VsrD(0) = src->VsrD(0);
  dfp[1].VsrD(0) = src->VsrD(1);
+dfp[0].VsrD(1) = 0ULL;
+dfp[1].VsrD(1) = 0ULL;
  }
  
  static void set_dfp128_to_avr(ppc_avr_t *dst, ppc_vsr_t *src)

Re: [PING PATCH v2] linux-user: Passthrough MADV_DONTNEED for certain file mappings

[Richard Henderson]

On 9/1/22 09:45, Ilya Leoshkevich wrote:

+/*
+ * For linux-user, indicates that the page is mapped with the same
semantics
+ * in both guest and host.
+ */
+#define PAGE_PASSTHROUGH 0x0800


I would expect a change to PAGE_STICKY in accel/tcg/translate-all.c, so that this bit is 
preserved across mprotect.  Yes?



@@ -845,7 +861,7 @@ static bool
can_passthrough_madv_dontneed(abi_ulong start, abi_ulong end)
  }
  
  for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {

-    if (!(page_get_flags(addr) & PAGE_ANON)) {
+    if (!(page_get_flags(addr) & (PAGE_ANON |
PAGE_PASSTHROUGH))) {


Do you want both PAGE_ANON and PAGE_PASSTHROUGH?
If not, is PAGE_PASSTHOUGH is sufficient by itself, why check PAGE_ANON?


I would like to ping this patch and two others that I used for
debugging it:

[PATCH] linux-user: Fix stracing in-memory mmap arguments
https://patchew.org/QEMU/20220630165901.2459135-1-...@linux.ibm.com/


Queued to linux-user-next.


[PATCH] linux-user: Implement stracing madvise()
https://patchew.org/QEMU/20220725134440.172892-1-...@linux.ibm.com/


There are many more MADV_* than just the 5 you list.


r~

Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Janis Schoetterl-Glausch]

On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> In the S390x CPU topology the core_id specifies the CPU address
> and the position of the core withing the topology.
> 
> Let's build the topology based on the core_id.
> 
> Signed-off-by: Pierre Morel 
> ---
>  hw/s390x/cpu-topology.c | 135 
>  hw/s390x/meson.build|   1 +
>  hw/s390x/s390-virtio-ccw.c  |  10 +++
>  include/hw/s390x/cpu-topology.h |  42 ++
>  4 files changed, 188 insertions(+)
>  create mode 100644 hw/s390x/cpu-topology.c
>  create mode 100644 include/hw/s390x/cpu-topology.h
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> 
[...]

> +/**
> + * s390_topology_realize:
> + * @dev: the device state
> + * @errp: the error pointer (not used)
> + *
> + * During realize the machine CPU topology is initialized with the
> + * QEMU -smp parameters.
> + * The maximum count of CPU TLE in the all Topology can not be greater
> + * than the maximum CPUs.
> + */
> +static void s390_topology_realize(DeviceState *dev, Error **errp)
> +{
> +MachineState *ms = MACHINE(qdev_get_machine());
> +S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> +int n;
> +
> +topo->sockets = ms->smp.sockets;
> +topo->cores = ms->smp.cores;
> +topo->tles = ms->smp.max_cpus;
> +
> +n = topo->sockets;
> +topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> +topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));

Seems like a good use case for g_new0.

[...]
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 00..6911f975f4
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,42 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright 2022 IBM Corp.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#ifndef HW_S390X_CPU_TOPOLOGY_H
> +#define HW_S390X_CPU_TOPOLOGY_H

Is there a reason this is before the includes?
> +
> +typedef struct S390TopoContainer {
> +int active_count;
> +} S390TopoContainer;
> +
> +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)

This is correct because cpu_id == core_id for s390, right?
So the cpu limit also applies to the core id.
You could do ((S390_MAX_CPUS + 63) / 64) instead.
But if you chose this for simplicity's sake, I'm fine with it.

> +typedef struct S390TopoTLE {
> +int active_count;

Do you use (read) this field somewhere?
Is this in anticipation of there being multiple TLE arrays, for
different polarizations, etc? If so I would defer this for later.

> +uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +struct S390Topology {
> +SysBusDevice parent_obj;
> +int sockets;
> +int cores;

These are just cached values from machine_state.smp, right?
Not sure if I like the redundancy, it doesn't aid in comprehension.

> +int tles;
> +S390TopoContainer *socket;
> +S390TopoTLE *tle;
> +};
> +typedef struct S390Topology S390Topology;

The DECLARE macro takes care of this typedef.

> +
> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
> +
> +S390Topology *s390_get_topology(void);
> +void s390_topology_new_cpu(int core_id);
> +
> +#endif

Re: [PATCH for-7.1? 0/2] Re-enable ppc32 as a linux-user host

[Richard Henderson]

On 7/29/22 18:21, Richard Henderson wrote:

This is, technically, a regression from 6.2, so it's not
implausible to apply before rc1.  Thoughts?


r~


Richard Henderson (2):
   common-user/host/ppc: Implement safe-syscall.inc.S
   linux-user: Implment host/ppc/host-signal.h


Queued to linux-user-next.


r~

Re: [PATCH] linux-user: fix bug about missing signum convert of sigqueue

[Richard Henderson]

On 8/31/22 05:10, fa...@mail.ustc.edu.cn wrote:

 From 4ebe8a67ed7c4b1220957b2b67a62ba60e0e80ec Mon Sep 17 00:00:00 2001
From: fanwenjie 
Date: Wed, 31 Aug 2022 11:55:25 +0800
Subject: [PATCH] linux-user: fix bug about missing signum convert of sigqueue

Signed-off-by: fanwenjie 


Queued to linux-user-next.


r~

Re: [PATCH v6 14/14] qmp/hmp, device_tree.c: introduce dumpdtb

[Daniel Henrique Barboza]

On 9/5/22 10:41, Markus Armbruster wrote:

Daniel Henrique Barboza  writes:


To save the FDT blob we have the '-machine dumpdtb=' property.
With this property set, the machine saves the FDT in  and exit.
The created file can then be converted to plain text dts format using
'dtc'.

There's nothing particularly sophisticated into saving the FDT that
can't be done with the machine at any state, as long as the machine has
a valid FDT to be saved.

The 'dumpdtb' command receives a 'filename' paramenter and, if a valid
FDT is available, it'll save it in a file 'filename'. In short, this is
a '-machine dumpdtb' that can be fired on demand via QMP/HMP.

A valid FDT consists of a FDT that was created using libfdt being
retrieved via 'current_machine->fdt' in device_tree.c. This condition is
met by most FDT users in QEMU.

This command will always be executed in-band (i.e. holding BQL),
avoiding potential race conditions with machines that might change the
FDT during runtime (e.g. PowerPC 'pseries' machine).

Cc: Dr. David Alan Gilbert 
Cc: Markus Armbruster 
Cc: Alistair Francis 
Cc: David Gibson 
Signed-off-by: Daniel Henrique Barboza 
---
  hmp-commands.hx  | 15 +++
  include/sysemu/device_tree.h |  1 +
  monitor/misc.c   |  1 +
  qapi/machine.json| 18 ++
  softmmu/device_tree.c| 31 +++
  5 files changed, 66 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 182e639d14..9a3e57504f 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1800,3 +1800,18 @@ ERST
"\n\t\t\t\t\t limit on a specified virtual cpu",
  .cmd= hmp_cancel_vcpu_dirty_limit,
  },
+
+#if defined(CONFIG_FDT)
+SRST
+``dumpdtb`` *filename*
+  Save the FDT in the 'filename' file to be decoded using dtc.
+  Requires 'libfdt' support.


Does the #if suppress the documentation snippet when
!defined(CONFIG_FDT)?

If yes, the "Requires" line is redundant.

Other conditional commands don't have such lines.


I tested using a target that doesn't rely on libfdt (in this case I picked
the tricore softmmu) and disabled FDT support in ./configure:

configure --target-list=tricore-softmmu --enable-fdt=disabled

This didn't prevent the 'dumpdtb' section from appearing in the generated
docs at build/docs/manual/system/monitor.html. So I guess the documentation
isn't being suppressed by 'CONFIG_FDT'.

That said, I can remove the "Requires libfdt" observations to be in line
with other commands, and then we can then discuss in separate whether the
documentation should follow these preprocessing macros.


Thanks,


Daniel






+ERST
+{
+.name   = "dumpdtb",
+.args_type  = "filename:F",
+.params = "filename",
+.help   = "save the FDT in the 'filename' file to be decoded using 
dtc",
+.cmd= hmp_dumpdtb,
+},
+#endif
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index ef060a9759..e7c5441f56 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -136,6 +136,7 @@ int qemu_fdt_add_path(void *fdt, const char *path);
  } while (0)
  
  void qemu_fdt_dumpdtb(void *fdt, int size);

+void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
  
  /**

   * qemu_fdt_setprop_sized_cells_from_array:
diff --git a/monitor/misc.c b/monitor/misc.c
index 3d2312ba8d..e7dd63030b 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -49,6 +49,7 @@
  #include "sysemu/blockdev.h"
  #include "sysemu/sysemu.h"
  #include "sysemu/tpm.h"
+#include "sysemu/device_tree.h"
  #include "qapi/qmp/qdict.h"
  #include "qapi/qmp/qerror.h"
  #include "qapi/qmp/qstring.h"
diff --git a/qapi/machine.json b/qapi/machine.json
index 6afd1936b0..f968a5d343 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1664,3 +1664,21 @@
   '*size': 'size',
   '*max-size': 'size',
   '*slots': 'uint64' } }
+
+##
+# @dumpdtb:
+#
+# Save the FDT in dtb format. Requires 'libfdt' support.


We rarely document conditionals like this.  It's kind of redundant with
the

 If

 CONFIG_FDT

part in the generated documentation.  Aside: this "If" part could
certainly be improved.


+#
+# @filename: name of the FDT file to be created
+#
+# Since: 7.2
+#
+# Example:
+#   {"execute": "dumpdtb"}
+#"arguments": { "filename": "/tmp/fdt.dtb" } }


Excuse my nitpicking...  Creating files with a fixed filename in /tmp is
a bad habit, and this example promotes it.  We have similar examples
elsewhere.  Still, I'd prefer not to add more.


+#
+##
+{ 'command': 'dumpdtb',
+  'data': { 'filename': 'str' },
+  'if': 'CONFIG_FDT' }
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 6ca3fad285..cdd41b6de6 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -26,6 +26,9 @@
  #include "hw/loader.h"
  #include "hw/boards.h"
  #include "qemu/config-file.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/qmp/qdict.h"

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 17:31, Víctor Colombo wrote:

I have a message bookmarked here ([1]), but I don't know if there is a
previous one with a more in depth scheme.


There may have been a previous, but that's the one I was thinking of.


Anyway, I was also analyzing recently the idea of removing all these
reset_fpstatus() calls from instructions helpers. I think this would
require to actually call it from the end of the (previous) instructions instead of the 
beginning? Like adding the call to

do_float_check_status() and float_invalid_op_*() as a focal point to
'hide' the calls to reset_fpstatus().


Well, there would of course be no separate call, but do_float_check_status 
would:

int status = get_float_exception_flags(&env->fp_status);

set_float_exception_flags(0, &env->fp_status);

straight away.  No extra call overhead, and the steady-state of softfp exception flags 
outside of an in-progress fp operation is 0.



However there are also insns
helpers that don't call these auxiliary functions, which I think would
cause the refactor to not be worthy overall.


Anything that can raise a softfp exception and doesn't do something with it, either 
immediately within the same helper, or just afterward with helper_float_check_status, is 
buggy.  With those fixed, helper_reset_fpstatus may be removed entirely.




r~

Re: [RFC] module: removed unused function argument "mayfail"

[Richard Henderson]

On 9/5/22 16:55, Claudio Fontana wrote:

mayfail is always passed as false for every invocation throughout the program.
It controls whether to printf or not to printf an error on
g_module_open failure.

Remove this unused argument.

Signed-off-by: Claudio Fontana
---
  include/qemu/module.h |  8 
  softmmu/qtest.c   |  2 +-
  util/module.c | 20 +---
  3 files changed, 14 insertions(+), 16 deletions(-)


Reviewed-by: Richard Henderson 


r~

Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled

[Ani Sinha]

On Mon, 5 Sep 2022, Ani Sinha wrote:

>
>

> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 0355bd3dda..3dc9379f27 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1348,10 +1348,12 @@ static Aml *build_q35_osc_method(bool 
> > > enable_native_pcie_hotplug)
> > >  {
> > >  Aml *if_ctx;
> > >  Aml *if_ctx2;
> > > +Aml *if_ctx3;
> > >  Aml *else_ctx;
> > >  Aml *method;
> > >  Aml *a_cwd1 = aml_name("CDW1");
> > >  Aml *a_ctrl = aml_local(0);
> > > +Aml *a_pcie_nhp_ctl = aml_local(1);
> > >
> > >  method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > >  aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), 
> > > "CDW1"));
> > > @@ -1366,11 +1368,26 @@ static Aml *build_q35_osc_method(bool 
> > > enable_native_pcie_hotplug)
> > >  /*
> > >   * Always allow native PME, AER (no dependencies)
> > >   * Allow SHPC (PCI bridges can have SHPC controller)
> > > - * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled.
> > >   */
> > > -aml_append(if_ctx, aml_and(a_ctrl,
> > > -aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)), 
> > > a_ctrl));
> > > +aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> >
> > that makes us not actually mask any capabilities since you forgot to mask
> > bit 1 later under if_ctx3 context.
> >
> > So OSPM will see a permanent failure (_OSC failure bit in CWD1)
> > and will have no idea that PCI Hotplug is not supported since we return CWD3
> > with this bit still set whoever much it tries to negotiate.
>
> The failure is only returned when the OS requests/probes native hotplug
> capability in CWD1.

I meant CWD3.

Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled

[Ani Sinha]

On Mon, 5 Sep 2022, Igor Mammedov wrote:

> On Mon,  5 Sep 2022 12:55:31 +0530
> Ani Sinha  wrote:
>
> > Possible fix for https://bugzilla.redhat.com/show_bug.cgi?id=2089545
> >
> > Change in AML:
> >
> > @@ -47,33 +47,39 @@
> >  Scope (_SB)
> >  {
> >  Device (PCI0)
> >  {
> >  Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // 
> > _HID: Hardware ID
> >  Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: 
> > Compatible ID
> >  Name (_ADR, Zero)  // _ADR: Address
> >  Name (_UID, Zero)  // _UID: Unique ID
> >  Method (_OSC, 4, NotSerialized)  // _OSC: Operating System 
> > Capabilities
> >  {
> >  CreateDWordField (Arg3, Zero, CDW1)
> >  If ((Arg0 == ToUUID 
> > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> >  {
> >  CreateDWordField (Arg3, 0x04, CDW2)
> >  CreateDWordField (Arg3, 0x08, CDW3)
> >  Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > -Local0 &= 0x1E
> > +Local0 &= 0x1F
> > +Local1 = (CDW3 & One)
> > +If ((One == Local1))
> > +{
> > +CDW1 |= 0x12
> > +}
> > +
> >  If ((Arg1 != One))
> >  {
> >  CDW1 |= 0x08
> >  }
> >
> >  If ((CDW3 != Local0))
> >  {
> >  CDW1 |= 0x10
> >  }
> >
> >  CDW3 = Local0
> >  }
> >  Else
> >  {
> >  CDW1 |= 0x04
> >  }
> > **
> >
> > Signed-off-by: Ani Sinha 
> > ---
> >  hw/i386/acpi-build.c | 23 ---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 0355bd3dda..3dc9379f27 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1348,10 +1348,12 @@ static Aml *build_q35_osc_method(bool 
> > enable_native_pcie_hotplug)
> >  {
> >  Aml *if_ctx;
> >  Aml *if_ctx2;
> > +Aml *if_ctx3;
> >  Aml *else_ctx;
> >  Aml *method;
> >  Aml *a_cwd1 = aml_name("CDW1");
> >  Aml *a_ctrl = aml_local(0);
> > +Aml *a_pcie_nhp_ctl = aml_local(1);
> >
> >  method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> >  aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), 
> > "CDW1"));
> > @@ -1366,11 +1368,26 @@ static Aml *build_q35_osc_method(bool 
> > enable_native_pcie_hotplug)
> >  /*
> >   * Always allow native PME, AER (no dependencies)
> >   * Allow SHPC (PCI bridges can have SHPC controller)
> > - * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled.
> >   */
> > -aml_append(if_ctx, aml_and(a_ctrl,
> > -aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)), a_ctrl));
> > +aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
>
> that makes us not actually mask any capabilities since you forgot to mask
> bit 1 later under if_ctx3 context.
>
> So OSPM will see a permanent failure (_OSC failure bit in CWD1)
> and will have no idea that PCI Hotplug is not supported since we return CWD3
> with this bit still set whoever much it tries to negotiate.

The failure is only returned when the OS requests/probes native hotplug
capability in CWD1.

Also maybe I was mistaken but in the offline thread, I thought MST
suggested we left the native hotplug bit as is without ever masking it.
I guess he was coming from the fact that experiments indicated that when
the native hotplug is off in OSC, Windows did not detect ATS.


> So if it boots at all, guest will probably not use any requested features
> since _OSC failed to confirm any without error.
>
> And even if we clear hotplug bit it still doesn't help.
>
> some more testing shows that ATS cap doesn't depended hard on native hotplug
> (i.e. run QEMU with native hotplug enabled but disable hotplug on root-port 
> in question)
> To me it still looks like a bug in Windows' acpi hotplug impl
> (or perhaps it's no more properly maintained, so it doesn't account for new 
> features).

Yeah I also think this is a Windows bug. ATS and native hotplug does not
seem to be related in anyway.

>
> > +/*
> > + * if ACPI PCI Hot-plug is enabled, do not let OSPM set OSC PCIE
> > + * Native hotplug ctrl bit.
> > + */
> > +if (!enable_native_pcie_hotplug) {
> > +/* check if the ACPI native hotplug bit is set by the OS in DWORD3 
> > */
> > +aml_append(if_ctx, aml_and(aml_name("CDW3"),
> > +   aml_int(0x01), a_pcie_nhp_ctl));
> > +if_ctx3 = aml_if(aml_equal(aml_int(1), a_pcie_nhp_ctl));
> > +/*
> > + * ACPI spe

[RFC PATCH] docs/system: clean up code escape for riscv virt platform

[Alex Bennée]

The example code is rendered slightly mangled due to missing code
block. Properly escape the code block and add shell prompt and qemu to
fit in with the other examples on the page.

Signed-off-by: Alex Bennée 
---
 docs/system/riscv/virt.rst | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index f8ecec95f3..4b16e41d7f 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -168,14 +168,19 @@ Enabling TPM
 
 A TPM device can be connected to the virt board by following the steps below.
 
-First launch the TPM emulator
+First launch the TPM emulator:
 
-swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
+.. code-block:: bash
+
+  $ swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
 --ctrl type=unixio,path=swtpm-sock
 
-Then launch QEMU with:
+Then launch QEMU with some additional arguments to link a TPM device to the 
backend:
+
+.. code-block:: bash
 
-...
+  $ qemu-system-riscv64 \
+... other args  \
 -chardev socket,id=chrtpm,path=swtpm-sock \
 -tpmdev emulator,id=tpm0,chardev=chrtpm \
 -device tpm-tis-device,tpmdev=tpm0
-- 
2.34.1

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 13:21, Richard Henderson wrote:

On 9/5/22 17:19, Víctor Colombo wrote:

Existing bug, but this is missing to clear fp status to start.

Reviewed-by: Richard Henderson 

r~



Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).


Oops, ok.



It's probably better to move the call to inside the helper.


I did write about a scheme by which all of these calls should go away.  
I guess it has

been a while...


r~


I have a message bookmarked here ([1]), but I don't know if there is a
previous one with a more in depth scheme.
Anyway, I was also analyzing recently the idea of removing all these
reset_fpstatus() calls from instructions helpers. I think this would
require to actually call it from the end of the (previous) instructions 
instead of the beginning? Like adding the call to

do_float_check_status() and float_invalid_op_*() as a focal point to
'hide' the calls to reset_fpstatus(). However there are also insns
helpers that don't call these auxiliary functions, which I think would
cause the refactor to not be worthy overall.
Did you have another idea that could be simpler?

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg00064.html


--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH v3 13/15] virtio-net: support queue reset

[Kangjie Xu]

在 2022/9/5 16:30, Jason Wang 写道:


在 2022/8/25 16:08, Kangjie Xu 写道:

From: Xuan Zhuo 

virtio-net and vhost-kernel implement queue reset.
Queued packets in the corresponding queue pair are flushed
or purged.

Signed-off-by: Xuan Zhuo 
Signed-off-by: Kangjie Xu 
---
  hw/net/virtio-net.c | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27b59c0ad6..d774a3e652 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -540,6 +540,23 @@ static RxFilterInfo 
*virtio_net_query_rxfilter(NetClientState *nc)

  return info;
  }
  +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t 
queue_index)

+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
+    if (!nc->peer) {
+    return;
+    }
+
+    if (get_vhost_net(nc->peer) &&
+    nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
+    vhost_net_virtqueue_reset(vdev, nc, queue_index);
+    }
+
+    flush_or_purge_queued_packets(nc);



But the codes doesn't prevent the usersapce datapath from being used? 
(e.g vhost=off)


E.g vhost_net_start_one() had:

    if (net->nc->info->poll) {
    net->nc->info->poll(net->nc, false);
    }

I review this part in vhost_net_start/stop_one().

I didn't take the usersapce datapath into consideration. If we don't 
prevent it, the userspace datapath may access it and causes some 
problems. From this point, we should disable it.


But if we add net->nc->info->poll in vq reset, it can only operate at 
queue-pair level, which obeys "per-virtqueue".


What's your opinion on this point? I think it's ok to add it in vq reset.



And I will wonder if it's better to consider to:

1) factor out the per virtqueue start/stop from 
vhost_net_start/stop_one()


2) simply use the helper factored out via step 1)

Thanks

I have a different idea about it, vhost_virtqueue_start/stop()(in 
hw/virtio/vhost.c) are already good abstractions of per virtqueue 
start/stop.


These two functions are used in vhost_dev_start. It's just because 
vhost-net needs some configuration, so we add net->nc->info->poll and 
set_backend for it. But for other devices using vhost_dev_start, 
set_backend and net->nc->info->poll may be not necessary.


I think your apporach to abstract per virtqueue start/stop from 
vhost_net_start/stop_one() will break the generality of 
vhost_dev_start(), which is a common interface for different devices.


To conclude my opinions

1. I think we need to add net->nc->info->poll in our 
vhost_net_virtqueue_reset() and vhost_net_virtqueue_restart()


2. We still need vhost_net_virtqueue_reset() and 
vhost_net_virtqueue_restart().


    a. vhost_net_virtqueue_reset() is a wrapper for vhost_virtqueue_stop().

    b. vhost_net_virtqueue_restart() is a wrapper for 
vhost_virtqueue_start().


Thanks




+}
+
  static void virtio_net_reset(VirtIODevice *vdev)
  {
  VirtIONet *n = VIRTIO_NET(vdev);
@@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass 
*klass, void *data)

  vdc->set_features = virtio_net_set_features;
  vdc->bad_features = virtio_net_bad_features;
  vdc->reset = virtio_net_reset;
+    vdc->queue_reset = virtio_net_queue_reset;
  vdc->set_status = virtio_net_set_status;
  vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
  vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 17:19, Víctor Colombo wrote:

Existing bug, but this is missing to clear fp status to start.

Reviewed-by: Richard Henderson 

r~



Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).


Oops, ok.



It's probably better to move the call to inside the helper.


I did write about a scheme by which all of these calls should go away.  I guess it has 
been a while...



r~

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 12:56, Richard Henderson wrote:

On 9/5/22 13:37, Víctor Colombo wrote:

These two helpers are almost identical, differing only by the softfloat
operation it calls. Merge them into one using a macro.
Also, take this opportunity to capitalize the helper name as we moved
the instruction to decodetree in a previous patch.

Signed-off-by: Víctor Colombo 
---
  target/ppc/fpu_helper.c    | 35 +++---
  target/ppc/helper.h    |  4 ++--
  target/ppc/translate/fp-impl.c.inc |  4 ++--
  3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0f045b70f8..32995179b5 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState 
*env, int flags,

  }
  }

-/* fsqrt - fsqrt. */
-float64 helper_fsqrt(CPUPPCState *env, float64 arg)
-{
-    float64 ret = float64_sqrt(arg, &env->fp_status);
-    int flags = get_float_exception_flags(&env->fp_status);
-
-    if (unlikely(flags & float_flag_invalid)) {
-    float_invalid_op_sqrt(env, flags, 1, GETPC());
-    }
-
-    return ret;
+#define FPU_FSQRT(name, 
op)   \
+float64 helper_##name(CPUPPCState *env, float64 
arg)  \
+{ 
\
+    float64 ret = op(arg, 
&env->fp_status);   \
+    int flags = 
get_float_exception_flags(&env->fp_status);   \
+  
\
+    if (unlikely(flags & float_flag_invalid)) 
{   \
+    float_invalid_op_sqrt(env, flags, 1, 
GETPC());    \
+
} 
\


Existing bug, but this is missing to clear fp status to start.

Reviewed-by: Richard Henderson 

r~



Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).
It's probably better to move the call to inside the helper.

+  
\
+    return 
ret;   \

  }

-/* fsqrts - fsqrts. */
-float64 helper_fsqrts(CPUPPCState *env, float64 arg)
-{
-    float64 ret = float64r32_sqrt(arg, &env->fp_status);
-    int flags = get_float_exception_flags(&env->fp_status);
-
-    if (unlikely(flags & float_flag_invalid)) {
-    float_invalid_op_sqrt(env, flags, 1, GETPC());
-    }
-    return ret;
-}
+FPU_FSQRT(FSQRT, float64_sqrt)
+FPU_FSQRT(FSQRTS, float64r32_sqrt)

  /* fre - fre. */
  float64 helper_fre(CPUPPCState *env, float64 arg)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 159b352f6e..68610896b8 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
  DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
  DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
  DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
-DEF_HELPER_2(fsqrt, f64, env, f64)
-DEF_HELPER_2(fsqrts, f64, env, f64)
+DEF_HELPER_2(FSQRT, f64, env, f64)
+DEF_HELPER_2(FSQRTS, f64, env, f64)
  DEF_HELPER_2(fre, i64, env, i64)
  DEF_HELPER_2(fres, i64, env, i64)
  DEF_HELPER_2(frsqrte, i64, env, i64)
diff --git a/target/ppc/translate/fp-impl.c.inc 
b/target/ppc/translate/fp-impl.c.inc

index 7a90c0e350..8d5cf0f982 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, 
arg_A_tb *a,

  return true;
  }

-TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
-TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
+TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
+TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);

  /*** Floating-Point 
multiply-and-add   ***/

  /* fmadd - fmadds */





--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Richard Henderson]

On 9/5/22 16:43, Claudio Fontana wrote:

You might think 'mayfail' can be called by other code as true in some cases, 
but no, it's always false.
I wonder why this "mayfail" argument exists and is propagated at all, when it 
cannot be anything else than false.
I tried to remove the "mayfail" parameter completely and things seem just fine.

In any case, the only thing that "mayfail" seems to control, is in 
module_load_file, and is a single printf:

 g_module = g_module_open(fname, flags);
 if (!g_module) {
 if (!mayfail) {
 fprintf(stderr, "Failed to open module: %s\n",
 g_module_error());
 }
 ret = -EINVAL;
 goto out;
 }


Weird.. Is someone building proprietary modules on top of QEMU? Is this what 
this is currently trying to address?
But then, the result is just a printf...


I thought it was for things like vga_interface_available, which probes for the vga 
modules, but then gracefully handles an error.


There's definitely something wrong with the plumbing here.


r~

Re: [PATCH v2] kvm: fix memory leak on failure to read stats descriptors

[Richard Henderson]

On 9/5/22 13:39, Paolo Bonzini wrote:

Reported by Coverity as CID 1490142.  Since the size is constant and the
lifetime is the same as the StatsDescriptors struct, embed the struct
directly instead of using a separate allocation.

Suggested-by: Richard Henderson
Signed-off-by: Paolo Bonzini
---
  accel/kvm/kvm-all.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)


Thanks,

Reviewed-by: Richard Henderson 


r~

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:

These two helpers are almost identical, differing only by the softfloat
operation it calls. Merge them into one using a macro.
Also, take this opportunity to capitalize the helper name as we moved
the instruction to decodetree in a previous patch.

Signed-off-by: Víctor Colombo 
---
  target/ppc/fpu_helper.c| 35 +++---
  target/ppc/helper.h|  4 ++--
  target/ppc/translate/fp-impl.c.inc |  4 ++--
  3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0f045b70f8..32995179b5 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState *env, int 
flags,
  }
  }
  
-/* fsqrt - fsqrt. */

-float64 helper_fsqrt(CPUPPCState *env, float64 arg)
-{
-float64 ret = float64_sqrt(arg, &env->fp_status);
-int flags = get_float_exception_flags(&env->fp_status);
-
-if (unlikely(flags & float_flag_invalid)) {
-float_invalid_op_sqrt(env, flags, 1, GETPC());
-}
-
-return ret;
+#define FPU_FSQRT(name, op)   \
+float64 helper_##name(CPUPPCState *env, float64 arg)  \
+{ \
+float64 ret = op(arg, &env->fp_status);   \
+int flags = get_float_exception_flags(&env->fp_status);   \
+  \
+if (unlikely(flags & float_flag_invalid)) {   \
+float_invalid_op_sqrt(env, flags, 1, GETPC());\
+} \


Existing bug, but this is missing to clear fp status to start.

Reviewed-by: Richard Henderson 

r~


+  \
+return ret;   \
  }
  
-/* fsqrts - fsqrts. */

-float64 helper_fsqrts(CPUPPCState *env, float64 arg)
-{
-float64 ret = float64r32_sqrt(arg, &env->fp_status);
-int flags = get_float_exception_flags(&env->fp_status);
-
-if (unlikely(flags & float_flag_invalid)) {
-float_invalid_op_sqrt(env, flags, 1, GETPC());
-}
-return ret;
-}
+FPU_FSQRT(FSQRT, float64_sqrt)
+FPU_FSQRT(FSQRTS, float64r32_sqrt)
  
  /* fre - fre. */

  float64 helper_fre(CPUPPCState *env, float64 arg)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 159b352f6e..68610896b8 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
  DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
  DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
  DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
-DEF_HELPER_2(fsqrt, f64, env, f64)
-DEF_HELPER_2(fsqrts, f64, env, f64)
+DEF_HELPER_2(FSQRT, f64, env, f64)
+DEF_HELPER_2(FSQRTS, f64, env, f64)
  DEF_HELPER_2(fre, i64, env, i64)
  DEF_HELPER_2(fres, i64, env, i64)
  DEF_HELPER_2(frsqrte, i64, env, i64)
diff --git a/target/ppc/translate/fp-impl.c.inc 
b/target/ppc/translate/fp-impl.c.inc
index 7a90c0e350..8d5cf0f982 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a,
  return true;
  }
  
-TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);

-TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
+TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
+TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
  
  /*** Floating-Point multiply-and-add   ***/

  /* fmadd - fmadds */

[RFC] module: removed unused function argument "mayfail"

[Claudio Fontana]

mayfail is always passed as false for every invocation throughout the program.
It controls whether to printf or not to printf an error on
g_module_open failure.

Remove this unused argument.

Signed-off-by: Claudio Fontana 
---
 include/qemu/module.h |  8 
 softmmu/qtest.c   |  2 +-
 util/module.c | 20 +---
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index bd73607104..8c012bbe03 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,15 +61,15 @@ typedef enum {
 #define fuzz_target_init(function) module_init(function, \
MODULE_INIT_FUZZ_TARGET)
 #define migration_init(function) module_init(function, MODULE_INIT_MIGRATION)
-#define block_module_load_one(lib) module_load_one("block-", lib, false)
-#define ui_module_load_one(lib) module_load_one("ui-", lib, false)
-#define audio_module_load_one(lib) module_load_one("audio-", lib, false)
+#define block_module_load_one(lib) module_load_one("block-", lib)
+#define ui_module_load_one(lib) module_load_one("ui-", lib)
+#define audio_module_load_one(lib) module_load_one("audio-", lib)
 
 void register_module_init(void (*fn)(void), module_init_type type);
 void register_dso_module_init(void (*fn)(void), module_init_type type);
 
 void module_call_init(module_init_type type);
-bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);
+bool module_load_one(const char *prefix, const char *lib_name);
 void module_load_qom_one(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8acef2628..76eb7bac56 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -756,7 +756,7 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 g_assert(words[1] && words[2]);
 
 qtest_send_prefix(chr);
-if (module_load_one(words[1], words[2], false)) {
+if (module_load_one(words[1], words[2])) {
 qtest_sendf(chr, "OK\n");
 } else {
 qtest_sendf(chr, "FAIL\n");
diff --git a/util/module.c b/util/module.c
index 8ddb0e18f5..8563edd626 100644
--- a/util/module.c
+++ b/util/module.c
@@ -144,7 +144,7 @@ static bool module_check_arch(const QemuModinfo *modinfo)
 return true;
 }
 
-static int module_load_file(const char *fname, bool mayfail, bool 
export_symbols)
+static int module_load_file(const char *fname, bool export_symbols)
 {
 GModule *g_module;
 void (*sym)(void);
@@ -172,10 +172,8 @@ static int module_load_file(const char *fname, bool 
mayfail, bool export_symbols
 }
 g_module = g_module_open(fname, flags);
 if (!g_module) {
-if (!mayfail) {
-fprintf(stderr, "Failed to open module: %s\n",
-g_module_error());
-}
+fprintf(stderr, "Failed to open module: %s\n",
+g_module_error());
 ret = -EINVAL;
 goto out;
 }
@@ -208,7 +206,7 @@ out:
 }
 #endif
 
-bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
+bool module_load_one(const char *prefix, const char *lib_name)
 {
 bool success = false;
 
@@ -256,7 +254,7 @@ bool module_load_one(const char *prefix, const char 
*lib_name, bool mayfail)
 if (strcmp(modinfo->name, module_name) == 0) {
 /* we depend on other module(s) */
 for (sl = modinfo->deps; *sl != NULL; sl++) {
-module_load_one("", *sl, false);
+module_load_one("", *sl);
 }
 } else {
 for (sl = modinfo->deps; *sl != NULL; sl++) {
@@ -287,7 +285,7 @@ bool module_load_one(const char *prefix, const char 
*lib_name, bool mayfail)
 for (i = 0; i < n_dirs; i++) {
 fname = g_strdup_printf("%s/%s%s",
 dirs[i], module_name, CONFIG_HOST_DSOSUF);
-ret = module_load_file(fname, mayfail, export_symbols);
+ret = module_load_file(fname, export_symbols);
 g_free(fname);
 fname = NULL;
 /* Try loading until loaded a module file */
@@ -333,7 +331,7 @@ void module_load_qom_one(const char *type)
 }
 for (sl = modinfo->objs; *sl != NULL; sl++) {
 if (strcmp(type, *sl) == 0) {
-module_load_one("", modinfo->name, false);
+module_load_one("", modinfo->name);
 }
 }
 }
@@ -354,7 +352,7 @@ void module_load_qom_all(void)
 if (!module_check_arch(modinfo)) {
 continue;
 }
-module_load_one("", modinfo->name, false);
+module_load_one("", modinfo->name);
 }
 module_loaded_qom_all = true;
 }
@@ -370,7 +368,7 @@ void qemu_load_module_for_opts(const char *group)
 }
 for (sl = modinfo->opts; *sl != NULL; sl++) {
 if (strcmp(group, *sl) == 0) {
-module_load_one("",

Re: [PATCH 2/3] target/ppc: Move fsqrts to decodetree

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:

Signed-off-by: Víctor Colombo
---
  target/ppc/insn32.decode   |  1 +
  target/ppc/translate/fp-impl.c.inc | 23 +--
  target/ppc/translate/fp-ops.c.inc  |  1 -
  3 files changed, 2 insertions(+), 23 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled

[Igor Mammedov]

On Mon,  5 Sep 2022 12:55:31 +0530
Ani Sinha  wrote:

> Possible fix for https://bugzilla.redhat.com/show_bug.cgi?id=2089545
> 
> Change in AML:
> 
> @@ -47,33 +47,39 @@
>  Scope (_SB)
>  {
>  Device (PCI0)
>  {
>  Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: 
> Hardware ID
>  Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: 
> Compatible ID
>  Name (_ADR, Zero)  // _ADR: Address
>  Name (_UID, Zero)  // _UID: Unique ID
>  Method (_OSC, 4, NotSerialized)  // _OSC: Operating System 
> Capabilities
>  {
>  CreateDWordField (Arg3, Zero, CDW1)
>  If ((Arg0 == ToUUID ("33db4d5b-1ff7-401c-9657-7441c03dd766") 
> /* PCI Host Bridge Device */))
>  {
>  CreateDWordField (Arg3, 0x04, CDW2)
>  CreateDWordField (Arg3, 0x08, CDW3)
>  Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> -Local0 &= 0x1E
> +Local0 &= 0x1F
> +Local1 = (CDW3 & One)
> +If ((One == Local1))
> +{
> +CDW1 |= 0x12
> +}
> +
>  If ((Arg1 != One))
>  {
>  CDW1 |= 0x08
>  }
> 
>  If ((CDW3 != Local0))
>  {
>  CDW1 |= 0x10
>  }
> 
>  CDW3 = Local0
>  }
>  Else
>  {
>  CDW1 |= 0x04
>  }
> **
> 
> Signed-off-by: Ani Sinha 
> ---
>  hw/i386/acpi-build.c | 23 ---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0355bd3dda..3dc9379f27 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1348,10 +1348,12 @@ static Aml *build_q35_osc_method(bool 
> enable_native_pcie_hotplug)
>  {
>  Aml *if_ctx;
>  Aml *if_ctx2;
> +Aml *if_ctx3;
>  Aml *else_ctx;
>  Aml *method;
>  Aml *a_cwd1 = aml_name("CDW1");
>  Aml *a_ctrl = aml_local(0);
> +Aml *a_pcie_nhp_ctl = aml_local(1);
>  
>  method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>  aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), 
> "CDW1"));
> @@ -1366,11 +1368,26 @@ static Aml *build_q35_osc_method(bool 
> enable_native_pcie_hotplug)
>  /*
>   * Always allow native PME, AER (no dependencies)
>   * Allow SHPC (PCI bridges can have SHPC controller)
> - * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled.
>   */
> -aml_append(if_ctx, aml_and(a_ctrl,
> -aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)), a_ctrl));
> +aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));

that makes us not actually mask any capabilities since you forgot to mask
bit 1 later under if_ctx3 context.

So OSPM will see a permanent failure (_OSC failure bit in CWD1)
and will have no idea that PCI Hotplug is not supported since we return CWD3
with this bit still set whoever much it tries to negotiate.

So if it boots at all, guest will probably not use any requested features
since _OSC failed to confirm any without error.

And even if we clear hotplug bit it still doesn't help.

some more testing shows that ATS cap doesn't depended hard on native hotplug
(i.e. run QEMU with native hotplug enabled but disable hotplug on root-port in 
question)
To me it still looks like a bug in Windows' acpi hotplug impl
(or perhaps it's no more properly maintained, so it doesn't account for new 
features).

> +/*
> + * if ACPI PCI Hot-plug is enabled, do not let OSPM set OSC PCIE
> + * Native hotplug ctrl bit.
> + */
> +if (!enable_native_pcie_hotplug) {
> +/* check if the ACPI native hotplug bit is set by the OS in DWORD3 */
> +aml_append(if_ctx, aml_and(aml_name("CDW3"),
> +   aml_int(0x01), a_pcie_nhp_ctl));
> +if_ctx3 = aml_if(aml_equal(aml_int(1), a_pcie_nhp_ctl));
> +/*
> + * ACPI spec 5.1, section 6.2.11
> + * bit 1 in first DWORD - _OSC failure
> + * bit 4 in first DWORD - capabilities masked
> + */
> +aml_append(if_ctx3, aml_or(a_cwd1, aml_int(0x12), a_cwd1));
> +aml_append(if_ctx, if_ctx3);
> +}
>  if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1;
>  /* Unknown revision */
>  aml_append(if_ctx2, aml_or(a_cwd1, aml_int(0x08), a_cwd1));

[PATCH v2] tests/tcg/x86_64: add cross-modifying code test

[Ilya Leoshkevich]

commit f025692c992c ("accel/tcg: Clear PAGE_WRITE before translation")
fixed cross-modifying code handling, but did not add a test. The
changed code was further improved recently [1], and I was not sure
whether these modifications were safe (spoiler: they were fine).

Add a test to make sure there are no regressions.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00034.html

Signed-off-by: Ilya Leoshkevich 
---

v1: https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00455.html
v1 -> v2: Fix tweaking the flags (Alex).
  Keep the custom build rule for now.

 tests/tcg/x86_64/Makefile.target|  6 +-
 tests/tcg/x86_64/cross-modifying-code.c | 80 +
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/x86_64/cross-modifying-code.c

diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index 6177fd845a..3d9bc3377b 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -10,6 +10,7 @@ include $(SRC_PATH)/tests/tcg/i386/Makefile.target
 
 ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET))
 X86_64_TESTS += vsyscall
+X86_64_TESTS += cross-modifying-code
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)
@@ -23,5 +24,8 @@ test-x86_64: LDFLAGS+=-lm -lc
 test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
 
-vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c
+%: $(SRC_PATH)/tests/tcg/x86_64/%.c
$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
+
+cross-modifying-code: CFLAGS+=-pthread
+cross-modifying-code: LDFLAGS+=-pthread
diff --git a/tests/tcg/x86_64/cross-modifying-code.c 
b/tests/tcg/x86_64/cross-modifying-code.c
new file mode 100644
index 00..2704df6061
--- /dev/null
+++ b/tests/tcg/x86_64/cross-modifying-code.c
@@ -0,0 +1,80 @@
+/*
+ * Test patching code, running in one thread, from another thread.
+ *
+ * Intel SDM calls this "cross-modifying code" and recommends a special
+ * sequence, which requires both threads to cooperate.
+ *
+ * Linux kernel uses a different sequence that does not require cooperation and
+ * involves patching the first byte with int3.
+ *
+ * Finally, there is user-mode software out there that simply uses atomics, and
+ * that seems to be good enough in practice. Test that QEMU has no problems
+ * with this as well.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+void add1_or_nop(long *x);
+asm(".pushsection .rwx,\"awx\",@progbits\n"
+".globl add1_or_nop\n"
+/* addq $0x1,(%rdi) */
+"add1_or_nop: .byte 0x48, 0x83, 0x07, 0x01\n"
+"ret\n"
+".popsection\n");
+
+#define THREAD_WAIT 0
+#define THREAD_PATCH 1
+#define THREAD_STOP 2
+
+static void *thread_func(void *arg)
+{
+int val = 0x0026748d; /* nop */
+
+while (true) {
+switch (__atomic_load_n((int *)arg, __ATOMIC_SEQ_CST)) {
+case THREAD_WAIT:
+break;
+case THREAD_PATCH:
+val = __atomic_exchange_n((int *)&add1_or_nop, val,
+  __ATOMIC_SEQ_CST);
+break;
+case THREAD_STOP:
+return NULL;
+default:
+assert(false);
+__builtin_unreachable();
+}
+}
+}
+
+#define INITIAL 42
+#define COUNT 100
+
+int main(void)
+{
+int command = THREAD_WAIT;
+pthread_t thread;
+long x = 0;
+int err;
+int i;
+
+err = pthread_create(&thread, NULL, &thread_func, &command);
+assert(err == 0);
+
+__atomic_store_n(&command, THREAD_PATCH, __ATOMIC_SEQ_CST);
+for (i = 0; i < COUNT; i++) {
+add1_or_nop(&x);
+}
+__atomic_store_n(&command, THREAD_STOP, __ATOMIC_SEQ_CST);
+
+err = pthread_join(thread, NULL);
+assert(err == 0);
+
+assert(x >= INITIAL);
+assert(x <= INITIAL + COUNT);
+
+return EXIT_SUCCESS;
+}
-- 
2.37.2

Re: [PATCH 1/3] target/ppc: Move fsqrt to decodetree

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:

Signed-off-by: Víctor Colombo
---
  target/ppc/insn32.decode   |  7 +++
  target/ppc/translate/fp-impl.c.inc | 29 +
  target/ppc/translate/fp-ops.c.inc  |  1 -
  3 files changed, 24 insertions(+), 13 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH] tests/tcg/x86_64: add cross-modifying code test

[Ilya Leoshkevich]

On Sat, 2022-09-03 at 10:13 +0100, Alex Bennée wrote:
> 
> Ilya Leoshkevich  writes:
> 
> > commit f025692c992c ("accel/tcg: Clear PAGE_WRITE before
> > translation")
> > fixed cross-modifying code handling, but did not add a test. The
> > changed code was further improved recently [1], and I was not sure
> > whether these modifications were safe (spoiler: they were fine).
> > 
> > Add a test to make sure there are no regressions.
> > 
> > [1]
> > https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00034.html
> > 
> > Signed-off-by: Ilya Leoshkevich 
> > ---
> >  tests/tcg/x86_64/Makefile.target    |  6 +-
> >  tests/tcg/x86_64/cross-modifying-code.c | 80
> > +
> >  2 files changed, 85 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/tcg/x86_64/cross-modifying-code.c
> > 
> > diff --git a/tests/tcg/x86_64/Makefile.target
> > b/tests/tcg/x86_64/Makefile.target
> > index b71a6bcd5e..58e7bfd681 100644
> > --- a/tests/tcg/x86_64/Makefile.target
> > +++ b/tests/tcg/x86_64/Makefile.target
> > @@ -10,6 +10,7 @@ include
> > $(SRC_PATH)/tests/tcg/i386/Makefile.target
> >  
> >  ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET))
> >  X86_64_TESTS += vsyscall
> > +X86_64_TESTS += cross-modifying-code
> >  TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
> >  else
> >  TESTS=$(MULTIARCH_TESTS)
> > @@ -20,5 +21,8 @@ test-x86_64: LDFLAGS+=-lm -lc
> >  test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-
> > muldiv.h
> > $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
> >  
> > -vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c
> > +%: $(SRC_PATH)/tests/tcg/x86_64/%.c
> > $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
> 
> You shouldn't need to redefine the default rule when you can tweak
> the flags

Without this rule, I get:

make[1]: *** No rule to make target 'vsyscall', needed by 'all'.  Stop.

I think this is because the default rule has %.c as a dependency, and
we run from a different directory here.

> > +
> > +smc: CFLAGS+=-pthread
> > +smc: LDFLAGS+=-pthread
> 
> I think this must be from an old iteration because:
> 
> make[1]: Entering directory
> '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user'
> cc -Wall -Werror -O0 -g -fno-strict-aliasing
> /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c -o
> cross-modifying-code -static
> /usr/bin/ld: /tmp/ccK05RAk.o: in function `main':
> /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c:64:
> undefined reference to `pthread_create'
> /usr/bin/ld: /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-
> modifying-code.c:73: undefined reference to `pthread_join'
> collect2: error: ld returned 1 exit status
> make[1]: ***
> [/home/alex/lsrc/qemu.git/tests/tcg/x86_64/Makefile.target:25: cross-
> modifying-code] Error 1
> make[1]: Leaving directory
> '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user'
> make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:52: build-
> tcg-tests-x86_64-linux-user] Error 2

Sorry about that, I should have tested a clean build.
I will send a v2.

Best regards,
Ilya

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

On 9/5/22 16:36, Claudio Fontana wrote:
> On 9/5/22 14:06, Richard Henderson wrote:
>> On 9/5/22 11:13, Claudio Fontana wrote:
>>> If module_load_one, module_load_file fail for any reason
>>> (permissions, plugin not installed, ...), we need to provide some 
>>> notification
>>> to the user to understand that this is happening; otherwise the errors
>>> reported on initialization will make no sense to the user.
>>>
>>> Signed-off-by: Claudio Fontana 
>>> ---
>>>   accel/accel-softmmu.c | 10 --
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>>> index 67276e4f52..807708ee86 100644
>>> --- a/accel/accel-softmmu.c
>>> +++ b/accel/accel-softmmu.c
>>> @@ -66,15 +66,21 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>>   {
>>>   const char *ac_name;
>>>   char *ops_name;
>>> +ObjectClass *oc;
>>>   AccelOpsClass *ops;
>>>   
>>>   ac_name = object_class_get_name(OBJECT_CLASS(ac));
>>>   g_assert(ac_name != NULL);
>>>   
>>>   ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
>>> -ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
>>> +oc = module_object_class_by_name(ops_name);
>>> +if (!oc) {
>>> +error_report("fatal: could not find module object of type \"%s\", "
>>> + "plugin might not be loaded correctly", ops_name);
>>> +exit(EXIT_FAILURE);
>>> +}
>>
>> The change is correct, in that we certainly cannot continue without the 
>> accelerator loaded.
>>
>> But I'm very disappointed that the module interface does not use Error, so 
>> you have no 
>> choice but to use an extremely vague message here.  I would much prefer to 
>> plumb down an 
>> error parameter so that here one could simply pass &error_fatal.
>>
>>
>> r~
>>
> 
> I agree. I see it as also connected to:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00578.html
> 
> module_load_file actually has the pertinent information of what it going 
> wrong at the time it goes wrong, so I presume we should collect the Error 
> there,
> and find a way not to lose the return value along the way..
> 
> Claudio
> 

Currently module_load_qom_one() is called among other things inside 
qom/object.c::object_initialize() as well.

Curiously enough, module_load_one(), which is in turn called by it, takes an 
argument "bool mayfail", which is always false,
never passed as true in the whole codebase:

bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);

/* mayfail is always false */

module_load_one calls in turn module_load_file, which also takes a bool mayfail 
argument:

static int module_load_file(const char *fname, bool mayfail, bool 
export_symbols);

You might think 'mayfail' can be called by other code as true in some cases, 
but no, it's always false.
I wonder why this "mayfail" argument exists and is propagated at all, when it 
cannot be anything else than false.
I tried to remove the "mayfail" parameter completely and things seem just fine.

In any case, the only thing that "mayfail" seems to control, is in 
module_load_file, and is a single printf:

g_module = g_module_open(fname, flags);
if (!g_module) {
if (!mayfail) {
fprintf(stderr, "Failed to open module: %s\n",
g_module_error());
}
ret = -EINVAL;
goto out;
}


Weird.. Is someone building proprietary modules on top of QEMU? Is this what 
this is currently trying to address?
But then, the result is just a printf...

Thanks,

C

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/5/22 17:23, Janis Schoetterl-Glausch wrote:

On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote:


On 9/5/22 13:32, Nico Boehr wrote:

Quoting Pierre Morel (2022-09-02 09:55:22)

S390x do not support multithreading in the guest.
Do not let admin falsely specify multithreading on QEMU
smp commandline.

Signed-off-by: Pierre Morel 
---
   hw/s390x/s390-virtio-ccw.c | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70229b102b..b5ca154e2f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
   MachineClass *mc = MACHINE_GET_CLASS(machine);
   int i;
   
+/* Explicitely do not support threads */

^
Explicitly


+assert(machine->smp.threads == 1);


It might be nicer to give a better error message to the user.
What do you think about something like (broken whitespace ahead):

  if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
  error_setg(&error_fatal, "More than one thread specified, but 
multithreading unsupported");
  return;
  }




OK, I think I wanted to do this and I changed my mind, obviously, I do
not recall why.
I will do almost the same but after a look at error.h I will use
error_report()/exit() instead of error_setg()/return as in:


+/* Explicitly do not support threads */
+if (machine->smp.threads != 1) {
+error_report("More than one thread specified, but
multithreading unsupported");
+exit(1);
+}


I agree that an assert is not a good solution, and I'm not sure
aborting is a good idea either.
I'm assuming that currently if you specify threads > 0 qemu will run
with the number of CPUs multiplied by threads (compared to threads=1).
If that is true, then a new qemu version will break existing
invocations.

An alternative would be to print a warning and do:
cores *= threads
threads = 1

The questions would be what the best place to do that is.
I guess we'd need a new compat variable if that's done in machine-smp.c


Right, I think we can use the new "topology_disable" machine property.





Thanks,

Regards,
Pierre





--
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 09/10] s390x/cpu_topology: activating CPU topology

[Pierre Morel]

On 9/2/22 09:55, Pierre Morel wrote:

Starting with a new machine, s390-virtio-ccw-7.2, the machine
property topology-disable is set to false while it is kept to
true for older machine.
This allows migrating older machine without disabling the ctop
CPU feature for older machine, thus keeping existing start scripts.

The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
the topology facility for the guest in the case the topology
is not disabled.

Signed-off-by: Pierre Morel 
---


Sorry, forget this patch I made a stupid rebase error and lost half of 
the code.


--
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Janis Schoetterl-Glausch]

On Mon, 2022-09-05 at 17:10 +0200, Pierre Morel wrote:
> 
> On 9/5/22 13:32, Nico Boehr wrote:
> > Quoting Pierre Morel (2022-09-02 09:55:22)
> > > S390x do not support multithreading in the guest.
> > > Do not let admin falsely specify multithreading on QEMU
> > > smp commandline.
> > > 
> > > Signed-off-by: Pierre Morel 
> > > ---
> > >   hw/s390x/s390-virtio-ccw.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index 70229b102b..b5ca154e2f 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
> > >   MachineClass *mc = MACHINE_GET_CLASS(machine);
> > >   int i;
> > >   
> > > +/* Explicitely do not support threads */
> >^
> >Explicitly
> > 
> > > +assert(machine->smp.threads == 1);
> > 
> > It might be nicer to give a better error message to the user.
> > What do you think about something like (broken whitespace ahead):
> > 
> >  if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
> >  error_setg(&error_fatal, "More than one thread specified, but 
> > multithreading unsupported");
> >  return;
> >  }
> > 
> 
> 
> OK, I think I wanted to do this and I changed my mind, obviously, I do 
> not recall why.
> I will do almost the same but after a look at error.h I will use 
> error_report()/exit() instead of error_setg()/return as in:
> 
> 
> +/* Explicitly do not support threads */
> +if (machine->smp.threads != 1) {
> +error_report("More than one thread specified, but 
> multithreading unsupported");
> +exit(1);
> +}

I agree that an assert is not a good solution, and I'm not sure
aborting is a good idea either.
I'm assuming that currently if you specify threads > 0 qemu will run
with the number of CPUs multiplied by threads (compared to threads=1).
If that is true, then a new qemu version will break existing
invocations.

An alternative would be to print a warning and do:
cores *= threads
threads = 1

The questions would be what the best place to do that is.
I guess we'd need a new compat variable if that's done in machine-smp.c
> 
> 
> Thanks,
> 
> Regards,
> Pierre
> 

Re: [PATCH] tests: unit: add NULL-pointer check

[Alex Bennée]

Paolo Bonzini  writes:

> In CID 1432593, Coverity complains that the result of qdict_crumple()
> might leak if it is not a dictionary.  This is not a practical concern
> since the test would fail immediately with a NULL pointer dereference
> in qdict_size().
>
> However, it is not nice to depend on qdict_size() crashing, so add an
> explicit assertion that that the crumpled object was indeed a dictionary.
>
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2] 9pfs: use GHashTable for fid table

[Philippe Mathieu-Daudé via]

On 5/9/22 17:03, Linus Heckemann wrote:

The previous implementation would iterate over the fid table for
lookup operations, resulting in an operation with O(n) complexity on
the number of open files and poor cache locality -- for every open,
stat, read, write, etc operation.

This change uses a hashtable for this instead, significantly improving
the performance of the 9p filesystem. The runtime of NixOS's simple
installer test, which copies ~122k files totalling ~1.8GiB from 9p,
decreased by a factor of about 10.

Signed-off-by: Linus Heckemann 
---
  hw/9pfs/9p.c | 130 +++
  hw/9pfs/9p.h |   2 +-
  2 files changed, 69 insertions(+), 63 deletions(-)


Watch out to iterate the version when respining patches:

"Send each new revision as a new top-level thread, rather than burying 
it in-reply-to an earlier revision, as many reviewers are not looking 
inside deep threads for new patches."

https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v9 01/10] s390x/cpus: Make absence of multithreading clear

[Pierre Morel]

On 9/5/22 13:32, Nico Boehr wrote:

Quoting Pierre Morel (2022-09-02 09:55:22)

S390x do not support multithreading in the guest.
Do not let admin falsely specify multithreading on QEMU
smp commandline.

Signed-off-by: Pierre Morel 
---
  hw/s390x/s390-virtio-ccw.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70229b102b..b5ca154e2f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -86,6 +86,9 @@ static void s390_init_cpus(MachineState *machine)
  MachineClass *mc = MACHINE_GET_CLASS(machine);
  int i;
  
+/* Explicitely do not support threads */

   ^
   Explicitly


+assert(machine->smp.threads == 1);


It might be nicer to give a better error message to the user.
What do you think about something like (broken whitespace ahead):

 if (machine->smp.threads != 1) {if (machine->smp.threads != 1) {
 error_setg(&error_fatal, "More than one thread specified, but 
multithreading unsupported");
 return;
 }




OK, I think I wanted to do this and I changed my mind, obviously, I do 
not recall why.
I will do almost the same but after a look at error.h I will use 
error_report()/exit() instead of error_setg()/return as in:



+/* Explicitly do not support threads */
+if (machine->smp.threads != 1) {
+error_report("More than one thread specified, but 
multithreading unsupported");

+exit(1);
+}


Thanks,

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index.

[Philippe Mathieu-Daudé via]

On 2/9/22 00:50, Tyler Ng wrote:

Fixes a bug in which the index of the interrupt priority is off by 1.
For example, using an IRQ number of 3 with a priority of 1 is supposed to set
plic->source_priority[2] = 1, but instead it sets
plic->source_priority[3] = 1. When an interrupt is claimed to be
serviced, it checks the index 2 instead of 3.

Signed-off-by: Tyler Ng 
---
  hw/intc/sifive_plic.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index af4ae3630e..e75c47300a 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr
addr, uint64_t value,
  SiFivePLICState *plic = opaque;

  if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
-uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
+uint32_t irq = ((addr - plic->priority_base) >> 2) + 0;

  plic->source_priority[irq] = value & 7;
  sifive_plic_update(plic);
--
2.30.2



What about sifive_plic_read()?